feat: make SEL rotation and retention limits env-tunable (#4993)

[patrigao]

Problem / Motivation

SEL segment rotation limits are hardcoded source constants (_SEGMENT_MAX_BYTES = 32 MiB, _SEGMENT_KEEP = 7), fixing the audit-log disk ceiling at ~256 MiB for every install with no runtime override (zero environ/getenv reads in sel.py). This bites in both directions: a small-disk or container install cannot lower the ceiling, and a compliance-retention install cannot raise the keep-count to match _RETENTION_DAYS = 365 — the count bound can silently discard history well inside the stated retention window.

Why it matters

The audit log is the tamper-evident record of every tool invocation and permission decision. An operator who cannot size it for their disk either loses the machine to log growth (container installs) or loses audit history their retention policy requires them to keep (compliance installs). Both are operational failures of the one subsystem whose whole job is being reliably present.

What changed (motivation → approach → change)

Goal: operator tunability without a config-package integration (the security.* config section was previously ruled out to avoid an import cycle; environment variables sidestep that entirely, and the issue explicitly scoped the request that way).

Approach: three environment variables read once at SecurityEventLog construction — KIROCREW_SEL_MAX_BYTES (default 33554432), KIROCREW_SEL_KEEP (default 7), KIROCREW_SEL_RETENTION_DAYS (default 365) — via a module-level, fail-soft _env_int() helper. Fail-soft because an audit logger that refuses to start converts a typo into lost audit coverage. The fallback/clamp warnings name the variable only, never the rejected value: a mis-pasted deployment value can contain a secret, and the SEL process log is itself a small exfiltration surface. Zero/negative values clamp to the default — treating 0 as never-rotate would remove the disk bound this subsystem exists to enforce.

Change: the module constants stay the single source of the defaults; the rotation gate, the rotation size re-check, the segment count-retention sweep, and prune()'s default window now read instance attributes. The overrides are raise-only and bounded (the erasure hazard and the enumeration bound were both found by pre-push/CI model review, see Review dispositions below):

• All three variables are raise-only — each floor is its compiled default. The audited agent controls the environment of child processes it spawns, so a lowerable limit would let it have the trusted SEL writer erase shared audit history as ordinary rotation/retention housekeeping (segment deletion leaves a clean chain by design — erasure without a forensic marker). Raising is monotonically non-destructive: rotation happens later, the sweep deletes no sooner, prune keeps more. Lowering attempts clamp up with a warning. This also subsumes the mis-scaled-value hazard (32 intended as MiB, read as bytes, rotating per append on the hot path).
• KIROCREW_SEL_KEEP is additionally capped at 4092 (_SEGMENT_SCAN_CAP - 4) — segment enumeration examines at most 4096 directory entries, so a keep-count the cap cannot admit would make count retention silently stop; −4 reserves the transient segment, the rotation lock file, the retention-floor marker, and the floor lock file.
• The two deletion bounds (KEEP, RETENTION_DAYS) are additionally process-shared — raise-only per process is not enough on a shared directory, because a child process with a scrubbed environment would enforce the compiled defaults and delete segments a sibling's raised policy protects. They are backed by a high-water marker (security_events.d/retention_floor.json) that is monotone by construction: the single write site re-reads the stored floor and writes max(stored, requested) per field under the existing cross-process rotation lock — the same mutex every deletion sweep holds — acquired with a non-blocking try: a contended stamp defers outright and retries at the next construction, rotation, or sweep, so nothing on the construction path can ever block and a deferred stamp never degrades to an unlocked write. Concurrent processes raised via different variables therefore can never publish a lower field over each other's raise. One narrow window is deliberately accepted rather than closed with a blocking acquire: a contention-deferred first stamp is not yet visible to siblings until its retry lands, and the raised process itself never deletes below its own bounds. Construction takes max(own, persisted) and eagerly creates the fenced segment directory when raised above the compiled defaults (so the stamp lands before any sibling can prune); both deletion sites re-read the marker before deleting (_enforce_segment_retention_locked, prune()'s default path), covering processes constructed before the raise, and prune() runs its floor refresh and both delete phases under that same rotation lock — when the lock is unusable on a degraded filesystem the sweep is skipped entirely, deferring deletion (safe direction; the next healthy sweep catches up). Reads are nofollow-fenced (a pre-planted link at the directory, marker, or lock path is never trusted or followed) and fd-fenced: only a regular, single-link file within a 4 KiB cap is trusted, opened non-blocking, so a planted FIFO, hard-linked alias, or oversized decoy reads as no floor and cannot stall an event-loop-thread construction. Max-bytes needs no marker — deletion is governed solely by keep/retention, so a divergent smaller size cap only rotates earlier, never deletes more.
• prune() guards its cutoff computation against OverflowError — a retention window past the representable date range means "keep everything", so the daily sweep prunes nothing (with a warning) instead of failing.

Scope consequence: of the issue's two directions, this PR serves the widen direction (compliance retention) fully. The narrow direction (small-disk installs lowering the ceiling) is deliberately not servable via environment variables at all — any lowerable env knob reintroduces the audit-erasure channel above. Narrowing today means editing the source constants (an operator-authenticated act); a config-file key would be the follow-up shape if wanted, and needs the known import cycle resolved first.

Docs updated in the same commit per the spec-management rule: docs/system-specs/modules/sel.md (bounds, fail-soft contract, per-process semantics) and the operator-facing env-var table in docs/guides/install.md.

Review dispositions (pre-push dual model review gpt-5.6-sol + claude-opus-5 verified by a focused checker, plus CI review rounds):

• Fixed (CI GPT round 1, blocking): agent-controlled child environments could erase protected audit history — KIROCREW_SEL_KEEP=1 plus a small MAX_BYTES on any child CLI had the trusted writer rotate the shared log early and unlink protected segments, with no forensic marker → all three overrides made raise-only (floor = compiled default). Regression test reproduces the exact PoC env and asserts every lowering attempt clamps up with a visible warning. The demanded full revert was disproportional: the erasure channel exists only for lowering, and raise-only preserves the issue's compliance use case.
• Fixed (CI GPT round 2, blocking): a scrubbed-env child process on a shared log directory enforced the compiled deletion bounds and could delete segments the operator's raised policy protected → the two deletion bounds moved to the process-shared monotone high-water marker described above (exactly the demanded "operator-authenticated, process-shared source": the marker lives inside the SEL sensitive-path floor, is monotone under a cross-process lock, and is re-read at every deletion decision). Five regression tests below, including the review's PoC verbatim and its reverse ordering.
• Fixed (pre-push local review round 4, GPT + Opus mirrors): the blind marker write let two concurrently constructing processes raised via different variables clobber each other's raise (last replace could lower a field) → per-field read-max-write under a cross-process lock; a pre-planted marker/dir/lock link could suppress the operator's stamp by feeding fake high values → nofollow fences on all three reads; the marker consumed the scan-cap reserve → _SEGMENT_KEEP_MAX reserve widened.
• Fixed (CI GPT round 3, blocking ×2 — restructure round per the same-span rule): the round-4 dedicated floor lock could block an event-loop-thread construction on another process's rotation, and its contention path fell open to an unlocked racy write → the dedicated lock was removed entirely: stamps take the existing rotation lock with a non-blocking try and defer on contention (regression test locks both behaviors: no write while contended, write after release), prune() fences refresh-and-delete under that same lock, and the scan-cap reserve reverted to -3 (keep cap 4093).
• Fixed (pre-push local gate on the restructure): Opus PASS + 2 advisories (accepted: the doc's "regardless of construction order" overclaim softened to the explicit accepted-window statement; the prune full-skip availability delta documented). GPT raised 4: the marker read could block on a planted FIFO or bloat on an oversized file, and a hard-linked marker stayed externally mutable → fd-fenced read (O_NONBLOCK|O_NOFOLLOW open, fstat gate: regular file, st_nlink == 1, 4 KiB cap; three regression tests). GPT's other two rebutted on the PR: extending the floor to max-bytes conflates rotation cadence with deletion bounds, and "defer construction until the floor is durably visible" contradicts the round-3 no-blocking invariant and the module's fail-soft construction contract — the window is documented as deliberately accepted instead.
• Fixed: keep-count above the enumeration scan cap disabling count retention (both reviewers) → _SEGMENT_KEEP_MAX clamp.
• Fixed, then subsumed: sub-event size cap causing per-append rotation → initially a 64 KiB floor; now subsumed by the raise-only floor (32 MiB default), and _SEGMENT_MIN_BYTES removed.
• Fixed: huge retention window overflowing timedelta in the daily prune → OverflowError guard returning 0.
• Fixed: brittle test assertion (redaction check could collide with the interpolated default's digits) → assertion scoped before the using the default suffix.
• Fixed: the three variables were documented only in the module spec → added to the install.md environment-variable table.
• Rebutted for max-bytes, superseded for the deletion bounds: "a gateway and a CLI sharing one log directory can enforce different limits when only one has the variable set." For the two deletion bounds this round-2 concern is now fixed by the process-shared marker above, not rebutted. For max-bytes the per-process semantics stand as documented: a divergent smaller size cap only rotates earlier — it can never delete more — so no shared state is warranted for it. sel.md states the per-process semantics and the marker's guarantees.

Note for maintainers: issue #4992 is being fixed concurrently in the same file (fix/sel-nofollow-open-4992); this change is confined to the constants region, constructor, rotation/prune sites, and the SEL test module to minimize the conflict surface. Whichever PR lands second must rebase onto main and re-run gates.

Tests

All in test/test_sel.py::TestEnvOverrides (plus one adapted existing test):

• test_defaults_unchanged_when_vars_unset — behavior at defaults is byte-identical to the constants.
• test_lowering_attempts_clamp_to_the_compiled_defaults — the review PoC (MAX_BYTES=65536 KEEP=1 RETENTION_DAYS=1): all three clamp up to the defaults with visible warnings.
• test_max_bytes_raise_reaches_the_rotation_gate — an env raise governs the actual rotation trigger (proven against a patched-down default so the raise is genuine).
• test_keep_raise_controls_retained_segment_count — a raised keep-count retains exactly that many closed segments.
• test_retention_days_raise_reaches_prune — prune() with no args picks up the widened window (an entry inside the raise but outside the default survives).
• test_explicit_keep_days_still_wins_over_the_override — an explicit argument beats the env var.
• test_malformed_values_fall_back_with_a_redacting_warning (parametrized: non-integer, 0, -5, 1.5) — default kept, warning present, rejected value never echoed.
• test_each_variable_falls_back_independently — one bad variable does not poison the others.
• test_keep_count_clamps_below_the_scan_cap — an over-cap keep clamps below the enumeration scan cap with a warning.
• test_scrubbed_env_child_cannot_lower_a_raised_deletion_floor — the round-2 PoC verbatim: operator raises KEEP=100/RETENTION=400, a scrubbed-env process on the same directory inherits both from the marker.
• test_a_child_constructed_before_the_raise_prunes_with_the_raised_floor — the reverse ordering: a process constructed before the raise picks the floor up at its next sweep (deletion-time re-read).
• test_the_floor_merge_is_per_field_monotone — differently-raised concurrent writers cannot clobber each other's field; a no-raise persist never rewrites the marker.
• test_a_planted_marker_link_is_not_trusted_and_is_replaced — a pre-planted marker symlink is ignored on read (no value inflation, no stamp suppression) and replaced by a real file; the link target is untouched.
• test_a_damaged_floor_marker_fails_soft — a corrupt marker never stops construction; defaults apply.
• test_a_contended_stamp_defers_instead_of_writing — the round-3 contract: while another process holds the rotation lock the stamp returns without touching the marker (no fail-open), and lands after release.
• test_a_planted_fifo_marker_neither_blocks_nor_is_trusted — a FIFO at the marker path returns no floor promptly (thread-bounded; a blocking open would hang it).
• test_an_oversized_marker_decoy_is_ignored — a marker past the 4 KiB cap is rejected instead of parsed.
• test_a_hard_linked_marker_is_not_trusted — st_nlink != 1 reads as no floor.
• test_a_retention_window_past_the_date_range_prunes_nothing — 10⁹ days returns 0 instead of raising.
• Adapted: the retention-stops-at-undeletable-segment test now tightens the keep-count on the instance (read-once semantics).

The root conftest.py host floor clears the three variables suite-wide so a developer-exported override cannot move the limits the SEL tests assert against.

Manual verification

N/A — unit coverage sufficient: the overrides are pure construction-time reads exercised end-to-end (rotation, retention, prune) by the tests above; there is no UI or external-service path.

Screenshots / video

Why no screenshot: backend-only change (audit-log rotation limits); no rendered surface is touched.

Related Issues

Closes #4993

Checklist

• Single commit with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (if applicable)
• No secrets, credentials, or internal references in the diff

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 7e3ee6e9bbaa035d09e94c00a3bd9af27eb5e39e — 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.

First-Principles-Verdict: CONCERNS

KIROCREW_SEL_MAX_BYTES ships with its only stated motivation (lowering for small disks) unservable by design, and sel.py:248 still claims lowering works.

Not justified as shipped

• Item 3 — inherited: every stated motivation for a max-bytes knob is the narrow direction ("a small-disk or container install cannot lower the ceiling"), which raise-only forecloses; no harm removed by raising it is named anywhere.
• Item 8 — symptom-level: the overflow is caught where it blew up instead of bounding the input; _env_int already takes maximum= (used by KEEP one call above, sel.py:799).

What this change ships

Intent: let an operator size the SEL audit log's rotation and retention for their disk or compliance window — an ADDITION.

1. KIROCREW_SEL_KEEP widens retained segment count (raise-only, capped) — justified
2. KIROCREW_SEL_RETENTION_DAYS widens the prune window (raise-only) — justified
3. KIROCREW_SEL_MAX_BYTES raises segment size before rotation — inherited: stated motivation is the unservable lowering case
4. New persisted file security_events.d/retention_floor.json, a shared monotone deletion floor — justified
5. Both deletion sweeps re-read that floor before deleting — justified
6. prune()'s default window is now the instance/env value (sole no-arg caller: heartbeat.py:221) — justified
7. A lock-unavailable prune() now skips the whole sweep (was: segment sweep ran anyway) — justified
8. An over-wide retention window prunes nothing with a warning instead of crashing the daily sweep — symptom-level
9. Test isolation clears the three vars in conftest.py — rides along
10. Docs rows in install.md + sel.md rewrite — justified

Watch

• Item 3 is permanent operator surface (documented in install.md) whose derivable use — retention past the KEEP cap, 4093 × 32 MiB ≈ 128 GiB — appears nowhere in the PR; the shipped comment asserts the opposite capability. The description also describes an earlier revision (cap "4092", "_SEGMENT_SCAN_CAP - 4", a "floor lock file"); shipped is _SEGMENT_SCAN_CAP - 3 = 4093 (sel.py:308) with no floor lock file.
  Clears when: a named case needing >keep-cap retention is cited, or the variable is dropped.

Subtractions

• Delete the clause "a small-disk or container install can lower the size ceiling" at src/kiro_crew/sel.py:248-249 — minimum=_SEGMENT_MAX_BYTES (sel.py:796) makes it false.
• Defer KIROCREW_SEL_MAX_BYTES (env read sel.py:796, install.md row): ship only the two deletion bounds, which fully serve the issue's servable direction.
• Replace prune()'s OverflowError branch (sel.py:~3060) with maximum= on the RETENTION_DAYS read plus the same cap in _refresh_deletion_floor — the overflow becomes unreachable.

[FIRST-PRINCIPLES-REVIEWED] 7e3ee6e

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

Design-level review of 7e3ee6e9bbaa035d09e94c00a3bd9af27eb5e39e — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: CONCERNS

Raise-only is the right call; but the floor marker lives in a sandbox-VISIBLE directory, so its anti-erasure guarantee is tool-gate-only, weaker than documented.

Watch

• The marker's core guarantee is overstated. security_events.d is in _CREW_SANDBOX_VISIBLE_LEAVES (src/kiro_crew/sandbox.py:349) — no OS fence in any mode — yet sel.md claims "the audited agent's file tools cannot lower it." A spawned shell's unlink() never routes through the tool gate (the keystone doctrine's own point), so the agent can delete retention_floor.json, then spawn a scrubbed-env sibling that prunes with compiled defaults — exactly the round-2 erasure channel the marker exists to close. The threat model is also internally inconsistent: the FIFO/hardlink/decoy fences presume an attacker who can write this directory, and that attacker can also delete. This mirrors the carried sel_hmac.key residual, so it is CONCERNS not BLOCK — but it must be named, not claimed closed.
  Clears when: sel.md documents this as an accepted residual (like the sel_hmac.key note in AGENTS.md), or the marker moves to an OS-fenced disposition (e.g. a READONLY leaf stamped only by the unsandboxed gateway).
• Monotone-forever with no walk-back story: a fat-fingered raise (KEEP=4000 ≈ 128 GiB ceiling) is committed permanently — unsetting the variable never lowers the floor, and the one remedy (operator deletes the marker by hand) appears in no doc.
  Clears when: install.md or sel.md states the operator-level lowering procedure and its audit implications.

Suggestions

• Delete the stale constants-region comment in sel.py ("a small-disk or container install can lower the size ceiling") — it contradicts the raise-only bound beside it and invites a future maintainer to "restore" lowering.

[DESIGN-REVIEWED] 7e3ee6e

[github-actions]

GPT 5.6 Review — 🔴 changes requested (blocking)

GPT 5.6 found at least one blocking issue that must be resolved before merging 7e3ee6e9bbaa035d09e94c00a3bd9af27eb5e39e. 1 of 1 blocking finding(s) are security-class and were withheld from adjudication, so the blocking verdict stands.

This comment is updated in place on each push.

BLOCKING -- src/kiro_crew/sel.py:810 -- Retention floor trusts a sandbox-writable marker
floor_keep, floor_days = _read_retention_floor(self._segment_dir)
Agent shell deletes the visible marker -> scrubbed-env SEL process restores defaults -> rotation/prune deletes operator-protected segments.
Anchor: residual/security
Fix: Store the floor in an OS-protected, gateway-owned location and expose it read-only to sandboxed processes.
[BLOCK-MERGE] 7e3ee6e
[GPT-REVIEWED] 7e3ee6e

Adjudication (Opus 4.8) — is blocking on each finding proportionate?

The adjudicable block is empty; F1 is fenced (annotate-only).

F1 — Harm rung: UNBOUNDED (destruction of tamper-evident audit history / operator-set retention ceiling). The floor marker's entire defense rests on the claim (sel.py:316-317) that the segment directory "sits on the SEL sensitive-path floor, so the audited agent's file tools cannot lower the marker." But that only fences the file-tool gate. At the OS sandbox layer security_events.d is deliberately VISIBLE — sandbox.py:349 lists it in _CREW_SANDBOX_VISIBLE_LEAVES because in-sandbox MCP servers must append to the log — so it is neither HIDDEN nor READONLY. A spawned command/cron/MCP unlink() therefore reaches retention_floor.json directly (the exact tool-gate-bypass the keystone note describes). With the marker gone, _read_retention_floor fail-softs to (0,0) (sel.py:330-332, 386), a scrubbed-env construction (sel.py:810) reverts to compiled defaults, and the next rotation/prune deletes operator-protected segments irreversibly — no recovery path exists once segments are pruned. This is precisely the scrubbed-env sibling attack the marker was built to stop, and raising the retention bound is a normal operator action this very PR enables, so the condition combination is plausible, not extreme. I cannot complete a rarity/recovery record that would justify FLAG.

[ADJUDICATION] 7e3ee6e total=0 uphold=0 downgrade=0
[GPT-ADJUDICATED] 7e3ee6e
[ADJUDICATION-FENCED] 7e3ee6e fenced=1 flagged=0
UPHOLD-FENCED F1 src/kiro_crew/sel.py:810 -- security_events.d is OS-VISIBLE (sandbox.py:349), so a spawned command can delete the floor marker, reverting scrubbed-env pruning to defaults and irreversibly destroying operator-retained audit segments — the exact attack the marker claims to prevent.
[GPT-ADJUDICATED-FENCED] 7e3ee6e

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 7e3ee6e9bbaa035d09e94c00a3bd9af27eb5e39e: <one-sentence reason>

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 7e3ee6e9bbaa035d09e94c00a3bd9af27eb5e39e — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 7e3ee6e

Verdict parsed from the review's SHA-scoped output markers for commit 7e3ee6e9bbaa035d09e94c00a3bd9af27eb5e39e.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 7e3ee6e9bbaa035d09e94c00a3bd9af27eb5e39e: <one-sentence reason>

[patrigao]

Disposition: GPT blocking finding on 7a1091f18

Finding (span ff91a2389e17): Agent-controlled child environments can erase protected audit history — KIROCREW_SEL_MAX_BYTES=65536 KIROCREW_SEL_KEEP=1 kirocrew <any-cli> makes the child's trusted SEL writer rotate the shared log early and unlink protected segments, with no forensic marker (retention deletion leaves a clean chain by design).

Verdict: LEGITIMATE — FIXED in c71355fc7 (demanded fix narrowed: revert was disproportional; the erasure channel only exists for lowering).

The overrides are now raise-only: each variable's floor is its compiled default (KIROCREW_SEL_MAX_BYTES ≥ 32 MiB, KIROCREW_SEL_KEEP ≥ 7 and ≤ 4094, KIROCREW_SEL_RETENTION_DAYS ≥ 365). Raising is monotonically non-destructive — rotation happens later, the sweep deletes no sooner, prune keeps more — so the compliance use case from #4993 (widen retention) is fully served while the environment can never narrow the shared log's protection. Lowering attempts clamp up with a warning. Narrowing now requires editing the source constants, an operator-authenticated act a child env cannot reach.

The prior 64 KiB floor is subsumed and removed. Regression test added for the exact PoC (test_lowering_attempts_clamp_to_the_compiled_defaults sets MAX_BYTES=65536 KEEP=1 RETENTION_DAYS=1 and asserts all three clamp to defaults with visible warnings); the raise-path tests prove env values still reach the rotation gate, the count sweep, and prune().

Deferred (documented in the PR body): the issue's small-disk lower-ceiling use case cannot be safely served via env vars at all — any lowerable env knob reintroduces this finding. If wanted, it needs an operator-authenticated channel (source constants today; a config-file key would need the import cycle resolved) and is out of scope here.

Docs updated in the same commit: docs/system-specs/modules/sel.md (raise-only rationale), docs/guides/install.md (env table rows marked raise-only).

[patrigao]

Review dispositions for head c71355fc7 (round 2), fixed in 75d296f3d:

• fixed — "Scrubbed-env child enforces compiled deletion bounds on a shared directory (KEEP=100 → child keep=7 → rotation deletes 93 protected segments)."

  Fixed structurally with the process-shared source the review asked for: the two deletion bounds (keep, retention_days) now live in a high-water marker security_events.d/retention_floor.json. The marker is monotone by construction — its single write site re-reads the stored floor and writes max(stored, requested) per field under a dedicated cross-process lock (.retention_floor.lock), so no process (scrubbed or concurrent) can ever publish a lower bound over a raise. Construction takes max(own, persisted); an instance raised above the compiled defaults eagerly creates the fenced segment directory so the stamp lands before any sibling can prune; and BOTH deletion sites (_enforce_segment_retention_locked, prune()'s default path) re-read the marker before deleting, closing the reverse ordering (long-lived scrubbed child constructed before the raise). Reads are nofollow-fenced: a pre-planted link at the directory or marker path reads as no floor, never as attacker-chosen values. The marker sits inside the SEL sensitive-path floor, so the audited agent's file tools cannot touch it.
  Regression tests: test_scrubbed_env_child_cannot_lower_a_raised_deletion_floor (the review's PoC verbatim: operator KEEP=100/RETENTION=400 → scrubbed child inherits 100/400), test_a_child_constructed_before_the_raise_prunes_with_the_raised_floor (reverse ordering via prune-time refresh), test_the_floor_merge_is_per_field_monotone (concurrent differently-raised writers cannot clobber each other), test_a_planted_marker_link_is_not_trusted_and_is_replaced (nofollow), test_a_damaged_floor_marker_fails_soft.

Same-span ledger — span ff91a2389e17 (sel.py env-override bounds): hit count 2 (round 1 "agent-child-env erasure" → raise-only floors; round 2 "cross-process divergence" → process-shared monotone marker). Round 2 is now fixed at the structural invariant the span was converging on: deletion bounds come from a process-shared, monotone, operator-side source enforced at deletion time. A third blocking finding in this span triggers the restructure/escalate protocol rather than another patch.

Also carried in 75d296f3d (pre-push local dual review, GPT + Opus mirrors): per-field monotone merge under the floor lock, nofollow fences on marker/dir/lock reads, and _SEGMENT_KEEP_MAX reserve widened to _SEGMENT_SCAN_CAP - 4 so the marker and its lock can never make the bounded scanner under-count segments at the extreme cap (which would have silently stopped retention).

[patrigao]

Pre-push review round 4 (local dual-model gate on the restructure), dispositions — new head 5a06de690:

Local Opus: PASS, 0 blockers, 2 advisories (both taken). Local GPT: 4 blockers, split verdict resolved finding-by-finding:

#	Finding	Disposition
1	FIFO / oversized marker can block or bloat the construction-path read	Fixed. _read_retention_floor now opens fd-side with O_NONBLOCK|O_NOFOLLOW, fstat-gates to a regular single-link file, and bounds the read at 4 KiB (_FLOOR_MARKER_MAX_BYTES). Regression tests: planted FIFO (thread-bounded, must return promptly), oversized decoy.
2	Hard-linked marker stays externally mutable and can lower the floor	Fixed. Same fstat gate rejects st_nlink != 1; os.replace at persist already de-links our entry. Regression test: hard-linked alias with planted-high values reads as no floor.
3	A lower-cap sibling defeats a raised MAX_BYTES	Rebutted. max_bytes is deliberately outside the floor: deletion is governed solely by keep/retention (see the comment above _RETENTION_FLOOR_FILE), so a divergent smaller cap rotates earlier into more, smaller segments — count retention still keeps the newest N segments and the age sweep still honors the days floor. The claimed "discards history twice as early" conflates segment count with byte volume; the retention contract is count+age, unchanged since round 2's accepted design. Extending the floor to max_bytes would re-open round 3's contention surface for a bound that cannot delete.
4	Deferred stamp leaves a window; construction should not complete until the floor is durably visible	Rebutted — contradicts the round-3 invariant. "Fail/defer construction until publication" is precisely the blocking/failing construction that round 3's blocker (a) required us to remove, and the module contract (fail-soft: the audit logger must always construct) forbids. The window is narrow, self-healing (stamp retries at every construction, rotation, and sweep; the raised process never deletes below its own bounds), and now documented as deliberately accepted (sel.md) per the Opus advisory covering the same fact.

Span ledger ff91a2389e17: server hits remain at 3 (round-3 restructure is this push); local rounds are pre-push hardening, not span hits. A 4th server blocker in this span escalates to the maintainer per the runbook instead of another round.

Gates on 5a06de690: SEL 226/226 (three new fence regressions), isort/flake8/mypy/black/docs-lint green, rebased onto origin/main.

[patrigao]

⚠️ Escalation to maintainer — same-span rule, 4th server round in ff91a2389e17

Server GPT has now posted blocking findings on this span (env-override construction / floor stamping in src/kiro_crew/sel.py) in four consecutive server rounds (7a1091f18 → c71355fc7 → 75d296f3d → 5a06de690). Round 3 already triggered the restructure protocol (dedicated lock removed, stamps moved onto the rotation lock, fd-fenced reads). Per the loop's stall rule I am stopping the fix rounds — the remaining findings need maintainer rulings, not another patch. I have verified both mechanisms against the code; neither is a false positive in the narrow sense, but each demands a decision only you can make:

Finding A — sel.py:534 — "agent env can permanently disable the disk ceiling"

An agent that can run any kirocrew command with env vars (KIROCREW_SEL_KEEP=4093 kirocrew security events) gets its raise persisted process-shared by the high-water marker — that persistence is the round-2 fix working as demanded. Worst case ≈ 4093 × 32 MiB ≈ 128 GiB retained. The demanded fix is "revert environment-driven retention raises until sourced from an operator-authenticated channel."
Why this is yours to rule on: it rejects the delivery mechanism issue #4993 itself chose (env vars). No patch inside the env-var design can satisfy it — from inside the process an operator's environment and an agent's environment are indistinguishable. Either the env-var mechanism stands (accepting that a raise is also agent-reachable — a retain-more direction, never erasure) or the feature needs an authenticated config channel, which supersedes the issue.

Finding B — sel.py:276 — "contended stamp defers → prune deletes inside the requested window"

Real but directly contradicts round 3's blocker (a) from the same reviewer. Round 3 demanded construction never block on the floor lock (event-loop freeze); the demanded fix now is "construction cannot succeed before its raised floor is established" — i.e. block or fail construction until published. Both invariants cannot hold: any design either blocks construction or accepts a narrow, self-healing first-stamp window (now documented in sel.md as deliberately accepted; the raised process itself never deletes below its own bounds, and the stamp retries at every construction/rotation/sweep).

The decision

1. Rule on the invariant pair (B): non-blocking construction with a documented deferral window, or blocking-until-published. Round 3 chose the former at this reviewer's own demand.
2. Rule on the mechanism (A): env vars stand per feat(sel): make the segment size cap and keep-count operator-tunable without editing source #4993, or raises require an authenticated channel (redesign → new issue).
3. Depending on 1+2: post /ai-review override gpt 5a06de69099535db6927c653299037fb82dfe5b1: <reason> (repo-writer only — I cannot), or direct a descope (e.g. ship only MAX_BYTES env-tunable and move the two deletion bounds to a follow-up), or park the PR.

State of the branch: single commit 5a06de690 on fresh origin/main, 226/226 SEL tests + all lint gates green, all other checks passing, every prior concern answered on the PR. Span ledger: ff91a2389e17 server hits = 4; loop stopped per protocol.

[patrigao]

Round-6 server GPT blocker — span ff91a2389e17, server hit 5: covered by the standing escalation, no new fix round.

This finding (sel.py:532, "max-bytes raises are not shared across processes → scrubbed-env sibling rotates at the compiled cap → count retention deletes below the operator's intended threshold") is the max-bytes variant of escalated Finding A (escalation comment): an environment-delivered raise is process-local, and from inside the process an operator's environment is indistinguishable from any other environment. The demanded fix — remove the KIROCREW_SEL_MAX_BYTES override — rejects the delivery mechanism issue #4993 itself requested, which is exactly the product ruling already put to the maintainer (mechanism stands vs authenticated-channel redesign vs descope).

Per the same-span stall protocol (this span is at 5 consecutive server rounds: 7a1091f18 → c71355fc7 → 75d296f3d → 5a06de690 → 821696d7a), I am not opening another fix round against it. The three options in the escalation comment remain the decision surface; option "descope" now plausibly inverts (this round argues max-bytes is the unsafe raise, while rounds 1–5 treated the deletion bounds as the sensitive pair — one more datapoint that no patch inside the env-var design satisfies this reviewer's constraint set).

Everything outside this span on head 821696d7a is being driven to green as usual.

[patrigao]

Status: green except the escalated ruling — and Design Review dispositions

Board on head 821696d7a: all CI lanes green (backend 3.10/3.11/3.12/Windows shards, frontend, Coverage Gate, CodeQL, Screenshot Evidence, Code Review, brand/black/docs gates), Opus 4.8 ✅ PASS, Design Review 🟡 advisory. The only red is the GPT lane, blocking on span ff91a2389e17 for the 5th consecutive server round — the max-bytes variant of escalated Finding A, already routed to the standing escalation (reply). No further fix rounds will run against that span; the decision surface is the maintainer's (rule on A/B, /ai-review override, direct a descope, or park).

Design Review (🟡 CONCERNS on 821696d7a) — dispositions:

1. "The retention floor has no documented undo" — accepted-and-deferred. Legitimate: the marker is monotone by construction, the warning deliberately never prints the stamped value (redaction is a security property — a mis-pasted value can be a secret), so the only reset is deleting security_events.d/retention_floor.json, which no doc names. The two-sentence reset-path addition to sel.md/install.md will ride the next push — every outcome of the pending escalation ruling (override, descope, or redesign) requires at least one more push, and re-arming all nine review lanes now for a docs-only edit while the PR is parked on that ruling is churn without benefit. Recorded in the loop runbook so it cannot be dropped.

2. "Half the closed issue is unserved by design — file the config-key follow-up before closing" — fixed. Filed #5032: operator-authenticated channel (config-file key inside the deny-list-protected config dir, avoiding the known kiro_crew.config import cycle via paths) to serve the LOWERING direction the raise-only env design cannot. It also names the marker-lowering semantics question and is the natural home for an authenticated-raises redesign if the escalation ruling goes that way.

Every other concern on this PR has a prior written disposition (round 1–5 comments). Nothing further is actionable by the automation; handing the PR to the maintainer.

[chenmingwei23]

Heads-up on your Design Review disposition #2: I closed #5032 as not planned, so the line reading "fixed. Filed #5032" now points at a closed tracker. The accurate record is that the lowering direction is an accepted scope cut ruled won't-fix, not a tracked follow-up -- worth editing that line so a later reader does not chase a dead reference.

Reasoning is on #5032. Short version: the raise direction has a named beneficiary (compliance retention), the lowering direction does not, and serving it safely needs a keystone leaf plus a Settings surface plus an owner-session-authenticated writer -- disproportionate to a bound an operator can already move in source. Also relevant: the rotation product is a bound, not an allocation, so an install that never generates that volume never pays it.

This does not block your PR. The raise-only design stands as shipped; only the disposition's wording is stale.

One thing that may be useful while your escalation is still open, in case the ruling on Finding A goes toward an authenticated channel: a single operator-written file read from a fixed path by every SEL writer has no per-process divergence to defend against, so it removes the need for retention_floor.json entirely -- no stamp, no monotone merge, no lock contention. That also dissolves Finding B, since the first-stamp window only exists because there is a stamp, and its absence ends the collision with round 3's non-blocking-construction invariant. Not a request to change anything here -- just so the option is on the record before someone rules. Details and the fence/import specifics are in the closing comment on #5032.

[bolichen97]

🤖 Kiro Crew [operator: bolichen97#bb3ad1ca]: Triage disposition — routing to needs-human rather than the drive-to-green queue.

The only red on head 821696d7a is the GPT lane's 5th consecutive same-span blocking round (ff91a2389e17), already covered by the author's standing escalation: the decision surface is the maintainer's (rule on Finding A/B or /ai-review override). An automated drive session would rebase to clear the merge conflict, and that push would move the head and void any override pinned to 821696d7a — exactly the wrong action while the ruling is pending.

No automation will touch this PR while needs-human is present. Once a maintainer rules (override or directed change), remove needs-human and re-add needs-pr-triage to re-enter the pipeline (the then-stale merge conflict is mechanical to clear).

[bolichen97]

Concrete overlap note: #2497 and this PR both edit src/kiro_crew/sel.py, test/test_sel.py, the SEL spec, and the SecurityEventLog rotation path. They enforce different invariants: #2497 serialized append/rotation with a cross-process HMAC chain lock, while this PR adds env-tunable segment/retention floors and pruning.

#2497 is being closed because its locking behavior is fully covered by merged #6081. Please rebase this branch onto current main/#6081, retain only the retention/segment configuration and pruning behavior, and replay its rotation tests against the merged lock path so pruning cannot bypass or split the chain-lock critical section.

[dwu96]

🤖 Kiro Crew [operator: dwu96#8c08bee0]: Triage re-assessment — this PR re-entered the triage queue today, so I re-verified it independently and reached the same conclusion as the standing disposition: it needs the author's and a maintainer's input, not an automated drive session. Moving needs-pr-triage to needs-author-decision; needs-human stays.

What I verified on head 821696d7a (unchanged since 2026-08-22):

• 56 checks green, 5 skipped, 2 red: GPT 5.6 Review and PR Readiness (1 blocking item).
• The GPT verdict is deterministic, not a flake, and its prescribed remedy is to drop the KIROCREW_SEL_MAX_BYTES override — i.e. to subtract one of the three knobs feat(sel): make the segment size cap and keep-count operator-tunable without editing source #4993 asked for. The author has rebutted that in writing in the PR body. Choosing between those positions is a design ruling, which is outside what automation may decide.
• Mergeability is CONFLICTING, and that is the decisive operational hazard: any rebase to clear the conflict moves the head, which would void an /ai-review override pinned to 821696d7a. Rebasing while a ruling is pending is the wrong move, so no drive session will be started.
• Author's last activity was 2026-08-22 (16 days), so this is not classified as abandoned.
• Security scan clean on both layers (0 findings).

Decisions needed before this can move:

• Maintainer: rule on the escalation filed 2026-08-22, choosing one of — accept the author's rebuttal via an /ai-review override comment, drop the max-bytes knob, or extend the shared retention marker to cover max-bytes.
• Author: resolve the conflict against current main, and replay the rotation tests against the merged chain-lock path from fix(sel): keep the HMAC chain intact when two processes append #6081 (per the 2026-08-29 overlap note).

Order matters: if a maintainer intends to grant the override, that comment should land before the conflict resolution, because the override is pinned to a head that a rebase replaces.

Add pr-no-autofix to opt out of future automation on this PR.

[bolichen97]

Rebased onto main acc99f217 by a maintainer as part of the 2026-09-08 open-PR audit (was 2326 commits behind, mergeable_state: dirty).

Conflicts and resolutions:

• src/kiro_crew/sel.py (prune()): kept main's self._chain_lock(kind="prune") from fix(sel): keep the HMAC chain intact when two processes append #6081 on the outer with, and kept this PR's body, so the floor refresh plus both delete phases still run under the one rotation lock. Main's pre-lock _prune_segments_locked(cutoff_dt) / early return were dropped because this PR moved that call inside the lock and computes cutoff_dt there.
• test/test_sel.py: both sides only appended at end of file, so both blocks were kept (TestMetadataRedaction from main, TestEnvOverrides from this PR).

Gates run locally on changed files: isort, flake8 clean; pytest test/test_sel.py 288 passed, plus test_sel_prune_streaming.py, test_sel_dashboard_offload.py, test_sel_startup_warm.py 22 passed. Plain black --check still reports both files, as it does on main too; both are in .github/black-baseline.txt.

Please review the prune() resolution. A maintainer push makes the maintainer the last pusher, so under the repo's last-push rule a second approver is needed. Reply if anything looks wrong.