Skip to content

fix(acp): normalize file change path expectation on macOS - #96

Open
AsperforMias wants to merge 2 commits into
langgenius:mainfrom
AsperforMias:fix/issue-93-macos-acp-file-system-path
Open

fix(acp): normalize file change path expectation on macOS#96
AsperforMias wants to merge 2 commits into
langgenius:mainfrom
AsperforMias:fix/issue-93-macos-acp-file-system-path

Conversation

@AsperforMias

Copy link
Copy Markdown

Summary

  • Canonicalize ACP file change paths so macOS and Linux report the same canonical path.
  • Update the ACP file system test to assert against the canonical path and ignore unrelated extra fields.

Validation

  • bun test tests/acp-file-system.test.ts
  • bun test tests/acp-*.test.ts
  • bun test tests

Closes #93

Copilot AI review requested due to automatic review settings July 31, 2026 21:36

Copilot AI 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.

🟡 Not ready to approve

Canonicalization misses final-component symlinks, and the required sourceEventId validation was removed.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

Canonicalizes ACP file-change paths across macOS and Linux and updates related assertions.

Changes:

  • Resolves writable paths to canonical locations.
  • Uses partial event matching while preserving event-count validation.
File summaries
File Description
src/runtimes/acp/acp-file-system.ts Canonicalizes written and reported paths.
tests/acp-file-system.test.ts Updates cross-platform path expectations.
Review details
  • Files reviewed: 2/2 changed files
  • Comments generated: 2
  • Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

Comment thread tests/acp-file-system.test.ts Outdated
Comment thread src/runtimes/acp/acp-file-system.ts Outdated
Copilot AI review requested due to automatic review settings July 31, 2026 21:40
@AsperforMias
AsperforMias force-pushed the fix/issue-93-macos-acp-file-system-path branch from ab42173 to 691d43e Compare July 31, 2026 21:40

Copilot AI 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.

🟢 Ready to approve

The focused portability fix satisfies the issue’s acceptance criteria without changing production behavior.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

@AsperforMias

Copy link
Copy Markdown
Author

@Yevanchen needs help for running CI

Copilot AI review requested due to automatic review settings August 1, 2026 07:55

Copilot AI 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.

🟡 Not ready to approve

The revised process test no longer exercises the force-kill fallback because the descendant accepts SIGTERM.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (1)

tests/openai-app-server-client.test.ts:966

  • The descendant no longer ignores SIGTERM, so stop() can take the early return after graceful termination (app-server-client.ts:634-645) and this test no longer exercises the advertised force-SIGKILL fallback. Keep a SIGTERM listener installed so the descendant remains alive until the force-kill path, then retain the new PID-exit assertion to verify that fallback actually removed it.
writeFileSync(${JSON.stringify(descendantReadyPath)}, "ready");
setInterval(() => {}, 1000);
  • Files reviewed: 2/2 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

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.

test(acp): normalize file-change path expectation on macOS

2 participants