feat: one seam for every .procoder/ read and write (#117 phase 0)

[piwi3910]

What

Every read and write under .procoder/ now goes through one package,
internal/store, which locks the file, writes it atomically, and knows
which repository it is serving. Twenty-six packages lose their direct
filesystem calls; none of them changes an exported signature, and none of
them changes what procoder prints.

One thing is visible to users: procoder config gains a repo identity
line, and .procoder/config.toml gains an optional [service] repo key
that overrides the computed answer.

This is phase 0 of #117 — the seam the local daemon needs. There is no
daemon here, no socket, no server, and nothing that uses the seam yet.

Why

Issue #117 proposes running procoder as a service. A daemon serving
several sessions and several repositories cannot be built on what is here
today, because of three defects that are already real and merely rare:

Nothing locks. There was no locking anywhere in internal/ — the only
mutex in the tree was in a test. dispatch.json, claims.json and the ask
ledger are read-modify-write: load the whole file, change one field, write
it back. Two writers lose one of the two updates, silently. That is rare
only because every hook is its own short-lived process.

Nothing is atomic. Every write was an os.WriteFile, which truncates
first. A process killed in that window leaves a truncated claims.json,
and the next reader reports a corrupt ledger for a crash that had nothing
to do with it.

There is no repository identity. The only key procoder had was the
filesystem path, which means something different on every machine. One
daemon serving ten checkouts cannot tell them apart with that.

Fixing them now, with no daemon in the picture, is cheaper than fixing
them as the daemon's first three bugs.

How

internal/store has three primitives and one named operation per owner on
top of them.

The lock is an O_EXCL lockfile under .procoder/state/locks/, not
flock. go.mod has no require block at all, and a portable
flock/LockFileEx pair costs golang.org/x/sys; procoder is not
spending its first dependency on this. TestNoModuleDependencies keeps
that claim honest. Locks are taken in sorted path order regardless of the
order the caller asked for, so two callers wanting the same two files
cannot deadlock. A failed multi-lock releases what it already took.

The stale rule is the part worth reviewing. A lock is dead when its
own mtime is older than 30s; the contents get a say only when they parse.
That is not where it started — see the note below.

Writes stage into a temp file in the destination directory (rename is
only atomic within a filesystem), fsync, then rename over the target. A
reader sees the whole old file or the whole new one; a failure leaves the
old one untouched.

Identity resolves down a fixed ladder: [service] repo, then the
origin remote, then the first remote alphabetically, then the resolved
absolute root path. It always answers and never returns an empty key.
origin beats an alphabetically earlier remote deliberately — pure
alphabetical order is simpler and wrong, because a colleague who adds a
personal remote named fork would key the same repository differently
from everybody else, which defeats the one thing an identity is for. The
rung comes back with the key, so a surprising identity is traceable rather
than mysterious.

The payload is bytes, not each owner's type. A store returning
[]dispatch.Wave would import dispatch, which imports the store — an
import cycle. Marshalling stays where it already was; only the filesystem
call moved.

A bug found mid-build, and the spec correction it forced

Task 5's concurrency test found a defect in the lock written three commits
earlier — one that had already been mutation-checked and closed.

O_EXCL creates the lock file empty; the pid and timestamp are written
a moment later. So for that instant, every live lock has contents that do
not parse. The original rule — unparsable contents mean stale — let a
second caller break a lock created a microsecond earlier and take it. Two
holders of one lock, which is precisely the defect this package exists to
remove. It showed up as six of twenty concurrent appends still going
missing after the lock was supposedly in place.

The acceptance criterion was wrong as written, not merely wrongly
implemented. The spec, the plan and the already-closed task record were all
corrected (e440d41), the last under a dated correction heading, so none of
them reads as though the first attempt had been right.

Three helpers the plan did not anticipate

• Rel — several packages list .procoder/ files as absolute paths
  and hand them around (spec.Files, plan.Files, analysis.Files,
  Item.Path, Task.Path). Their readers would otherwise have to reach
  past the store to open what they were given. Rel refuses a path outside
  root, so "read what I was handed" cannot become "read anything".
• OpenIn — the index's tag file runs to megabytes and two readers
  scan it line by line. Handing them the bytes would undo that.
• inDir / markerPath — refuse a name containing a separator.
  Nothing passes a hostile name today, but joining an unchecked name lets
  .. walk out of the directory the caller named.

Four hand-rolled atomic writes found and removed

envsync, internal/ask, internal/codeindex and the index Refresh
each had their own temp-and-rename. All four are now the store's, which
does the same thing and also takes the lock none of them had — so an index
Refresh firing from the write hook can no longer race an index build
running in another session.

Testing

go test ./... green. procoder check clean on every commit — the commit
gate ran on all fifteen.

Parity is proven, not asserted. TestMigrationOutputUnchanged compares
procoder status, the SessionStart principles hook, the Stop hook's
handoff note and procoder config against committed goldens. The first
three goldens were captured from a binary built at c4bb353 — the commit
before internal/store existed — in a throwaway worktree, and diff
reported no difference. That equality is the whole value: a golden
regenerated from current code only asserts that the code does what it does.
The config golden is captured from current code instead, because that
output changed on purpose.

TestCapturesAreDeterministic earned its place immediately: the handoff
note stamps the wall clock, so the first goldens failed one second after
being written. That line and the config report's absolute root path are now
replaced by line rather than dropped, so the fact that each is printed is
still asserted.

Functional equivalence against the pre-seam binary

The goldens cover four captures. That is not the same as "the CLI still
does what it did", so the whole surface was compared: a binary built at
c4bb353 and one built at HEAD, run against fresh identical fixtures
carrying a file in every .procoder/ owner, with volatile values held out
by line.

Comparison	Result
58 CLI invocations across all 47 commands	57 identical, 1 intended
37 files written under .procoder/ after exercising every write path	36 byte-identical, 1 explained below
8 index query commands after a real index build	identical
procoder check and all four hooks (both PostToolUse and both PreToolUse shapes)	identical
17 commands × 2 repo shapes with no .procoder/ at all, with and without git	identical, except the same one
13 commands × 19 broken-input scenarios — corrupt JSON, corrupt config, empty files, binary files, missing directories, a file where a directory belongs, unreadable files, read-only trees, CRLF, a BOM	226 identical, 19 the same config line, 2 investigated below

245 comparisons. Two differences that are not the repo identity
line, one of them mine.

The write paths matter most here, because that is what the store changed:
dispatch open/start/seal/return, claims add/list/release, env --sync,
index build, bench --save, sprint open, todo close, backlog close story, ask, and all four hooks were run in sequence, then the resulting
trees compared file by file.

One was my test harness, not the product. The binary-spec scenario
filled the file from /dev/urandom, so the two fixtures never held the
same bytes. Re-run with identical bytes, every command matches.

One is a real behaviour change, and it is the only one in the diff.
With .procoder/state/ read-only and the rest of the tree writable,
procoder ask used to write .procoder/ask/QA.md and now refuses: every
file's lock lives under .procoder/state/locks, so an unwritable state
directory stops writes to content that has nothing to do with state.

The condition is narrower than it first reads — it bites only while
.procoder/state/locks/ does not yet exist, and the first successful
write in a repository creates it. dispatch and env --sync refuse on
both binaries there, since their targets are themselves inside the
frozen directory; only their messages differ.

Accepted deliberately rather than worked around, and the spec says why.
Locks beside their files show up in git status and in review. Locks in
the OS temp directory stop being mutually exclusive the moment two
processes have different TMPDIR values — on macOS the ordinary case
across sessions — and a lock that silently stops locking is the one
failure this package cannot have.
TestReadOnlyStateBlocksContentWrites pins it, so it stays a decision.

The one file that differed was not a regression. .procoder/index/refs.json
came out byte-different — but the OLD binary produces a different
refs.json on every run too, which is what checking it against itself
showed. Canonicalising the JSON makes the two compare equal, so what varies
is element order from map iteration somewhere between SCIP and the merged
file. Pre-existing, filed as its own task, and the same class as #236 and
#245 — unnoticed only because the index is gitignored.

Three structural guards keep the seam from eroding: no direct
.procoder/ file IO outside the store (go/ast walk), every .procoder/
path literal listed with the operation that serves it, and no module
dependencies.

Twenty-one mutations run, each with a snapshot taken immediately before and
restored immediately after. Every one was caught. Examples: dropping the
sort deadlocks the ordering test; emulating the old truncate-first write
fails both atomicity tests; removing the origin branch makes
origin-plus-fork return the fork; splitting the git config key on dots
loses an up.stream remote; adding a direct os.ReadFile on a
.procoder/ path fails the IO guard.

Three mutations were not valid and are recorded as such rather than
counted. Reverting a status.go call to os.ReadFile broke the build on
unused imports instead of failing the test. Removing the break-file guard
with if false left a variable unused and did the same. And a sed for the
heartbeat line matched the wrong indentation and silently changed nothing,
which looked exactly like a passing mutation — the reason to read what a
mutation actually did rather than trust its exit code.

What the pre-PR review found

A fresh-context reviewer read the whole diff before this was opened and
returned 1 Critical, 8 Important, 7 Minor. Every Critical and Important
is fixed in the six commits at the head of this branch. The Critical is
worth reading in full because it was in the part of the package that exists
to prevent exactly it.

Critical — two callers could break one stale lock and both hold it.
Judging a lock stale and removing it were two operations with nothing
between them. Two callers that both judged the same lock stale interleaved:
the first removed and re-created, the second removed what the first had
just created, and both then held. Breaking is now serialised through an
O_EXCL break file, so the second caller never reaches its remove.
TestBreakingAStaleLockIsSerialised proves it deterministically —
TestOnlyOneCallerEverHoldsALock sits beside it and is documented in the
source as not the proof, because the window is two syscalls wide and
sixteen goroutines rarely land in it.

The eight Important, each fixed:

Finding	Fix
Release removed by path without proving ownership	os.SameFile against the file it created
A held lock was broken at 30s — nothing refreshed the mtime	Heartbeat every 10s; mtime is now the sole liveness signal
The rename was not fsynced, only the data	syncDir after the rename, best effort
A port made host:8443/o/r and host/8443/o/r one key	Numeric port stripped from the authority
@ searched the whole URL, so host/o/r@2 returned 2	Bounded to the authority
An empty normalised URL was returned as a real identity	An empty result means the rung did not answer; the ladder continues
Rel ignored symlinks — refused legitimate paths and allowed an escape through a link inside the root	Both operands resolved
AppendLearn was an unbounded read-modify-write on every command	O_APPEND under the lock again

The IO guard was itself failing open, two ways, and the reviewer
bypassed it twice: constants were collected per file while Go constants
are package-scoped, and a path held in a local variable was invisible. Both
bypasses now fail the test — and the taint tracking immediately found a
real miss in internal/hook that reading had not.

Seven of the eight simplification findings were taken, including deleting
forceRenameFailure and its wrapper from the shipping binary. One was
declined and is recorded here rather than silently: Lock stays variadic
with sorted ordering, because S-5 in the spec requires it and has a
passing test. That is the shape a YAGNI rationalisation takes, so it is
stated plainly for you to overrule.

And a second pass over the fixes

The same reviewer verified the fixes and returned 0 Critical, 4
Important, 6 Minor. It confirmed the Critical is properly closed and
could not reproduce it. Of the four Important, two were caused by the
fixes themselves, which is the argument for the second pass:

• go test -race failed. The heartbeat read staleAfter on its own
  goroutine, so every test restoring it raced. CI does not run -race, so
  this would never have gone red — it would just have meant nobody could
  use the detector on the one package where they most want to.
• The heartbeat kept a stranger's lock alive. It touched by path with
  no identity check, so if our lock was ever broken it went on refreshing
  whichever file replaced it — and that lock would then never be judged
  stale even after its own owner died.
• The unguarded stat-then-remove I had just removed from the lock was
  reintroduced on the break file, in both of its removes. The double-hold
  did not follow, because each entrant re-checks stale(p), but the mutual
  exclusion the scheme is named for was gone in that window.
• A bare or file:// remote lost its identity entirely — a behaviour
  regression against main. /srv/git/repo.git has no host, so the origin
  rung stopped answering and two clones of one bare shared remote keyed by
  their own checkout paths, with procoder config reporting "no remote"
  when there plainly was one.

Every one is fixed, and the six Minor with them: resolve could recurse
forever on a filesystem root it could not resolve; the release closure
panicked if called twice; brief() used a 60ms window that is below mtime
granularity on HFS+, exFAT and container overlay filesystems; the IPv6 scp
form was mangled; and doubled slashes, a .GIT suffix and a query string
each produced a second key for one repository.

The IO guard missed three more shapes, one live in the tree: a path
assigned to a struct field (t.Path, it.Path), an alias, and a range
variable. All three are caught. The fourth — a helper that returns a
.procoder path — is not, and the source says so; a documented gap is not
a gap that fails open by accident.

go test -race ./... is green.

Notes for the reviewer

Start at internal/store/lock.go. The stale rule is the subtlest thing
here and it has already been wrong once. stale() decides on the file's
own mtime first and only then on its contents; TestNewbornLockIsNotStolen
pins the case that broke.

Then internal/store/identity.go — normalise is the other place a
mistake is expensive, because two different repositories colliding on one
key would be silent.

The twenty-six migrated packages can be skimmed. They are mechanical,
their exported signatures are unchanged, and the parity goldens are what
say so. If you want a sample, internal/backlog and internal/todo are
the ones that needed Rel.

Windows found a real bug, and CI is what found it

The first CI run on this branch failed on windows-latest while ubuntu and
macOS passed, and main is green — so all of it was this branch's. It is
the clearest argument in the PR for the matrix, because none of the local
work would ever have found it.

Windows reports a file pending deletion as "Access is denied", not
"already exists." !os.IsExist(err) therefore treated ordinary lock
contention as fatal, and TestConcurrentAppendsBothSurvive lost five of
twenty appends there — the exact defect the lock exists to prevent, on the
one platform nobody had run it on. A create that failed is not proof the
lock is unobtainable; only the deadline is. Every create error now retries
until the deadline, and the deadline message carries the cause when it is
not plain contention.

A second, quieter one: release removed the lock while the heartbeat
could still be mid-os.Chtimes, and Windows will not remove a file another
handle has open. The removal failed silently and the lock sat there until
the next caller had waited out the whole timeout. Release now waits for the
heartbeat to return, and retries the removal.

Three more were tests written against POSIX rather than against the tree:
the read-only-directory tests cannot work on Windows at all (os.Chmod
toggles a file attribute there and does not restrict a directory, so they
skip and say so); the identity path-rung tests compared against a native
path while IdentityFor deliberately returns a slash-separated one; and
every golden failed on a difference nobody wrote, because a Windows
checkout converted the committed goldens to CRLF. .gitattributes pins
them — fixing the cause rather than teaching the comparison to ignore it.

CI is now green on ubuntu, macOS and Windows.

Known gaps, deliberate

• learn's record file has no bound. maxRecords shipped unwired in
  3baaac6 with a comment claiming a bound nothing enforced. This branch
  deletes the constant — it stops the tree lying about the gap, which is
  the part this branch is answerable for — and files the fix as its own
  task, because what learn retains is not what this branch is about.
• Nothing tests f.Sync(). Its failure mode is power loss between the
  rename and the data reaching disk, and no harness here produces that. The
  call stays because the pattern is only correct with it.
• procoder check and the two tool hooks are not golden-captured. All
  three exec gitleaks, semgrep, golangci-lint or the test runner, whose
  presence and version vary by machine; a golden of those would pin one
  laptop rather than procoder's behaviour and would fail in CI for reasons
  no change caused. Recorded in the spec's Out of scope.
• os.Stat is not guarded. Asking whether something exists is not
  reaching past the store, and gate adoption legitimately stats
  .procoder/ to decide whether a repository opted in.

Two process deviations, both deliberate

The plan's task order was changed before any code was written. It put
every guard last; the parity harness is the only thing that catches a
behaviour change in the twenty-six-package migration, so building it
afterwards means the riskiest task runs with no net. Task 7 became 7a
(harness, before the migration) and 7b (structural guards, after).

This branch is not in a worktree, which WORKFLOW.md asks for. It was
committed on a branch in the main checkout. The branch is clean and the
default branch was never dirtied, but it is a deviation and not worth
hiding.

Docs impact

procoder config prints a new line and .procoder/config.toml accepts a
new key, so docs/configuration.md gained a [service] section (8d60453):
the ladder table, the URL normalisation, why origin wins, and when to set
the key by hand. The README describes .procoder/ generically and links to
that page rather than enumerating keys, so it needs no change. Everything
else in this diff is internal plumbing with no reader-visible surface —
which the parity goldens are the evidence for.

Closes nothing on its own; #117 stays open for phase 1 (the local daemon).
Team mode is designed and parked in #248.

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.