Skip to content

test: stabilize app-server path assertions on Windows#19604

Merged
bolinfest merged 1 commit intomainfrom
pr19604
Apr 25, 2026
Merged

test: stabilize app-server path assertions on Windows#19604
bolinfest merged 1 commit intomainfrom
pr19604

Conversation

@bolinfest
Copy link
Copy Markdown
Collaborator

@bolinfest bolinfest commented Apr 25, 2026

Why

Windows can represent the same canonical local path with either a normal drive path or a verbatim device path prefix. The failure pattern that motivated this PR was an assertion diff like C:\... versus \\?\C:\...: different spellings, same file.

That became visible while validating the permissions stack above this PR. The stack increasingly routes paths through AbsolutePathBuf, which normalizes supported Windows device prefixes, while several existing tests still built expected values directly with std::fs::canonicalize() or compared AbsolutePathBuf::as_path() to a raw PathBuf. On Windows, that can make tests fail because the two sides choose different textual forms for an otherwise equivalent canonical path.

This PR is intentionally split out as the bottom PR below #19606. The runtime permissions migration should not carry unrelated Windows test stabilization, and reviewers should be able to verify this as a test-only change before looking at the larger permissions changes.

Failure Modes Covered

  • conversation_summary expected rollout paths were built from raw canonicalized PathBufs, while app-server responses could carry AbsolutePathBuf-normalized paths.
  • thread_resume compared returned thread paths directly to previously stored or fixture paths, so a verbatim-prefix spelling could fail an otherwise correct resume.
  • marketplace_add compared plugin install roots through as_path() against raw canonicalized paths, reproducing the same C:\... versus \\?\C:\... mismatch in both app-server and core-plugin coverage.

What Changed

  • In app-server/tests/suite/conversation_summary.rs, normalize both expected rollout paths and received ConversationSummary.path values through AbsolutePathBuf before comparing the full summary object.
  • In app-server/tests/suite/v2/thread_resume.rs, normalize both sides of thread path comparisons before asserting equality. This keeps the tests focused on whether resume returned the same existing path, not whether Windows used the same string spelling.
  • In app-server/tests/suite/v2/marketplace_add.rs and core-plugins/src/marketplace_add.rs, compare install roots as AbsolutePathBuf values instead of comparing an absolute-path wrapper to a raw canonicalized PathBuf.

Behavior

This PR does not change production app-server or marketplace behavior. It only changes tests to assert semantic path identity across Windows path spelling variants. It also leaves API response values untouched; the normalization happens inside assertions only.

Verification

Targeted local checks run while extracting this fix:

  • cargo test -p codex-app-server get_conversation_summary_by_thread_id_reads_rollout
  • cargo test -p codex-app-server get_conversation_summary_by_relative_rollout_path_resolves_from_codex_home
  • cargo test -p codex-app-server thread_resume_prefers_path_over_thread_id

Windows-specific confidence comes from the Bazel Windows CI job for this PR, since the failure is platform-specific.

Docs

No docs update is needed because this is test-only infrastructure stabilization.


Stack created with Sapling. Best reviewed with ReviewStack.

@bolinfest bolinfest merged commit d54493b into main Apr 25, 2026
25 checks passed
@bolinfest bolinfest deleted the pr19604 branch April 25, 2026 23:25
@github-actions github-actions Bot locked and limited conversation to collaborators Apr 25, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants