fix(session-lock): identify a harness session on Windows/Git Bash - #3553
lmktechnology wants to merge 6 commits into
Conversation
AGENTS.md is loaded into every firstmate session on every turn, so anything in it that only applies to a nameable situation is paying always-loaded cost for occasional value. Apply the firstmate-coding-guidelines knowledge placement tree to every section and relocate content to its real owner. No behavior or safety boundary changes: every hard rule, escalation boundary, and lifecycle contract is still enforced, either inline because it applies every turn or through a skill trigger that fires on the same condition that made it visible before. AGENTS.md drops from 72768 to 50720 bytes (30.3% smaller). Relocations: - Section 2's exhaustive state/ and config/ tree collapses to an orientation tree of the paths referenced constantly, plus the standing rule that every other record is written and retired only by its owning script. It was a one-owner violation: docs/configuration.md already declares itself the single owner of the home layout and explicitly declines to hold one exhaustive state tree. - Section 3's nine-stage digest enumeration was a drifting second copy of bin/fm-session-start.sh's header. The behavioral contract stays: run once, read once, lock-refused read-only, ABSENT semantics, deferred network, bootstrap consent. - Section 4's quota selection procedure moves to quota-array-dispatch and the effort fallback to harness-adapters, both of which already owned them. The intake boundary, load trigger, and malformed-config refusal stay inline. - Section 7's validation-run supersession sequence moves to a new agent-only validation-run-changes skill; the decision-delivery contract moves to ask-user-authority, which already owned the decision itself. The custom check authoring contract moves to bin/fm-check-register.sh's header and the promoted-worker instructions to bin/fm-promote.sh's header. Cross-references updated so no contract has two owners: project-management no longer restates the per-task surface classification, quota-array-dispatch states the split with section 4, and bin/fm-spawn.sh's muse effort comment now cites harness-adapters for the max-effort rule. bin/fm-classify-lib.sh documents its own open-decisions cursor. bin/fm-lint.sh and bin/fm-doc-audience-check.sh pass.
Restores the captain's in-progress WIP (herdr.sh treehouse-lease helper, fm-pr-lib.sh chmod-tolerant mode matching, fm-teardown.sh quarantine validation, new PR-check-security migration tests, plugin JS edits) that was stashed to allow main to sync to real upstream. Drops the local edits to bin/fm-pr-check-migrate.sh since upstream already removed that file.
Every session start on Cygwin (Git for Windows) refused the fleet lock and dropped to read-only with "cannot locate harness process in ancestry", so spawning, steering, merging, the wake-queue drain, and supervision repair were skipped on every start. Two independent causes, both on the identity path. Cygwin's ps has no -o option at all and fails the whole invocation with "unknown option -- o", so the ancestry walk aborted on its first hop. The walk now reads comm, args, and ppid through accessors that fall back to Cygwin's fixed ps columns, leaving the procps/BSD path unchanged. That alone does not resolve the session: the parent link from a shell the harness spawns does not cross the Cygwin boundary, and Cygwin reports that shell's PPID as 1, so no walk can reach a harness that is a native Windows process. Identity is instead taken from the session pid the harness publishes and confirmed against the Windows process table before it is used - the pid must still be live and its executable must independently identify a verified harness - so an absent, stale, or non-harness value is discarded rather than bound. Walking the real Windows parent chain was implemented and then removed as unsafe. MSYS emulates exec by spawning a fresh Windows process and exiting the old one, so intermediate shells vanish and a child's recorded parent is routinely a pid that no longer exists; Windows never reparents an orphan, so that dangling id stays and can be reissued to an unrelated process. Following it can bind a home's lock to the wrong process, which is the failure this file exists to prevent. A harness that publishes nothing stays unresolved, which leaves the session read-only exactly as before. Windows pids are tagged rather than stored bare. They are a different namespace: kill -0 reports a live Windows process as dead, and the number can collide with an unrelated live Cygwin pid. The tag makes the value non-numeric, so a consumer that treats it as a local pid - including a future kill - refuses it instead of acting on the wrong process. fm-sessionstart-nudge.sh carried a private second copy of the ownership walk and so stayed wrong after the owner was fixed, nudging a session that already held the lock. It now asks the owning function. Verified on Windows 11 (Git Bash, Cygwin ps 3.4.10): the lock is acquired, reports its holder, is idempotent, and refuses a bogus, dead, or non-harness published pid. Regressions cover both platform departures behind a fake process table, so they run on Linux and macOS CI too. The pre-existing e2e failure in this suite on Windows is unchanged from main; it cannot exec its symlinked fixture. Refs kunchenguid#3396 Claude-Session: https://claude.ai/code/session_01F9T29YDqYSkQeDTC2Nfi7h
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the previously reported tagged-identity completion issue is fixed because both completion recording and clear/compact validation now accept and preserve the same Reviews (3): Last reviewed commit: "fix(session-lock): accept a tagged ident..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| esac | ||
| comm=$(fm_win_command "$winpid") || continue | ||
| fm_harness_process_matches "$comm" "$comm" || continue | ||
| printf '%s%s\n' "$FM_WIN_PID_PREFIX" "$winpid" |
There was a problem hiding this comment.
When Git Bash acquires the lock as win:<pid>, fm-session-start.sh rejects that identity as nonnumeric and does not record startup completion. Later clear or compact events therefore run the full startup sequence instead of the intended context re-emission.
Knowledge Base Used: Session orchestration
Delegating the nudge to fm_session_lock_owned_by_self changed the question it asks. The nudge asks whether a process in this ancestry took the lock; the ownership predicate additionally requires a verified harness in that ancestry, so a session whose lock was written by a plain shell started being nudged to run session start again. tests/fm-sessionstart-nudge.test.sh pins the looser contract deliberately. The walk stays local, now reading ppid through the portable accessor so it also survives a ps with no -o option, and defers to the ownership predicate only for a Windows-tagged holder, which is not in this process table at all.
…the lock The Windows identity added in this branch introduced a second shape into state/.lock. Six gates read that field, and each one answered "is this value usable" with its own inline numeric test, so every one of them read a valid Windows holder as malformed. The visible cost was concentrated in startup completion: the record was never written, and the clear/compact check that consumes it could never match, so every clear or compact on Windows repeated the full startup sequence. The deferred network sweeps reported ownership as changed when it had not, and the Stop auto-arm treated a dead Windows session as an unreadable lock rather than a recoverable one. fm-lease.sh was the outlier: rather than refusing a value it could not use, `tr -cd '0-9'` reduced the tag to its digits and produced a number naming an unrelated process in the local table. It happened to fail closed downstream, but deriving a wrong-namespace pid is precisely what the tag exists to prevent, so it now takes the value whole and requires a local pid. fm_session_pid_valid is now the single owner of that question and every gate delegates to it, so a third identity shape cannot split them again. Verified against the shipped bytes of each gate with a tagged lock: startup completion now records and is recognized (main's gate reruns the full startup on the same input), both network-ownership gates authorize their sweeps, and a tagged lock yields no lease holder pid instead of 7204.
|
Confirmed and fixed in 9256182 - thank you, this was a real defect and the reasoning was exactly right. I verified both call sites, then audited every reader of
The lease one is the one I would flag back to a reviewer. It did not reject the value, it mangled it into a number naming an unrelated process in the local table. It happened to fail closed downstream, because the equality check against the untouched lock value could never match, but deriving a wrong-namespace pid is precisely what the tag exists to prevent. It now takes the value whole and requires a local pid. Root cause was structural rather than six separate oversights: "is this lock value usable at all" had six independent inline implementations, so adding a second identity shape was guaranteed to split them. Verified against the shipped bytes of each gate with a tagged lock in place, contrasted with the same function on Added |
|
@lmktechnology, thanks for the thorough Windows investigation and fixes here. Before resolving the current conflicts, we need to establish a clear update path so completed work does not become stranded:
The refreshed branch should contain only the three Windows session-identification commits:
The three earlier commits appear unrelated and currently expand the PR to 30 files, making conflict resolution substantially harder. One additional requirement surfaced on native Windows: the current implementation recognizes If you are unavailable and approve the replacement route, we will prepare a clean PR from current |
|
Following up: this remains blocked, and I’m holding off on implementation until there’s an agreed landing path. @lmktechnology: are you available to refresh this PR, or do you approve a clean replacement preserving your authorship? @kunchenguid: alternatively, would you approve a maintainer-assisted refresh of this existing PR, limited to the three Windows session-identification commits identified above, with Pi support tracked separately? No implementation work or replacement PR will begin pending that confirmation. |
|
Tested this branch on native Windows (Git Bash) against a current upstream base. Environment: Windows 11 (build 26120), Git for Windows 2.55.0, Cygwin Applying to current main: cherry-picked
Results on the Windows host:
Outside this PR's scope, seen in the same run (for whoever tracks native Windows):
Happy to rerun anything on this host if it helps the refresh. |
|
@cr101. due to time i approve a clean replacement preserving my authorship. Will still try to |
|
The replacement PR, #4803, is now ready for review. It preserves Lloyd's original commits and attribution, along with the subsequent fixes and history-preserving upstream integration. Please continue review there. It has not been merged. |
|
@lmktechnology, the three session-identity commits from this PR ( |
3 participants
What and why
Fixes the read-only wedge in #3396. On Cygwin (Git for Windows) every session start refused the fleet lock with
error: cannot locate harness process in ancestry, so spawning, steering, merging, the wake-queue drain, and supervision repair were skipped on every start.There are two independent causes on the identity path, and fixing only the first still leaves the session read-only.
Cause 1 -
ps -odoes not exist on Cygwin. Cygwin'spsaccepts only[-aefls] [-u UID] [-p PID]and fails the whole invocation withunknown option -- o, so the ancestry walk aborted on hop one.comm,args, andppidnow go through accessors that fall back to Cygwin's fixed columns; the procps/BSD path is unchanged.Cause 2 - the parent link does not cross the Cygwin boundary. The harness is a native Windows process, and Cygwin reports the PPID of a shell it spawns as
1, so no walk can reach it. Identity now comes from the session pid the harness publishes, confirmed against the Windows process table before use: the pid must still be live and its executable must independently identify a verified harness by the same rules every other platform uses. An absent, stale, or non-harness value is discarded, not bound.A Windows parent-chain walk was implemented, then deliberately removed
The obvious fallback - walk the real Windows parent chain via
Win32_Process- was built and tested, and it is not safe on this platform:execby spawning a fresh Windows process and exiting the old one, so intermediate shells vanish constantly and a child's recorded parent is routinely a pid that no longer exists. Measured directly: a parent id appeared in neitherps -Wnor a fullWin32_Processsnapshot taken moments earlier. The chain simply breaks.A harness that publishes nothing is left unresolved instead, which keeps the session read-only exactly as before. That is the safe direction, and
bin/fm-lock.shnow says so specifically rather than blaming an ancestry that could never contain the answer.Windows pids are tagged, not stored bare
They are a different namespace.
kill -0reports a live Windows process as dead, and the number can collide with an unrelated live Cygwin pid. Storing a bare number would be indistinguishable from a local pid, so the value is taggedwin:<pid>. That makes it non-numeric, so any consumer treating it as a local pid - including a futurekill- refuses it instead of acting on the wrong process:Nothing signals the session lock today; the two
kill -TERMsites read the watcher lock, a different file.Every gate that reads the lock accepts the tagged identity
Introducing a second identity shape into
state/.lockwas the risky part of this change, and the first revision got it wrong. Six gates read that field, and each answered "is this value usable at all" with its own inline numeric test, so every one of them read a valid Windows holder as malformed. Thanks to the review bot for catching the first two; auditing the field turned up four more.The visible cost was concentrated in startup completion: the record was never written, and the clear/compact check that consumes it could never match, so every clear or compact on Windows repeated the full startup sequence. The deferred network sweeps reported ownership as changed when it had not, and the Stop auto-arm treated a dead Windows session as an unreadable lock rather than a recoverable one.
bin/fm-lease.shwas the outlier and the one worth calling out. Rather than refusing a value it could not use,tr -cd '0-9'reducedwin:7204to7204- a number naming an unrelated process in the local table. It happened to fail closed downstream because the equality check against the untouched lock value could never match, but deriving a wrong-namespace pid is exactly what the tag exists to prevent, so it now takes the value whole and requires a local pid, falling through to the shell pid when the lock holds anything else.fm_session_pid_validis now the single owner of that question and all six gates delegate to it, so a third identity shape cannot silently split them again.bin/fm-sessionstart-nudge.shIts private ancestry walk had the same
ps -oproblem, so it now reads ppid through the portable accessor, and defers tofm_session_lock_owned_by_selffor a Windows-tagged holder, which is not in the local process table at all.Its own question is otherwise left alone. An earlier revision of this branch replaced the whole walk with
fm_session_lock_owned_by_self; that was wrong. The nudge asks whether a process in this ancestry took the lock, while the ownership predicate additionally requires a verified harness in that ancestry, so a lock written by a plain shell started being nudged to run session start again.tests/fm-sessionstart-nudge.test.shpins the looser contract deliberately and caught it.Relationship to #3551
#3551 fixes cause 1 only. I simulated its exact walk on the affected machine:
ps -f -p $$returns parent1, so it stops on hop one and the session stays read-only. The accessors here are deliberately close to that PR's shape so whichever lands first, the other is a small merge. Credit to that author for the-odiagnosis.Verification
Windows 11 26200, Git Bash, Cygwin
ps3.4.10, harnessclaude:lock acquired: harness pid win:7204win:7204(confirmed as the realclaude.exe)statuslock: held by live harness pid win:7204explorer.exeC:\tools\claude-notes\helper.exeEach identity gate was then exercised against its shipped bytes with a tagged lock in place, comparing this branch to the same function on
main:mainsession_start_completed(clear/compact)win:7204lock_unchanged(network sweeps)network_mutation_authorized7204(wrong namespace)Five regressions added to
tests/fm-session-lock-ancestry.test.sh. Both platform departures are reproduced behind a fake process table - the-orejection and the severed parent link - so they run on Linux and macOS CI rather than only on Windows. They also pin the namespace separation (a tagged pid whose number is a live harness in the Cygwin table must still be refused) and that a published identity is accepted by the gates that read it back.Run on Linux (WSL Ubuntu), since neither the pinned ShellCheck nor this suite's e2e layer can run on the Windows host. Note
tests/fm-session-start.test.shpins a minimalFM_TEST_BASE_PATH, sojqmust be reachable from it or the home-summary assertions fail for environmental reasons onmaintoo:fm-session-lock-ancestryfm-session-startmain: 47/47)fm-bootstrapfm-bootstrap-network-parallelfm-startup-networkfm-claude-stop-autoarmfm-sessionstart-nudgeOpenCode exact nudge delivery: expected exit 0, got 127, identical onmain(noopencodebinary in that environment)bin/fm-lint.shclean on the full tree with ShellCheck 0.11.0 (pinned 0.11.0), full extended analysis. Its only remark is thatactionlintis absent in that environment; no workflow files are touched by this branch.On the Windows host the e2e layer of the ancestry suite fails identically on
mainand on this branch - it cannot exec its symlinked fixture (theln -slimitation of #3267) - so it is unchanged, not regressed.🤖 Generated with Claude Code
https://claude.ai/code/session_01F9T29YDqYSkQeDTC2Nfi7h