Skip to content

After #871: the waiting set is not closed transitively, the safety gate's word can stick, and the seams the lane did not take #921

Description

@santoshkumarradha

Base: dev@e67f1827b (#871, nothing auxiliary blocks the person). Everything
here was found by the architecture re-review of that PR or reported by the lane
and deliberately not taken. Two are defects in the laws #871 landed — a law that
can be walked around is worse than no law, because it is read as proof — one is a
real race a person can see with Guardian on, one is doc rot, and the rest are
the named follow-ups.

#896 item 4a is CLOSED by #871. postPhaseNews and postLaneNews both hand
the news to a desk (internal/session/sidecar.go) instead of calling the
reader down the caller's stack, so the engine goroutine no longer pays a Bubble
Tea Update+View cycle at the exit of every model call.
TestASlowListenerNeverHoldsTheTurnThatIsTellingIt is the runtime guard and
TestAListenerIsAlwaysToldFromADesk the structural one. internal/provider
needed no change: its own header already states the division, and the reader it
had installed was this package's.


1 — the waiting set is not closed transitively, so one helper reopens the gate

internal/session/sidecar_law_test.go derives the set of readings from the
tree — every selector a readBeside ask calls — and then refuses either a
reading performed, or the waiting verb takeAtTheEnd awaited, anywhere the turn
is not already committed to ending. Both halves are right and the set is one hop
deep. A function that does the waiting for you is not in it, and neither is
anything that calls it.

Replication (deterministic, no model)

Add this to internal/session — nothing else, no edit to the law:

// settleTheMark waits for the drawing and hands the turn over.
func (a *Agent) settleTheMark(ctx context.Context, aside *markAside, …) bool {
	landed, _ := aside.takeAtTheEnd()
	return a.handOverRunningTurn(ctx, landed, …).moved
}

and call it from the step boundary in runTurn, where the turn carries on:

if a.settleTheMark(ctx, marked, …) {
	return
}

go test ./internal/session -run TestEveryReadingBesideTheWorkGoesThroughTheOneDoor
is green. The helper itself is honest by the law's own property — it reaches
an ending door with nothing between — and its caller is not watched at all,
because settleTheMark is neither a readBeside ask nor the waiting verb. The
whole 8.1-second mid-turn gate the census measured is back, behind one wrapper,
with the gate that exists to stop it reporting success.

The same hole read from the other end is 1b: TestAListenerIsAlwaysToldFromADesk
matches a listener call only as a bare ident(args) (calledOutsideADesk tests
call.Fun.(*ast.Ident)). A listener reached as a field or through a slice —
a.reader(news), readers[i](news) — is a SelectorExpr or an IndexExpr and
the law never sees it. Plant func post(a *Agent, news PhaseNews) { a.reader(news) }
and the desk law stays green.

Where

internal/session/sidecar_law_test.go — readingsFrom, besideLaw,
calledOutsideADesk. Searchable: the law is reading the wrong tree.

The fix

Close both watched sets to a fixed point rather than one hop. A function is
watched if it performs a watched call or awaits the waiting verb anywhere the
turn is not committed to ending; iterate until the set stops growing, then judge
callers by the same property. That is the honest reading of the sentence already
in the file — whatever a readBeside ask calls IS a reading — carried one
level further, and it is roughly fifteen lines: a worklist over the package's
own FuncDecls. For 1b, match a call whose Fun resolves to a listener through
Ident, SelectorExpr or IndexExpr rather than Ident alone.

Acceptance

  • Unit (the law, on the laws gate): a fourth planted shape — the wrapper
    above, called from a branch that carries on — is named by
    TestTheLawNamesAWaitPutBackInFrontOfTheWork, and the complaint names
    the caller, not the helper, because the helper is honest.
  • Unit: a fifth plant for 1b — a.reader(news) through a struct field — is
    named by TestTheLawNamesAListenerCalledOnTheTurnsOwnGoroutine.
  • Unit: TestTheLawLetsAnEndingReadInLine still passes unchanged, so the
    closure did not swallow the three honest shapes. A fixed point that fails
    everything is not a fix.
  • e2e: none is owed. This is a structural law about the tree and the
    behaviour it guards already has its runtime door
    (TestTheOnlyWaitAPersonExperiencesIsTheModelGenerating).

2 — two guarded calls in one batch can leave checking whether this is safe to run stuck on the line

Agent.interruptPhase says a word over the top of whatever the turn is saying
and hands back the way to put the old one back. It captures under one lock and
posts under another
:

a.phase.mu.Lock()
held := a.phase.held
a.phase.mu.Unlock()
a.tellPhase(phase, detail, since)      // takes the same lock again

A tool batch runs its calls concurrently — loop.go starts one goroutine per
call and waits on a WaitGroup — and every one of them goes through the consent
door, so with Config.Guardian on two of them reach this function at once:

A (go test) B (read)
1 captures running go test
2 tells checking whether this is safe to run
3 captures checking whether this is safe to run
4 tells checking whether this is safe to run
5 restores running go test
6 restores checking whether this is safe to run

The batch is still running and the line now says the gate's word until something
else replaces it — on a go test that is minutes. It is the mirror of the defect
#871 fixed (the gate clearing the batch's word); the fix took the capture out of
the same critical section as the post, and the concurrent case was missed.

Replication (deterministic, no model)

internal/session, no provider and no key: build an agent with
Config.Guardian on and a stubbed guardian that answers slowly, drive one round
whose batch has two calls both landing on PROMPT, collect PhaseNews through
OnPhaseNews, and assert the last phase before the batch ends. Today it is
PhaseChecking with guardianPhaseWho as its detail; it should be
PhaseRunning naming the batch's first tool. The interleaving is forced without
sleeps by releasing the two stubbed guardian answers in the order above.

Where

internal/session/phasenews.go — Agent.interruptPhase. Searchable:
THE OLD STAGE KEEPS ITS OWN START. The caller is
internal/session/guardian.go, searchable restorePhase.

The fix

One critical section for capture-and-post, and an identity check in the
restore: put the old word back only if what is held is still the word this
interrupt installed. A restore that finds a stranger's word does nothing, which
is right — somebody else owns the line now and will restore it themselves. The
slot stays one slot; what changes is that a nested interrupt cannot capture a
word that is itself an interrupt.

Only reachable with Config.Guardian on, which is off by default (see §6),
so nobody has seen it yet — that is a reason to fix it before somebody does, not
a reason to leave it.

Acceptance

  • e2e: the tmux suite with Guardian on, a two-call batch on a long tool,
    asserting the status line reads running go test while the build runs and
    never sits on checking whether this is safe to run after both gates answer.
  • Unit: the deterministic replication above, asserting the phase sequence
    through OnPhaseNews.
  • Unit: a restore whose word has since been replaced by a third party is a
    no-op.

3 — readRemains never goes through the door, so the law cannot see it

The mark's other reading — the one asked when the model stops without a tool
call — is called straight:

decision := a.decideRemains(ctx, a.readRemains(ctx), said)

from two places in checkpoint.go (the turn's end, and the handover road). It
never passes through readBeside, so readingsFrom never learns the name and
§1's law does not watch it. It is awaited on a road that really does end the
turn, so it is not a live defect — it is a reading the law is blind to, and
the next person to move that call has no gate.

This is the same seam as the lane report's §6.2: making the end-of-turn readers
land after EventTurnDone needs an announcement lane that outlives the turn,
because launchRouteTask announces through the hub and the hub closes when
runTurn returns. Both want the same decision from whoever owns eventHub, so
they are one item.

Replication (deterministic, no model)

go test ./internal/session -run TestEveryReadingBesideTheWorkGoesThroughTheOneDoor -v
and print the derived set: readRemains is not in it. Move the
a.readRemains(ctx) call to the top of runTurn and the law is still green.

Where

internal/session/checkpoint.go — Agent.readRemains, and its two callers.
Searchable: readRemains asks the mark's own reader.

The fix

Decide the announcement question first, because it sets the shape: either
eventHub gains a post-turn announcement lane that survives runTurn's return
— in which case both readRemains and the route judge become ordinary readings
started beside the work and taken at the end — or it does not, and readRemains
is converted to readBeside anyway so the law can see it, awaited through
takeAtTheEnd on the ending road it already sits on. The second is small and
buys the gate; the first is the actual speed win and is a design call.

Acceptance

  • Unit: readRemains appears in the set readingsFrom derives, and moving
    its call to a road that carries on fails the law by name.
  • e2e: go test ./internal/session -run 'Remains|Reopen|Handover' green — the
    reading still decides the same three outcomes (re-open, let the turn end, hand
    over), asserted on the decision rather than on the call count.

4 — doc rot: closeMarkAside is gone and checkpoint.go still names it as the caller

// is for an ENDING and nowhere else ([Agent.closeMarkAside] is the one caller).

Agent.closeMarkAside was deleted answering the review — the settle is inlined
into runTurn's own exit defer, which is where the tree can see the ending — and
this doc link now points at nothing. A dead [Agent.…] link is worse than a
plain sentence: it reads as a claim about where to look.

Where

internal/session/checkpoint.go, on markAside.takeAtTheEnd. Searchable:
is for an ENDING and nowhere else.

The fix

Name the real caller: runTurn's exit defer (loop.go). One line.

Acceptance

  • Unit: go vet ./internal/session/ and the doc-link check pass; a grep for
    closeMarkAside across the tree returns nothing.

5 — Agent.PrefetchRecall(draft): typing time is free latency

The largest pre-turn win left, and it is not in internal/session's gift alone.
#871 took the recall off the critical path — it starts when the turn starts and
is applied wherever it lands — but it still starts at submit. A person types
for seconds before they press enter, and the recall's own cue is in the draft.

The fix

Agent.PrefetchRecall(draft string) starts the reading against the current draft
after a short dwell, and startTurnLocked adopts a prefetch whose cue still
matches what was actually sent rather than starting a second one. The surface
(internal/tui3) calls it from the composer. A prefetch that does not match is
let go, which costs one cheap call on a person who rewrites their message.

Acceptance

  • e2e: the tmux suite types a message with a clear cue, waits past the dwell,
    presses enter, and the turn's pace journal row shows the recall already
    landed
    at assembly (recall in the decomposition, no recall:late note).
  • Unit: a draft that is edited after the prefetch fires starts at most one
    more reading, and the turn never carries memory read against a cue the person
    deleted.

6 — the smaller seams, each with its door

  • checkpointBrief has no window at all. The handover's first model call
    takes the whole transcript on the conversation model with no bound; measured
    15–30 s between the model's last word and the task appearing. It is an ENDING
    so nothing can run beside it, and briefing a worker covers it — but no bound
    is still no bound. Recommend lane.RoleJudge.GiveUp() (90 s), matching
    writeHandoff's own order of magnitude rather than a flat wall. Acceptance:
    a stubbed conversation model that never answers ends the handover inside the
    give-up with the brief's fall-through, asserted through
    go test ./internal/session -run Handover.
  • The caption and the two namers are still hand-rolled goroutines.
    maybeCaption, taskname.go and jobname.go each have their own channel
    discipline and each is a reading beside the work by every part of the
    definition. Converting them to readBeside is mechanical and deletes three
    shapes. Acceptance: they appear in the set readingsFrom derives, and
    TestThereIsOneDoorForAReadingBesideTheWork still finds exactly one door.
  • internal/tui3/phase_test.go posts a PhasePreparing with "saved context".
    Nothing emits that phase any more — Nothing auxiliary blocks the person: one reading beside the work, never in front of it #871 deleted it, because it named a wait
    that is gone. The test is green either way (it renders news it makes itself),
    which is exactly why it will outlive everyone who remembers what it was for.
    Acceptance: the case is dropped or repointed at a phase the engine still
    posts, asserted by the phase word appearing in internal/session.
  • Config.Guardian is off by default, so checking whether this is safe to run — and §2's defect — are invisible on a stock install. Somebody should say
    whether that default is deliberate. Acceptance: whichever way it goes, the
    manual page that describes the safety gate
    (internal/manual/chat/permissions.md) states the default in the product's own
    words, and internal/manual/chat_test.go's probe table reaches it from "is
    anything checking what tools run".

Why one issue and not seven

§1 and §2 are the ones that should not sit: a law with a fifteen-line hole in it
is read by the next lane as proof, and §2 is a person-visible wedge waiting for
whoever turns the guardian on. §3 and §6's first two are the same conversation
about where a reading lives. §4 is a minute. §5 is the only one that is a new
capability rather than a correction, and it is the biggest remaining win.

🤖 Generated with Claude Code


Drafted with CodeAF · reviewed and owned by the author

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    area:sessionThe engine — turns, tasks, the toolbelt, checkpointsbugSomething the code does that it should notsev:seriousWrong or missing behaviour a person meets in ordinary use

    Type

    No type

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions