Windows full-test compatibility and POSIX capability markers (#173) - #184
Merged
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Introduces the first Windows CI compatibility slice and shared platform-test utilities.
Changes:
- Adds a Windows Python 3.12 full-test job.
- Adds POSIX/Windows capability helpers and action pin validation.
- Adds tests and implementation notes for the new CI invariants.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
.github/workflows/ci.yml |
Adds the Windows test job. |
tests/platforms.py |
Defines shared platform and workflow helpers. |
tests/test_platforms.py |
Tests helpers and Windows CI structure. |
.superpowers/sdd/task-1-report.md |
Records implementation and verification evidence. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ask 3) Preserve fail-closed, atomic-write, fsync, and chmod-request invariants on Windows while acknowledging NTFS/Python POSIX-mode limitations: - test_audit: Accept cross-platform OSError family for bad-path constructor laziness; split fsync count assertion (POSIX ≥2, Windows ≥1); mark directory-fsync-failure test posix_only with adjacent file-fsync test. - test_config: Mode assertion uses spy on Windows to verify os.chmod(0o600) was requested; POSIX continues to assert stat result. - test_logexport: Private-mode test verifies stat on POSIX, file existence on Windows (NTFS ignores POSIX bits in stat). - test_server: Endpoint mode tests branch on os.name; Windows verifies atomic creation and valid content instead of stat mode bits. - test_token_store: Mode test asserts stat on POSIX, file existence + valid JSON content on Windows. All tests document the NTFS/Python POSIX-mode limitation explicitly in docstrings. No production code changes needed — the code already passes 0o600 at creation which is the strongest portable guarantee. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… clarify docs - Remove duplicate test_append_fails_closed_when_file_fsync_fails; add docstring to existing test_append_fails_closed_when_fsync_fails explaining it is the cross-platform (Windows-inclusive) counterpart to the adjacent posix_only directory-sync test. - Replace inline os.name != 'nt' guards with shared POSIX constant from tests.platforms in test_config, test_logexport, test_server, test_token_store. - Clarify MCP 'not merely chmodded' docstring: Windows branch explicitly states NTFS uses ACLs not mode bits and no ACL confidentiality is claimed. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…view) Critical product fix: - _run_write / _run_write_inner / _run_write_shielded now accept Callable[[], Awaitable[None]] instead of Awaitable[None]. - Factory is invoked only after intent audit succeeds; a blocked or cancelled write never creates the mutation coroutine. - Removed the fragile close() hack for leaked coroutines — the factory design eliminates the problem at the source. - All call sites (confirm worker, operator install, subscription uninstall, agent write shield, proposal execution) converted to pass lambda/factory. Related fixes: - Replace Path.home staticmethod monkeypatches with portable HOME + USERPROFILE env setup; test_local_path_tilde_expanded also sets USERPROFILE for Windows. - Path picker: rstrip(os.sep) before appending os.sep avoids // on root. - __main__.py crash restart prompt: em dash replaced with ASCII --. Tests: - test_blocked_audit_never_invokes_op_factory: factory never called when intent audit fails (directory at log path). - test_cancelled_before_factory_leaks_no_coroutine: threading-gated audit, cancel during to_thread, warnings-as-errors — factory never called, no unawaited coroutine. - test_busy_bird: updated to pass factory not pre-created coroutine. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ease (#173 Task 4) - test_write_ops: replace unbounded while/sleep loop with until(pilot, entered.is_set) for the gated-audit cancellation test. - test_write_confirm_characterization: update group (a) prose to reflect factory-based _run_write (coroutine constructed post-audit, not inside confirmed branch). - test_proposals_ui: remove inert rec.release.set() lines — factory design + _settle_interrupted_execution.write.cancel() properly unwinds the coroutine without needing the gate release. - task-4-report: mark #6 proposals root cause as superseded by #7 factory. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ase (#173 Task 4) Wrap the audit_gate.set() in a finally block so the executor thread cannot hang at process exit if until() times out or the test fails before reaching the release. Drop the catch_warnings/simplefilter framing — the factory_calls invariant is what proves no coroutine was created; warnings-as-errors was overclaiming. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…173 Task 4) Product fix: - on_external_proposals_changed: suppress NoMatches during teardown when StatusBar is already unmounted (same pattern used elsewhere in app.py). Normal errors still propagate. Test fixes: - test_c_cordons_node_after_approval: poll until success audit record is written (to_thread may still be in-flight after rec.calls lands on slower Windows). - test_local_dir_pick_appends_remote_basename_verbatim: use str(tmp_path / 'report ') not hardcoded '/'; product Path join is correct and produces native separators. Regression test: - test_proposals_changed_during_teardown_does_not_raise: posts ExternalProposalsChanged after run_test exits (widget tree torn down); proves the guard suppresses NoMatches without swallowing normal errors. Note: Linux 3.11 failure in the same CI workflow had no available log blob — not addressed without evidence. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…er (#173 Task 4) Replace vacuous post-after-exit test with one that runs inside run_test: removes the StatusBar widget to simulate teardown state while the message pump is active, then calls the handler directly. A spy on _refresh_status proves the handler entered it (NoMatches suppressed inside _refresh_status, not silently skipped). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…#173 Task 4) Product fix: - _refresh_status: catch NoMatches from missing StatusBar internally (teardown race). This is the single control point for 20+ callers including MCP _switch worker, proposals handler, navigation — none need individual guards. Removed the now-redundant handler-level suppress from on_external_proposals_changed. Test fixes: - test_review_approve_executes_with_the_bound_uid: poll store state == executed inside run_test with until() before exiting; the worker may still be marking the proposal as executed after rec.calls lands. - test_refresh_status_tolerates_missing_status_bar: removes StatusBar while app is live, calls _refresh_status directly — proves the guard works for any caller path (MCP switch, navigation, etc.) and that normal refresh still works with the bar present. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ty (#173 Task 4) Replace remaining em dash in the crash-cap stderr diagnostic (line ~1135) with ASCII '--' so cp1252 terminals cannot raise UnicodeEncodeError. Regression: test_crash_cap_message_is_ascii_encodable exercises the cap path and encodes the captured stderr as ASCII — any non-ASCII character (em dash, arrow, etc.) would fail the encode. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 32 out of 32 changed files in this pull request and generated no new comments.
Suppressed comments (3)
docs/windows.md:30
- This still leaves the issue's clean-Windows acceptance criterion unmet. The unchanged
test_pack_dereferences_symlink_sourcecallsPath.symlink_to()directly, so a normal Windows account without Developer Mode/elevation raises WinError 1314 instead of producing the documented pass/skip result. Add capability-aware handling for that test (for example, skip only when symlink creation is unavailable) and include it in the expected-skip contract.
- Symlink tests depend on Windows **Developer Mode** (or an elevated shell):
native `Path.symlink_to()` can fail before korvid logic runs if symlink
creation is not allowed.
tests/test_platforms.py:144
- These values pin an older run even though the PR's final verification is run 30936032385 at the current head, whose Windows job reports 3376 passed and 37 skipped. As written, this invariant forces the contributor documentation to retain stale verification evidence; update both snippets together with
docs/windows.md.
"30930727214",
"3373 passed / 37 skipped / 0 failures",
docs/windows.md:13
- This cites an earlier commit as the proving run, while the PR description identifies final verification run 30936032385 and that run's Windows log reports 3376 passed / 37 skipped. Point the support claim at the successful current-head run and update the structural test that currently requires these old values.
This issue also appears on line 28 of the same file.
`windows-test` CI job green. The proving run for issue #173 was
`30930727214`: **3373 passed / 37 skipped / 0 failures**.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Owner
Author
|
First-review suppressed findings addressed in aa8c41d: capability-aware symlink handling skips only when Windows returns privilege-unavailable (WinError 1314/EPERM), docs and structural tests now cite final run 30936032385 (3376 passed / 37 skipped / 0 failures), and accidentally tracked SDD reports were removed. Independent local review approved. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated no new comments.
Suppressed comments (1)
docs/windows.md:24
- This breakdown counts the newly added POSIX-only directory-fsync test as “pre-existing.” Before this PR there were three audit mode skips;
tests/core/test_audit.py:255adds the fourth audit skip, so the accurate split is 3 newly classified capability skips and 13 pre-existing ones (still 16 total).
- **14 pre-existing platform skips** for POSIX-only permission semantics:
7 local transfer permission-bit cases, 4 audit-log mode/directory-fsync
cases, 2 transfer-stream late-permission-loss cases, and 1 unreadable CA
bundle permission case.
Transfer tests asserted audit entries after run_test exited, but the threaded audit append (to_thread) may still be in-flight. Fix by polling with until() for the expected outcome record before leaving run_test: - test_download_writes_file_and_audits: poll for 'success' audit - test_upload_requires_approval_then_transfers: same - test_progress_screen_escape_cancels_transfer: poll for 'cancelled' - test_download_failure_notifies_and_audits_error: poll for 'error' No sleeps; audit semantics unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The verified total remains 16 capability skips: 3 newly classified (~user x2 plus directory fsync) and 13 pre-existing platform skips. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Owner
Author
|
Second-review suppressed count finding fixed in f1e871a: the verified 16 capability skips are documented as 3 newly classified ( |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 29 out of 29 changed files in this pull request and generated no new comments.
Suppressed comments (1)
tests/core/test_config.py:604
- The new suppression omits the required rationale (
# type: ignore[code] # reason); the analogous spy at this file's line 666 includes one. Since this spy is only called with thePathpassed by_atomic_write_text, narrowing its signature removes the suppression entirely.
real_chmod(path, mode, *args, **kw) # type: ignore[arg-type]
Remove the unnecessary type suppression and match the exact two-argument call made by the atomic config writer. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Owner
Author
|
Review-loop limit reached: rounds 2 and 3 contained only suppressed advisory findings and no unresolved blocking threads. The final credible instruction issue was fixed by narrowing the chmod spy signature (no type suppression). Fresh full gate after the fix: 3397 passed, 21 skipped; ruff/mypy/tach green. Per AGENTS.md §8–9 no further Copilot review is requested; proceeding to required-check verification. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #173
Result
A required Windows Python 3.12 job now runs the complete pytest suite on
every PR. Final verification on run
30936032385:The existing Linux matrix and coverage command are unchanged.
What changed
Explicit capability markers
tests.platformsshared helpers (WINDOWS,POSIX,posix_only(reason)).~useraccount lookup./tmppaths and mocked shell/approval/audit flows remain testedon Windows.
Security invariants remain cross-platform
POSIX additionally verifies directory/parent fsync.
writes and owner-only
0o600creation requests.Python/NTFS mode emulation does not prove ACL confidentiality.
0o600requestinstead of preserving meaningless emulated
0o666.Product portability fixes
newline="") instead of CRLF translation.pathliband native separators; remote paths stayPOSIX.
HOMEandUSERPROFILE.Write cancellation correctness
The Windows run exposed a real unawaited-coroutine race: mutation coroutines
were created before the audit-intent write and could be discarded unstarted
if cancellation landed during audit I/O.
_run_write, shielded agent writes, and proposal writes now receive anoperation factory.
Documentation
New
docs/windows.mddocuments setup, CI guarantees, expected skips,Developer Mode/symlink and terminal limitations, and the NTFS ACL boundary.