fix(baseline): preserve exact findings and protect baseline output

[chrisknvidia]

Baseline regeneration rejects failed scans before changing output, warns when analysis is partial, and preserves exact active findings and source identities before report compaction. Addresses #656; reuses the active_findings contract from merged #637.

JSON and YAML baselines round-trip Unicode reasons and filenames, validate loader limits before writing, create private new files, and preserve existing ownership, modes and access ACLs. Writable shared files owned by another user use a validated descriptor; this documented fallback is non-atomic for readers and can leave partial writes on I/O failure. Credential-free CLI and MCP transport regressions run in the normal test job.

Validation after integrating current main: 164 focused tests passed, including real CLI scan/baseline/rescan and MCP stdio transport. Native macOS ACL, permission, failure and concurrent-writer checks passed. A separate CLI matrix covers JSON/YAML baselines and four report formats. A real second OS user was unavailable; the non-owner branch was exercised against real files with simulated identity. The earlier broad suite had two concurrent-writer failures; those paths were fixed and the focused concurrency checks rerun. Hosted CI remains the broad-suite gate.

[rng1995]

[SkillSpector Review]

Hi @chrisknvidia, thank you for the thorough baseline fix and for the real CLI, SARIF and MCP round-trip coverage that comes with it!

Value and readiness: The core fix is correct. The report node returns baseline_findings, the sanitized kept findings before deduplication and output limits, and baseline fingerprints those. That is the same list, at the same call site, as #637's active_findings, so it covers #630, #633 and #656. The PR also adds useful protections: pre-write validation against the loader's size and record limits (this closes the gap I noted on #637), refusing to accept a failed scan (consistent with scan exiting 2), a warning for partial scans, atomic replacement that keeps the old baseline on failure, and rejection of symlink and special-file destinations. However, the rewritten writer introduces two regressions that main doesn't have (findings 1 and 2), and the PR mixes in permission policy and unrelated test changes. It is not ready yet. On overlap: this is a functional superset of #637, not a duplicate. It is not superseded and should not be closed. It conflicts with #637 on the same lines in cli.py, report.py and state.py, so whichever merges second needs a rebase.

Material findings

1. [Blocker] src/skillspector/suppression.py:697: json.dumps(..., ensure_ascii=False) emits U+007F and C1 controls (U+0080-U+009F) raw. PyYAML, which both the new pre-write check and load_baseline use, rejects them. Failing input: a scanned skill with a file named notes<U+007F>.md that triggers any finding, or a --reason containing such a character. skillspector baseline <skill> -o baseline.json then stops with ReaderError: unacceptable character #x007f: special characters are not allowed and exits 2. U+0085 is accepted but folded to a space on load, so the stored reason changes. On main, ensure_ascii=True escapes these characters and they round-trip. File names come from the untrusted skill, and _sanitize_finding does not clean file. Expected: keep astral characters raw, but escape [\x7f-\x9f��] as \uXXXX after json.dumps (these characters can only appear inside strings). Extend test_dump_baseline_preserves_unicode_reason with U+007F, U+0086, U+0085, and a file-name case.
2. [Blocker] src/skillspector/suppression.py:747: regenerating a baseline the caller can write but does not own now fails. Scenario: a shared checkout where .skillspector-baseline.yaml is alice:devs 0664 and bob, a member of devs, regenerates it. The writability probe passes. Then os.fchown(..., alice_uid, ...) raises EPERM for a non-root caller, and the command exits 2 with "Operation not permitted". On main, write_text rewrites the file in place. Even if ownership were kept, line 750 narrows 0664 to 0600, which locks the rest of the group out. Expected: keep regeneration working for a writable baseline owned by someone else. For example, only fchown when running as root or when the caller already owns the file, and otherwise write in place after the full validation. Refusing up front with a clear message is an alternative if the PIC accepts the restriction. Add a test for the not-owned case (mocking os.fchown to raise PermissionError is enough).
3. [Non-blocking] src/skillspector/suppression.py:677-678: on macOS, any acl_set_fd_np failure aborts generation. On volumes without ACL support (exFAT/FAT, some SMB/NFS mounts) I'd expect ENOTSUP, which would make every baseline write there exit 2. I couldn't verify this. Please treat ENOTSUP/EOPNOTSUPP as "no ACLs to clear", or confirm the behavior. This ctypes call is the only native FFI in the write path.
4. [Non-blocking] tests/nodes/test_build_context.py:499-500 and tests/unit/test_mcp_registry.py:507: these timing changes have nothing to do with baselines, and they loosen guards. The dense-directory bound goes from 5 s to the 30 s and 60 s stage deadlines, and the FIFO subprocess timeout goes from 5 s to 120 s. Please move them to a separate PR with the CI flake they address.
5. [Non-blocking] src/skillspector/cli.py:3203-3205: if baseline_findings is ever missing, the code quietly falls back to effective_findings(), the incomplete path this PR replaces. If #637 merges first, please rebase onto it, reuse its active_findings key instead of adding a second state key that holds the same list, and drop the fallback. If this PR ends up as the fix that closes them, add Fixes #630 and Fixes #633 to the description.

PIC tradeoffs:

• Owner-only permissions: new baselines are created 0600, and regeneration clears group and other bits. Baselines hold hashes, rule IDs, paths and reasons, not secrets, and the docs tell users to commit them. Consequence: a baseline generated as root in a container on a bind mount can't be read by the host user or by a later non-root CI step, and scan --baseline then exits 2. Main followed the umask (usually 0644). If the goal is integrity against other local users, removing only group and other write access would be enough.
• Failed-scan rejection: baseline now exits 2 when the scan has a fatal ledger exception, such as failed semantic analysis or unreadable primary content. This matches scan, which already exits 2 there, and it fails closed. Users who used to generate baselines despite provider errors will now need to rerun or pass --no-llm. This is worth a release note.
• Symlink destinations are now rejected, and replacement needs a writable parent directory. Rejecting symlinks stops a planted .skillspector-baseline.yaml symlink from overwriting its target, which is a good change. It does break workflows that symlink the baseline to a shared location.
• The new Baseline CLI and MCP Integration Tests job adds a 7th check. Maintainers should decide whether to make it required. The alternative is to keep the round trip in tests/unit, as #637 does, because --no-llm graph runs already execute in test-unit.
• Core-fix choice between #637 (minimal, approved) and this PR: my recommendation is to merge #637 first, then land this PR, rebased, as baseline-writing hardening on top. #631 and #643 should close in favor of #637.

Verification and gaps: I read the full diff: 4 commits on 8bf9c67c, which is an ancestor of current main, and GitHub reports the branch mergeable and clean. I traced report() (sanitized selected_findings -> partition_findings -> baseline_findings). I checked the new failed and partial checks against finalize_ledger (fatal exceptions -> execution_successful) and against scan's existing exit-2 path. I also read dump_baseline's validation, replacement and permission logic, and compared the core change with #637 and #631. I confirmed the PyYAML control-character behavior with a standalone stdlib json + PyYAML snippet, without importing PR code. The fchown and macOS ACL behavior is from reading the code only. All 7 CI checks pass, including the new job. Per policy, I did not run the PR's tests locally.

---

Decision: Changes Requested (reviewed head 806c3899ab616baefaa0cddb0db300b686429b6d)

[rng1995]

[SkillSpector Review]

Hi @chrisknvidia, thank you for the careful baseline hardening and for the real CLI and MCP round trips that came with it!

Value and readiness: Ready once CI passes on the new head. Both blockers from the last round are fixed. While re-checking, I found two small regressions against main and one flaky test. I fixed all three on the branch (details below).

Previous findings (review at 806c389)

• [Blocker] DEL/C1 controls in JSON baselines: Resolved in f879242. I ran a real CLI round trip with notes\x7f.md, notes\x85.md and notes\x86.md, and a --reason containing DEL, NEL, SSA and emoji. JSON and YAML both exit 0, the rescan suppresses all 12 findings, and file names and reasons round-trip exactly.
• [Blocker] Non-owner regeneration (fchown EPERM, 0664 narrowed to 0600): Resolved in f879242/601491f. A non-owner writes through the locked, validated descriptor, so the inode, owner, group and mode are kept.
• [Non-blocking] macOS ACL ENOTSUP: Handled in code (ENOTSUP/EOPNOTSUPP are tolerated) and covered by a simulated test. I couldn't exercise a real exFAT volume here.
• [Non-blocking] Unrelated timing guards: Resolved; they were reverted.
• [Non-blocking] Fallback / active_findings: Resolved. The branch now uses #637's active_findings with no fallback, and no state is duplicated after merging main.
• Tradeoffs from the last review: New baselines are still created 0600. A failed scan exits 2 and leaves the old file intact. Symlink destinations are rejected. The extra CI job is gone; the CLI and MCP tests run in test-unit.

What I changed on the branch (signed off)

• f7c0c86 and ffefeae merge current main, with no conflicts.
• 7724aec U+2028/U+2029 in JSON baselines. PyYAML treats these as line breaks. It dropped the spaces next to them, and 
--- or 
... raised a ScannerError. A skill file named notes
--- x.md made baseline -o c.json exit 2, while main exits 0. JSON output now escapes them along with DEL/C1. A CLI round-trip test covers these names in JSON and YAML.
• 698e1aa Owner outside the file's group. os.fchown(tmp, uid, dest_gid) raised EPERM for a non-root owner who isn't in the file's group. A real macOS repro: a file with group wheel in a staff directory exited 2, while main exits 0. The owner path now falls back to the same validated in-place rewrite, keeping owner, group, mode and ACLs. The docstring and docs/SUPPRESSION.md are updated.
• d0104a8 Concurrent-writer flake. test_dump_baseline_concurrent_writers_publish_complete_documents failed in about 1 in 120 trials with "changed while opening". A cooperating writer's atomic replace is now revalidated from lstat one more time (_BASELINE_DESTINATION_ATTEMPTS = 2). A path that keeps changing still fails closed, and nothing is written to an unvalidated inode. I ran 200 8-writer trials per format twice: 0 failures, against 4 before the change.
• e331a53 restores main's name test_scan_transitive_counts_only_active_post_baseline_findings and renames the misleading has_active_findings parameter.

Non-blocking note: the owner's atomic-replace path breaks hard links to the baseline; the shared in-place path does not. This is worth a line in the docs at some point.

Verification:

• On the integrated tree (current main at merge time):
  • suppression, baseline coverage, baseline MCP and report suites: 321 passed;
  • tests/unit -k "baseline or suppression": 217 passed.
• On e331a53, the full tests/unit passed except one 15-second MCP stdio initialize timeout. That test fails the same way on origin/main on this heavily loaded machine.
• ruff check and ruff format --check are clean.

---

Decision: Ready to merge once CI passes. A human maintainer needs to approve (reviewed head ffefeae3c2c7777d9974a00cb81cf0501e79b336). This bot pushed commits to the branch, so it does not approve the PR itself.