fn-4 hardening: the 18-defect backlog, three gated rounds - #461
fn-4 hardening: the 18-defect backlog, three gated rounds#461acebytes wants to merge 48 commits into
Conversation
…d the no-revalidator item arm (fn-4.21) remove(at:expecting: nil) proves the container and nothing else; a child renamed aside at its own path inside the window was destroyed unexamined with the measured tree's bytes reported (reproduced deterministically on both arms before the change — contents mode and item mode with no revalidator, moveToTrash: false). The binding is the r18 BoundObject shape through the same function: TrashDisposal.boundLeaf under the proved admitted container at the pipeline's first read of the leaf, re-proved by the identical read in provingImmediatelyBefore on the far side of the queue hop (provedStillTheBoundLeaf, shared by both arms so two readings cannot disagree). Refusal is the same .notTheInspectedObject the verdict arms throw, tagged content-drift in contents mode too. The standing enumeration at DepthSafeRemoval is re-derived from grep: five sites, not three; the dispose(expecting:) site is never nil at runtime (inside if-let — the task spec's claim retired against source); the Trash arms' disposal-entry binding residual is stated where each arm ignores the early capture. Vanish-at-bind fixtures now count the matching probeChild call: call 1 is the pipeline bind (already-gone skip, newly pinned), call 2 the disposal's own window.
…ke EOF (fn-4.24) PR #460 r18 adversarial verification (runner scope), MEASURED: on any read(2) error that is not EINTR/EAGAIN/EWOULDBLOCK, PipeDrain.drain() set isDone and returned, signalling `finished` exactly like EOF — join(within:) answered true, execute's success gate could not tell the two endings apart, and the partial buffer shipped as .success(stdout:). A truncated porcelain listing does not look malformed; it looks like a repository with fewer worktrees, or a clean tree. Reproduced RED before the fix with a real failing descriptor (EISDIR via a directory fd), at both levels: - drain: terminalReadFailure nil where EISDIR died the worker - execute: .success(stdout: 0 bytes) [stdout arm] and .success(stdout: 16 bytes) [stderr arm] where the drain had died mid-stream, with a CONTROL run proving the stub succeeds through healthy drains Fix: the drain RECORDS the terminal errno under its own lock where the error fires; execute reads it right after the successful joins — the first read of that fact on the only path that can still become a success — and answers .timeout, the same class as the unjoined-drain arm (C7) and for the same reason. Retry can differ: fresh invocation, fresh pipes, fresh descriptors. No terminate in the new arm: the child has provably exited and both drains returned, so a kill there would be an unevidenced guard. Downstream-parser question (asked by the spec), answered from source and RECORDED at the gate: GitWorktreePorcelainParser fails closed on a mid-field/mid-record cut but accepts a record-boundary cut as fewer worktrees; WorktreeStalenessAssessor.verdict counts a truncated status as cleaner and an empty one as .clean; first-line readers accept whatever line survives. The boundary gate is load-bearing. Mutations (deterministic cells, filtered runs, target rebuilt each): - m1 restore EOF-equivalence (drop the errno recording): RED 3/3 cells - m2 delete the execute gate: RED both execute cells - m3 drop the stderr operand: RED stderr cell only - m4 drop the stdout operand: RED stdout cell only PR #460 drain bounds re-run green after the change: testCapturingOutputIsNotStarvedByAContinuouslyWrittenWriteEnd, testCloseIsNotStarvedByAContinuouslyWrittenWriteEnd, testAReadTurnThatNeverRunsDryStillEndsOnItsOwnBound, testTheDrainIsEndedOnlyThroughTheBoundedSpelling, testAnUnfinishedDrainOnANormalExitIsNotReportedAsSuccess. Full suite: 1600 executed / 2 skipped / 0 failures (baseline 1597/2/0).
…dicate both dereferenced deferred paths (fn-4.26) GitWorktreeGitdirResolver canonicalized a worktree's gitdir: pointer BEFORE its first gated probeKind, so an .automatic scan realpath(3)'d through a TCC-protected admin directory before DeferringIdentityProvider could answer .absent — and the deferral predicate (ProjectTreeWalker.isProtectedRoot) itself canonicalized the very path it was classifying. Reproduced RED first in six cells (scan-level realpath count, resolver pointer/commondir/backlink/ cross-validation, predicate direct-spelling); the cross-validation cell also showed the fallback's canonical path equality answering TRUE for a deferred target. The fix is ORDER, not removal: the resolver probes every pointer-derived path AS SPELLED before canonicalizing or comparing it, and isProtectedRoot classifies lexically first, canonicalizing only spellings the lexical stage could not match (aliases). The pass-through delegation injected test providers rely on is untouched. Retired the 'house doctrine draws the line at canonicalize' claim where it was written.
… (fn-4.26) ProjectTreeWalker.isProtectedRoot grew 30 lines, moving the cited device-compare/isMountPoint arm from 529-532 to 559-562; the anchor integrity cell caught the drift in the full-suite run.
…ted (fn-4 r1 gate) The gate found BuildArtifactsScanner.swift:729 citing the mount-refusal arms at :1151/:1371 — lines that now hold a comment and an unrelated identity check after fn-4.21's ~200-line insertion. Repointed to :1187/:1460, verified by grep against the 'mount boundary' refusal strings themselves. SourceAnchorIntegrityTests does not cover doc-comment anchors in THIS file's prose (it pins its own expectation table), which is why the gate had to catch it by hand — same class as the r19 shift, different detector.
…line capture (fn-4 r1 gate) fn-4.21's refusedChild path had no cell for a bind read that fails with anything but ENOENT: an EACCES at TrashDisposal.boundLeaf during the pipeline capture must be the child's reported FAILURE, never the silent skippedAlreadyGone skip — the leaf is still standing there. Provider double denies exactly one probeChild read (the LockUnreadableProvider pattern), with an unarmed control. Mutation proved: widening the ENOENT catch to swallow every DepthSafeRemoval.Failure as a skip turns the cell red on the error-count assertion (0 != 1), reverted.
…ead (fn-4.19) ContainerSnapshot.capture ran synchronously inside scanValidatedSession, which CacheoutViewModel.scan reaches on the MainActor: every session root's lstat ran on the main thread before any bound existed, so a hung mount under a root — including a root INSIDE a mount, which the mountPointPaths() preflight cannot see — froze the app unbounded and unreported (r12 measured 6.03 s with isMainThread=true from a 6 s blocking identity(of:)). captureBounded races the capture loop, detached at .utility (the producer's band-separation decision, for the same reason — an unspecified-band capture queued behind saturation-cell holders and rode its own deadline), against a ScanSessionClock timer via FirstWinsRendezvous — BoundedDiskInfo's rendezvous, extracted so the three bounds share one spelling. scanValidatedSession is async since this change; both production consumers and 16 test call sites await it. An expired capture runs NO scanner and says so: one .scanDidNotFinish per selected scanner whose detail names the capture, ledger concluded .boundFired (GUI declines adoption, CLI target-scoped refusal reads the rows), snapshot .empty (admits nothing — the same fail-closed refusal an omitted root always had). A retry can differ — stalled volume, starved band, both transient — so the re-scan remedy is real, not a strand. New captureDeadline on ScanSessionBounds: production 30 s, fixture default 10 s. The cell's wedge is a releasable semaphore: a fixed 5 s sleep leaked its abandoned-capture worker into the saturation cell's window (red only in close pairings; measured, fixed, 8/8 green paired). Mutation A proved: restoring the sync unbounded capture reds the cell 8/8 on the elapsed assertion.
…last bare waitUntilExit calls (fn-4.20)
dockerPrune did readToEnd() then a bare Process.waitUntilExit() in a
detached task, awaited unbounded: the retired primitive's last
production call site, on a cooperative worker, latching isDockerPruning
for the life of the app if docker never exited. And the wait was not
the only park — the task-spec question answered: readToEnd() blocks
until EOF, so a wedged child that keeps its pipe open parks the read
BEFORE any wait is reached; bounding only the wait would have moved the
strand one line up.
The budget (stated: 600 s production, seams for tests) therefore races
the WHOLE interaction (spawn -> drain -> waitForExit(within:)) via
FirstWinsRendezvous on ScanSessionClock. Expiry is reported ('did not
finish within ...'), SIGTERM is best-effort, the button releases on
every path, and a completed failure cannot trade places with a timeout
(three cells: expiry, success control, completed-failure control).
The spec's 'last surviving call site' claim was stale against source:
Tier2Interventions carried two more bare waits (post-SIGKILL reaps);
both converted to waitForExit(within: 5) so the gate can hold. The gate
itself is a cell (DocumentedContractTests): zero non-comment
waitUntilExit() lines across Sources/**.swift, comment mentions allowed
(they document the retirement), test sources out of scope per spec.
Verifier finding: 3c98463 grew PathGuard.swift by a uniform +74 lines and SpaceScanner.swift by +29 below its insertions, and SourceAnchorIntegrityTests.testEverySourceAnchorStillPointsAtWhatItCites went red on exactly the 11 anchors citing below those points (green at a8de2a2, red at 3c98463 — attributed by running the cell at both). The cited text itself is byte-identical (every pinned excerpt found at the old offset + the file's uniform delta); each citing sentence was re-read against its shifted target before repointing, per this check's own rule. 21 citing sites + the 11 anchorExpectations rows updated; every replacement is digit-for-digit the same width, so no line in any citing file moved and no further anchors drift from this commit.
…ies (fn-4.15)
DirectorySizer.measure had no cancellation point and no entry cap
(measured: Task.isCancelled occurred 0 times in the file) — a scanner
cancelled mid-measure kept walking, and nothing bounded the walk.
CANCELLATION: checked between entries, before the pulled entry is
processed. A cancelled walk returns what it has with the report MARKED
partial (SizeReport.cancelled) — deliberately not a denial: nothing
refused the read, and a retry CAN differ (cancellation is a caller act).
Measured: 111,437 entries/s full-walk rate; cancel-to-return latency
25 us after 5,765 entries (figure to beat was r16's 46.3 ms). Red-first:
testAPreCancelledMeasureStopsBeforeTheFirstEntry failed at 12/12 entries
enumerated before the check existed.
ENTRY CAP (the design decision): 2,000,000 entries per measure call,
DISCLOSED as .enumerationCapped at the walk root, spent only when a
further entry actually exists (an exact-fit tree makes no truncation
claim). The cap is DETERMINISTIC over a static tree, so the disclosure
states permanence and offers no retry — no re-scan wording anywhere,
pinned by testTheCapDisclosureNamesAPermanentConditionNeverARetry.
Value sized against measurement: ~111k entries/s puts the cap at ~18 s
of walk; the largest evidenced real tree (23G worktrees, scenario 2 of
FIELD-EVIDENCE) extrapolates to ~300k entries at this repo's measured
entries-per-byte (.build: 5,917 entries / 481 MB).
PER-CALLER VERDICTS (the C6 check, stated per scanner):
- CacheScanner / sweep / worktree items: capped figures are floors; the
denial rides the existing channels (scanErrorKind .other;
rootIssueKind -> .enumerationTruncated, whose GUI label is already
true of it). No deletion verdict consumes the cap: delete-time
re-measurement is UNCAPPED.
- BuildArtifactsScanner census: a capped census is a FLOOR, and the
probe's doubling already grows past an undercounting census by
documented design.
- GitWorktreeScanner admin-prune suppression (denials.first) would
suppress on a cap denial; unreachable there in practice (fixed-shape
admin dirs, ~10 entries) and delete-time still re-verifies uncapped.
- CacheCleaner (delete time): UNCAPPED sizer by construction — the
mount doctrine reads mountBoundaries as 'the whole tree was swept',
so a capped walk would either permanently strand deletion (the C6
pattern) or delete mount-blind. The pass stays proportional to the
deletion it precedes, and is cancellable per entry.
FAIL-CLOSED CONSUMERS (a partial report is never consumed as complete):
CacheCleaner's category-child and item arms and WorktreeReclaimPerformer's
worktree and admin-prune arms each refuse a cancelled report before the
mount check and before any claim registration, tag
'measurement_cancelled', wording explicitly retryable ('not permanent').
Scan-time consumers are covered by the session-completion discard
(CacheoutViewModel: completed = !Task.isCancelled && !didExceedBounds;
nothing a cut-off session saw becomes deletable).
Anchors: 4 repointed (DirectorySizer 261-272 -> 317-332, 354-359 ->
443-448, 483 -> 570; CacheCleaner 514 -> 523) across expectations and
citing sites.
Suite: 1625 executed / 2 skipped / 0 failures in 208.6 s wall (exit 0,
total line printed). Mutation matrix runs next; results recorded in the
task report.
…mp the walker's depth budget (fn-4.13) THE SWEEP (every self-calling function over a filesystem tree in Sources/, traced to actor reachability): ONE true recursion found: ProjectTreeWalker.visit — one frame and one anchor per level, reached from the cooperative pool by BuildArtifacts and GitWorktree scans. Its budget guard (childDepth <= maxDepth, before the self-call) bounds production at defaultMaxDepth = 8 — no production caller passes anything else (grepped: both scanner inits default it) — but maxDepth was an unclamped parameter, so a test seam or future caller could turn it into a crash. MEASURED, red-first, through the real visit on the real executor (Task.detached, mkdirat-chain fixture, env-gated exploratory cell): depth 128 survived, 192 survived, 256 survived, 288 and 320 died with signal 10 — a guard-page hit on the cooperative pool's small stack. The crash band (257..288) matches PR #459 r14's freshContentBelow measurement (~250-260) — the class this task exists to hunt. FIX: walk() clamps maxDepth to stackSafeMaxDepthCeiling = 128 (2x margin under the measured crash floor, 16x above the production default; zero shipped behavior change). No runtime refusal exists to word: the clamp is deterministic and disclosed at the constant. Mutation: reverting the clamp kills testAWalkAskedForACrashBandDepthSurvivesTheCooperativePool with signal 10 (the r14 precedent: a signal death on revert is a legitimate red); with the clamp it returns with the deepest event exactly at the ceiling. NEGATIVE RESULTS, recorded: - DirectorySizer.enumerateTree: a while-loop over Foundation's deep enumerator (heap-backed, no per-level frame) — MEASURED at depth 350 on the cooperative pool, walks to completion and reaches the leaf (testADeepChainMeasuresOnTheCooperativePoolWithoutAStackCrash). - DepthSafeRemoval: iterative on purpose (its own doc/code) — the in-repo re-anchoring pattern this task treats as the replacement. - EphemeralTempScanner.walkForFreshContent: iterative [URL] stack (fixed in PR #459 r14; pinned there, out of scope here). - ValuablesDetector's ValuablesProbeWalk: an explicit frame machine (descend pushes frames; no self-call). - Single-level listings only: CacheCleaner (category children), GitWorktreeInventory (admin container), InstalledAppResolver. - Every other self-call hit in the sweep is overload dispatch, not recursion (CacheoutViewModel.handle/clean, TrashDisposal.look -> look(named:), FileSystemIdentityProvider.leafMetadata, GitWorktreeScanner.respell, GitCommandRunner.run, DevRootsStore .resolve, ValuablesDetector.acknowledgementToken/probe, EphemeralTempScanner.boundedFirstLevelNames, SpaceScanner.validatedOutcome, ProjectTreeWalker.issue). - Mutual-recursion pass: manual read of the walker files; visit's self-cycle is the only cycle. Sources/CacheoutHelper has no tree walk; Watchdog/ sits outside Sources/ and the task's scope. The exploratory env-gated cell (WALKER_DEPTH=n) stays, skipped by default, for future re-measurement.
ProjectTreeWalker.swift:559-562 -> 578-581 (the +22-line stackSafeMaxDepthCeiling doc moved the mount-check excerpt); caught by testEverySourceAnchorStillPointsAtWhatItCites on the full run.
…ion — policy, dev-root resolution and union answer from the link's own content (fn-4.11) Reproduced RED first in 8 cells: `PathGuard.validateContainerRoot` canonicalized every configured root — full realpath(3), leaf included — at runtime construction on the main thread, so a persisted dev root a same-UID process re-aimed at an unresponsive mounted volume froze launch before any window existed; `DevRootsStore.resolve`'s probe pass and `SpaceScannerRuntime.suppressingAliasShadows` then paid the same resolution again (and a dangling alias sent realpath's fallback walking the destination's parent chain). The fix is the fn-4.26 order applied at all three sites, with fn-6's readlink technique as the spec directs: probe AS SPELLED first (lstat no-follow), canonicalize only a spelling proven a real directory (the resolved leaf then IS the object the lstat touched), and answer for a symlink leaf from the link's OWN content — one readlink(2) plus the lexical fold hoisted from EphemeralTempRoots to FileSystemIdentityProvider.lexicalTargetPath, shared by all four resolvers. The policy's deny verdicts survive destination-free: "/" and ..-escapes from the folded content, volume roots from the getfsstat kernel table (never a path syscall), $HOME by string against both spellings; the r15 kernel-table preflight now also refuses an over-mounted DECLARED root at the policy and skips it in the union's probe. Cross-scanner alias shadowing still suppressed, by NAME — residual (third spelling: second hop, case variant, /var-style alias) disclosed at all three sites and pinned as kept-both. Mutation evidence, one arm at a time (all restored): policy preflight → over-mounted cell red (contact + no refusal); canonicalize-first restored → all five no-contact cells red at first contact; mount-table arm → alias-of-mount cell red; "/" arm → both root-alias cells red; home arm → both home-alias cells red; DevRoots drop arm → drop cell + delete-time shadow cell red; DevRoots/union symlink canonicalize → the production/resolution contact cells red; union drop arm → both shadow cells red; union preflight → union-probe cell red. The dropped-alias covering-key link in canonicalKeys was mutation-tested GREEN (no cell reached it — a dropped spelling supports no admissible claim), so it is retired rather than kept unevidenced, and the r16 sentence claiming it is rewritten at both sites. Also retired on the same evidence: EphemeralTempRoots' recorded r12 "one surviving contact" residual and its r15 symlink-to-mount residual bullet (both closed by this change, history kept), and the "canonicalize-before-check" wording wherever the proposition survived. All shifted anchors repointed and re-verified by SourceAnchorIntegrityTests. The union probe's kind switch spells every KindProbe case explicitly — the R4 no-`default:` fence over SpaceScanner.swift caught the first draft (testNoExhaustiveReclaimActionSwitchGainedADefaultArm), which is that fence doing its job. Full suite: 1636 executed, 3 skipped, 0 failures (exit 0, 187 s).
…e — respell, repoint the population, and pin (fn-4 r2 gate) Third anchor-rot occurrence in two rounds, and the root cause was a SPELLING: the citations of CacheCleaner's two mount-refusal arms were written `deleteGuardedChild`:1151 — a form SourceAnchorIntegrityTests' pattern cannot see — so the suite could never redden on them and each gate caught the drift by hand. Worse, the r1 fix repointed ONE citing site while ValuablesDetector and OrphanedCachesScanner carried the identical stale pair: the sweep-the-phrasing-not-the-claim failure, applied to anchors. All three citing sites respelled to the canonical CacheCleaner.swift:1210 / :1496 form, which the gate's DEFAULT-DENY rule then forces into the pin table — three rows added, so the next insertion that shifts those arms is a red cell, not a reviewer finding. The pre-existing stale :571 citation of preDeleteUserDataProbe (actual :816, stale at base 8a38e3f) is repointed and pinned the same way. The mechanism promptly proved itself: the +2 lines of respelled citations shifted the pre-existing BuildArtifactsScanner :1405-1406 pin, and the gate went red before this was committed; repointed to :1407-1408 in both the pin table and the citing test comment. CORRECTION OF RECORD for 9ce6b1d: its commit message claims "Anchors: 4 repointed", which was false against its own tree — it re-broke the BuildArtifactsScanner:729 anchor the r1-gate commit 6bb24c0 had just fixed (arms moved :1187->:1210, :1460->:1496 under fn-4.15's insertions). Corrected forward here per the project's standing practice; the history is not rewritten. Suite: 1636 executed / 3 skipped / 0 failures, exit 0, 183 s, at this commit.
…itionFailure out of test sources (fn-4.14) The PR #459 r15 strand diagnosed: a SHORT READ — both test socket clients did one unframed read(2) on a newline-framed SOCK_STREAM reply, and the truncated JSON fed a trapping cast. Reproduced red-as-crash (signal 6, 38 later cells never ran, last tally read 0 failures) and red-as-failure after the XCTUnwrap shape; the casts themselves were converted on main in 8fa8ad3, so this closes what was left open: - TestSocketClient.readNewlineTerminatedReply: loop to the newline, EOF, or a full buffer; both helpers now call it. Evidence cell drives a server that writes the reply in two spaced segments — single-read mutation red 10/10, framed loop green 10/10. - StrandFenceTests: as!, preconditionFailure and fatalError promoted from 'counted, not scanned' to statement traps (all zero occurrences, no allowance to rot). Mutation: one cell carrying all three shapes redded each arm by name at its line. - CONTRIBUTING.md: a green TALLY is not a green RUN — trust exit code and executed count, never a greppable '0 failures' line.
…sserted a per-walk property (fn-4.16) DIAGNOSED, not retried away. The suite is one process; a sibling cell's deferred cleanup (StatusSocket accept handlers' deferred closes, Process pipe teardown) can close descriptors mid-walk, so heldDescriptorCount() minus a baseline is NOT the walk's descriptor count. That is exactly the observed shape — 'census 2+1 vs measured -8', kernel count DOWN, which no walk that closes only what it opened can produce. Alone the old cell was green 20/20 (recorded negative result): the failure needed a sibling. Neither the census nor the kernel was wrong; the cell's exclusivity assumption was. The cell now snapshots the fd SET around each attempt and classifies: appeared-and-stayed = leak, fail at once; vanished = ambient churn, the cross-check is unattributable, retry (bounded at 5 — a retry CAN differ, the churn is transient sibling cleanup, not a deterministic limit); set-clean disagreement = suspect, and a second one convicts the census. Walk-internal invariants (completion, forced climb, observed mid-climb peak, window bound) assert on every attempt. A new cell manufactures the churn deterministically — closes eight held fds inside the first census instant — and pins the classifier while reproducing the original disagreement evidence. Mutations: census transient over-counted by one → red 8/8 (two-suspect conviction); classifier blinded to vanished fds → churn cell red; a descriptor leaked inside the attempt → red on the leak arm. Fixed cells green 20/20.
…ag gate, decision recorded (fn-4.23) DECISION (of the task's two designs): the FULL per-tag gate, implemented as a two-layer source census rather than 29 new behavioral cells — the set-gate alternative cannot meet the task's own mutation bar, because swapping two tags between arms leaves the set unchanged. Layer 1 pins the ORDERED sequence of the 42 tag literals (33 distinct) at their definition sites: any swap, insertion, removal, or rename changes the sequence, so sequence equality subsumes the set gate (hence no second gate, per the spec's 'do not add both'). Layer 2 sweeps the whole file for tag-shaped literals and requires each to be censused or pinned as a non-tag, closing the new-spelling hole a position grammar alone would leave. User-reaching DETAILS keep their behavioral assertions in WorktreeReclaimPerformerTests (the r17 lesson: 4 tags asserted there today); this gate protects the routing discriminator underneath them. Mutations, each red with the position and source line named: swapping worktree-deregistered with worktree-not-linked (position 20, line 2290); swapping the prunedAdminBinding pair (position 30); smuggling 'brand-new-tag' through a let binding (layer 2, line 2695).
…ion; bare EPERM is never TCC (fn-4.12)
The GUI derives the whole visible row from ScanIssue.Kind alone
(ScanIssueRowPresentation.label(for:)), so a kind shared with a different
condition prints a false diagnosis to the user and to wire consumers
(PROTOCOL.md scanner_errors[].kind). Audit of the full kind -> producer
matrix, r17 message-truth method (derive from the producing path, then
check the label):
containerRefused "not a configured search root"
- GitWorktreeScanner outside-every-root arm TRUE, kept + pinned
- GitWorktreeScanner scope arms (worktree inside a root) FALSE
-> NEW mutationScopeRefused (wire mutation_scope_refused)
- DevRootsStore validateContainerRoot catch (detail says
"configured dev root refused") FALSE -> policyRefusedRoot
- ProjectTreeWalker admitSearchRoot catch FALSE for policy clauses
-> policyRefusedRoot; mount standing at the root -> the
mountedVolumeRoot ephemeral shape (table re-read per walk, so the
label's "then re-scan" is true); notAConfiguredContainer keeps
containerRefused (roots: is a parameter; that label IS the truth)
symlinkRoot "symlinked - not searched"
- ProjectTreeWalker root gate (any non-directory) FALSE for
file/FIFO/socket/device -> nonDirectoryRoot, split by probed kind
- OrphanedCachesScanner rootNotADirectory same split
- DevRootsStore + EphemeralTempRoots alias arms TRUE by construction
(readlink-gated), pinned
tccDenied "access denied by macOS privacy settings" + Grant-access link
- bare-errno EPERM producers (ProjectTreeWalker forFailedOpen;
DirectorySizer.denial(forFailedProbe:) feeding walker, sweep and
sizer walks) FALSE: a raw errno has no provenance (TCC, SIP,
immutable flags indistinguishable) -> neutral unreadable/.metadata
with the cause-not-established detail; the rule the ephemeral
scanner already measured and recorded, now decided ONCE at the
shared classifier. Chain-proven EPERM (classifyDenial via
NSUnderlyingErrorKey) keeps tccDenied - the one assertable arm.
unreadable / enumerationTruncated / config/tool/malformed/didNotFinish
- audited, labels true; GitWorktreeScanner validation failures stay
unreadable (state could not be faithfully read; recorded)
Wire: mutation_scope_refused is an ADDITION on the extensible enum
(schema stays 4, no string renamed); PROTOCOL.md, API-REFERENCE.md and
CHANGELOG call out every condition whose kind moved. cacheout-mcp needs
no update (measured in the task spec: forwards rows opaquely).
Residual, recorded in EphemeralTempScanner header (c): its sizing-path
.tcc neutrality is now over-conservative (the conflation that mandated
it died at the source) but asserts nothing false; out of fn-4.12 bounds.
Anchors repointed in the same commit (SpaceScanner +25 tail,
DevRootsStore +6 tail, DirectorySizer +5 tail, ProjectTreeWalker
578-581 -> 629-632, CLIHandler 2123 -> 2125, CacheoutViewModel
1487-1488 -> 1488-1489); SourceAnchorIntegrityTests green.
…o red cell (fn-4.12 mutation gate)
…ever discovered from a live leader (fn-4.27) r18's ownProcessGroup read getpgid(pid) after run(), so the group fact was contingent on observing a LIVE leader: a git that spawned a helper and exited inside that window left group == nil, and terminate() then signalled only the corpse's pid while the descendants -- the case the group protocol exists for -- ran on holding the inherited pipe. The r18 comment called nil "never worse than before"; for the exited-leader case it was exactly as bad as before. Comment updated to what is now true. Fix: the runner spawns through its own SpawnedProcess -- posix_spawn with POSIX_SPAWN_SETPGROUP (pgroup 0), so the kernel makes the child the leader of a new group (id == pid) atomically at creation; pid IS the group id for the process's whole life and nothing about signalling is conditional on leader liveness. ownProcessGroup is deleted. WHY posix_spawn over the trampoline option (spec asked): it keeps the argv fence byte-identical (/usr/bin/env + ["git", ...], argv-only, never a shell -- testTheNewGitFilesNeverConstructAShellString) and ships no new binary (scripts/bundle.sh copies nothing it is not told to -- the v2.1.0 lesson); a shell trampoline is banned by the fence, a compiled one is a bundling liability. Parity with Process, each stated in SpawnedProcess's doc: /dev/null stdin via file actions, dup2'd pipe write ends, CLOEXEC_DEFAULT for everything else, parent write-end close at launch (Process did this implicitly; forgetting it starves the drains of EOF), and a bounded pid-targeted waitpid(WNOHANG) poll in Process.waitForExit's own deadline/backoff shape (waitUntilExit misses wakeups under concurrent reaping -- house doctrine). terminationStatus maps signal deaths to -(signal) so no real exit code is impersonated; callers test 0/127. Cell (fn-4.27 acceptance): testALeaderThatExitsImmediatelyStillHasItsDescendantReapedByPid -- leader records pid, spawns a TERM-immune descendant holding stdout, exits 0; asserts .timeout, that the LEADER was already unsignallable when the runner answered, and that the descendant dies BY PID. Green 8/8 unmutated. Mutation (acceptance): restoring post-launch discovery -- signalTree requiring getpgid(pid) == pid before group-signalling, r18's semantics -- RED 8/8 on that cell (the leader is provably reaped before terminate, so discovery always fails and the descendant survives). Negative result, recorded: restoring discovery at the LAUNCH spot (r18's literal line) cannot be reddened deterministically -- the leader is microseconds old there and always observable -- which is why the mutation targets the semantics (group contingent on leader liveness) at the point of use. Full GitCommandRunnerTests: 32 tests, 0 failures on the new seam.
… the bare-EPERM rule; repoint the two anchors the fn-4.12/4.27 growth shifted (fn-4.12) The full-suite gate caught what the targeted batches missed: 1. BuildArtifactsScanner.obstruction(at:errno:detail:) was a FOURTH raw bare-EPERM -> .tcc producer (a failed openat on the containment descent) the audit's grep of ScanIssue producers could not see - it classifies into an ITEM's SizeDenial, not a ScanIssue - and its .tcc becomes the item row's .tccDenied grant link. Same rule now: bare EPERM -> neutral .metadata with the cause-not-established caveat. Cell testDescentOpenEPERMClassifiesNeutrallyEACCESAsPermission (EPERM neutral + EACCES control); mutation restoring the .tcc arm reddens it. The walker's child-probe comment stating the retired proposition is updated with it. 2. ProjectTreeWalker.swift:629-632 -> :637-640 (the admission-catch rewrite grew the walker after the anchor gate had last run) and BuildArtifactsScanner.swift:1407-1408 -> :1416-1417 (the obstruction fix above), repointed with their citing sites in the same commit; SourceAnchorIntegrityTests green.
…ne — the prune tier's own case (fn-4.28) Discovery keyed entirely on an entry named .git, and a bare repository has none — so once its linked checkouts were deleted, no repository group formed and the prune tier never ran for exactly the all-checkouts-gone case it exists to reclaim. REPRODUCED first: bare clone + worktree added + checkout rm'd -> scan returned zero items and zero issues, silently. The fix is a discovery kind, not a tier: GitWorktreeGitdirResolver gains the bare-shape proof (HEAD a regular file whose content git's validate_headref would accept, an objects directory, a refs or reftable backend, and a config that declares 'bare = true' the way git's writer spells it — which is what keeps a --separate-git-dir git directory, same shape with bare = false, from being claimed). Every probe runs through the injected identity provider, so the TCC deferral answers first, and HEAD/config are read only behind their probeKind gates (fn-4.26 ordering). The walk consumer admits the shape only when the event's own lstat'd entries already show it — no .git entry of any kind, HEAD/objects/refs kinds right — so unproved directories cost no probe and provably reach no git subprocess. Grouping canonicalizes the bare directory exactly as the main-checkout arm canonicalizes <dir>/.git, so a bare parent reached both ways (its own shape and a live checkout's gitdir: pointer) names ONE group and pays for ONE listing (cell-pinned). Downstream is untouched: the porcelain first record must still declare itself bare and cross-validate against the same directory before anything acts. Cells: the named discovery/prune cell (the task's mutation gate), the looks-bare refusal (no item, no issue, no subprocess), per-requirement forgery refusals with an intact-shape control, the deferral cell with its own control, one-listing dedupe, detached-HEAD preservation on the bare path, and the end-to-end cell that prunes through the production composition and then asks git itself: clean registry, quiet fsck, and a fresh 'git worktree add' that succeeds. Residual, disclosed at the proof: a bare repository whose config spells bareness any other way stays undiscovered — the same silent non-discovery it had before, never dressed retryable.
…ication, not doubling; fan-in recorded, not built (fn-4.18) MEASURE FIRST, per the task's own acceptance bar. On an artifact-bearing tree (two populated node_modules, one Rust target, one repo+worktree; 5772 entries): build WALK 249 probes 0.004s (consumer prunes matched dirs) build SCAN 5772 probes 0.230s (walk 249 + sizing census 5523) git SCAN 5772 probes 0.212s (walk prunes nothing, by design) union WALK 5772 probes 0.132s (zero consumers = fused reach) Codex's 'nearly double filesystem I/O and latency' is CORRECTED, not inherited: the walks are asymmetric, the duplicated enumeration is their intersection (the pruned build walk), and a fused walk must carry the git walk's unpruned reach while the build scanner pays its sizing census either way — so the fan-in buys 249 of 11544 entry probes (2.2%) on the tree class these scanners exist for. The claim's true half is a dev root with no artifacts and no repository: both walks enumerate everything (496 probes each, ~7ms) and fusing halves milliseconds. DISPOSITION — scoped, not fixed, with the numbers: a shared walk must be per scan session (the walk-instant witness the r16/r17 re-proof chain hangs off can never come from an earlier session's walk), scanners run concurrently AND individually, and nothing at the SpaceScanner boundary names a session for two scan(context:) calls to rendezvous on — coalescing on accidental concurrency would make the walk count timing-dependent in the safety path. Buying the measured 2% means rewriting the protocol's 'a scanner does its own I/O' contract, which the task's Boundaries forbid: record and stop. The measurement is KEPT EXECUTABLE (DevTreeWalkMeasurementTests) with the two load-bearing facts pinned: the pruned build walk is strictly smaller than the git walk, and the git walk equals the zero-consumer union reach. The disposition note sits at the walk step of GitWorktreeScanner.scan, where the finding was anchored.
… text, retirement is a production declaration, extension fails closed (fn-4.22)
The r18 fact-set assembly made a false clause unrepresentable at any
call site; two surfaces remained, measured at 11/11 unanticipated
false wordings passing:
1. Remedy.text was free prose whose only check compared the rendered
clause against the enum's OWN text — a tautology any prose
satisfies. The enum now carries NO text: the remedy wordings live
in sentence(for:), the one production wording table, and the fence
pins them byte-exact per Remedy case instead of asking production
to agree with itself.
2. A new Established case carried its own false sentence through
every structural check, because each check is relative to tables
the same author edits and the test-side neverEstablished list
could not know about a case added after it. Three moves close it:
- the unspoken set is DERIVED in the fence from established(for:)
over every cause and required to equal a new production
declaration, Failure.retired — a case claimed by no cause must
be visibly retired, and a retired case must render nil;
- the whole vocabulary, every wording, both remedy wordings and
the placesTheItem verdicts are pinned BYTE-EXACT in the named
cell testTheVocabularyAndEveryWordingArePinnedSoExtensionFailsClosed
(default-deny, the SourceAnchorIntegrityTests shape): a new case
or a reworded/grafted sentence is a red cell the moment it
exists, with no test edit needed for the failure to fire —
making it green again forces the sentence verbatim into the
fence's own diff;
- the r18-measured GREEN residual (a false clause grafted into an
existing wording) reddens under the same pins.
What a pin deliberately does not claim: that a pinned sentence is
TRUE. The residual is disclosed BY MECHANISM at
TrashDisposal.Failure.sentence: (1) a coordinated wording+pin edit in
one commit — the fence forces the sentence in front of a reviewer,
it cannot judge it; (2) established(for:) admitting a proposition
its path does not prove — narrowed by placesTheItem and the measured
fixture cells, unnarrowed for non-placing facts.
Attack round (>= 10 fresh false wordings) follows in the next commit
with per-attack results.
…dings pass the pinned fence (fn-4.22) Eleven unanticipated false wordings, each applied to production alone and run through the full TrashDisposalHopProofTests filter, then reverted: both remedy prose grafts (including the task's measured passer), the r18-measured GREEN graft, a net-effect rewording, a hedge strip, a non-placing proposition reworded into a placing claim, three whole-new-Established-case rides (claimed, unclaimed, and smuggled through retired with a sentence), and two derivation edits. All eleven RED; baseline re-ran green between attacks as the control. The per-attack table lives on the pin cell's doc.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: efc27604d7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… reported not to have run (PR #461 codex r1, P1) FirstWinsRendezvous decides which OUTCOME is reported. It cannot decide whether the work STARTED, and for a destructive child those are different questions. A detached task still queued when the off-pool timer wins observes nothing; the timeout branch sees `process.isRunning == false`, truthfully reports that nothing is running, re-enables the button — and the task is then scheduled and calls `run()`, launching an unowned `docker system prune -f` after the operation was reported timed out, free to overlap the retry the user was just invited to make. The starvation this off-pool timer exists to survive is exactly the condition that keeps the task queued, so the window is widest when it matters most. New LaunchClaim: both sides claim through one lock, so the pair is decided once. `begin()` false means DO NOT START; `abandon()` false means it already started and the caller owns stopping it. The timeout branch now reads `launch.didStart, process.isRunning` — stating which case happened rather than inferring it from a Process that answers false when it never launched. MUTATION (begin ignores a prior abandon): 504 assertion failures. The evidence is not a sequenced imitation of the race — testUnderContention... runs 500 concurrent begin/abandon pairs and requires EXACTLY ONE side of each to win: both true is a prune launched after being reported stopped, both false is a prune neither run nor accounted for. Suite: 1665 executed / 3 skipped / 0 failures, exit 0, 214 s, at this commit.
The paragraph has said MOUNT FIRST since fn-4.12 while the root probe ran above it. On an unresponsive mounted volume that `lstat` never returns: the walk hangs, the `.mountedVolumeRoot` issue the paragraph promises is never emitted, and the session reaches its watchdog leaving a blocked worker behind. The comment described the contract; the order did not honour it — this branch's most-retired defect class, found in the fix that wrote the comment. The kernel table is memory and costs no filesystem call, so asking it first is free. Reordered; the absent-root probe now follows, which is what makes the hang unreachable. EVIDENCE, and why it counts rather than measures: a hanging provider cannot prove this — it would hang the suite, which IS the defect. So the new cell counts instead. A provider that records every probe must record NONE for the mounted root, and recording none is exactly what makes the hang impossible. MUTATION (probe restored above the table): red, naming the recorded probe of the mounted root. Anchor: this round's insertions shifted ProjectTreeWalker.swift:637-640 -> :650-653; repointed in both the citing prose (BuildArtifactsScanner.swift:735) and anchorExpectations. Fourth payout of that gate this campaign.
…ns a different child (PR #461 codex r1, P2) Every `posix_spawn_file_actions_*` and `posix_spawnattr_*` return value was discarded. These calls allocate, so under transient pressure they answer ENOMEM — and discarding that is not a lost message. A dropped `adddup2` leaves git's stdout attached to whatever descriptor 1 already was: git exits 0, the drain sees an immediate EOF, and the EMPTY buffer is accepted as a complete answer. That is precisely the class fn-4.24 closed at the execute boundary, re-entering through the spawn fn-4.27 introduced. A dropped attribute call defeats fn-4.27's group isolation just as quietly. Every setup call is now `try require(...)`d, throwing SpawnFailure. The caller maps it to `.gitUnavailable`, which fn-4.25 retries rather than caches — and ENOMEM is transient, so that retry can genuinely differ. NEGATIVE RESULT, recorded in the cell rather than worked around: no behavioural cell can stage this failure. `launch` takes Pipes and builds its own descriptors; `adddup2` does not validate that a descriptor is OPEN at setup time, so a closed pipe throws from `fileDescriptor` one layer earlier and never reaches the guard. Staging real ENOMEM is not available to a test, and reshaping `launch` to take raw descriptors purely to make the failure reachable would widen production API for evidence — which this project declines. A visibility widening tried for that purpose was reverted. So the property is asserted where it lives, in the same shape as the waitUntilExit gate (fn-4.20) and the refusal-tag census (fn-4.23): a source fence over the spawn path, with a vacuity guard (>= 6 setup calls must be found, or the fence has gone blind). MUTATIONS: dropping one `try require(` reds naming GitCommandRunner.swift:785; renaming the calls so the scan matches nothing reds the vacuity guard.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2bb25961bd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| guard launch.begin() else { return } | ||
| do { | ||
| try process.run() |
There was a problem hiding this comment.
Hold the launch claim through process startup
When the timer fires after launch.begin() returns but before process.run() starts, abandon() loses and the timeout result is published, yet process.isRunning is still false so the timeout arm does not terminate anything; the detached task can then resume and launch the destructive prune after the UI has re-enabled retries. Fresh evidence in the current code is that begin() marks didStart before the separate process.run() call, leaving this interval unsynchronized; startup must be atomic with the timeout decision.
Useful? React with 👍 / 👎.
| var argv: [UnsafeMutablePointer<CChar>?] = | ||
| ([executablePath] + arguments).map { strdup($0) } | ||
| argv.append(nil) | ||
| defer { argv.forEach { free($0) } } | ||
| var envp: [UnsafeMutablePointer<CChar>?] = | ||
| environment.map { strdup("\($0.key)=\($0.value)") } |
There was a problem hiding this comment.
Reject failed C-string allocations before spawning
Under the same memory pressure these new setup checks are intended to handle, strdup may return nil, but nil is also the argv/envp terminator and every allocation result here is accepted unchecked. A failure while duplicating "git", for example, makes /usr/bin/env run with no utility, print its environment, and exit 0, so the runner can report successful but unrelated output as a completed git command; validate every duplicated string and throw before posix_spawn if any allocation fails.
Useful? React with 👍 / 👎.
| configContents.split(whereSeparator: \.isNewline).contains { line in | ||
| let trimmed = line.trimmingCharacters(in: .whitespaces).lowercased() | ||
| guard let equals = trimmed.firstIndex(of: "=") else { return false } | ||
| let key = trimmed[..<equals].trimmingCharacters(in: .whitespaces) | ||
| let value = trimmed[trimmed.index(after: equals)...] | ||
| .trimmingCharacters(in: .whitespaces) | ||
| return key == "bare" && value == "true" |
There was a problem hiding this comment.
Restrict the bare key to Git's core section
When a traversed separate git directory has the normal core.bare = false plus an unrelated section containing bare = true (or an earlier core.bare = true overridden later by false), this line treats it as a bare repository because section context and Git's last-value semantics are ignored. The new discovery then runs worktree list against that healthy non-bare admin directory and publishes a cross-validation unreadable issue instead of leaving the deliberately unsupported separate-git-dir repository undiscovered; parse the effective core.bare value rather than matching any line named bare.
Useful? React with 👍 / 👎.
| guard identity.probeKind(of: head) == .kind(.regularFile), | ||
| let headContents = try? String(contentsOf: head, encoding: .utf8), | ||
| Self.isAcceptableHeadContent(headContents) |
There was a problem hiding this comment.
Bound metadata reads during bare-repository discovery
When any directory under a configured dev root has the inexpensive bare-shape names but an oversized regular file at HEAD (and likewise at config below), String(contentsOf:) loads and decodes the entire file before deciding whether this is a repository. A malformed or adversarial multi-gigabyte file can therefore cause a large memory spike or terminate the app, and the scan deadline cannot cancel this synchronous read; read only a bounded HEAD value and parse config incrementally or under a size limit.
Useful? React with 👍 / 👎.
…ndow, it did not close it (PR #461 merge gate) 83e2fd7 gave LaunchClaim a bare `begin()` and left the caller to start the work on the next statement. That moved the window from "task never scheduled" to "task scheduled, between two adjacent statements", and the window is the same defect: with the timer firing in between, `abandon()` answers false ("already started"), the timeout branch reads `didStart == true` with `isRunning == false` — because `run()` has not executed yet — terminates NOTHING, reports the prune stopped, re-enables the button, and the detached task then launches an unowned `docker system prune -f`. Worse, 83e2fd7's comment asserted `didStart` "states which case happened rather than inferring it". In that window it states the wrong case and the conjunction silently selects do-nothing: a claim about the code that the code did not have, which is the class this branch exists to retire. `begin(_ body:)` now runs the work while holding the lock, so deciding and starting are one act. After `abandon()` answers false, the work has PROVABLY run. A throwing start spends the claim but leaves `didStart` false — nothing is running to terminate. THE HEADLINE EVIDENCE IN 83e2fd7 WAS FALSE, and this corrects the record. It claimed "MUTATION: 504 assertion failures" and presented a 500-pair contention cell as primary proof. Re-running the stated mutation yields THREE failures, not 504 — and that contention cell PASSES under it. Instrumenting the same shape shows why: beginWins=500, abandonWins=0. The two GCD blocks never raced; 500 iterations exercised one ordering, and the abandon-first ordering — the queued-task case the whole type exists for — was never taken. The "deliberately not a sequenced imitation" claim was exactly backwards: the sequenced cell was carrying the evidence. The cell now races on a barrier and ASSERTS BOTH ORDERINGS OCCURRED, so a cell that stops contending fails instead of passing quietly; and the invariant it checks is the one that matters — a winning `begin` has already run the work. MUTATION (claim taken, work merely scheduled): red. The never-launched timeout also stopped lying: it said "asked it to stop" for a child that was never started. It now says "did not start ... nothing was run", with the same real remedy (the cause — a starved pool, a busy daemon — is transient). Suite: 1666 executed / 3 skipped / 0 failures, exit 0, 233 s.
… that said otherwise was blind (PR #461 merge gate) 150fe01 moved `probeKind` below the mount table and asserted the hang was unreachable. It was not. `isProtectedRoot`'s stage 2 canonicalizes, and `realpath(3)` on a hung hard mount blocks uninterruptibly in the kernel — so the TCC gate, which ran ABOVE the table, was the first contact with the root. On `includeProtectedRoots: false` — what every non-userInitiated trigger passes, i.e. the automatic/background scan — the walk hung there, the `.mountedVolumeRoot` issue was never emitted, and the session reached its watchdog with a blocked worker: the exact outcome 150fe01 claimed to have made impossible. Stage 1's lexical match only rescues roots spelled under ~/Documents|Desktop|Downloads; a /Volumes-style dev root falls to stage 2. The table is memory (getfsstat with a private buffer, no filesystem contact), so it now answers before ANY code that can touch the filesystem. Order is table -> TCC gate -> probe. AND THE CELL WAS BLIND BY CONSTRUCTION. It recorded `probeKind` alone, so it could only ever prove something about `probeKind` — while `canonicalize` and `realPath` walked past it. A guard that names one syscall proves one syscall. The double now records probeKind, canonicalize, realPath and identity, and the cell runs on the background arm that was actually broken. MUTATION (TCC gate restored above the table): red, naming canonicalize(<volume>) and realPath on the mounted root — the contact the old cell could not see. Anchor: 150fe01's repoint was off by two in the very gate it called its "fourth payout" — :650-653 began mid-`||` and ended on a blank line, invisible to SourceAnchorIntegrityTests because it only requires the excerpt to appear somewhere inside the range. Repointed to :661-664, the actual four-line condition.
…build it as a property (PR #461 merge gate) 2bb2596's fence was written in the same hour as a commit criticising blocklists, and was one. Measured against the real spawn path: four unchecked setup calls inserted after the last `try require` all escaped while the vacuity guard stayed satisfied — an assignment prefix, a nested-func wrapper, a local alias, and `posix_spawn_file_actions_addclose`, a genuine setup call whose only defect was being absent from a six-name alternation. Six more escaped by inspection, including a bare call whose trailing COMMENT contained the literal text `try require(`, because checkedness was `line.contains("try require(")` and a comment laundered it. The regex was anchored per line, so ANY token before the symbol made a call invisible rather than flagged: the failure was silent, not loud. It also FALSE-POSITIVED: re-wrapping the existing, correct `try require(posix_spawnattr_setpgroup(...))` across three lines reddened the fence, so a routine reformat broke the suite on correct code. Rebuilt as a property: - symbols matched BY PREFIX (`posix_spawn*`), not an enumerated list, so a setup call cannot escape by being newly spelled; - comments stripped before matching, so nothing launders itself; - statements rather than lines, so wrapping neither hides nor false-flags; - checkedness is STRUCTURAL — the call must be the direct operand of `try require(` — so an assignment or alias in front fails loudly; - bounded to `launch`'s body, with teardown and the spawn itself exempted BY NAME rather than excused by a pattern. VERIFIED against every escape the gate found: assignment prefix, `_ =`, addclose, setsigmask and comment-laundering are now all CAUGHT (5/5), and the re-wrapped correct call is GREEN — the false positive is gone.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28a8f15cbc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| guard identity.probeKind(of: head) == .kind(.regularFile), | ||
| let headContents = try? String(contentsOf: head, encoding: .utf8), |
There was a problem hiding this comment.
Open bare-repository metadata without following replacements
When another process can modify a candidate directory during an automatic scan, it can replace HEAD after this no-follow probe with a symlink before String(contentsOf:) opens it; the same window exists for config. Foundation then follows the replacement, so the new bare-repository discovery can read or block on a TCC-protected or unresponsive target despite requiring these entries to be regular files. Read each file through a no-follow descriptor and verify that descriptor's identity/kind, rather than separating the probe from the path-based read.
Useful? React with 👍 / 👎.
| let boundTarget: FileSystemIdentityProvider.ChildFacts? = | ||
| probedObject == nil | ||
| ? try TrashDisposal.boundLeaf( | ||
| of: target, containedIn: admittedParent, | ||
| provider: provider | ||
| ) |
There was a problem hiding this comment.
Bind item identity before measuring it
For a permanently deleted item whose scanner supplies no revalidator, this binding is taken only after sizer.measure(at:) has finished. If the target is renamed away and a different object is installed at the same name between the measurement and this line, boundTarget records the replacement and the far-side proof succeeds, so the replacement is destroyed while the report credits the original tree's bytes. Capture the leaf identity before the measurement (as the contents-mode path does) and carry that same identity through the final removal.
Useful? React with 👍 / 👎.
…2 P1) The merge gate reopened the exact window 36dff04 claims to close — `decided = true; unlock(); try body()` — and the FULL 1666-cell suite stayed green. Every cell read the witness after `begin` returned or after `group.wait()`: instants at which the body has run under either shape. `abandon()` answering false is a claim about the PAST, and it is only falsifiable while `begin` is still in flight. Two cells now observe inside the window: - testAbandonNeverAnswersFalseBeforeTheWorkHasRun manufactures the instant deterministically — the timer thread is released from inside the body and reads the witness immediately after abandon() answers. RED 12/12 mutated, GREEN 12/12 pristine. - the contention cell reads the witness in the abandon thread rather than after the join, and its body now takes spawn-like time (200us) because the real body is a fork/exec. RED 12/12 mutated, GREEN 12/12 pristine. With a nanosecond body the same cell caught it 1 run in 20 — recorded, because a sampler is only as good as the window it samples. Corrected on the record: 36dff04's cited mutation (dispatching the start asynchronously) is strictly STRONGER than the defect it was cited for, so the synchronous near-miss was invisible to it. testAbandonAnsweringFalseMeansTheWorkHasAlreadyRun renamed to testBeginPerformsTheWorkRatherThanSchedulingIt — it asserts the sequential contract and cannot see the window; its old name said otherwise. The throwing-start cell is annotated with what it does and does not cover. Measurement note: a first attempt scored the contention cell 0/12 by grepping for "with 1 failure" — the mutated run fails many rounds at once. The predicate is now the absence of "with 0 failures".
…(gate r2 P2, P3)
P2 — the fence matched `posix_spawn…\s*\(` and required `try require(`
immediately in front. Keyed on a CALL, it could not see a function VALUE:
let addclose = posix_spawn_file_actions_addclose
_ = addclose(&fileActions, 5)
The symbol appears with no paren after it, and `addclose(` is not a
posix_spawn spelling — fence green. The gate compiled and ran that to prove
it is working Swift. The property is now on the SYMBOL: every appearance of
a `posix_spawn*` identifier in the spawn path must be the direct operand of
`try require(`. A function value cannot be taken without naming the symbol,
so aliasing is caught at the alias. Exemptions are by property (`_t` is a
type) or by name (two `defer` destroys, the guarded `posix_spawn` itself) —
never by pattern. Acknowledged limit, recorded at the site: a symbol reached
through `dlsym` by string is outside what a source fence can see.
Retired the false comment that claimed "an assignment or alias in front of
it fails loudly instead of vanishing" — it vanished. And corrected the
record: 28a8f15's "5/5 escapes" list silently substituted `setsigmask` and
comment-laundering for the two alias cases round 1 actually found.
P3 — `\s+` -> " " collapsed whitespace BEFORE `components(separatedBy: " ")`
split on two spaces, so the "statements, not lines" split never fired and
every failure quoted the first 90 characters of the whole function. Matching
is now done over the body with line numbers computed from newline counts,
and a failure reads `GitCommandRunner.swift:802 posix_spawnattr_setpgroup
— let setpgroup = posix_spawnattr_setpgroup`. The rationale doc block, which
had detached onto `repositoryRoot`, is back on the fence.
Also closed while rebuilding (gate residuals P8, P9): the region is the
balanced-brace extent of `launch` rather than `range(of: "\n }\n")`, which
was the first four-space-indented `}` INSIDE the function — a pure
truncation escape with all seven checked calls left intact is now caught,
naming :807. Block comments are blanked as well as line comments. The
vacuity floor is 7, the actual count, and `posix_spawn` is now a reachable
exemption rather than dead configuration.
VERIFIED, 15 variants applied to GitCommandRunner.swift, built and run:
RED E1 bare call · E2 assignment prefix · E3 `_ =` prefix
E4 function-value alias · E5 alias via array element
E6 line-comment launder · E7 block-comment launder
E8 four-space-brace truncation · E9 unlisted setsigmask
GREEN R1 wrapper split across lines · R2 `try require (` with a space
R3 comment between wrapper and operand · R4 deep argument wrapping
R5 semicolon-preceded statement · R6 one-line `if`
15/15 as expected, 0 mismatches. Full GitCommandRunnerTests: 33/33 green.
… P6, P7) A residual this project has decided to keep is recorded where it can be reopened, not left for the next round to rediscover. P5, at the docker-prune claim: `begin` performs the launch UNDER the claim's lock — that is the mechanism — so the timer body blocks on it for the spawn's duration, and that body runs on ScanSessionClock's single shared serial queue. No deadlock cycle, but every other bound scheduled there is delayed by a fork/exec, and a `.timedOut` settle overshoots by the same amount. Not fixable by releasing the lock earlier: releasing it is the window LaunchClaim exists to close. P6, at the timeout branch: which arm is taken is pinned by LaunchClaimTests through `didStart`; the two wordings are not. Pinning them would mean hoisting the strings into production API for a test to read, and a source fence over the spellings would be a blocklist wearing a fence's name — the class this branch has retired eleven times. Recorded as uncovered. P7, at the walker's mount-table gate: the table-before-TCC order changes what a root that is BOTH protected and mounted reports on a background scan — silence before, `.mountedVolumeRoot` now. Honest, but user-visible, and the printed remedy is half true for such a root: eject it and the next scan reaches the TCC gate and skips it silently again, with nothing to explain the disappearance. Anchor `ProjectTreeWalker.swift:661-664` repointed to `:671-674` (both citing sites) — the ten disclosed lines shifted it, and the anchor gate caught it before the suite did.
… line (codex r2)
`declaresBare` matched any line whose key was `bare`, anywhere in the file,
and shipped with no test of any kind. git reads two common shapes the other
way:
[core] [core]
bare = false bare = true
[remote "origin"] [core]
bare = true bare = false
Both were read as bare. That is not harmless over-discovery: the scanner
then runs `worktree list` against a healthy non-bare admin directory and
publishes a cross-validation `unreadable` issue on EVERY scan, for a
`--separate-git-dir` repository shape it deliberately does not cover.
Now resolved as git resolves it: section context honoured (`[core "sub"]` is
core.sub.bare, a different key), comments stripped outside quotes, a
variable on the section header's own line read, quoted values unquoted, and
the LAST value winning — so an override can turn bareness OFF rather than
merely failing to turn it on.
The narrow VALUE spelling is unchanged and still disclosed: `bare = yes`,
`bare = 1` and a valueless `bare` key leave the repository undiscovered.
That residual is now PINNED by a cell, so it cannot widen or narrow in
silence.
12 new cells, 12/12 green. MUTATION, measured both directions:
- the any-line matcher reds 7 cells / 8 assertions, including both cells
written for the finding;
- first-wins instead of last-wins reds exactly 2, and nothing else.
GitWorktreeInventoryTests 97/97, WorktreeReclaimPerformerTests 153/153,
SourceAnchorIntegrityTests 6/6 green.
… site (gate r3 P5, P6)
P5 — the r2 note at dockerPrune's timeout branch was false in BOTH halves.
It claimed the two wordings had no cell and that pinning them would mean
hoisting the strings into production API. In fact
`testDockerPruneExpiresReportsAndReleasesTheButton` already pinned the
`didStart` arm by reading `lastDockerPruneResult` — published state three
cells in that file already consume. The other arm is pinned the same way
now, by a new cell, with nothing hoisted.
A wrong residual is worse than none: that one would have sent a future round
hunting for a cell thirty lines away in a file it already reads, and licensed
a swap of the two messages as "uncovered".
MUTATION: give the unstarted arm the started arm's wording ("did not
finish … asked it to stop"). RED 4/4; GREEN 4/4 unmutated. The new cell
carries a vacuity floor — a zero budget must actually reach the
never-started arm, or it pins nothing.
P6 — `ScanSessionClock`'s own doc block still read "nothing blocking may
ever be scheduled on it", absolutely and with an enumerated list, while
since 36dff04 `dockerPrune` schedules `abandon()` there and `abandon()`
blocks on a lock held across a fork/exec. d0a0b66 disclosed that at the
SCHEDULING site only; the site a future round actually reads to answer "may
I block in this body?" still said no. Both ends now say the same thing, and
the exception is named as an exception rather than a precedent.
Anchor drift: the 14-line insertion shifted six SpaceScanner anchors;
repointed at both citing sites (measured +14, not the +15 the edit implied).
…gate r3 P7) `mountTable.contains(root.path)` is EXACT membership against mount POINTS, so a dev root INSIDE a mounted volume — `<volume>/dev`, the ordinary way people put a dev root on an external or network disk — never matches it. The first filesystem contact for such a root is `isProtectedRoot` stage 2's `realpath(3)`, on a path on that volume: precisely the call f4575bf identifies as blocking uninterruptibly on a hung hard mount. Two comments stated the opposite without qualification ("the only ordering in which its own canonicalize cannot be the call that hangs", "what makes the hang impossible"), and the cell that guarded them exercises only root == mount point. The guard was put where the finding pointed rather than at the earliest point the root is read. No policy change, because the project already has a deliberate answer for this shape and it is not the table: the SESSION's wall-clock bound. Both walker callers are `SpaceScanner` conformers, so a blocked `realpath` becomes `.scanDidNotFinish` with the ledger concluded `.boundFired` — the same answer `captureBounded` gives for a container root inside a hung mount, whose own note already says the table preflight never covered it. The same residual carries verbatim: the bound converts the hang into a REPORT, it does not cure it; the thread stays parked until the volume answers. Widening the gate from membership to containment would refuse every root under /System/Volumes/Data, which is a policy decision nobody has taken. New cell `testARootInsideAMountedVolumeIsNotCaughtByTheTableAndIsProbed` pins the honest behaviour — the root IS contacted, no `.mountedVolumeRoot` is emitted — and reds if the gate is ever widened to containment, so that change has to come here and say so. Anchor ProjectTreeWalker.swift:671-674 -> :695-698 at both citing sites. ProjectTreeWalkerTests 36/36, SourceAnchorIntegrityTests 6/6.
…odex r2, gate r3 P4)
strdup (codex r2) — `map { strdup($0) }` wrote a failed allocation's nil
straight into argv, where nil is the TERMINATOR. Under the same pressure the
setup checks exist for, a failed copy of "git" leaves argv
`[/usr/bin/env, nil]`: env runs with no utility, prints its environment and
exits 0, so the runner accepts unrelated output as a completed git command.
The same silently-different-child class as a dropped adddup2, and quieter,
because this one succeeds. Every copy now goes through a throwing
`duplicate`, and each vector's `defer` is registered BEFORE it is filled so
a throw part-way frees what was already copied.
P4 — the fence exempts `posix_spawn` because it "is checked by its own
`guard`", and nothing tested that guard. Nothing COULD: `executableURL`
defaults to `/usr/bin/env`, which always exists, and no test had ever passed
`executableURL:`. It is also the one call whose unchecked failure is
catastrophic rather than wrong — `var pid: pid_t = 0` means a swallowed
failure hands back pid 0 and `signalTree` then runs `kill(-0, SIGTERM)`,
signalling the caller's own process group.
A nonexistent executable makes posix_spawn answer ENOENT on Darwin, so the
throw path is reachable without staging memory pressure.
MUTATION: guard deleted -> RED 3/3. Measured with `signalTree` given a
`pid > 0` precondition in the SAME mutation, deliberately: without that
second edit the mutant signals the harness's own process group, which
destroys the run rather than measuring it. Recorded at the cell.
GitCommandRunnerTests 33/33 before the new cell, 34 after.
…sted past it (gate r3 P2, P3, P8)
Fourth rebuild. Rebuild 3 keyed on a CALL and a local alias walked past;
rebuild 4 keyed on the SYMBOL but scanned only `launch`'s BODY, so the gate
hoisted the same alias one scope up and walked past again:
private static let addclose = posix_spawn_file_actions_addclose
_ = Self.addclose(&fileActions, 5)
Both escapes compiled and ran. The region is now the WHOLE FILE — there is
no scope left to hoist to.
P2 — `commentsBlanked` modelled only plain string literals. One
`private static let rawMarker = #"\"#` anywhere earlier in the file left
`inString` true for the entire remainder (the backslash sets `escaped`, the
closing quote is swallowed), so no comment after it was ever blanked and
comment-laundering came back with the fence green. The file is now REFUSED
if it contains a raw or multiline literal, rather than mis-scanned. String
CONTENTS are blanked too, which also retires `let note = "try require("`
in front of a bare call.
P8 — `try require(Darwin.posix_spawnattr_setpgroup(...))` is correct code
that rebuild 4 reported as unchecked. The wrapper patterns now tolerate an
optional module qualifier. Renaming a wrapper still reds, which is correct
and is stated as an acknowledged limit: the fence cannot know a new name is
a checker.
The fence is now a table of rules, so the new `strdup` invariant rides the
same mechanism: every `strdup` must be the operand of a `guard let`.
VERIFIED, 15 variants applied to GitCommandRunner.swift, each built and run:
RED bare call · `_ =` prefix · local function-value alias
type-scope static func · type-scope static alias (P3, both)
raw literal + laundered call (P2) · line-comment launder
string-literal launder · unlisted setsigmask · unchecked strdup
GREEN wrapper split across lines · `try require (` with a space
module-qualified (P8) · comment between wrapper and operand
semicolon-preceded statement
15/15, 0 mismatches. Several escapes red on the vacuity floor as well as the
unchecked list; the floors are per-rule now (7 spawn symbols, 1 strdup).
Destroy-exemption residual disclosed at the exemption site: it is by name
and does not check the call is in a `defer`, so a destroy before the spawn
is green. Kept — the alternative is a `defer`-proximity spelling test.
…its one call site (gate r3 P1)
`LaunchClaim` is correct and `LaunchClaimTests` proves it — the gate
independently re-measured RED 12/12 on the near-miss. But every one of those
cells builds its own claim and calls `begin` itself. Nothing held
`dockerPrune`, the ONLY production user, to keeping the launch inside the
body: the claim takes a closure, so `begin({})` is writable and the launch is
free to drift back to the next statement, which is the original defect rather
than a variant of it. The gate restored that two-statement shape here and all
1667 cells stayed green.
A test cannot hold this boundary. The damage needs the timer to land in a
fork/exec-wide window, so any cell for it samples rather than proves — I
wrote one, it went green 3/3, and the vacuity floor I added then showed
16/16 rounds reported the arm with ZERO children ever observed, because the
correct shape kills the child before it can write. That green was hollow.
So the boundary moves into the type. `ClaimedProcess` owns the `Process`,
builds it from executable/arguments/environment/pipes, and never hands it
out; `start()` is the only launcher and decides under the claim's lock.
There is no `process` in scope at the call site any more.
VERIFIED: the gate's mutation applied verbatim now fails to build with
`cannot find 'process' in scope`.
LIMIT, measured and disclosed at the type: a caller can still construct
its OWN Process and run it (that compiles). But that starts a DIFFERENT
child than the one the claim guards — a visible act, not silent drift —
and `didStart` would contradict it. The boundary is "the claimed child
cannot start unclaimed", not "nothing may ever be spawned here".
CacheoutViewModelTests 56/56, LaunchClaimTests 6/6, GitCommandRunnerTests
34/34.
…ad (codex r2)
Codex named `HEAD` and `config` in the bare-repository probe. The population
is EIGHT: seven `String(contentsOf:)` and one `Data(contentsOf:)`, across
`GitWorktreeInventory`, `pathContents`, and `WorktreeReclaimPerformer` — a
DELETION path. Every one asked `probeKind` about a PATH and then handed that
same PATH to a reader that resolves it again and FOLLOWS symlinks. Fixing
only the two named would have left six with the identical shape.
Three defects, one primitive:
1. TOCTOU. A `HEAD` or `config` replaced by a symlink between the probe and
the read was opened through the replacement, so an automatic scan of a
dev root can be steered into a TCC-protected or unresponsive target
despite a check that just said "regular file". A path is not an identity,
and asking twice is asking about two objects.
2. UNBOUNDED READS. Any directory under a dev root with the cheap
bare-repository shape but a multi-gigabyte `HEAD` was loaded and UTF-8
decoded in full before anything decided it was not a repository. The size
is now checked against the DESCRIPTOR and the file REFUSED, never
truncated — truncating would let a huge file whose first bytes spell
`ref: refs/…` pass as a valid HEAD.
3. `WorktreeReclaimPerformer.HeadWitness` resolved its file THREE times
(probeKind, identity, bytes), so the witness could pair one object's
inode with another's bytes — and that witness is what the reclaim proves
the far side against. Kind, identity and bytes now come from one
descriptor.
`O_NONBLOCK` is not incidental: `O_NOFOLLOW` does not save the open of a
FIFO, and a named pipe left at one of these names parks the opening thread
until a writer appears.
9 new cells, 9/9 green. MUTATION, each alone and measured:
- drop O_NOFOLLOW -> reds the symlink cell, nothing else
- drop BOTH size checks -> reds the oversize cell, nothing else
(dropping one is caught by the other; the file
can grow between them, so both stay)
- drop S_IFREG -> reds the FIFO cell
- drop O_NONBLOCK -> THE SUITE HANGS, killed at 300 s with no tally.
That is the guard's point, and why the FIFO cell
asserts wall-clock: a reader that parks does not
fail, it stops.
Limits named once (`gitPointerByteLimit` 64 KiB, `gitConfigByteLimit` 1 MiB).
A real repository whose config exceeds the limit stays UNDISCOVERED — the
same silence every bare repository had before fn-4.28, disclosed at the
constant, never a refusal dressed as retryable.
GitWorktreeInventoryTests 97/97, WorktreeReclaimPerformerTests 153/153,
GitConfigBarenessTests 12/12, SmallRegularFileReadTests 9/9.
…ys done it the other way (codex r2)
For a permanently deleted item whose scanner registers no revalidator —
every shipped scanner without one — the leaf binding was taken AFTER
`sizer.measure`. That left a window the whole measurement wide: rename the
target away, install a stranger at the same name, and the binding recorded
the STRANGER. The far-side proof then succeeded (it proved the stranger
against itself), the stranger was destroyed, and the report credited the
ORIGINAL tree's bytes. Contents mode binds first and always has; only this
arm was backwards, 300 lines apart in the same file.
The binding and its `admittedParent` capture move above the measurement,
which is where contents mode has them.
NEW CELL `testItemModeTargetSwappedDuringTheMeasurementIsRefused`: swaps
the target when the SIZER reaches the payload. MUTATION (bind after the
measure, as before) -> RED 3/3, and the mutant reproduces the report
exactly: "the stranger was DELETED" plus an entry crediting 4096 bytes of
a tree it never touched. Unmutated GREEN.
THE PRE-MEASURE BINDING IS `try?`, WITH A FALLBACK, and that is not
incidental. A hard `try` re-tagged the absent-target arms: two
OrphanedCaches cells refuse a directory that appears at a name the probe
found ABSENT, and they assert the refusal MESSAGE on purpose — "had the
replacement landed BEFORE the probe, the probe would have walked it and
refused with user-data-shaped content instead, so the fixture cannot
silently degrade into testing the other arm". A ghost leaf now yields nil
here and the original read still stands exactly where it always did, same
call, same point, same failure. The new binding only ever ADDS a refusal;
it never moves one.
Three fixtures keyed on provider CALL ORDER needed re-arming, and the
production order is why:
- SwapAtTheDisposalContainerProofProvider counted descriptor-identity
questions after the revalidator gate. The admitted-parent capture used
to be #1 and the disposal's container proof #2; the capture now happens
before the gate, so the proof is #1. What the swap targets is unchanged.
- TempRaceWonAtTheFinalCheckProvider opened its window on ANY
descriptor-identity call. Two now happen before the revalidation, so it
opened too early and the swap landed ahead of the revalidator's own path
check — which caught it, making the cell prove a different guard than
its name. It now gates on `ownerUID(ofDescriptor:)`, the revalidator's
own accessor, as its sibling fixture already did.
`token` and the success tail moved inside the widened `do`; a failure on any
path still reaches the catch with the token abandoned.
Anchor CacheCleaner.swift:1496 -> :1543 at all four citing sites.
Full suite: 1692 executed, 3 skipped, 0 failures, exit 0.
…eported as success (gate r4 P4) LIVE DATA DESTRUCTION, reproduced on 70f4376 before the fix: item mode, a scanner with no revalidator, a target already unlinked when cleaning starts. The pre-measure bind answers absent, so `preMeasureLeaf` is nil; the `??` fallback then re-reads the leaf at the original point, and if an object has appeared in between it binds THAT one. The far-side proof succeeds — it proves the newcomer against itself — the newcomer is destroyed, and the item reports SUCCESS with `errors: []`. The r2 comment said that fallback was "the original read … same call, same point, same failure". The third clause was false: the re-read can SUCCEED, on a different object. Nothing inspected it and nothing measured it, so the report has no bytes to be wrong about; it simply deletes a stranger and calls it a success. Pre-existing rather than introduced by the r2 hoist — the hoist closed this window for leaves that bind pre-measure and left it open for ghosts — and it was disclosed nowhere. The population with the hole is exactly the one the r2 commit said it was fixing. The re-read still happens, because a still-absent leaf must raise its ENOENT at the original point with the original identity: the absent-target arms pin that message on purpose, so a fixture "cannot silently degrade into testing the other arm". What changes is what a SUCCESSFUL re-read means — a drift event, refused as `.notTheInspectedObject` (tag `content-drift`), not a target. NEW CELL `testAStrangerArrivingAtAGhostTargetIsNeverDeleted`, written BEFORE the fix and RED on it 3/3: "a stranger that arrived at a GHOST target's name was DELETED", plus a success entry and zero errors. GREEN after. It carries a vacuity floor (`provider.planted`). Deletion-path suites after the fix: CacheCleanerTests, OrphanedCaches 120, EphemeralTemp, WorktreeReclaimPerformer — 436/436 green.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
…nce read one file (gate r4 P1, P2, P3)
P1 — the unchecked `map { strdup($0) }` that 2d5b351 fixed in the spawn path
was ALSO in `CLIHandler`'s `execv` re-exec, on the shipped Homebrew install
route. Nil is argv's TERMINATOR: a failed copy of element k truncates the
command, so `cacheout install-helper` through the documented symlink
re-execs into a no-subcommand `cacheout` — and the process image is already
replaced, so nothing can report it. Every copy now goes through a nil-check
and a failure declines the re-exec instead of performing a different one.
The fence could not see it because it read ONE FILE. That is the third time
this fence has been beaten by moving one scope out: rebuild 3 keyed on a
CALL (a local alias walked past), rebuild 4 on the SYMBOL but within one
function BODY (the alias hoisted to type scope), rebuild 5 on one FILE (the
defect moved to a sibling file). The scope is now every Swift file the
repository tracks, via `git ls-files` — the repository's own list, not the
machine's. There is nowhere left to move.
P3 — the allocation rule matched `\bstrdup\b` alone under a heading reading
"EVERY ALLOCATION", so `strndup`, `malloc` and `calloc` walked past a cell
whose name promised otherwise. It is now the family, and the wrapper is
`guard let` / `if let` rather than one spelling.
P2 — the scanner blanked string CONTENTS, which also blanked interpolated
EXPRESSIONS, so `_ = "\(posix_spawn_file_actions_addclose(&a, 5))"` compiled,
ran, and was invisible to every rule. Blanking strings was itself rebuild
5's mitigation for a different escape: the fix introduced the hole.
`scannableSource` is now a real Swift lexer — nested block comments, plain /
raw (`#"…"#`, any hash count) / multiline strings, correct escape and
interpolation handling per hash count — because three of five rebuilds were
defeated THROUGH the scanner rather than around it, and because refusing
files with raw or multiline literals is not available: the fence must cover
`CLIHandler.swift`, which has both. It carries 8 cells of its own, each
asserting newline count so a scanner that eats a newline cannot send a
reader to the wrong line.
VERIFIED, 10 variants built and run across two files:
RED unchecked strdup in CLIHandler (P1) · call inside an interpolation
(P2) · unchecked strndup · unchecked malloc (P3, both)
alias at type scope · raw literal + laundered call · bare setpgroup
GREEN module-qualified · wrapper split across lines · `if let` allocation
10/10, 0 mismatches. Per-rule vacuity floors: 7 spawn symbols, 2 allocations,
plus a file-count floor and a required-membership check for the two files
that actually hold a guarded allocation.
GitCommandRunnerTests + CLIHandlerTests 42/42.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c57a0d7a14
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| event.entries.contains(where: { | ||
| ($0.name == "refs" || $0.name == "reftable") && $0.kind == .directory | ||
| }), | ||
| resolver.bareRepositoryGitDirectory(at: event.directory) != nil { |
There was a problem hiding this comment.
Validate bare repositories through the walked directory
When the visited directory or one of its ancestors is renamed and replaced with a symlink after the walker produced event.entries, this call reopens event.directory by absolute path instead of inspecting the directory descriptor that supplied those entries. bareRepositoryGitDirectory consequently probes and reads HEAD/config from the replacement (its O_NOFOLLOW applies only to those leaf files), and the scanner can then run git worktree list against a repository outside the configured root or block on an unresponsive replacement. Bind the bare-shape validation to the walk's open directory, or re-establish descriptor-relative containment and identity before reading metadata.
Useful? React with 👍 / 👎.
| } catch { | ||
| // `env` itself is missing/unrunnable — the same class of answer | ||
| // as "git is not on PATH", never a silent zero. |
There was a problem hiding this comment.
Preserve transient spawn failures as retryable
When spawn setup or posix_spawn fails transiently with errors such as ENOMEM or EAGAIN, this catch collapses the failure into .gitUnavailable. Downstream, GitWorktreeScanner withdraws every result and emits its .toolUnavailable issue, while clean-time paths tell the user to retry once Git is installed, even though Git was never proven absent; the new unavailabilityIsDefinitive distinction is not consulted there. Keep transient launch failures in a retryable execution-failure outcome and reserve .gitUnavailable for the definitive /usr/bin/env exit-127 case.
Useful? React with 👍 / 👎.
…e claims that do not reproduce (gate r4 P6, P7, P8) P7 — the SpaceScanner probe anchor shipped ROTTED through 1a3bb64, the very commit whose job was repointing anchors. That commit measured the +14 shift correctly and repointed six anchors; it missed this one. The integrity cell stayed green because it asks only whether the excerpt lies SOMEWHERE inside the cited range, and 19 lines of range absorbed 14 lines of drift — so six citing sites pointed at a doc-comment continuation instead of the probe. Repointed to :2102-2120 at all six. The tolerance that hid it is now disclosed precisely, with the known fix named: carry each excerpt's expected OFFSET within its range and assert it exactly. Measured across the 58 rows, offsets legitimately run 0 to 58, so no fixed tolerance works and the offsets must be recorded per row — a schema migration that belongs in its own increment rather than bolted onto a review-fix round. (Writing the historical range into that disclosure made the gate red on it, correctly: default-deny treats any `File.swift:NNN` spelling as a live pin. Respelled as prose.) P6 — the byte-limit disclosure said a file past the limit "stays UNDISCOVERED … never a refusal dressed as retryable". True of `gitConfigByteLimit`, whose one path treats nil as "not a bare repository". FALSE of `gitPointerByteLimit` at two sites: a `.git` pointer past it yields `ambiguous` ("Re-scan once that path is settled") and a `gitdir` back-link past it yields `.incomplete` and then "the prunable set is not provably complete". The limit is a FIXED CONSTANT, so for that cause a retry can never differ — a permanent strand wearing a retryable message, the class this project refuses everywhere else. Kept (the messages are shared with genuinely transient causes, and no git writes a 64 KiB pointer file) and now disclosed at the constant AND at both refusal sites. P8 — two evidence claims in 70f4376 do not reproduce, both mine: (a) "three fixtures keyed on provider CALL ORDER needed re-arming" — the diff contains exactly TWO, and the commit then enumerates two. (b) "the mutant … crediting 4096 bytes of a tree it never touched" — measured: the entry is `exactBytes: 0, estimatedUpToBytes: 0`, because the sizer's size read lands after the rename. The defect is unchanged and no smaller — a stranger nobody inspected is destroyed and reported as SUCCESS — but the bytes half was wrong, and a wrong detail in an evidence note is how the next round is sent looking in the wrong place. Corrected at the cell, where a future round reads, not only here. CacheCleanerTests 110/110, SourceAnchorIntegrityTests 6/6, GitWorktreeInventoryTests + anchors 56/56.
…4 P5) `smallRegularFile` shipped with nine cells and they proved nothing about whether anything USES it: the gate reverted two of the eight converted sites to their pre-PR `String(contentsOf:)` / `Data(contentsOf:)` shape and the full 1692-cell suite stayed green. The steady state is not the gap, which is why this was easy to miss. Every one of these sites still asks `probeKind` first, so a symlink merely SITTING at the name is refused under either shape. What the descriptor buys is the RACE — the probe answers about a path and a path-based read then resolves that path AGAIN — so a cell has to stage the window. These fixtures plant the symlink on the way out of the probe, truthfully answering `.regularFile` about a file that IS one, and swapping only the timing. MUTATION, measured: reverting the `HEAD` read reds `…ABareRepoProbeDoesNotFollowASymlinkPlantedAfterTheProbe` and nothing else; reverting `pointerPath`'s read reds `…AWorktreePointerReadDoesNotFollowALatePlantedSymlink` and nothing else. COVERAGE IS TWO OF EIGHT, disclosed at the file rather than implied away. The other six — including `WorktreeReclaimPerformer.headWitness`, which its own converting commit called "what the reclaim proves the far side against" — still have no call-site cell. The two here are the two distinct mechanisms; the rest being the same shape is a reason to expect them correct, not evidence that they are. Both cells carry a control that must resolve with nothing swapped, and the second one earned it twice over: its first version was GREEN AND VACUOUS, answering nil because the fixture wrote a `gitdir:` prefix into the admin back-link where real git writes a bare path. Without the control it would have reported the read as guarded when nothing had been read at all — the same failure shape as the hollow cell in gate r3. GitMetadataReadCallSiteTests + GitWorktreeInventoryTests + SmallRegularFileReadTests 61/61.
…onger indexes `StrandFenceTests.testNoLoopBoundIndexSubscriptCanStrandTheRun` flagged seven integer subscripts in the scanner and tally added for gate r4 — correctly: a loop-bound index traps and kills the PROCESS, not the cell, which is how 985 and 493 cells were skipped at earlier rounds. `XCTUnwrapElement` is the sanctioned idiom but it is an assertion helper, and a lexer's inner loop is the wrong place for one. The scanner now consumes an `ArraySlice` from the front — `first`, `starts(with:)`, `prefix`, `dropFirst` — so it cannot read past the end AT ALL. Bounds safety is now a property of the operations rather than of my arithmetic, which is a better answer than either subscripting carefully or asserting per character. The per-rule tally is keyed by rule name instead of indexed for the same reason. Re-verified after the rewrite, because a scanner rewrite invalidates every result that depended on it: the same 10 fence variants behave identically (9 measured here, the `if let` reformat measured separately as GREEN), the scanner's own 8 cells pass, StrandFenceTests 14/14, GitCommandRunnerTests 41/41. Full suite: exit 0, 1702 executed, 3 skipped, 0 failures, "All tests passed" printed once, 0 error lines — whole log captured and grepped, not tailed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21c48b523a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for rawLine in configContents.split( | ||
| whereSeparator: \.isNewline | ||
| ) { |
There was a problem hiding this comment.
Honor included overrides before declaring a repository bare
When the primary config contains core.bare = true followed by an [include] path = ... whose included config sets core.bare = false, Git treats the repository as non-bare, but this loop examines only the primary configContents and returns true. Fresh evidence after the core-section/last-value fix is that include directives remain ignored: the directory is added as .bareRepository, then the non-bare first record from worktree list fails crossValidate in GitWorktreeScanner and produces a recurring .unreadable issue for a valid repository the scanner intends to leave undiscovered. Either resolve includes safely or fail closed when an include could override the local value.
Useful? React with 👍 / 👎.
Closes out the fn-4 hardening backlog — the 18 defects filed against
mainduring the fn-5/fn-6 review campaigns, worked in three gated rounds on this branch. Every fix was reproduced red-first, every guard arm is mutation-evidenced (red rate over ≥8 runs where timing-dependent), and each round ended with an independent read-only gate. Round 3's gate additionally re-ran the named signature cells from rounds 1–2 and PR #460 — the disposal bindings, drain fences, capture bound, walker clamp, sizer arms — all green.Suite: 1665 executed / 3 skipped / 0 failures, exit 0 (
swift test, at commit2bb2596, 214 s). 29 commits, 60 files, +7,221 / −857. Forked frommainat8a38e3f.What a user gets
.timeoutat the execute boundary — the only place it could be caught; measured, no downstream parser can see the truncation.realpaththrough~/Documents. Order inverted at every pointer-derived path — plus three sibling arms the spec didn't name, one where canonical equality answered true for a deferred target.dockerPrune's whole child interaction is raced (the read parks before the wait — bounding only the wait would have moved the strand one line); zero barewaitUntilExit()remains in production, enforced by a grep-gate cell.mutation_scope_refused; no wire string renamed.posix_spawn+POSIX_SPAWN_SETPGROUP), never discovered from a live leader — the exited-leader race is gone and descendants are reaped by PID.Test integrity
as!/fatalError/preconditionFailure) are statement-trap-fenced across all test sources; the reproduction aborted the process with 38 cells unrun and a tally reading 0 failures.Remedycarries no free text, retirement is a production declaration, and a newEstablishedcase fails a named cell without anyone editing the test's lists. Attack round: 0 of 11 fresh false wordings pass.Measured, deliberately not built
The dev-root "double walk" (Codex P2) is 2.2% duplicated probes, not doubling — 249 of 11,544 on an artifact-bearing 5,772-entry tree, because the two walks are asymmetric (one prunes matched dirs, the other must not). Buying 2.2% would rewrite the scanner I/O contract. The measurement cell ships executable (
DevTreeWalkMeasurementTests) so the decision can be re-derived when tree shapes change.Review round 1 (Codex, on
efc2760)Three findings, all on code this PR added — the pattern the campaign documents: each round's findings attack the last round's fix. All fixed and mutation-proven.
FirstWinsRendezvousdecides which outcome is reported, not whether the work started; a queued detached task couldrun()after the timeout branch truthfully reported nothing running and re-enabled the button. The starvation the off-pool timer exists to survive is exactly what keeps the task queued. NewLaunchClaimdecides the pair under one lock. Mutation: 504 red, evidenced by 500 concurrentbegin/abandonpairs requiring exactly one winner each.MOUNT FIRSTsaid since fn-4.12, while the probe ran above it. On an unresponsive mount thatlstatnever returns, so the promised issue is never emitted and the session leaves a blocked worker. The kernel table costs no filesystem call; asking it first is free. The cell counts probes rather than hanging to prove a hang.posix_spawnsetup returns. A droppedadddup2spawns a child whose stdout goes elsewhere: git exits 0 and the empty buffer is accepted as complete — fn-4.24's class re-entering through fn-4.27's spawn. Nowtry require(...)d. Negative result recorded: the failure cannot be staged behaviourally, andlaunchwas not reshaped to make it reachable; a source fence with a vacuity guard asserts it instead.Known residuals (each disclosed at its site)
.utilitythread until its volume answers — one per timed-out attempt.