fix(sel): open legacy lock keys in binary mode

[bolichen97]

Problem / Motivation

On Windows, the SEL fallback path may use the legacy HMAC key file itself as the byte-range lock target when the protected trust directory cannot be created. That descriptor was opened with O_RDWR but without O_BINARY.

The Windows CRT therefore treats the binary key as a text file. If a valid 64-byte random key ends in DOS EOF byte 0x1A, opening it for update truncates it to 63 bytes before the lock helper receives the descriptor. The existing random fixture hit this with roughly 1/256 probability, so unrelated PRs could fail the cross-process SEL test.

Why it matters

This is not only a flaky assertion: the test exposed real mutation of the audit log's signing key on a supported fallback path. Retrying the test would leave both CI attribution and key integrity unreliable.

What changed

• Open the SEL chain-lock target with getattr(os, "O_BINARY", 0) in addition to the existing flags. It is a no-op off Windows.
• Make the regression fixture deterministic with a 64-byte key ending in 0x1A, so removing O_BINARY always fails instead of relying on random chance.

No lock timing, retry, timeout, or production write semantics change.

Tests

• Mutation check: removing O_BINARY makes the deterministic regression fail; restoring it passes consistently.
• Focused Windows regression passed before and after the final rebase to current main.
• Full test_sel.py: 231 passed, 25 skipped; the only host-specific failure is the existing symlink-privilege capability case owned by test(windows): list seven symlink-creating tests as requiring real symlinks #6752.
• Four processes × 50 appends: 200/200 chain-valid events, key unchanged across restart.
• Single-process 1000-append stress passed.
• Related SEL/platform-lock tests, isort, Flake8, mypy, and git diff --check passed.
• Full OPEN PR audit found direct overlap only with larger, currently conflicting feat: make SEL rotation and retention limits env-tunable (#4993) #4997; this smaller deterministic integrity fix should land first, then feat: make SEL rotation and retention limits env-tunable (#4993) #4997 can replay on top. test(windows): list seven symlink-creating tests as requiring real symlinks #6752 does not touch these files.

No retry, sleep, timeout increase, or random rerun is introduced.

Screenshots / video

Why no screenshot: This changes a Windows file-open flag and backend regression fixture; it has no rendered UI effect.

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

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

Review details

No findings.
[GPT-REVIEWED] d31e727

[github-actions]

Design Review (Fable 5, fork) — ✅ PASS

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

Both hunks check out against the base: _chain_lock_path (src/kiro_crew/sel.py:809-826) confirms the fallback lock target is the legacy HMAC key file itself, and its docstring's claim that "locking the key file cannot disturb its bytes" is exactly what the missing O_BINARY falsified — this fix restores that invariant at the root cause rather than papering over the flaky test. The pattern also matches the existing O_BINARY usage elsewhere in sel.py (line 2962), and the deterministic 0x1A-tail fixture makes the regression testable without randomness. No scope creep, no contract change, trivially reversible.

Design-Verdict: PASS

Root-cause one-flag fix restoring a documented invariant (key bytes undisturbed by locking), with a deterministic regression pinning it.

[DESIGN-REVIEWED] d31e727

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

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

Review details

The candidate list produced no candidates, and the diff is a purely defensive change: O_BINARY resolves to 0 on POSIX and prevents CRT text-mode translation on Windows for the lock fd, which is only locked and (on the sidecar path) primed with b"\0" — never affecting the key bytes. The test change removes latent nondeterminism (a random 64-byte key could contain 0x1A) while keeping the 64-byte length that satisfies short-key rejection. Nothing introduces a reachable defect.

No findings.

[OPUS-REVIEWED] d31e727

[github-actions]

First Principles Review (Fable 5, fork) — ✅ PASS

Premise-level review of d31e72772d614ed07a27638e22326ba607dab3e8 via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Everything is verified. The fix targets the only O_RDWR open in sel.py, the fallback onto the raw HMAC key file is real (_chain_lock_path, sel.py:809), the getattr(os, "O_BINARY", 0) idiom is the codebase's established spelling (grepped: ~10 sites already use it), and the only other lock file (.rotate.lock) holds no data so it has no truncation sibling. No new surface, no consumers to count, no riders.

First-Principles-Verdict: PASS

Two-line fix at the mechanism level: the open that corrupted the signing key gains the flag ten other call sites already use.

What this change ships

Intent: stop Windows from truncating the SEL audit log's HMAC signing key when the chain lock falls back onto the key file itself — a FIX.

1. The chain-lock open no longer strips a trailing 0x1A from the legacy key on Windows — justified
2. The regression test's key is deterministic (k×63 + 0x1A) instead of random — justified, pins the fix

The fix sits at mechanism level: the open that mutated the key is the open that changed. Sibling count is zero — sel.py's only other O_RDWR lock target is this one; .rotate.lock (sel.py:1514) and secrets/migrate.py:355 lock dedicated files whose bytes carry no data, so text mode cannot harm them, and the codebase's other binary opens (grepped O_BINARY, ~10 sites) already carry the flag. The deeper choice — using the raw key file as a lock target at all — predates this change and is a documented, deliberate fallback (sel.py:748–750); reversing it is out of this fix's scope. No undeclared items, no new public surface, and the fix reuses the existing getattr(os, "O_BINARY", 0) spelling rather than inventing a second one.

[FIRST-PRINCIPLES-REVIEWED] d31e727

[buluoray]

Root-cause, correctly-scoped fix: the SEL chain lock's Windows fallback opened the raw HMAC key file O_RDWR without O_BINARY, and CRT text mode truncated a key ending in 0x1A at open time — mutating the audit-log signing key, not merely flaking a test. Adding getattr(os, "O_BINARY", 0) restores the "locking the key cannot disturb its bytes" invariant, and the deterministic 0x1A-tail fixture pins it instead of leaving a ~1/256 flake.

Verified

• sel.py:734 is the only O_RDWR open in sel.py, so the fix targets exactly the corrupting call site.
• The fallback lock target really is self._hmac_key_file (_chain_lock_path, when the trust subdir is uncreatable), and this fd is never written on that path — the priming write is guarded by lock_path != self._hmac_key_file. Open-time text-mode truncation was therefore the sole corruption vector, which O_BINARY closes.
• Other os.open sites are unaffected: the read side already carries O_BINARY, the dir open is O_DIRECTORY, and the log append is intentional UTF-8 text on a non-key file.
• Fixture correctness: with _HMAC_KEY_MIN_BYTES=32, a 64→63-byte truncation changes the key bytes without tripping the length floor, so legacy.read_bytes() == log._hmac_key is what reddens on Windows if the flag is removed.
• Focused tests pass locally (2 passed). A local mutation on POSIX is inconclusive by design (O_BINARY == 0 there), so the falsification evidence is the deterministic fixture plus the four green Windows shards at this head — noting that explicitly so it does not read as a coverage gap.

Blue

• The log-append handle (sel.py:1119) wraps its fd in a text-mode fdopen("a"), so Windows applies newline translation. Pre-existing and out of scope here, but it is the next place to look if byte-exact cross-platform log output is ever required.