test(sage): run the Code Review Sage suite on Windows too

[aniruddhaadak80]

Problem / Motivation

Code Review Sage's test suite is collected nowhere on Windows: tests/conftest.py sets collect_ignore_glob = ["*"] for the whole directory, so the platform the app is being brought up on has zero automated coverage of it (#4988). The gate predates the app's Windows support work and conflates the app-level refusal in sage_lib/discovery.py's gh_bin() (its review prompts still name python3 — untouched here, tracked separately) with three harness details the tests themselves can express.

Why it matters

The coverage hole sits exactly where recent risk was: a Windows-only silent failure in the review worker had to be found by hand because no Windows shard ran a single sage test. Every PR that touches this app lands blind on the platform it now claims to support.

What changed (motivation → approach → change)

Lift the collection gate and give each platform-dependent test the guard its failure actually needs, instead of one blanket skip:

• Symlink plants (20 of 21 failures, all OSError: [WinError 1314]). The suite already had the right pattern — SYMLINKS_OK, a probe for unprivileged symlink creation, defined privately in test_followup.py. That probe now lives in tests/fixtures.py, the suite's shared module, and every test that stages a planted link skips only where creating a symlink needs a privilege the host does not grant (Developer Mode / elevated runners run them again). The no-follow guards they pin keep running everywhere else.
• Owner-only mode bits. The report-output privacy test asserted 0600 through st_mode, which Windows never reports (the lockdown there is an ACL). That assertion is scoped to POSIX while file-presence and temp-file-cleanup checks still run on every platform; the two existing skipUnless(platform_compat.IS_POSIX) sites are unchanged.
• Collection gate. Removed; the conftest keeps only its SEL-muting fixture.

Alternatives weighed: keeping the blanket gate until #4979 lands (leaves the coverage hole open longer), or making the mode assertions ACL-aware instead of POSIX-gated (no cross-platform "verify owner-only DACL" helper exists yet; that would be a new production-side seam, out of scope for a test-enablement change).

Tests

Test-only change; the diff modifies how tests are gated, not what they assert:

• 17 planted-symlink test sites now carry @unittest.skipUnless(SYMLINKS_OK, ...) with the suite's established reason string.
• test_outputs_are_private_and_leave_no_temp_behind pins its 0600 assertion behind platform_compat.IS_POSIX and keeps presence/cleanup assertions unconditional.
• test_followup.py imports the shared probe instead of defining its own copy (same semantics, one owner).

Manual verification

Measured locally on Windows 11 (Python 3.12, the CI pin set: pytest 9.0.3 / xdist 3.5.0 / pytest-timeout 2.2.0):

run	before	after
serial (-n0)	21 failed / 735 passed / 11 skipped	736 passed / 31 skipped / 0 failed
parallel (-n4 --dist loadgroup)	—	736 passed / 31 skipped / 0 failed

Gates: flake8 clean, isort --check-only clean, mypy clean over the suite's 30 files, git diff --check clean. The five touched files sit in .github/black-baseline.txt; their formatting state is unchanged and the diff adds no new black findings. N/A for browser/UI checks — nothing user-visible moves.

Related Issues

Fixes #4988

Checklist

• Single commit with a Conventional Commits title (test: ...)
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable) — N/A: test-gating only, no documented behaviour changes
• No secrets, credentials, or internal references in the diff

[iamwhatever]

👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated.

Missing sections:

• ## Problem / Motivation
• ## Why it matters
• ## What changed
• ## Tests

Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle.

[bolichen97]

👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated.

Missing sections:

• ## Problem / Motivation
• ## Why it matters
• ## What changed
• ## Tests

Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle.

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed 6d21cdcb8e30e458b5a905277fb11c64ddc75d5a via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 6d21cdc

[github-actions]

Design Review (Fable 5, fork) — ✅ PASS

Design-level review of 6d21cdcb8e30e458b5a905277fb11c64ddc75d5a via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

Blanket collection gate replaced with cause-specific per-test guards — the root-cause shape, using the suite's own established probe and skip patterns, with measured Windows results.

[DESIGN-REVIEWED] 6d21cdc

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed 6d21cdcb8e30e458b5a905277fb11c64ddc75d5a via the fork AI-review pipeline; updated in place on each push.

Review details

I've verified the mechanism. Let me confirm the sibling tests' guard pattern and that test_consolidate.py genuinely lacks any module-level skip.

The candidate is grounded: test_consolidate.py was not modified by this PR, has no skipUnless(SYMLINKS_OK, ...) guard and no module-level skip, yet test_a_symlinked_merge_file_is_refused calls out.symlink_to(secret). With the conftest suite-wide os.name == "nt" gate removed, this test now collects on Windows. There, symlink_to raises OSError (WinError 1314) inside dispatch; that propagates out of asyncio.to_thread, is caught by _consolidate_bg's except Exception as exc (routes.py:1964), the state is set to error, and the function returns. The ruleset stays ["Kept"], nothing leaks, candidate_count() == 1 — every assertion passes vacuously, so the O_NOFOLLOW merge-file guard is never exercised.

This is a real, concrete test-quality regression introduced by the diff (the conftest docstring now claims "Tests that pin inherently POSIX behaviour carry their own skipUnless guards ... unprivileged symlinks" — false for this test). But the harm is false-green test coverage on Windows, not a production security hole, crash, data loss, or corruption. It is advisory, not blocking.

One symlink-planting test was missed by the PR's own skip-guard sweep, so it passes vacuously on Windows instead of skipping.

FINDING — src/kiro_crew/apps/builtins/code_review_sage/tests/test_consolidate.py:294 — test_a_symlinked_merge_file_is_refused has no skipUnless(SYMLINKS_OK, …), so once the conftest Windows gate is removed its out.symlink_to(secret) raises OSError, is swallowed by _consolidate_bg's except Exception, and all assertions pass without ever exercising the O_NOFOLLOW guard → false green coverage on Windows → Fix: import SYMLINKS_OK from .fixtures and decorate the test with @unittest.skipUnless(SYMLINKS_OK, "platform forbids unprivileged symlinks"), matching every other symlink-planting test in this PR.

[OPUS-REVIEWED] 6d21cdc

[iamwhatever]

Tier 1 auto-approve: test (6 files). Criteria: no conflict, no requested changes, security path denylist clean, design-doc gate clean, SAST annotations clean, security checklist all-NO, AI reviewers green. Category: test-harness only -- drops the blanket collect_ignore_glob/skipif Windows gate from the Code Review Sage conftest and replaces it with per-test guards (owner-only mode bits, unprivileged symlink creation), so the suite collects on Windows. No production files touched. CodeQL is not applicable on this fork PR (default-setup emits no check-run); SAST coverage is Semgrep only, latest run success with 0 annotations.