Skip to content

test(scan): exercise large git output on Windows - #614

Open
mldangelo-oai wants to merge 5 commits into
mainfrom
mdangelo/codex/pr-572-windows-test
Open

test(scan): exercise large git output on Windows#614
mldangelo-oai wants to merge 5 commits into
mainfrom
mdangelo/codex/pr-572-windows-test

Conversation

@mldangelo-oai

@mldangelo-oai mldangelo-oai commented Aug 22, 2026

Copy link
Copy Markdown
Collaborator

Summary

Stacked follow-up to #572, which fixes committed-diff scan preparation when the tracked-file inventory exceeds Node's default subprocess buffer. This keeps its regression coverage active on Windows. Merge #572 first, then refresh this branch against main.

Changes

  • remove the Windows early return that made the regression report a pass without exercising the code
  • use short multibyte filenames to produce more than 1 MiB of real git ls-files -t -z output without overlong Windows paths

Testing

Run from sdk/typescript unless noted otherwise:

  • corepack pnpm dlx bun@1.3.14 test --timeout 30000 tests-ts/targets-large-output.test.ts --seed 12345 (1 passed)
  • corepack pnpm dlx bun@1.3.14 test --timeout 30000 ./tests-ts --seed 12345 (1,436 passed, 23 platform-specific skips; one unrelated concurrency assertion failed and passed on immediate isolated rerun)
  • corepack pnpm dlx bun@1.3.14 test --timeout 30000 tests-ts/api-credentials.test.ts --seed 12345 -t 'runs parallel ChatGPT scans with isolated mutable configuration' (1 passed)
  • exact-head GitHub Actions: all required checks passed; the regression passed on Windows with Node 22 and Node 24
  • corepack pnpm run types, corepack pnpm run format, corepack pnpm run build, and git diff --check (passed)

Risk and rollout

Test-only follow-up. The regression creates 6,000 temporary empty files and removes its temporary repository after each run. It does not change runtime behavior or public CLI surface. Until #572 merges, GitHub's comparison against main also includes the parent pull request's commits.

Public disclosure review

  • No customer, partner, prospect, or user identities, data, or identifying details are included.
  • No credentials, personal data, private source, scan findings, or nonpublic links or tickets are included.
  • I reviewed the branch name, title, description, commits, changes, comments, logs, screenshots, attachments, and links for public disclosure.

@github-actions github-actions Bot added the skip-release-notes Omit internal changes from generated release notes label Aug 22, 2026
@mldangelo-oai

Copy link
Copy Markdown
Collaborator Author

@codex review

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Aug 22, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-08-22T20:07:02.853495Z 9d08cf4 Draft marked ready
🔒 Security Review Completed 2026-08-22T20:07:42.045597Z 9d08cf4 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e75024e602

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread sdk/typescript/tests-ts/targets-large-output.test.ts Outdated
@mldangelo-oai

Copy link
Copy Markdown
Collaborator Author

@codex review

@chatgpt-codex-connector

Copy link
Copy Markdown

Codex Review: Didn't find any major issues. 👍

Reviewed commit: 9d08cf41e7

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

@mldangelo-oai
mldangelo-oai marked this pull request as ready for review August 22, 2026 20:05

@kmbroai kmbroai left a comment

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.

Critical review

Reviewed 9d08cf41e711e8558f3293f0676c5306ef9af3c3. The regression is necessary, and I found no blocking correctness issue in the revised fixture.

Necessity and correctness

The production problem is an incidental subprocess buffer limiting valid Git output. The one-line maxBuffer: Infinity change removes that arbitrary limit for the existing local Git operation. The test measures actual UTF-8 output size and drives normalizeTarget followed by validateCommittedDiffCheckout; it does not simply assert the option is present.

The previous oversized-filename finding is fixed: 60 three-byte characters plus the numeric prefix/extension keep each filename below the usual 255-byte component limit, while 6,000 entries still exceed 1 MiB in aggregate. Keeping the shared test active on Windows is valuable; reporting a skipped code path as a passing regression would not be.

Simplification and scope

Keep the fixture to one real Git repository and one boundary assertion. It does not need randomized directory trees, a benchmark, or a new production cap. The 6,000 concurrent file writes are a test cost, not a reason to broaden production code. If Windows CI shows resource exhaustion, bound fixture creation concurrency in this test rather than disabling it or reintroducing an output cap.

The description correctly says the comparison includes the parent fix until #572 is integrated. Preserve that dependency distinction: this branch should not become a second independent implementation of the same buffer change. After refreshing the base, retain the meaningful Windows regression even if the source hunk disappears.

Verification

Ran bun test --timeout 30000 --seed 12345 tests-ts/targets-large-output.test.ts: 1 passed, 0 failed on Bun 1.3.14/Linux. The measured Git output exceeded 1 MiB and the public target-validation flow completed. I did not execute native Windows or the full package suite.

@faizan-oai faizan-oai left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed the complete two-file diff. The large-output regression passes locally, and required Windows, macOS, and Linux CI is green. No blocking findings on this head.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-release-notes Omit internal changes from generated release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants