From 7387d991d80da21283c4654d7f5df9d8bad2316d Mon Sep 17 00:00:00 2001 From: ZeroPoint95 <329227198+ZeroPoint95@users.noreply.github.com> Date: Wed, 11 Nov 2026 23:54:04 -0500 Subject: [PATCH 1/9] Factory manager: answer, stop, and clear instead of archive The desktop app's Factory manager answered nothing, could not be stopped and could be archived out of reach. - Stop is bounded: the app's stop carries its bound over the wire (Interrupt's `within`) and the engine abandons a turn that has not let go ten seconds later (session.Agent.InterruptWithin). A lent factory stage is interrupted but never abandoned from the app. - A foreman whose conversation works in the codeaf home is made again in /chats/foreman; the one it replaces is stopped and archived. - factory_floor waits a bounded time on the factory and returns at once when the turn is stopped. - factory.foreman_open is the person's own open and takes a closed or archived manager out of Closed. factory.foreman_fresh starts the manager over in a new conversation (the app's Clear). - factory_floor: `repos` counts jobs by repository and state, `shipped` takes `days` and counts merged pull requests, and every table opens with the whole factory's counts. The brief no longer limits the manager to factory_floor. The change entry still needs its pull request number. Co-Authored-By: Claude --- cmd/codeaf/app.go | 29 +- cmd/codeaf/app_lend.go | 7 + cmd/codeaf/app_stop_bound_test.go | 90 +++++ cmd/codeaf/appfactory_foreman.go | 57 +++- cmd/codeaf/appmoney.go | 2 +- cmd/codeaf/factory_foreman.go | 140 ++++++-- cmd/codeaf/factory_foreman_fresh_test.go | 217 ++++++++++++ .../0-factory-manager-answers-stops-clears.md | 23 ++ docs/design/app-v3/PROTOCOL.md | 35 +- internal/appwire/factory_foreman.go | 51 +++ internal/appwire/factory_foreman_test.go | 46 +++ internal/appwire/server.go | 1 + internal/manual/chat/factory.md | 19 ++ internal/remote/client.go | 7 + internal/remote/server.go | 15 + internal/remote/stopbound_test.go | 111 ++++++ internal/remote/wire.go | 9 +- internal/session/abandon.go | 36 +- internal/session/abandon_test.go | 93 +++++ internal/session/tools_floor.go | 322 +++++++++++++++--- internal/session/tools_floor_test.go | 180 +++++++++- 21 files changed, 1402 insertions(+), 88 deletions(-) create mode 100644 cmd/codeaf/app_stop_bound_test.go create mode 100644 cmd/codeaf/factory_foreman_fresh_test.go create mode 100644 docs/changes/unreleased/0-factory-manager-answers-stops-clears.md create mode 100644 internal/remote/stopbound_test.go diff --git a/cmd/codeaf/app.go b/cmd/codeaf/app.go index 910808abb0..884f9b594a 100644 --- a/cmd/codeaf/app.go +++ b/cmd/codeaf/app.go @@ -1680,7 +1680,7 @@ func (b *appBackend) bootSettle(a *appAgent, started bool) { } if stop { b.locked(func() { live.stopping = live.running }) - live.conn.Conv.Interrupt() + appStopByPerson(live.conn.Conv) return } for _, s := range early { @@ -3397,11 +3397,36 @@ func (b *appBackend) Stop(id string) error { } if live := b.liveOf(a); live != nil { b.locked(func() { live.stopping = live.running }) - live.conn.Conv.Interrupt() + appStopByPerson(live.conn.Conv) } return nil } +// appStopGrace is how long the app's Stop waits for a turn to let go before +// the engine abandons it. It is the terminal's figure and argued there +// (internal/tui3's stopGrace): far above every settle that can be measured, so +// it fires only on a wait that never looks at its context. +const appStopGrace = 10 * time.Second + +// appBoundedStop is a conversation whose stop can carry its second stage. +type appBoundedStop interface { + InterruptWithin(grace time.Duration) +} + +// appStopByPerson is the person's Stop, BOUNDED WHERE THE CONVERSATION CAN BE. +// An interrupt alone only cuts the turn's context, and a turn parked on a wait +// that ignores it (a tool that takes a lock, a read that never looks) stayed +// "working" until the app was quit: the app has no second key to press, so the +// engine is given the bound with the stop ([session.Agent.InterruptWithin]). +// The turn's stream then closes and the turn settles as stopped by the person. +func appStopByPerson(conv appConversation) { + if bounded, ok := conv.(appBoundedStop); ok { + bounded.InterruptWithin(appStopGrace) + return + } + conv.Interrupt() +} + // CloseAgent lets the conversation go and says the agent is closed. Its journal // stays on disk; agents.open or agents.send opens it again. func (b *appBackend) CloseAgent(id string) error { diff --git a/cmd/codeaf/app_lend.go b/cmd/codeaf/app_lend.go index af21a3518a..c510de4e7a 100644 --- a/cmd/codeaf/app_lend.go +++ b/cmd/codeaf/app_lend.go @@ -2,6 +2,7 @@ package main import ( "sync" + "time" "github.com/Agent-Field/codeaf/internal/appwire" "github.com/Agent-Field/codeaf/internal/guard" @@ -166,6 +167,12 @@ func (l lentAgent) InterruptFor(door session.StopDoor) { l.Agent.InterruptFor(door) } +// InterruptWithin is the person's stop without the view's bound behind it. A +// stage's turn belongs to the factory's runner, which is still reading it: the +// view letting go of that turn would leave the runner carrying on over work +// that had not ended. The job's own Stop is the door that ends a stage. +func (l lentAgent) InterruptWithin(time.Duration) { l.Agent.Interrupt() } + // InterruptNamed is [lentAgent.InterruptFor] with the window the door named. func (l lentAgent) InterruptNamed(door session.StopDoor, name string) { if endsAView(door) { diff --git a/cmd/codeaf/app_stop_bound_test.go b/cmd/codeaf/app_stop_bound_test.go new file mode 100644 index 0000000000..78825119e7 --- /dev/null +++ b/cmd/codeaf/app_stop_bound_test.go @@ -0,0 +1,90 @@ +package main + +import ( + "sync" + "testing" + "time" + + "github.com/Agent-Field/codeaf/internal/appwire" +) + +// THE APP'S STOP IS A BOUNDED STOP. +// +// The desktop app reported a Stop that did nothing: the Factory manager's turn +// was parked inside a call that never looked at its context, the interrupt +// changed nothing about it, and the app has no second key. So the person's +// Stop now hands the engine its bound with the stop ([appStopByPerson]) and +// the engine lets go of the turn when the bound runs out. This pins the app's +// half: the bound is asked for, and a turn the engine let go of settles as +// stopped by the person, once. + +// boundedConversation is a [fakeConversation] whose stop can carry its second +// stage. Letting go is the engine closing the turn's stream, which the fake's +// hold stands for. +type boundedConversation struct { + *fakeConversation + + mu sync.Mutex + graces []time.Duration + letGo func() +} + +func (c *boundedConversation) InterruptWithin(grace time.Duration) { + c.mu.Lock() + c.graces = append(c.graces, grace) + letGo := c.letGo + c.mu.Unlock() + if letGo != nil { + letGo() + } +} + +func TestAppStopAsksForItsBoundAndTheTurnLetGoSettlesStopped(t *testing.T) { + hold := make(chan struct{}) + conv := &boundedConversation{fakeConversation: &fakeConversation{hold: hold}} + conv.letGo = sync.OnceFunc(func() { close(hold) }) + rec := newAppRecorder() + backend := newFakeBackend(rec, func(ask appOpenAsk) (*appConn, error) { + return &appConn{Conv: conv, Workspace: ask.Workspace, Transcript: appFakeTranscript, Release: func() {}}, nil + }) + defer backend.Close() + agent, err := mustCreate(t, backend, appwire.CreateAsk{Workspace: t.TempDir()}) + if err != nil { + t.Fatal(err) + } + if _, err := backend.Send(agent.ID, "park on something that cannot be cancelled", ""); err != nil { + t.Fatal(err) + } + if err := backend.Stop(agent.ID); err != nil { + t.Fatalf("stop = %v", err) + } + + conv.mu.Lock() + graces := append([]time.Duration(nil), conv.graces...) + conv.mu.Unlock() + if len(graces) != 1 || graces[0] != appStopGrace { + t.Fatalf("the stop asked for the bounds %v, want one of %s", graces, appStopGrace) + } + conv.fakeConversation.mu.Lock() + plain := conv.fakeConversation.interrupted + conv.fakeConversation.mu.Unlock() + if plain != 0 { + t.Fatalf("a bounded stop also sent %d plain interrupts", plain) + } + + i := rec.waitFor(t, 5*time.Second, appTurnDone) + rec.mu.Lock() + defer rec.mu.Unlock() + if d := rec.data[i].(appwire.TurnDone); d.OK || d.ErrorKind != appwire.ErrorStopped || d.Error != "stopped" { + t.Fatalf("turn.done = %+v", d) + } + receipts := 0 + for _, data := range rec.data { + if e, ok := data.(appwire.EntryAdded); ok && e.Entry.Kind == appwire.EntryFooter && e.Entry.Footer == appwire.FooterStoppedByYou { + receipts++ + } + } + if receipts != 1 { + t.Fatalf("%d stop receipts, want 1", receipts) + } +} diff --git a/cmd/codeaf/appfactory_foreman.go b/cmd/codeaf/appfactory_foreman.go index 0df6aa0238..aac54c8dfc 100644 --- a/cmd/codeaf/appfactory_foreman.go +++ b/cmd/codeaf/appfactory_foreman.go @@ -2,10 +2,12 @@ package main import ( "context" + "os" "time" "github.com/Agent-Field/codeaf/internal/appwire" "github.com/Agent-Field/codeaf/internal/factory" + "github.com/Agent-Field/codeaf/internal/factory/store" ) // FactoryForemanOpen is factory.foreman_open: the foreman conversation of the @@ -13,7 +15,28 @@ import ( // one every time after, held open so the app draws it at once. Its floor is // this host's own store, the factory home it belongs to and no other; a factory // this engine does not serve is refused, as its job chats are. +// +// THE DOOR IS THE PERSON'S OWN OPEN. They pressed the Factory manager's own +// button, so a manager they closed or archived comes out of Closed here: left +// as it was, the button opened a conversation that could not be written to, +// with no other door to a manager at all. func (b *appBackend) FactoryForemanOpen(ctx context.Context, p appwire.FactoryRef) (appwire.FactoryForeman, error) { + return b.foremanOpened(ctx, p, foremanMaker) +} + +// FactoryForemanFresh is factory.foreman_fresh: the manager started over. A +// new conversation takes the manager's place ([foremanFresh]) and is answered +// held open, as an open answers it; the one it replaces is stopped and put +// away, its history kept on disk. +func (b *appBackend) FactoryForemanFresh(ctx context.Context, p appwire.FactoryRef) (appwire.FactoryForeman, error) { + return b.foremanOpened(ctx, p, foremanFresh) +} + +// foremanOpened answers the foreman a maker names, held open and the +// factory's. A foreman the maker REPLACED (a fresh start, or one moved out of +// a folder it may not work in) is stopped and put away once the new one is +// named, so nothing is left running behind a conversation no door opens. +func (b *appBackend) foremanOpened(ctx context.Context, p appwire.FactoryRef, maker func(*store.Store, string, string, func() (factory.Snapshot, error)) func(context.Context) (string, error)) (appwire.FactoryForeman, error) { host, err := b.localHostOf(p.Factory) if err != nil { return appwire.FactoryForeman{}, err @@ -22,13 +45,45 @@ func (b *appBackend) FactoryForemanOpen(ctx context.Context, p appwire.FactoryRe if load == nil { load = factory.LocalSeam(host.floor, time.Now(), factoryPipelineOption(host.floor)).Load } - transcript, err := foremanMaker(host.floor, host.workspace, host.profile, load)(ctx) + before, _ := host.floor.Foreman() + transcript, err := maker(host.floor, host.workspace, host.profile, load)(ctx) if err != nil { return appwire.FactoryForeman{}, appwire.Errorf(appwire.CodeConflict, "the factory's manager could not be opened: %s", err) } + if before != "" && !sameSessionFile(before, transcript) { + b.putAwayForeman(before) + } id := appAgentID(transcript) + a, err := b.find(id) + if err != nil { + return appwire.FactoryForeman{}, err + } + // The factory's before anything is said of it, so the app never draws the + // manager among the person's own chats for the moment between the two. + b.locked(func() { a.view.FactoryOwned = true }) + if b.tidied(id) { + if _, err := b.Reopen(id); err != nil { + return appwire.FactoryForeman{}, err + } + } if _, err := b.holdJobChat(id); err != nil { return appwire.FactoryForeman{}, err } return appwire.FactoryForeman{Factory: p.Factory, Agent: id}, nil } + +// putAwayForeman stops a replaced foreman's turn and archives its +// conversation: its history stays on disk and in Archived, and nothing of it +// runs on. It is best effort. The new foreman is already the factory's, and a +// conversation that is gone or cannot be closed is no reason to refuse it. +func (b *appBackend) putAwayForeman(transcript string) { + if _, err := os.Stat(transcript); err != nil { + return + } + id := appAgentID(transcript) + if _, err := b.find(id); err != nil { + return + } + _ = b.Stop(id) + _ = b.Archive([]string{id}) +} diff --git a/cmd/codeaf/appmoney.go b/cmd/codeaf/appmoney.go index 2fa86e5f30..eb604614b5 100644 --- a/cmd/codeaf/appmoney.go +++ b/cmd/codeaf/appmoney.go @@ -361,7 +361,7 @@ func (b *appBackend) answerMoney(a *appAgent, askID, option, text string) (bool, case appwire.OptionStop: if running { b.locked(func() { live.stopping = true }) - live.conn.Conv.Interrupt() + appStopByPerson(live.conn.Conv) } b.closeMoneyAsks(a, appwire.CapAgent, appwire.OptionStop, "Stopped") return true, nil diff --git a/cmd/codeaf/factory_foreman.go b/cmd/codeaf/factory_foreman.go index 931983d801..53a02fad77 100644 --- a/cmd/codeaf/factory_foreman.go +++ b/cmd/codeaf/factory_foreman.go @@ -51,7 +51,7 @@ const foremanOldTitle = "foreman" // foremanOpening is the brief's fixed part, said once so the brief, the test // and the manual quote the same words. -const foremanOpening = "You are the foreman of this factory, its factory-wide manager. Every job, its state, its step and its pull request come through factory_floor; answer what is running, what waits, what is queued and what shipped only from it. Start a job or set its priority with factory_floor only when the person asks for it here, naming the job by its ref. Propose what to take on by selecting jobs. Never stop, ship, merge or post. Name jobs by their ref." +const foremanOpening = "You are the foreman of this factory, its factory-wide manager. Every job, its state, its step and its pull request come through factory_floor: read it before you answer what is running, what waits, what is queued, what shipped or how many jobs a repository has, and never answer those from memory. When the person's message comes with a block of the app's present state, it was true when they sent it: answer from it what it says, and read factory_floor for what it does not. Anything else the person asks, answer as any assistant would, with your other tools. Start a job or set its priority with factory_floor only when the person asks for it here, naming the job by its ref. Propose what to take on by selecting jobs. Never stop, ship, merge or post. Name jobs by their ref." // floorDoor is `factory_floor`'s door over the store, or nil. // @@ -178,54 +178,98 @@ func withForeman(seam factory.Seam, st *store.Store, workspace, profileDir strin // when its session file is still there, and otherwise one made now and named // in the store, so A SECOND ASK OPENS THE SAME CONVERSATION. load is the floor's // read, asked once at the making for the recipes' policy lines. +// +// A FOREMAN WHOSE FOLDER IS THE CODEAF HOME IS MADE AGAIN, in a folder of its +// own. A host that named no folder once gave the foreman the engine's working +// directory, which under the desk is the home itself: every save of that +// conversation then swept the whole home, and its tool calls waited behind a +// capture that never finished, so the manager answered nothing at all. No new +// foreman is made there ([factoryFolderRefusal]); this is the same refusal for +// the one a store already names. The old conversation is left on disk for its +// owner to put away ([appBackend.FactoryForemanOpen]). func foremanMaker(st *store.Store, workspace, profileDir string, load func() (factory.Snapshot, error)) func(context.Context) (string, error) { return func(context.Context) (string, error) { factoryReconcileOnce(profileDir, st) if chat, err := st.Foreman(); err == nil && chat != "" { - if _, serr := os.Stat(chat); serr == nil { + if _, serr := os.Stat(chat); serr == nil && foremanFolderRefusal(st, chat) == nil { foremanRetitle(profileDir, chat) return chat, nil } } - where := strings.TrimSpace(workspace) - if where == "" { - // A HOST WITH NO FOLDER OF ITS OWN (the desk's factory, or an - // engine started in the codeaf home) gives the foreman an empty - // folder under the floor: it reads the floor through its door, - // never through files (factory_chat_folder.go). - own, err := factoryForemanFolder(st) - if err != nil { - return "", err - } - where = own - } - if err := factoryFolderRefusal(st, where); err != nil { - return "", err - } - bucket, err := v3ProjectDir(where) - if err != nil { - return "", err - } - place, err := v3MintSession(bucket, where, v3StampLaunchDir(v3LaunchDir(), where), false) + return foremanMint(st, workspace, profileDir, load) + } +} + +// foremanFresh starts the foreman over: a new conversation with none of the +// old one's words, which takes its place in the store and in the `factory` +// team. It is the app's Clear ([appBackend.FactoryForemanFresh]); the old +// conversation is left on disk for its owner to stop and put away. +func foremanFresh(st *store.Store, workspace, profileDir string, load func() (factory.Snapshot, error)) func(context.Context) (string, error) { + return func(context.Context) (string, error) { + factoryReconcileOnce(profileDir, st) + return foremanMint(st, workspace, profileDir, load) + } +} + +// foremanFolderRefusal is [factoryFolderRefusal] for a foreman that already +// exists, by the folder its conversation records. A conversation whose record +// cannot be read, or names no folder, is not refused: this rule moves a +// foreman out of a folder known to be wrong, never one it cannot see. +func foremanFolderRefusal(st *store.Store, transcript string) error { + meta, err := session.LoadMeta(session.DirOf(transcript)) + if err != nil { + return nil + } + return factoryFolderRefusal(st, meta.Workspace) +} + +// foremanMint makes the foreman's conversation now and names it in the store. +// A foreman the store already names gives up its place in the `factory` team +// first, so the new one takes the `foreman` handle rather than a second-best +// one beside it. +func foremanMint(st *store.Store, workspace, profileDir string, load func() (factory.Snapshot, error)) (string, error) { + where := strings.TrimSpace(workspace) + // A HOST WITH NO FOLDER OF ITS OWN (the desk's factory, or an engine + // started in the codeaf home) gives the foreman an empty folder under the + // floor: it reads the floor through its door, never through files + // (factory_chat_folder.go). A host whose own folder is refused is the + // same host, for this purpose. + if where == "" || factoryFolderRefusal(st, where) != nil { + own, err := factoryForemanFolder(st) if err != nil { return "", err } - transcript := place.Transcript() - var snap factory.Snapshot - if load != nil { - snap, _ = load() - } - if err := session.SeedConversation(transcript, where, foremanTitle, foremanBrief(snap)); err != nil { - return "", err - } - if err := foremanJoinTeam(profileDir, transcript, where); err != nil { - return "", err - } - if err := st.SetForeman(transcript); err != nil { - return "", err - } - return transcript, nil + where = own } + if err := factoryFolderRefusal(st, where); err != nil { + return "", err + } + bucket, err := v3ProjectDir(where) + if err != nil { + return "", err + } + place, err := v3MintSession(bucket, where, v3StampLaunchDir(v3LaunchDir(), where), false) + if err != nil { + return "", err + } + transcript := place.Transcript() + var snap factory.Snapshot + if load != nil { + snap, _ = load() + } + if err := session.SeedConversation(transcript, where, foremanTitle, foremanBrief(snap)); err != nil { + return "", err + } + if old, err := st.Foreman(); err == nil && old != "" { + foremanLeaveTeam(profileDir, old) + } + if err := foremanJoinTeam(profileDir, transcript, where); err != nil { + return "", err + } + if err := st.SetForeman(transcript); err != nil { + return "", err + } + return transcript, nil } // foremanBrief is the foreman's opening note: [foremanOpening], then each @@ -259,6 +303,28 @@ func foremanJoinTeam(profileDir, transcript, where string) error { }) } +// foremanLeaveTeam takes a foreman that is being replaced out of every team +// that holds it, so its handle is free for the one made in its place. It is +// best effort: a team file that cannot be written leaves the old member where +// it was, and the new one joins beside it under another handle. +func foremanLeaveTeam(profileDir, transcript string) { + key := transcript + if real, err := filepath.EvalSymlinks(transcript); err == nil { + key = real + } + key = filepath.Clean(key) + _ = teams.UpdateOpen(profileDir, func(f *teams.File) error { + for _, t := range f.Teams { + if t.Holds(key) { + if err := f.RemoveMember(t.ID, key); err != nil { + return err + } + } + } + return nil + }) +} + // foremanRetitle renames a foreman made before the rename: its conversation's // title and its word in the `factory` team, when each still says the old // title. A title the person changed is theirs and stays. It is best effort; diff --git a/cmd/codeaf/factory_foreman_fresh_test.go b/cmd/codeaf/factory_foreman_fresh_test.go new file mode 100644 index 0000000000..90bfc49f09 --- /dev/null +++ b/cmd/codeaf/factory_foreman_fresh_test.go @@ -0,0 +1,217 @@ +package main + +// THE FOREMAN OUT OF THE CODEAF HOME, STARTED OVER, AND TAKEN OUT OF CLOSED. +// +// The Factory manager the desk made on one Mac worked in the codeaf home +// itself: every save swept nineteen gigabytes and its tool calls waited behind +// a capture that never finished, so it answered no question at all. It could +// also be archived, after which its one door opened a chat nobody could write +// to. These pin the three repairs: a foreman in a refused folder is made again +// in its own; factory.foreman_fresh starts one over; and the door, which is +// the person's own open, takes a put-away manager back out. + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/Agent-Field/codeaf/internal/appwire" + "github.com/Agent-Field/codeaf/internal/session" + "github.com/Agent-Field/codeaf/internal/teams" +) + +// foremanHandleHolder is the conversation key holding `@foreman` in the +// profile's teams, "" for none, and how many members hold it. +func foremanHandleHolder(t *testing.T, profile string) (key string, holders int) { + t.Helper() + f, err := teams.Load(profile) + if err != nil { + t.Fatal(err) + } + for _, team := range f.Teams { + for _, m := range team.Members { + if m.Handle == foremanHandle { + key, holders = m.Key, holders+1 + } + } + } + return key, holders +} + +func teamKey(transcript string) string { + key := transcript + if real, err := filepath.EvalSymlinks(transcript); err == nil { + key = real + } + return filepath.Clean(key) +} + +func TestAForemanWorkingInTheCodeafHomeIsMadeAgainInAFolderOfItsOwn(t *testing.T) { + state, floor := chatFolderHome(t) + profile := t.TempDir() + + // The foreman as the older build made it: in the engine's own folder, + // which under the desk is the codeaf home. + bucket, err := v3ProjectDir(state) + if err != nil { + t.Fatal(err) + } + place, err := v3MintSession(bucket, state, v3StampLaunchDir(v3LaunchDir(), state), false) + if err != nil { + t.Fatal(err) + } + old := place.Transcript() + if err := session.SeedConversation(old, state, foremanTitle, "the older brief"); err != nil { + t.Fatal(err) + } + if err := foremanJoinTeam(profile, old, state); err != nil { + t.Fatal(err) + } + if err := floor.SetForeman(old); err != nil { + t.Fatal(err) + } + + maker := foremanMaker(floor, factoryHostWorkspace(state), profile, nil) + made, err := maker(context.Background()) + if err != nil { + t.Fatal(err) + } + if sameSessionFile(made, old) { + t.Fatal("the foreman was opened where it was, in the codeaf home") + } + meta, err := session.LoadMetaIn(session.DirOf(made), "") + if err != nil { + t.Fatal(err) + } + if want := filepath.Join(floor.Root(), "chats", "foreman"); meta.Workspace != want { + t.Fatalf("the foreman made again works in %q; want %q", meta.Workspace, want) + } + if named, _ := floor.Foreman(); !sameSessionFile(named, made) { + t.Fatalf("the store still names %q as its foreman", named) + } + if key, holders := foremanHandleHolder(t, profile); holders != 1 || key != teamKey(made) { + t.Fatalf("@foreman is held by %q (%d holders); want the new conversation alone", key, holders) + } + // ITS HISTORY IS NOT DELETED: the owner of the conversation puts it away. + if _, err := os.Stat(old); err != nil { + t.Fatalf("the conversation it replaced is gone from disk: %v", err) + } + // AND THE NEXT ASK OPENS THE SAME ONE, as every ask after a making does. + if again, err := maker(context.Background()); err != nil || !sameSessionFile(again, made) { + t.Fatalf("the next open answered %q, %v; want %q", again, err, made) + } +} + +func TestForemanFreshStartsTheManagerOver(t *testing.T) { + st, web, profile := talkLab(t) + first, err := foremanMaker(st, web, profile, nil)(context.Background()) + if err != nil { + t.Fatal(err) + } + fresh, err := foremanFresh(st, web, profile, nil)(context.Background()) + if err != nil { + t.Fatal(err) + } + if sameSessionFile(first, fresh) { + t.Fatal("a fresh foreman is the conversation it was") + } + data, err := os.ReadFile(fresh) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(data), foremanOpening) { + t.Errorf("the fresh foreman lacks its brief:\n%s", data) + } + if key, holders := foremanHandleHolder(t, profile); holders != 1 || key != teamKey(fresh) { + t.Fatalf("@foreman is held by %q (%d holders); want the fresh conversation alone", key, holders) + } + // THE FLOOR IS THE FRESH ONE'S AND NO LONGER THE OLD ONE'S: the tool + // follows the store's mark, so the conversation put away reads nothing. + if foremanFloorDoor(st, web, profile, fresh) == nil { + t.Error("the fresh foreman has no factory_floor") + } + if foremanFloorDoor(st, web, profile, first) != nil { + t.Error("the replaced foreman still carries factory_floor") + } + if again, err := foremanMaker(st, web, profile, nil)(context.Background()); err != nil || !sameSessionFile(again, fresh) { + t.Fatalf("the open after a fresh start answered %q, %v; want the fresh one", again, err) + } +} + +// THE BRIEF NO LONGER FENCES THE MANAGER INTO ONE TOOL. It was told to answer +// "only from" factory_floor, so it could not use what the app tells it with +// each message, nor answer anything a plain assistant can. +func TestTheForemansBriefLetsItAnswerBeyondTheFloor(t *testing.T) { + for _, want := range []string{"factory_floor", "the app's present state", "your other tools", "only when the person asks", "Never stop, ship, merge or post"} { + if !strings.Contains(foremanOpening, want) { + t.Errorf("the brief lacks %q: %s", want, foremanOpening) + } + } + if strings.Contains(foremanOpening, "only from it") { + t.Errorf("the brief still fences every answer into the floor: %s", foremanOpening) + } +} + +func TestTheFactoryManagersDoorReopensItAndFreshPutsTheOldOneAway(t *testing.T) { + s := newLocalSetup(t) + s.backend.open = func(ask appOpenAsk) (*appConn, error) { + return &appConn{Conv: &fakeConversation{}, Workspace: ask.Workspace, Transcript: ask.Transcript, Release: func() {}}, nil + } + st := s.start() + ref := map[string]any{"factory": st.Factory} + + var first appwire.FactoryForeman + s.must(appwire.FeatureFactoryForemanOpen, ref, &first) + if !s.backend.isHeld(first.Agent) { + t.Fatal("the manager is not held open") + } + + // PUT AWAY BY THE PERSON, then opened by its own button: it comes back. + if err := s.backend.Archive([]string{first.Agent}); err != nil { + t.Fatal(err) + } + if !s.backend.tidied(first.Agent) || s.backend.isHeld(first.Agent) { + t.Fatal("the archive did not put the manager away; the premise is gone") + } + var again appwire.FactoryForeman + s.must(appwire.FeatureFactoryForemanOpen, ref, &again) + if again.Agent != first.Agent { + t.Fatalf("the door answered %q after an archive; want the same manager %q", again.Agent, first.Agent) + } + if s.backend.tidied(first.Agent) || !s.backend.isHeld(first.Agent) { + t.Fatal("the manager's own door left it archived") + } + + // STARTED OVER: a new manager, held open; the old one stopped and archived. + var fresh appwire.FactoryForeman + s.must(appwire.FeatureFactoryForemanFresh, ref, &fresh) + if fresh.Agent == "" || fresh.Agent == first.Agent { + t.Fatalf("a fresh start answered %q; want a new conversation", fresh.Agent) + } + if !s.backend.isHeld(fresh.Agent) || s.backend.tidied(fresh.Agent) { + t.Fatal("the fresh manager is not held open") + } + if !s.backend.tidied(first.Agent) || s.backend.isHeld(first.Agent) { + t.Fatal("the manager it replaced was left open") + } + var after appwire.FactoryForeman + s.must(appwire.FeatureFactoryForemanOpen, ref, &after) + if after.Agent != fresh.Agent { + t.Fatalf("the door after a fresh start answered %q; want %q", after.Agent, fresh.Agent) + } + // AND EACH IS THE FACTORY'S, never one of the person's own chats. + for _, id := range []string{first.Agent, fresh.Agent} { + a, err := s.backend.find(id) + if err != nil { + t.Fatal(err) + } + owned := false + s.backend.locked(func() { owned = a.view.FactoryOwned }) + if !owned { + t.Errorf("%s is not marked the factory's", id) + } + } + s.mustFail(appwire.FeatureFactoryForemanFresh, map[string]any{"factory": "factory_other"}, appwire.CodeNotFound) +} diff --git a/docs/changes/unreleased/0-factory-manager-answers-stops-clears.md b/docs/changes/unreleased/0-factory-manager-answers-stops-clears.md new file mode 100644 index 0000000000..55b9237324 --- /dev/null +++ b/docs/changes/unreleased/0-factory-manager-answers-stops-clears.md @@ -0,0 +1,23 @@ +--- +kind: fixed +title: the Factory manager answers, stops, and is cleared rather than archived +pr: 0 +surface: [factory, engine, app] +invalidates: + - "Was true: agents.stop only interrupted the turn, so a turn parked on a wait that never read its context stayed working until the app was quit. Is true now: the app's Stop is bounded. The stop carries its bound (Interrupt's `within`, in milliseconds) and the engine abandons a turn that has not let go ten seconds later; the turn ends stopped by the person and the next send opens an ordinary turn. A lent factory stage is interrupted but never abandoned from the app." + - "Was true: a Factory manager made by a build that gave it the engine's own folder kept working in the codeaf home, where every save swept the whole home and its factory_floor calls never returned. Is true now: factory.foreman_open makes such a manager again in `/chats/foreman`, and stops and archives the conversation it replaces." + - "Was true: factory_floor waited on the factory with no bound and ignored the turn's context. Is true now: a read waits at most 20 seconds and a start, priority or selection 60 seconds, a stop returns it at once, and a start or priority that got no answer says it may still take effect." + - "Was true: the Factory manager could be archived or closed, after which factory.foreman_open answered a conversation nobody could write to. Is true now: the open is the person's own and takes the manager out of Closed, and factory.foreman_fresh {factory} (hello feature factory.foreman_fresh) starts it over in a new conversation, stopping and archiving the old one." + - "Was true: factory_floor could not say how many repositories had work or what shipped before today, its head counted only new and waiting jobs, and the manager's brief told it to answer only from factory_floor. Is true now: `repos` counts jobs by repository and state, `shipped` takes `days` (up to 90) and counts merged pull requests, every table opens with the whole factory's counts by state, and the brief lets the manager answer from the app's present state sent with a message and from its other tools." +--- + +The desktop app's Factory manager answered no question, could not be stopped, +and could be archived out of reach. On the reported machine its conversation +worked in the codeaf home (19 GB): three of three `factory_floor` calls never +returned, for up to sixteen minutes, and the conversation had completed one +save since it was made. + +`session.Agent.InterruptWithin` is the stop with its second stage armed behind +it, for a surface across a wire that has no second door to press. The bound is +for the turn that was stopped and no other: the turn's number is taken at the +stop and compared when the clock fires. diff --git a/docs/design/app-v3/PROTOCOL.md b/docs/design/app-v3/PROTOCOL.md index 897d253df7..91d204b43e 100644 --- a/docs/design/app-v3/PROTOCOL.md +++ b/docs/design/app-v3/PROTOCOL.md @@ -80,6 +80,15 @@ decides which combinations are accepted. | `agents.stop` | `in.IDParams` | `Empty` | | `agents.suggest_reply` | `in.ReplySuggestionParams` | `ReplySuggestion` | +`agents.stop` is a bounded stop. It interrupts the running turn and answers at +once; a turn that has still not let go ten seconds later (one parked on a wait +that never reads its cancelled context) is abandoned by the engine: its stream +closes, the turn ends `turn.done` with `error_kind: stopped` and the "Stopped by +you" footer as any stopped turn does, and the next `agents.send` opens an +ordinary turn. The conversation's journal records the turn as let go of, with +what it had spent. A running factory stage lent to the app is interrupted but +never abandoned from here; the job's own Stop ends a stage. + ## asks methods | method | params | result | @@ -257,6 +266,7 @@ keeps the value in a private file the agent's commands read (the model's own | `factory.agentfield_run` | `in.FactoryAgentFieldRunParams` | `FactoryAgentFieldRun` | | `factory.agentfield_reasoners` | `in.FactoryAgentFieldReasonersParams` | `FactoryAgentFieldReasoners` | | `factory.foreman_open` | `in.FactoryRef` | `FactoryForeman` | +| `factory.foreman_fresh` | `in.FactoryRef` | `FactoryForeman` | | `factory.job_send` | `in.FactoryJobSendParams` | `FactoryJobSent` | | `factory.job_close` | `in.FactoryJobActionParams` | `FactoryJobActed` | | `factory.job_control` | `in.FactoryJobControlParams` | `FactoryJobActed` | @@ -2255,7 +2265,30 @@ oldest), `waiting`, `shipped` (shipped today, with pull requests and changes), `risky`, `cheapest`, `oldest`, `all` or `item`. When the person asks, `start` starts one job and `priority` (`p0`–`p3`, with `item`) sets one job's priority, each through the same factory verb the job page's own Start and priority send, -as the person. It never stops, ships, merges or posts. +as the person. It never stops, ships, merges or posts. `repos` lists every +repository with its jobs counted by state, `shipped` takes `days` (1 is today +and the default, at most 90) and counts the pull requests merged, and every +table opens with the whole factory's counts by state. One read or act waits a +bounded time on the factory (20 s to read, 60 s to act) and then answers that +the factory did not, so a floor that never answers cannot hold the turn open. + +The open is the person's own: a Factory manager they closed or archived is +taken out of Closed and answered open. A manager whose conversation works in +the codeaf home (one made by a build that gave it the engine's own folder) is +made again in `/chats/foreman` on its next open, and the conversation it +replaces is stopped and archived. + +### Starting the Factory manager over (`factory.foreman_fresh`) + +The hello feature `factory.foreman_fresh` announces the app's Clear for the +Factory manager. `factory.foreman_fresh {factory}` answers `{factory, agent}` +as the open does, but `agent` is a NEW conversation with only the manager's +brief in it; it takes the manager's place (every later `factory.foreman_open` +answers it) and its `@foreman` handle. The conversation it replaces is stopped +and archived: its history stays on disk and nothing of it runs on. A manager +has this where another conversation has Archive, because it is the factory's +one conversation with one door: archived, that door opened a chat nobody could +write to. Only the factory this engine serves is accepted. ### factory.job_publish_anyway Hello feature `factory.publish_anyway` advertises this person action. Params: diff --git a/internal/appwire/factory_foreman.go b/internal/appwire/factory_foreman.go index 59f545ca2a..af07e73118 100644 --- a/internal/appwire/factory_foreman.go +++ b/internal/appwire/factory_foreman.go @@ -31,6 +31,33 @@ type FactoryForeman struct { Agent string `json:"agent"` } +// FeatureFactoryForemanFresh is announced in hello when the engine serves +// factory.foreman_fresh. +// +// factory.foreman_fresh {factory} STARTS THE MANAGER OVER: a new, empty +// conversation takes its place and its agent id is answered, as an open +// answers it; the one it replaces is stopped and archived, its history kept. +// It is the app's Clear, and the reason a manager has one where every other +// conversation has Archive: the manager is the factory's one conversation +// with one door, so putting it away left that door opening a closed chat, +// while what a person wanted was an empty one. +const FeatureFactoryForemanFresh = "factory.foreman_fresh" + +// FactoryForemanFresher is the engine's half of factory.foreman_fresh. +type FactoryForemanFresher interface { + FactoryForemanFresh(ctx context.Context, p FactoryRef) (FactoryForeman, error) +} + +func withFactoryForemanFreshFeature(hello Hello, e Engine) Hello { + if _, ok := e.(Factory); !ok { + return hello + } + if _, ok := e.(FactoryForemanFresher); ok { + hello = withFeature(hello, FeatureFactoryForemanFresh) + } + return hello +} + // FactoryForemanOpener is the engine's half of factory.foreman_open. type FactoryForemanOpener interface { FactoryForemanOpen(ctx context.Context, p FactoryRef) (FactoryForeman, error) @@ -48,6 +75,30 @@ func withFactoryForemanOpenFeature(hello Hello, e Engine) Hello { func init() { methods[FeatureFactoryForemanOpen] = shaped[FactoryRef, FactoryForeman](foremanOpen) + methods[FeatureFactoryForemanFresh] = shaped[FactoryRef, FactoryForeman](foremanFreshCall) +} + +func foremanFreshCall(e Engine, raw json.RawMessage) (*FactoryForeman, error) { + r, ok := e.(FactoryForemanFresher) + if _, isFactory := e.(Factory); !ok || !isFactory { + return nil, Errorf(CodeUnknownMethod, "this engine does not serve %s", FeatureFactoryForemanFresh) + } + var p FactoryRef + if err := decodeStrict(FeatureFactoryForemanFresh, raw, &p); err != nil { + return nil, err + } + id, err := factoryID(p.Factory) + if err != nil { + return nil, err + } + p.Factory = id + ctx, cancel := factoryCall() + defer cancel() + out, err := r.FactoryForemanFresh(ctx, p) + if err != nil { + return nil, err + } + return &out, nil } func foremanOpen(e Engine, raw json.RawMessage) (*FactoryForeman, error) { diff --git a/internal/appwire/factory_foreman_test.go b/internal/appwire/factory_foreman_test.go index 062acc4a3a..0bcd0630a1 100644 --- a/internal/appwire/factory_foreman_test.go +++ b/internal/appwire/factory_foreman_test.go @@ -55,3 +55,49 @@ func TestForemanOpenIsRegisteredAnnouncedAndAbsentWithoutItsEngine(t *testing.T) } } } + +type freshForemanEngine struct { + foremanEngine + fresh []FactoryRef +} + +func (f *freshForemanEngine) FactoryForemanFresh(_ context.Context, p FactoryRef) (FactoryForeman, error) { + f.fresh = append(f.fresh, p) + return FactoryForeman{Factory: p.Factory, Agent: "foreman2"}, nil +} + +// THE MANAGER CAN BE STARTED OVER. factory.foreman_fresh answers the new +// conversation's agent id, is announced only by a factory engine that serves +// it, beside factory.foreman_open and never instead of it, and refuses a bad +// ref as the open does. +func TestForemanFreshIsRegisteredAnnouncedAndAbsentWithoutItsEngine(t *testing.T) { + const name = "factory.foreman_fresh" + if _, ok := methods[name]; !ok { + t.Fatalf("%s is not registered", name) + } + for _, e := range []Engine{&fakeEngine{}, &factoryEngine{}, &foremanEngine{}} { + if _, err := methods[name].call(e, json.RawMessage(`{"factory":"factory_local"}`)); wireCode(t, err) != CodeUnknownMethod { + t.Errorf("%T: %v", e, err) + } + if h := withFactoryForemanFreshFeature(Hello{}, e); len(h.Features) != 0 { + t.Errorf("%T announced %v", e, h.Features) + } + } + e := &freshForemanEngine{} + out, err := methods[name].call(e, json.RawMessage(`{"factory":"factory_local"}`)) + if err != nil { + t.Fatal(err) + } + if got, _ := json.Marshal(out); string(got) != `{"factory":"factory_local","agent":"foreman2"}` || len(e.fresh) != 1 || len(e.got) != 0 { + t.Fatalf("answered %s after %+v (opens %+v)", got, e.fresh, e.got) + } + h := withFactoryForemanFreshFeature(withFactoryForemanOpenFeature(Hello{}, e), e) + if len(h.Features) != 2 || h.Features[0] != FeatureFactoryForemanOpen || h.Features[1] != FeatureFactoryForemanFresh { + t.Fatalf("announced %v", h.Features) + } + for _, bad := range []string{`{"factory":""}`, `{"factory":"f","job":1}`} { + if _, err := methods[name].call(e, json.RawMessage(bad)); wireCode(t, err) != CodeBadParams { + t.Errorf("%s: %v", bad, err) + } + } +} diff --git a/internal/appwire/server.go b/internal/appwire/server.go index 8f0ea8a99a..7687d0bbee 100644 --- a/internal/appwire/server.go +++ b/internal/appwire/server.go @@ -250,6 +250,7 @@ func (s *Server) Serve(ctx context.Context, in io.Reader, hello Hello, engine En hello = withFactoryAgentFieldReasonersFeature(hello, engine) hello = withFactoryJobAnswerFeature(hello, engine) hello = withFactoryForemanOpenFeature(hello, engine) + hello = withFactoryForemanFreshFeature(hello, engine) hello = withFactoryJobProtectedFeature(hello, engine) hello = withFactoryEffortModelsFeature(hello, engine) hello = withFactoryUsageFeature(hello, engine) diff --git a/internal/manual/chat/factory.md b/internal/manual/chat/factory.md index 7c3416a4d7..68a99bfe9e 100644 --- a/internal/manual/chat/factory.md +++ b/internal/manual/chat/factory.md @@ -287,6 +287,25 @@ Each factory has one manager of its own beside the jobs' managers, titled "Facto When you ask ("start the oldest p1", "make #12 urgent"), it starts that one job or sets its priority (p0 to p3) through the job page's own Start and priority, as you. It never stops, ships, merges or posts, and it can select jobs as a proposal you take or leave. +## the Factory manager counts — how many jobs are running, how many repos are active, how many pull requests merged this week + +Every `factory_floor` table opens with the whole factory's counts by state, such as `in the factory: 2 running, 1 waiting on the person, 40 new`, so a how-many question is answered by one read, whichever table was asked for. The counts are of the factory and never of the rows listed, which stop at 50. + +- `repos`: every repository with its jobs counted by state, the one with work in hand first: `acme/api · 6 open: 1 running, 1 waiting on the person, 4 new · 3 done`. The first line says how many repositories have jobs, how many have open jobs, and how many have a job running, queued, waiting or following a pull request. +- `shipped` with `days`: jobs shipped over that many days, today included (1 is today and the default, at most 90), newest first, each with the day it shipped. The first line counts the pull requests merged among them. + +The Factory manager is not limited to `factory_floor`. When a message arrives with a block of the app's present state, it answers from that for what the block says and reads the factory for the rest, and it answers anything else as any conversation does, with its other tools. + +## the Factory manager did not answer, takes forever, stop does nothing, clear the Factory manager's conversation, archived Factory manager + +One `factory_floor` read waits at most 20 seconds on the factory, and a start or a priority 60 seconds; after that the tool answers "the factory did not answer within 20 seconds" and the turn goes on, and a stop returns it at once. A start or priority that got no answer in time says it may still take effect, so the job is read again before it is asked for twice. + +The app's Stop is bounded. It interrupts the turn, and a turn that has still not let go ten seconds later is let go of by the engine: it ends "Stopped by you" and the next message opens an ordinary turn. + +A Factory manager whose conversation works in the codeaf home is made again in a folder of its own (`chats/foreman` under the factory) the next time it is opened, and the conversation it replaces is stopped and archived. One made by an older build worked in the home itself, where every save swept the whole home and its tool calls never finished. + +The Factory manager is cleared, not archived. `factory.foreman_fresh` starts it over: a new conversation with only its brief takes its place and its `@foreman` address, and the old one is stopped and archived with its history kept. Opening the Factory manager is your own open, so one you closed or archived comes back open. + ## the default steps — what a job runs with no factory.yaml, how many rounds, security pass A repository without `.codeaf/factory.yaml` runs the shipped steps, and so does every job on it, whether a person started it or a start rule did: the same steps, asks and rounds. `codeaf factory steps` prints each one with its prompt, its until and its rounds. diff --git a/internal/remote/client.go b/internal/remote/client.go index 5f8729a085..f7a8a705d0 100644 --- a/internal/remote/client.go +++ b/internal/remote/client.go @@ -2137,6 +2137,13 @@ func (a *Agent) InterruptFor(door session.StopDoor) { _, _ = a.c.call(nil, MethodInterrupt, InterruptArgs{Door: string(door)}) } +// InterruptWithin is a person's stop with its bound on it: the engine abandons +// a turn that has not let go by then ([InterruptArgs]). Like every stop it +// does not wait and reports nothing. +func (a *Agent) InterruptWithin(grace time.Duration) { + _, _ = a.c.call(nil, MethodInterrupt, InterruptArgs{Door: string(session.StopByPerson), Within: grace.Milliseconds()}) +} + // AnswerLaneOffer answers the question a stalled PINNED lane raises: the // machine this person named has gone quiet, there is somewhere else to go, and // a pin is asked rather than overridden. The `y` they pressed takes this road diff --git a/internal/remote/server.go b/internal/remote/server.go index 9e41d9d1b4..7607b5abb1 100644 --- a/internal/remote/server.go +++ b/internal/remote/server.go @@ -129,6 +129,13 @@ type WrappedAgent interface { RewindAt(index int) ([]session.DisplayEntry, error) } +// boundedStopper is an agent whose stop can carry its own second stage: one +// asked of the agent rather than required of [WrappedAgent], so a wrapped +// agent that has never heard of abandoning a turn keeps the plain stop. +type boundedStopper interface { + InterruptWithin(grace time.Duration) +} + // Engine is one opened conversation and the doors that replace it. The door // that builds it (cmd/codeaf) owns config resolution, session-file resolution // and the locked-file fallback; this package owns nothing about how an agent is @@ -3082,6 +3089,14 @@ func (s *server) invoke(call Frame) (out json.RawMessage, err error) { agent.InterruptFor(door) return nil, nil } + // A PERSON'S STOP THAT CAME WITH A BOUND ARMS THE SECOND STAGE HERE, + // because the surface that sent it has no door of its own to press + // when the bound runs out ([InterruptArgs]). An agent that cannot be + // abandoned is stopped as it always was. + if bounded, ok := agent.(boundedStopper); ok && args.Within > 0 { + bounded.InterruptWithin(time.Duration(args.Within) * time.Millisecond) + return nil, nil + } agent.Interrupt() return nil, nil diff --git a/internal/remote/stopbound_test.go b/internal/remote/stopbound_test.go new file mode 100644 index 0000000000..3782b683d7 --- /dev/null +++ b/internal/remote/stopbound_test.go @@ -0,0 +1,111 @@ +package remote + +import ( + "encoding/json" + "sync" + "testing" + "time" + + "github.com/Agent-Field/codeaf/internal/session" +) + +// A STOP'S BOUND CROSSES THE WIRE, AND ONLY A PERSON'S STOP CARRIES ONE. +// +// The desktop app has no second key to press when a stopped turn will not let +// go, so its Stop sends the bound with the stop and the engine keeps it +// ([session.Agent.InterruptWithin]). These pin both ends of that: what the +// surface sends, and which door the engine answers it with. + +// boundedFakeAgent is a [fakeAgent] whose stop can carry its second stage. +type boundedFakeAgent struct { + *fakeAgent + + mu sync.Mutex + graces []time.Duration +} + +func (a *boundedFakeAgent) InterruptWithin(grace time.Duration) { + a.mu.Lock() + defer a.mu.Unlock() + a.graces = append(a.graces, grace) +} + +func (a *boundedFakeAgent) bounds() []time.Duration { + a.mu.Lock() + defer a.mu.Unlock() + return append([]time.Duration(nil), a.graces...) +} + +func TestAPersonsBoundedStopReachesTheAgentsSecondStage(t *testing.T) { + plain := &fakeAgent{} + bounded := &boundedFakeAgent{fakeAgent: plain} + engine := engineOn(plain) + engine.Agent = bounded + sess := NewSession(engine, true) + t.Cleanup(func() { _ = sess.Close() }) + l := dialSession(t, sess) + l.hello(Hello{Version: Version}) + + l.ok(1, MethodInterrupt, InterruptArgs{Door: string(session.StopByPerson), Within: 2500}) + if got := bounded.bounds(); len(got) != 1 || got[0] != 2500*time.Millisecond { + t.Fatalf("the bounded stop armed %v, want one bound of 2.5s", got) + } + plain.mu.Lock() + interrupts := plain.interrupts + plain.mu.Unlock() + if interrupts != 0 { + t.Fatalf("a bounded stop also sent %d plain interrupts", interrupts) + } + + // A STOP WITH NO BOUND IS THE STOP IT ALWAYS WAS, from the terminal and + // from every surface older than the field. + l.ok(2, MethodInterrupt, InterruptArgs{Door: string(session.StopByPerson)}) + l.ok(3, MethodInterrupt, nil) + // AND MACHINERY IS NEVER BOUNDED: a window leaving is not a person asking + // for the work to end, and a bound there would abandon a turn nobody stopped. + l.ok(4, MethodInterrupt, InterruptArgs{Door: string(session.StopByLeaving), Within: 2500}) + plain.mu.Lock() + interrupts, door := plain.interrupts, plain.stopDoor + plain.mu.Unlock() + if interrupts != 3 || door != session.StopByLeaving { + t.Fatalf("the unbounded stops reached the agent %d times, last through %q", interrupts, door) + } + if got := bounded.bounds(); len(got) != 1 { + t.Fatalf("an unbounded or machinery stop armed a bound: %v", got) + } +} + +// AN AGENT THAT CANNOT BE ABANDONED IS STOPPED AS IT ALWAYS WAS: the bound is +// asked of the agent, never required of it. +func TestABoundedStopToAnAgentWithoutASecondStageIsAPlainStop(t *testing.T) { + agent := &fakeAgent{} + sess := NewSession(engineOn(agent), true) + t.Cleanup(func() { _ = sess.Close() }) + l := dialSession(t, sess) + l.hello(Hello{Version: Version}) + + l.ok(1, MethodInterrupt, InterruptArgs{Door: string(session.StopByPerson), Within: 2500}) + agent.mu.Lock() + defer agent.mu.Unlock() + if agent.interrupts != 1 || agent.stopDoor != session.StopByPerson { + t.Fatalf("the stop reached the agent %d times through %q", agent.interrupts, agent.stopDoor) + } +} + +// AND THE SURFACE SENDS THE BOUND IN MILLISECONDS ON THE PERSON'S DOOR. +func TestTheClientsBoundedStopSendsItsBound(t *testing.T) { + client, e := newEngine(t) + client.Agent().InterruptWithin(10 * time.Second) + + calls := e.calls(MethodInterrupt) + if len(calls) != 1 { + t.Fatalf("the bounded stop travelled %d times", len(calls)) + } + var args InterruptArgs + if err := json.Unmarshal(calls[0].Payload, &args); err != nil { + t.Fatalf("stop payload: %v", err) + } + if args.Door != string(session.StopByPerson) || args.Within != 10_000 { + t.Fatalf("stop args = %+v", args) + } +} diff --git a/internal/remote/wire.go b/internal/remote/wire.go index 41daacb68b..c78a598176 100644 --- a/internal/remote/wire.go +++ b/internal/remote/wire.go @@ -1593,8 +1593,15 @@ type Moved struct { // AN EMPTY DOOR IS A PERSON'S OWN STOP, which is what an older surface that // sends no arguments at all means and what it always meant. That is the one // direction this may fail in that costs nothing: a stop is still a stop. +// +// WITHIN IS THE STOP'S BOUND, IN MILLISECONDS, for a surface that cannot keep +// the deadline itself: a turn that has not let go by then is abandoned by the +// engine ([session.Agent.InterruptWithin]). Zero is the plain stop every +// surface sent before the field existed, and an engine too old to read it +// stops as it always did, with no second stage: the field can only add one. type InterruptArgs struct { - Door string `json:"door,omitempty"` + Door string `json:"door,omitempty"` + Within int64 `json:"within,omitempty"` } type SubmitArgs struct { diff --git a/internal/session/abandon.go b/internal/session/abandon.go index b62f39d99c..f82713906b 100644 --- a/internal/session/abandon.go +++ b/internal/session/abandon.go @@ -3,6 +3,7 @@ package session import ( "context" "sync" + "time" "github.com/Agent-Field/agentfield/sdk/go/ai" ) @@ -100,8 +101,41 @@ const AbandonStopTimeout AbandonReason = "stop timeout" // arriving on the next keystroke opens an ordinary turn and cannot be refused // for a turn nobody is waiting for. func (a *Agent) Abandon(reason AbandonReason) (Usage, bool) { + return a.abandonTurn(reason, 0) +} + +// InterruptWithin is a person's stop WITH ITS SECOND STAGE ARMED BEHIND IT: the +// turn is interrupted now, and one that has still not let go when grace has +// run is abandoned ([Agent.Abandon]). +// +// IT IS FOR A SURFACE THAT CANNOT KEEP THE DEADLINE ITSELF. The terminal owns +// its own countdown and calls Abandon when it runs out (tui3's stopGrace). A +// surface on the far side of a wire has no second door to call, so its stop +// used to be the first stage alone: a turn parked on a wait that ignores its +// context stayed "working" until the engine was killed, which is how the +// desktop app's Stop came to do nothing at all. +// +// THE BOUND IS FOR THE TURN THAT WAS STOPPED AND NO OTHER. The turn's number is +// taken before the interrupt and compared when the clock fires, so a turn that +// ended in time, and whatever the person started after it, are left alone. A +// grace of zero or less arms nothing and is the plain stop. +func (a *Agent) InterruptWithin(grace time.Duration) { + a.mu.Lock() + turn, running := a.turnSeq, a.running && !a.closed + a.mu.Unlock() + a.Interrupt() + if !running || grace <= 0 { + return + } + time.AfterFunc(grace, func() { a.abandonTurn(AbandonStopTimeout, turn) }) +} + +// abandonTurn is [Agent.Abandon] for one turn by its number, or for whichever +// is running when turn is zero (a turn's number is never zero: it is counted +// from one). +func (a *Agent) abandonTurn(reason AbandonReason, turn uint64) (Usage, bool) { a.mu.Lock() - if a.closed || !a.running { + if a.closed || !a.running || (turn != 0 && a.turnSeq != turn) { a.mu.Unlock() return Usage{}, false } diff --git a/internal/session/abandon_test.go b/internal/session/abandon_test.go index a6df386aa2..cdb4c93dbc 100644 --- a/internal/session/abandon_test.go +++ b/internal/session/abandon_test.go @@ -141,6 +141,99 @@ func TestAnUncancellableWaitDoesNotKeepATurnFromBeingAbandoned(t *testing.T) { waitUntilClosed(t, "the abandoned turn's stream closing", closed, 10*time.Second) } +// A BOUNDED STOP LETS GO BY ITSELF. +// +// A surface across a wire has no second door to press: the desktop app's Stop +// was the first stage alone, so a turn parked like this one stayed "working" +// until the app was quit. [Agent.InterruptWithin] arms the second stage behind +// the stop, and the proof is the same one as above with nobody calling Abandon: +// the stream closes, and the journal says the turn was let go of, once. +func TestABoundedStopAbandonsATurnThatWouldNotLetGo(t *testing.T) { + completer := &scriptedCompleter{steps: []step{ + func(context.Context, []ai.Message) (*ai.Response, error) { + return toolResponse("call-1", "wedge", `{}`), nil + }, + }} + agent, entered, journal := wedged(t, completer) + + events, err := agent.Submit(context.Background(), "park on something that cannot be cancelled") + if err != nil { + t.Fatalf("Submit: %v", err) + } + waitUntilClosed(t, "the turn parking on the wedge", entered, 10*time.Second) + + agent.InterruptWithin(50 * time.Millisecond) + + closed := make(chan struct{}) + go func() { + for range events { + } + close(closed) + }() + waitUntilClosed(t, "the bounded stop letting go of the turn", closed, 10*time.Second) + if lines := abandonedLines(t, journal); len(lines) != 1 || lines[0].Reason != string(AbandonStopTimeout) { + t.Fatalf("want one abandoned line saying %q, got %+v", AbandonStopTimeout, lines) + } + if _, again := agent.Abandon(AbandonStopTimeout); again { + t.Fatal("the bounded stop left a turn for a second Abandon to let go of") + } +} + +// AND ITS CLOCK IS FOR THE TURN THAT WAS STOPPED, NEVER THE ONE AFTER IT. +// +// The first turn here ends on the interrupt alone, well inside its bound, and +// the person sends again before that bound runs out. The second turn parks on +// the wedge: when the first stop's clock fires it finds a turn running, and a +// clock that only asked "is anything running" would let go of work nobody +// stopped. +func TestABoundedStopLeavesTheTurnAfterItAlone(t *testing.T) { + reached := make(chan struct{}) + completer := &scriptedCompleter{steps: []step{ + func(ctx context.Context, _ []ai.Message) (*ai.Response, error) { + close(reached) + <-ctx.Done() + return nil, ctx.Err() + }, + func(context.Context, []ai.Message) (*ai.Response, error) { + return toolResponse("call-1", "wedge", `{}`), nil + }, + }} + agent, entered, journal := wedged(t, completer) + + first, err := agent.Submit(context.Background(), "ask something that never answers") + if err != nil { + t.Fatalf("Submit: %v", err) + } + waitUntilClosed(t, "the request going out", reached, 10*time.Second) + const grace = 400 * time.Millisecond + agent.InterruptWithin(grace) + for range first { + } + + second, err := agent.Submit(context.Background(), "and now park") + if err != nil { + t.Fatalf("the session refused the prompt after a stopped turn: %v", err) + } + waitUntilClosed(t, "the second turn parking on the wedge", entered, 10*time.Second) + + ended := make(chan struct{}) + go func() { + for range second { + } + close(ended) + }() + select { + case <-ended: + t.Fatal("the first turn's stop let go of the turn after it") + case <-time.After(3 * grace): + } + if lines := abandonedLines(t, journal); len(lines) != 0 { + t.Fatalf("a turn was journaled as abandoned though none was: %+v", lines) + } + agent.Abandon(AbandonStopTimeout) + waitUntilClosed(t, "the parked turn being let go of at the test's end", ended, 10*time.Second) +} + // AND THE NEXT THING THE PERSON TYPES OPENS AN ORDINARY TURN. // // A stop that freed the screen and left the session claiming to be busy would be diff --git a/internal/session/tools_floor.go b/internal/session/tools_floor.go index f83116f64e..33af23b076 100644 --- a/internal/session/tools_floor.go +++ b/internal/session/tools_floor.go @@ -34,7 +34,9 @@ package session import ( "context" "encoding/json" + "errors" "fmt" + "slices" "sort" "strconv" "strings" @@ -85,6 +87,24 @@ const ( floorAskRunning = "running" floorAskQueue = "queue" floorAskShipped = "shipped" + floorAskRepos = "repos" +) + +// floorAsks is every ask in the order the schema and the refusal list them. +var floorAsks = []string{floorAskRunning, floorAskQueue, floorAskWaiting, floorAskShipped, floorAskRepos, floorAskRisky, floorAskCheapest, floorAskOldest, floorAskAll, floorAskItem} + +// floorShippedDaysMost is how far back `shipped` reads when asked for more +// than today: a quarter, which is as far as "recent work" reaches. +const floorShippedDaysMost = 90 + +// floorReadWait and floorActWait are how long one call may wait on the +// factory before the tool answers that it did not. A healthy floor reads in +// milliseconds; the bound is for the read that never comes back, which once +// held a manager's turn open until the app was quit. A start or a priority is +// a request to the factory and is given longer. +const ( + floorReadWait = 20 * time.Second + floorActWait = 60 * time.Second ) // The floor's limits on one answer: how many rows a ranked ask lists, how many @@ -109,14 +129,17 @@ const ( const factoryFloorDescription = "Read the whole factory, start or prioritise a job when the person asks, and propose what to take on by selecting items. " + "You are the factory's foreman: you read, rank, propose and sum up. Start a job (start) or set its priority (priority with item) only when the person asks for that job in this conversation; nothing here ships, merges or posts. " + - "ask: running (jobs running or queued to run, with their step), queue (jobs not started yet, most urgent then oldest first), waiting (jobs that need the person), shipped (jobs shipped today, with their pull requests and changes, to sum up), risky, cheapest (ten, by estimate), oldest (ten), all, or item (one item in full: its read, facts, stages, question and body; set item). " + + "ask: running (jobs running or queued to run, with their step), queue (jobs not started yet, most urgent then oldest first), waiting (jobs that need the person), shipped (jobs shipped today, with their pull requests and changes, to sum up; set days to read further back), repos (every repository with its count of jobs in each state, for how many and where), risky, cheapest (ten, by estimate), oldest (ten), all, or item (one item in full: its read, facts, stages, question and body; set item). " + + "Every table opens with the factory's counts by state, so a how-many question is answered by one read. " + "start takes one job by the number in its ref and starts it as the job page's Start does. priority is p0 (urgent), p1 (high), p2 (medium) or p3 (low), set on item. " + "item, mark and unmark take items by the number in their ref (#4 is 4). Only a new item can be selected; selections stay until the person runs or unselects them. The fields are named mark and unmark; the person calls them select and unselect. " + + "A message that arrives with a block of the app's present state is true as of when it was sent; read here for what it does not say. " + "Name items by their ref in everything you say." func factoryFloorSchemaJSON() string { return `{"type":"object","properties":{` + - `"ask":{"type":"string","enum":["` + strings.Join([]string{floorAskRunning, floorAskQueue, floorAskWaiting, floorAskShipped, floorAskRisky, floorAskCheapest, floorAskOldest, floorAskAll, floorAskItem}, `","`) + `"],"description":"What to read from the factory."},` + + `"ask":{"type":"string","enum":["` + strings.Join(floorAsks, `","`) + `"],"description":"What to read from the factory."},` + + `"days":{"type":"integer","description":"With ask shipped: how many days to read, today included (1 is today and the default; at most ` + strconv.Itoa(floorShippedDaysMost) + `)."},` + `"item":{"type":"integer","description":"With ask item or priority: the number in the item's ref (#4 is 4)."},` + `"start":{"type":"integer","description":"The number in the ref of one job to start now, only when the person asked."},` + `"priority":{"type":"string","enum":["p0","p1","p2","p3"],"description":"With item: the priority to set on that job, only when the person asked."},` + @@ -142,6 +165,7 @@ func (a *Agent) factoryFloorTool() bare.Tool { Unmark []int `json:"unmark"` Start int `json:"start"` Priority string `json:"priority"` + Days int `json:"days"` } if len(args) > 0 { if err := decodeToolArguments(args, &parsed); err != nil { @@ -154,10 +178,11 @@ func (a *Agent) factoryFloorTool() bare.Tool { if ask == "" && len(parsed.Mark)+len(parsed.Unmark) == 0 && !acts { ask = floorAskAll } - switch ask { - case "", floorAskRunning, floorAskQueue, floorAskWaiting, floorAskShipped, floorAskRisky, floorAskCheapest, floorAskOldest, floorAskAll, floorAskItem: - default: - return rejectToolArguments(ctx, "Invalid arguments: ask is one of running, queue, waiting, shipped, risky, cheapest, oldest, all or item."), true, nil + if ask != "" && !slices.Contains(floorAsks, ask) { + return rejectToolArguments(ctx, "Invalid arguments: ask is one of "+strings.Join(floorAsks[:len(floorAsks)-1], ", ")+" or "+floorAsks[len(floorAsks)-1]+"."), true, nil + } + if parsed.Days < 0 || parsed.Days > floorShippedDaysMost || (parsed.Days > 0 && ask != floorAskShipped) { + return rejectToolArguments(ctx, "Invalid arguments: days goes with ask shipped, from 1 to "+strconv.Itoa(floorShippedDaysMost)+"."), true, nil } if priority != "" && parsed.Item <= 0 { return rejectToolArguments(ctx, "Invalid arguments: priority needs item, the number in the job's ref."), true, nil @@ -168,7 +193,7 @@ func (a *Agent) factoryFloorTool() bare.Tool { if ask == floorAskItem && parsed.Item <= 0 { return rejectToolArguments(ctx, "Invalid arguments: ask item needs item, the number in the item's ref."), true, nil } - return a.floorRun(ctx, ask, parsed.Item, parsed.Mark, parsed.Unmark) + return a.floorRun(ctx, ask, parsed.Item, parsed.Days, parsed.Mark, parsed.Unmark) }, } } @@ -180,7 +205,7 @@ func (a *Agent) floorAct(ctx context.Context, start, item int, priority string) if !ok { return "Nothing changed: this window cannot start or prioritise a job; the person can on the job's page.", true, nil } - snap, err := a.config.Floor.Snapshot(ctx) + snap, err := floorSnapshot(ctx, a.config.Floor) if err != nil { return "the factory could not be read: " + oneLine(err.Error()), true, nil } @@ -195,12 +220,17 @@ func (a *Agent) floorAct(ctx context.Context, start, item int, priority string) case len(found) > 1: return fmt.Sprintf("Nothing changed: #%d is on %d repositories (%s)", n, len(found), floorRepos(found)), true, nil } - var line string - if start > 0 { - line, err = actor.StartItem(ctx, found[0].ID) - } else { + line, err := floorWithin(ctx, floorActWait, func(ctx context.Context) (string, error) { + if start > 0 { + return actor.StartItem(ctx, found[0].ID) + } value, _ := factory.PriorityFromShort(priority) - line, err = actor.SetPriority(ctx, found[0].ID, value) + return actor.SetPriority(ctx, found[0].ID, value) + }) + if errors.Is(err, errFloorLate) { + // NOT "Nothing changed": the request was sent and its answer never + // came, so whether it landed is exactly what is not known. + return fmt.Sprintf("%s for %s. It may still take effect: read %s again before asking twice.", floorLateWords(floorActWait), found[0].Ref(), found[0].Ref()), true, nil } if err != nil { return oneLine(err.Error()), true, nil @@ -208,11 +238,61 @@ func (a *Agent) floorAct(ctx context.Context, start, item int, priority string) return line, false, nil } +// errFloorLate is a call on the floor that did not answer inside its wait. +var errFloorLate = errors.New("the factory did not answer in time") + +func floorLateWords(wait time.Duration) string { + return fmt.Sprintf("the factory did not answer within %d seconds", int(wait/time.Second)) +} + +// floorWithin runs one call on the floor's door and WAITS FOR IT NO LONGER +// THAN wait, and no longer than the turn. The door reads disk and may take a +// lock, and it is the engine's own code rather than a subprocess a context +// can kill: a door that never came back held the manager's turn open, with +// the person's stop changing nothing, until the app was quit. So the call +// runs on a goroutine of its own and this returns at the bound or the moment +// the turn is cut, whichever is first; a call that outlives it finishes with +// nobody waiting, which is the most a caller can do about code that will not +// look at its context. +func floorWithin[T any](ctx context.Context, wait time.Duration, call func(context.Context) (T, error)) (T, error) { + ctx, cancel := context.WithTimeout(ctx, wait) + defer cancel() + type answer struct { + value T + err error + } + done := make(chan answer, 1) + go func() { + value, err := call(ctx) + done <- answer{value, err} + }() + select { + case got := <-done: + return got.value, got.err + case <-ctx.Done(): + var none T + if errors.Is(ctx.Err(), context.DeadlineExceeded) { + return none, errFloorLate + } + return none, ctx.Err() + } +} + +// floorSnapshot is the floor read under [floorReadWait], in words the model +// can act on when it did not come. +func floorSnapshot(ctx context.Context, door FloorDoor) (factory.Snapshot, error) { + snap, err := floorWithin(ctx, floorReadWait, door.Snapshot) + if errors.Is(err, errFloorLate) { + return snap, errors.New(floorLateWords(floorReadWait) + "; say so rather than guess, and try once more if the person asks") + } + return snap, err +} + // floorRun does one call: the marks first, then the ask over the floor as it // is after them, so a call that marks and reads sees its own marks. -func (a *Agent) floorRun(ctx context.Context, ask string, item int, mark, unmark []int) (string, bool, error) { +func (a *Agent) floorRun(ctx context.Context, ask string, item, days int, mark, unmark []int) (string, bool, error) { door := a.config.Floor - snap, err := door.Snapshot(ctx) + snap, err := floorSnapshot(ctx, door) if err != nil { return "the factory could not be read: " + oneLine(err.Error()), true, nil } @@ -224,13 +304,13 @@ func (a *Agent) floorRun(ctx context.Context, ask string, item int, mark, unmark return strings.Join(out, "\n"), true, nil } if ask != "" { - if snap, err = door.Snapshot(ctx); err != nil { + if snap, err = floorSnapshot(ctx, door); err != nil { return strings.Join(out, "\n"), false, nil } } } if ask != "" { - out = append(out, FloorAnswer(snap, ask, item)) + out = append(out, floorAnswer(snap, ask, item, days)) } return strings.Join(out, "\n\n"), false, nil } @@ -258,20 +338,38 @@ func floorMarks(ctx context.Context, door FloorDoor, snap factory.Snapshot, mark return ids, refs } if ids, refs := resolve(unmark, false); len(ids) > 0 { - if err := door.Mark(ctx, ids, false); err != nil { - return append(said, "nothing unselected: "+oneLine(err.Error())), true + if err := floorMark(ctx, door, ids, false); err != nil { + return append(said, floorMarkRefused("unselected", err)), true } said = append(said, "unselected "+strings.Join(refs, " ")) } if ids, refs := resolve(mark, true); len(ids) > 0 { - if err := door.Mark(ctx, ids, true); err != nil { - return append(said, "nothing selected: "+oneLine(err.Error())), true + if err := floorMark(ctx, door, ids, true); err != nil { + return append(said, floorMarkRefused("selected", err)), true } said = append(said, "selected "+strings.Join(refs, " ")+FloorMarkedTail) } return append(said, notes...), false } +// floorMark is the door's Mark under [floorActWait]. +func floorMark(ctx context.Context, door FloorDoor, ids []int, on bool) error { + _, err := floorWithin(ctx, floorActWait, func(ctx context.Context) (struct{}, error) { + return struct{}{}, door.Mark(ctx, ids, on) + }) + return err +} + +// floorMarkRefused is the line for a selection the door did not make. One +// that got no answer in time is NOT said to have changed nothing: the write +// was asked for, and whether it landed is what is not known. +func floorMarkRefused(word string, err error) string { + if errors.Is(err, errFloorLate) { + return floorLateWords(floorActWait) + ": whether anything was " + word + " is not known; read the factory again" + } + return "nothing " + word + ": " + oneLine(err.Error()) +} + // floorFind is every item on the floor whose ref is #n, dismissed ones left // out. func floorFind(snap factory.Snapshot, n int) []factory.Item { @@ -305,12 +403,21 @@ func floorRepos(items []factory.Item) string { // FloorAnswer is one ask's text over a floor: a compact table, one item a // line, or one item in full for `item`. func FloorAnswer(snap factory.Snapshot, ask string, item int) string { + return floorAnswer(snap, ask, item, 0) +} + +// floorAnswer is [FloorAnswer] with how many days `shipped` reads; zero is +// today. +func floorAnswer(snap factory.Snapshot, ask string, item, days int) string { now := snap.Now if now.IsZero() { now = time.Now() } if ask == floorAskShipped { - return floorShippedToday(snap, now) + return floorShipped(snap, now, days) + } + if ask == floorAskRepos { + return floorReposAnswer(snap) } var open []factory.Item for _, it := range snap.Items { @@ -395,25 +502,120 @@ func FloorAnswer(snap factory.Snapshot, ask string, item int) string { } // floorSummary is the table's head: what was asked, how many rows answer it, -// and the floor's counts that matter to a morning's choice, each left out -// when it is nothing. +// and THE FLOOR'S COUNTS BY STATE, each left out when it is nothing. The +// counts are of the whole floor and not of the rows, so "how many are +// running" is answered by whichever table was read: a manager that had to +// count rows of a table cut at fifty answered with the cut. func floorSummary(open []factory.Item, rows int, head string) string { parts := []string{head, strconv.Itoa(rows) + " " + floorPlural(rows, "item", "items")} - count := func(keep func(factory.Item) bool) int { - return len(floorKeep(open, keep)) + if counts := floorCounts(open); counts != "" { + parts = append(parts, "in the factory: "+counts) } - if n := count(func(it factory.Item) bool { return it.State == factory.StateNew }); n > 0 { - parts = append(parts, strconv.Itoa(n)+" new in the factory") - } - if n := count(func(it factory.Item) bool { return it.State == factory.StateNeedsYou }); n > 0 { - parts = append(parts, strconv.Itoa(n)+" waiting on the person") - } - if n := count(func(it factory.Item) bool { return it.Marked && it.State == factory.StateNew }); n > 0 { + if n := len(floorKeep(open, func(it factory.Item) bool { return it.Marked && it.State == factory.StateNew })); n > 0 { parts = append(parts, strconv.Itoa(n)+" selected") } return strings.Join(parts, " · ") } +// The states a count names, in the order a person asks about them, each in +// the words the rows use. +var floorCountOrder = []struct { + word string + keep func(factory.Item) bool +}{ + {"running", func(it factory.Item) bool { return it.State == factory.StateRunning }}, + {"queued to run", func(it factory.Item) bool { return it.State == factory.StateQueued }}, + {"waiting on the person", func(it factory.Item) bool { return it.State == factory.StateNeedsYou }}, + {"following a pull request", func(it factory.Item) bool { return it.State == factory.StateShipped && floorFollowing(it) }}, + {"landed", func(it factory.Item) bool { return it.State == factory.StateLanded }}, + {"new", func(it factory.Item) bool { return it.State == factory.StateNew }}, +} + +// floorCounts is items counted by state in one line, `2 running, 1 waiting on +// the person, 40 new`, with nothing said for a state nothing is in. +func floorCounts(items []factory.Item) string { + var parts []string + for _, c := range floorCountOrder { + if n := len(floorKeep(items, c.keep)); n > 0 { + parts = append(parts, strconv.Itoa(n)+" "+c.word) + } + } + return strings.Join(parts, ", ") +} + +// floorReposAnswer is every repository the floor has jobs on, one a line with +// its jobs counted by state, the busiest first. It answers "how many +// repositories are active" and "where is the work", which no table of jobs cut +// at [floorAllRows] could. Dismissed jobs are counted nowhere; a repository +// whose every job is done says so rather than being left out, so the count of +// lines is the count of repositories the factory has worked on. +func floorReposAnswer(snap factory.Snapshot) string { + type line struct { + name string + open, done int + items, active []factory.Item + } + by := map[string]*line{} + var order []*line + for _, it := range snap.Items { + if it.State == factory.StateDismissed { + continue + } + l := by[it.Repo] + if l == nil { + l = &line{name: it.Repo} + by[it.Repo] = l + order = append(order, l) + } + if it.State == factory.StateShipped && !floorFollowing(it) { + l.done++ + continue + } + l.open++ + l.items = append(l.items, it) + if it.State != factory.StateNew { + l.active = append(l.active, it) + } + } + if len(order) == 0 { + return floorNothing + } + sort.SliceStable(order, func(i, j int) bool { + if len(order[i].active) != len(order[j].active) { + return len(order[i].active) > len(order[j].active) + } + if order[i].open != order[j].open { + return order[i].open > order[j].open + } + return order[i].name < order[j].name + }) + withOpen, withActive := 0, 0 + for _, l := range order { + if l.open > 0 { + withOpen++ + } + if len(l.active) > 0 { + withActive++ + } + } + var b strings.Builder + fmt.Fprintf(&b, "repositories · %d with jobs · %d with open jobs · %d with a job running, queued, waiting or following a pull request", len(order), withOpen, withActive) + for _, l := range order { + name := l.name + if strings.TrimSpace(name) == "" { + name = "(no repository)" + } + b.WriteString("\n" + name) + if l.open > 0 { + fmt.Fprintf(&b, " · %d open: %s", l.open, floorCounts(l.items)) + } + if l.done > 0 { + fmt.Fprintf(&b, " · %d done", l.done) + } + } + return b.String() +} + // FloorRow is one item in one line: its ref and title, then every fact the // floor knows about it, each labelled, and NOTHING FOR WHAT IS NOT KNOWN (the // emptiness law) — an item with no estimate says no estimate. @@ -627,30 +829,61 @@ func floorCut(s string, n int) string { return strings.TrimSpace(string(r[:n])) + "…" } -// floorShippedToday is every job that shipped since the start of the floor's -// day, each with its pull request and its change, for the foreman to sum up; -// one line says none when none did. -func floorShippedToday(snap factory.Snapshot, now time.Time) string { +// floorShipped is every job that shipped since the start of the floor's day, +// or of the day `days` back with today counted as the first, each with its +// pull request and its change, for the foreman to sum up; one line says none +// when none did. A job counts on the day it was done, or the day its pull +// request merged when no done time was kept. The head counts the pull requests +// merged among them, which is what "how many did we merge" asks. +func floorShipped(snap factory.Snapshot, now time.Time, days int) string { + if days < 1 { + days = 1 + } y, m, d := now.Date() - since := time.Date(y, m, d, 0, 0, 0, 0, now.Location()) - var rows []factory.Item + since := time.Date(y, m, d, 0, 0, 0, 0, now.Location()).AddDate(0, 0, -(days - 1)) + window := "today" + if days > 1 { + window = fmt.Sprintf("in the last %d days", days) + } + type shipped struct { + it factory.Item + at time.Time + } + var rows []shipped + merged := 0 for _, it := range snap.Items { at := it.DoneAt if at.IsZero() && it.PullRequest != nil { at = it.PullRequest.MergedAt } if it.State == factory.StateShipped && !at.Before(since) { - rows = append(rows, it) + rows = append(rows, shipped{it, at}) + if it.PullRequest != nil && it.PullRequest.State == factory.PRMerged { + merged++ + } } } if len(rows) == 0 { - return "nothing shipped today" + return "nothing shipped " + window } + sort.SliceStable(rows, func(i, j int) bool { return rows[i].at.After(rows[j].at) }) var b strings.Builder - fmt.Fprintf(&b, "shipped today · %d %s", len(rows), floorPlural(len(rows), "job", "jobs")) - for _, it := range rows { + fmt.Fprintf(&b, "shipped %s · %d %s", window, len(rows), floorPlural(len(rows), "job", "jobs")) + if merged > 0 { + fmt.Fprintf(&b, " · %d %s merged", merged, floorPlural(merged, "pull request", "pull requests")) + } + more := 0 + if len(rows) > floorAllRows { + more = len(rows) - floorAllRows + rows = rows[:floorAllRows] + } + for _, row := range rows { + it := row.it b.WriteString("\n") b.WriteString(FloorRow(it, now)) + if days > 1 && !row.at.IsZero() { + b.WriteString(" · shipped " + row.at.In(now.Location()).Format("2006-01-02")) + } if pr := it.PullRequest; pr != nil && strings.TrimSpace(pr.URL) != "" { b.WriteString(" · " + pr.URL) } @@ -662,6 +895,9 @@ func floorShippedToday(snap factory.Snapshot, now time.Time) string { b.WriteString(" · " + how) } } + if more > 0 { + fmt.Fprintf(&b, "\nand %d more, older", more) + } return b.String() } diff --git a/internal/session/tools_floor_test.go b/internal/session/tools_floor_test.go index 0c1cf261c7..01363c25f4 100644 --- a/internal/session/tools_floor_test.go +++ b/internal/session/tools_floor_test.go @@ -7,6 +7,7 @@ package session import ( "context" "encoding/json" + "errors" "strconv" "strings" "testing" @@ -143,7 +144,7 @@ func TestFactoryFloorEachAskShapesItsTable(t *testing.T) { {"risky", []string{"#1 ledger counts twice", "#7 new auth flow", "risk: touches money"}, []string{"#4 ", "#3 "}, []string{"#1 ", "#7 "}}, {"cheapest", []string{"least complex first"}, []string{"#3 ", "#9 "}, []string{"#4 ", "#1 ", "#6 ", "#7 "}}, {"oldest", []string{"oldest first", "9d old"}, []string{"#2 "}, []string{"#1 ", "#6 ", "#4 ", "#7 ", "#3 ", "#9 "}}, - {"all", []string{"every open item", "6 items", "4 new in the factory", "1 waiting on the person", "#9 cache warmup", "40m old"}, []string{"hidden thing", "$0.00", "est $0.0000"}, nil}, + {"all", []string{"every open item", "6 items", "in the factory: 1 running, 1 waiting on the person, 4 new", "#9 cache warmup", "40m old"}, []string{"hidden thing", "$0.00", "est $0.0000"}, nil}, } { text, isErr := floorCall(t, floorLab(), `{"ask":"`+c.ask+`"}`) if isErr { @@ -365,3 +366,180 @@ func TestFloorRowSaysAFollowingJobIsOnFollowPR(t *testing.T) { t.Errorf("a running job reads %q", row) } } + +// EVERY TABLE OPENS WITH THE WHOLE FACTORY'S COUNTS, whichever rows it lists. +// The owner's manager was asked how many jobs were running and had only a +// table cut at fifty rows to count, so a how-many question is answered by the +// head of any read, and the head counts the floor, never the rows. +func TestFactoryFloorHeadCountsTheWholeFactoryWhateverWasAsked(t *testing.T) { + const counts = "in the factory: 1 running, 1 waiting on the person, 4 new" + for _, ask := range []string{"running", "queue", "waiting", "risky", "oldest", "all"} { + text, isErr := floorCall(t, floorLab(), `{"ask":"`+ask+`"}`) + if head, _, _ := strings.Cut(text, "\n"); isErr || !strings.Contains(head, counts) { + t.Errorf("%s: the head does not count the factory: %q", ask, head) + } + } +} + +// `repos` IS WHERE THE WORK IS: every repository with its jobs counted by +// state, the one with work in hand first. "How many repos are active in our +// factory" was the question the manager could not answer from a table of jobs. +func TestFactoryFloorReposCountsJobsByRepository(t *testing.T) { + door := floorLab() + now := door.snap.Now + door.snap.Items = append(door.snap.Items, + factory.Item{ID: 20, Num: 1, Repo: "acme/web", Kind: factory.KindIssue, Title: "dark mode", State: factory.StateNew, Created: now}, + factory.Item{ID: 21, Num: 2, Repo: "acme/web", Kind: factory.KindIssue, Title: "old banner", State: factory.StateShipped, DoneAt: now.Add(-72 * time.Hour), DoneHow: "merged"}, + factory.Item{ID: 22, Num: 1, Repo: "acme/docs", Kind: factory.KindIssue, Title: "typo", State: factory.StateShipped, DoneAt: now.Add(-72 * time.Hour), DoneHow: "merged"}, + factory.Item{ID: 23, Num: 5, Repo: "acme/gone", Kind: factory.KindIssue, Title: "dropped", State: factory.StateDismissed}) + text, isErr := floorCall(t, door, `{"ask":"repos"}`) + if isErr { + t.Fatalf("refused: %s", text) + } + lines := strings.Split(text, "\n") + if len(lines) != 4 { + t.Fatalf("want a head and three repositories, got:\n%s", text) + } + if want := "repositories · 3 with jobs · 2 with open jobs · 1 with a job running, queued, waiting or following a pull request"; lines[0] != want { + t.Errorf("head = %q, want %q", lines[0], want) + } + for i, want := range []string{ + "acme/api · 6 open: 1 running, 1 waiting on the person, 4 new", + "acme/web · 1 open: 1 new · 1 done", + "acme/docs · 1 done", + } { + if lines[i+1] != want { + t.Errorf("line %d = %q, want %q", i+1, lines[i+1], want) + } + } + if strings.Contains(text, "acme/gone") { + t.Errorf("a repository with only a dismissed job is listed:\n%s", text) + } +} + +// `shipped` READS BACK AS FAR AS IT IS ASKED TO, and counts the pull requests +// merged. Today alone could not answer "how many did we merge lately". +func TestFactoryFloorShippedReadsBackByDaysAndCountsMergedPullRequests(t *testing.T) { + door := floorLab() + now := door.snap.Now + merged := func(id, num, pr int, ago time.Duration) factory.Item { + return factory.Item{ID: id, Num: num, Repo: "acme/api", Kind: factory.KindIssue, Title: "shipped " + strconv.Itoa(num), State: factory.StateShipped, DoneAt: now.Add(-ago), DoneHow: "merged", + PullRequest: &factory.PullRequest{Number: pr, URL: "https://github.com/acme/api/pull/" + strconv.Itoa(pr), State: factory.PRMerged}} + } + door.snap.Items = append(door.snap.Items, + merged(30, 31, 301, time.Hour), + merged(31, 32, 302, 50*time.Hour), + merged(32, 33, 303, 20*24*time.Hour), + factory.Item{ID: 33, Num: 34, Repo: "acme/api", Kind: factory.KindIssue, Title: "shipped by hand", State: factory.StateShipped, DoneAt: now.Add(-26 * time.Hour), DoneHow: "done by hand"}) + + text, _ := floorCall(t, door, `{"ask":"shipped"}`) + if head, _, _ := strings.Cut(text, "\n"); head != "shipped today · 1 job · 1 pull request merged" { + t.Errorf("today's head = %q", head) + } + text, _ = floorCall(t, door, `{"ask":"shipped","days":7}`) + lines := strings.Split(text, "\n") + if lines[0] != "shipped in the last 7 days · 3 jobs · 2 pull requests merged" { + t.Errorf("the week's head = %q", lines[0]) + } + // Newest first, each with the day it shipped, and nothing older than asked. + if len(lines) != 4 || !strings.HasPrefix(lines[1], "#31 ") || !strings.HasPrefix(lines[2], "#34 ") || !strings.HasPrefix(lines[3], "#32 ") { + t.Errorf("the week's rows are not the three shipped, newest first:\n%s", text) + } + if !strings.Contains(lines[3], "shipped 2026-10-06") || !strings.Contains(lines[3], "pull/302") || strings.Contains(text, "#33 ") { + t.Errorf("the week's rows:\n%s", text) + } + if text, _ := floorCall(t, door, `{"ask":"shipped","days":30}`); !strings.Contains(text, "#33 ") { + t.Errorf("thirty days does not reach a job shipped twenty days ago:\n%s", text) + } + for _, bad := range []string{`{"ask":"shipped","days":91}`, `{"ask":"shipped","days":-1}`, `{"ask":"running","days":7}`} { + if text, isErr := floorCall(t, door, bad); !isErr || !strings.Contains(text, "days goes with ask shipped") { + t.Errorf("%s was not refused: %q", bad, text) + } + } +} + +// stuckFloorDoor is a floor whose read never comes back and never looks at +// its context: the wait the manager's turn was found parked on. +type stuckFloorDoor struct { + fakeFloorDoor + entered chan struct{} + forever chan struct{} +} + +func (d *stuckFloorDoor) Snapshot(context.Context) (factory.Snapshot, error) { + select { + case d.entered <- struct{}{}: + default: + } + <-d.forever + return factory.Snapshot{}, nil +} + +// A FLOOR THAT NEVER ANSWERS DOES NOT HOLD THE TURN. +// +// The Factory manager's every question ended in a factory_floor call that did +// not return, for sixteen minutes on the measured run, and the person's stop +// changed nothing because the door never read its context. The call is waited +// for on the turn's context now: a stop returns it at once. +func TestFactoryFloorLetsGoOfAReadThatNeverComesBackWhenTheTurnIsStopped(t *testing.T) { + door := &stuckFloorDoor{entered: make(chan struct{}, 1), forever: make(chan struct{})} + t.Cleanup(func() { close(door.forever) }) + agent := &Agent{config: Config{Workspace: t.TempDir(), Floor: door}} + ctx, stop := context.WithCancel(context.Background()) + type result struct { + text string + isErr bool + } + done := make(chan result, 1) + go func() { + text, isErr, _ := agent.factoryFloorTool().Execute(ctx, json.RawMessage(`{"ask":"all"}`)) + done <- result{text, isErr} + }() + select { + case <-door.entered: + case <-time.After(10 * time.Second): + t.Fatal("the read never began") + } + stop() + select { + case got := <-done: + if !got.isErr || !strings.Contains(got.text, "the factory could not be read") { + t.Fatalf("a stopped read answered %q (error %v)", got.text, got.isErr) + } + case <-time.After(10 * time.Second): + t.Fatal("the tool went on waiting for a floor that never answers after the turn was stopped") + } +} + +// AND WITH NOBODY STOPPING IT, THE WAIT ENDS BY ITSELF, in words the model can +// pass on instead of a turn that says nothing. +func TestFloorWithinAnswersLateWhenTheDoorOutlivesItsWait(t *testing.T) { + forever := make(chan struct{}) + t.Cleanup(func() { close(forever) }) + _, err := floorWithin(context.Background(), 30*time.Millisecond, func(context.Context) (int, error) { + <-forever + return 0, nil + }) + if !errors.Is(err, errFloorLate) { + t.Fatalf("a door that outlived its wait answered %v", err) + } + if got, err := floorWithin(context.Background(), time.Minute, func(context.Context) (int, error) { return 7, nil }); got != 7 || err != nil { + t.Fatalf("a door that answered in time gave %d, %v", got, err) + } + if !strings.Contains(floorLateWords(floorReadWait), "within 20 seconds") { + t.Fatalf("the late sentence is %q", floorLateWords(floorReadWait)) + } +} + +// AN ACT THAT GOT NO ANSWER IS NOT SAID TO HAVE CHANGED NOTHING. The write was +// asked for; a "nothing selected" over a selection that then landed would have +// the manager select it twice or tell the person it had not. +func TestFloorMarkRefusedSaysALateSelectionIsNotKnown(t *testing.T) { + late := floorMarkRefused("selected", errFloorLate) + if strings.Contains(late, "nothing selected") || !strings.Contains(late, "is not known") || !strings.Contains(late, "within 60 seconds") { + t.Errorf("a late selection reads %q", late) + } + if got := floorMarkRefused("unselected", errors.New("the store is read-only")); got != "nothing unselected: the store is read-only" { + t.Errorf("a refused selection reads %q", got) + } +} From 0a6876d6caa32a4d2357fd4b9134527bd0c0e5d1 Mon Sep 17 00:00:00 2001 From: ZeroPoint95 <329227198+ZeroPoint95@users.noreply.github.com> Date: Thu, 12 Nov 2026 00:22:10 -0500 Subject: [PATCH 2/9] Factory manager: name the app's state block in the brief The desktop app starts each message to the manager with the afi-now block Afi is sent. The brief and factory_floor's description say what it is, that "Waiting on you" there is the person, and to read the factory for what it does not say. Co-Authored-By: Claude --- cmd/codeaf/factory_foreman.go | 6 ++++-- cmd/codeaf/factory_foreman_fresh_test.go | 6 +++++- internal/manual/chat/factory.md | 2 +- internal/session/tools_floor.go | 2 +- 4 files changed, 11 insertions(+), 5 deletions(-) diff --git a/cmd/codeaf/factory_foreman.go b/cmd/codeaf/factory_foreman.go index 53a02fad77..c05b944b0a 100644 --- a/cmd/codeaf/factory_foreman.go +++ b/cmd/codeaf/factory_foreman.go @@ -50,8 +50,10 @@ const foremanHandle = "foreman" const foremanOldTitle = "foreman" // foremanOpening is the brief's fixed part, said once so the brief, the test -// and the manual quote the same words. -const foremanOpening = "You are the foreman of this factory, its factory-wide manager. Every job, its state, its step and its pull request come through factory_floor: read it before you answer what is running, what waits, what is queued, what shipped or how many jobs a repository has, and never answer those from memory. When the person's message comes with a block of the app's present state, it was true when they sent it: answer from it what it says, and read factory_floor for what it does not. Anything else the person asks, answer as any assistant would, with your other tools. Start a job or set its priority with factory_floor only when the person asks for it here, naming the job by its ref. Propose what to take on by selecting jobs. Never stop, ship, merge or post. Name jobs by their ref." +// and the manual quote the same words. It is kept free of the characters a +// journal line escapes (angle brackets, double quotes), so the conversation's +// file holds it as written and can be searched for it. +const foremanOpening = "You are the foreman of this factory, its factory-wide manager. Every job, its state, its step and its pull request come through factory_floor: read it before you answer what is running, what waits, what is queued, what shipped or how many jobs a repository has, and never answer those from memory. A message from the person may start with a block tagged afi-now that the app writes: the time, the factory's counts, what waits on the person ('Waiting on you' is the person, never you), what is running, what was made today and what was noticed lately. It is the app's, not the person's words, and it was true when they sent the message: answer from it what it says, read factory_floor for what it does not, and never quote it back. Anything else the person asks, answer as any assistant would, with your other tools. Start a job or set its priority with factory_floor only when the person asks for it here, naming the job by its ref. Propose what to take on by selecting jobs. Never stop, ship, merge or post. Name jobs by their ref." // floorDoor is `factory_floor`'s door over the store, or nil. // diff --git a/cmd/codeaf/factory_foreman_fresh_test.go b/cmd/codeaf/factory_foreman_fresh_test.go index 90bfc49f09..a1d1dcfbd6 100644 --- a/cmd/codeaf/factory_foreman_fresh_test.go +++ b/cmd/codeaf/factory_foreman_fresh_test.go @@ -144,7 +144,7 @@ func TestForemanFreshStartsTheManagerOver(t *testing.T) { // "only from" factory_floor, so it could not use what the app tells it with // each message, nor answer anything a plain assistant can. func TestTheForemansBriefLetsItAnswerBeyondTheFloor(t *testing.T) { - for _, want := range []string{"factory_floor", "the app's present state", "your other tools", "only when the person asks", "Never stop, ship, merge or post"} { + for _, want := range []string{"factory_floor", "tagged afi-now", "is the person, never you", "your other tools", "only when the person asks", "Never stop, ship, merge or post"} { if !strings.Contains(foremanOpening, want) { t.Errorf("the brief lacks %q: %s", want, foremanOpening) } @@ -152,6 +152,10 @@ func TestTheForemansBriefLetsItAnswerBeyondTheFloor(t *testing.T) { if strings.Contains(foremanOpening, "only from it") { t.Errorf("the brief still fences every answer into the floor: %s", foremanOpening) } + // The journal escapes these, and the brief is found in it by its words. + if strings.ContainsAny(foremanOpening, `<>"&`) { + t.Errorf("the brief carries a character the journal escapes: %s", foremanOpening) + } } func TestTheFactoryManagersDoorReopensItAndFreshPutsTheOldOneAway(t *testing.T) { diff --git a/internal/manual/chat/factory.md b/internal/manual/chat/factory.md index 68a99bfe9e..603be727a8 100644 --- a/internal/manual/chat/factory.md +++ b/internal/manual/chat/factory.md @@ -294,7 +294,7 @@ Every `factory_floor` table opens with the whole factory's counts by state, such - `repos`: every repository with its jobs counted by state, the one with work in hand first: `acme/api · 6 open: 1 running, 1 waiting on the person, 4 new · 3 done`. The first line says how many repositories have jobs, how many have open jobs, and how many have a job running, queued, waiting or following a pull request. - `shipped` with `days`: jobs shipped over that many days, today included (1 is today and the default, at most 90), newest first, each with the day it shipped. The first line counts the pull requests merged among them. -The Factory manager is not limited to `factory_floor`. When a message arrives with a block of the app's present state, it answers from that for what the block says and reads the factory for the rest, and it answers anything else as any conversation does, with its other tools. +The Factory manager is not limited to `factory_floor`. The desktop app starts each message to it with an `` block of the app's present state (the time, the factory's counts, what waits on you, what is running, what was made today and what was noticed lately), the block Afi is sent. It answers from that for what the block says and reads the factory for the rest, and it answers anything else as any conversation does, with its other tools. ## the Factory manager did not answer, takes forever, stop does nothing, clear the Factory manager's conversation, archived Factory manager diff --git a/internal/session/tools_floor.go b/internal/session/tools_floor.go index 33af23b076..3cd0bef2b1 100644 --- a/internal/session/tools_floor.go +++ b/internal/session/tools_floor.go @@ -133,7 +133,7 @@ const factoryFloorDescription = "Read the whole factory, start or prioritise a j "Every table opens with the factory's counts by state, so a how-many question is answered by one read. " + "start takes one job by the number in its ref and starts it as the job page's Start does. priority is p0 (urgent), p1 (high), p2 (medium) or p3 (low), set on item. " + "item, mark and unmark take items by the number in their ref (#4 is 4). Only a new item can be selected; selections stay until the person runs or unselects them. The fields are named mark and unmark; the person calls them select and unselect. " + - "A message that arrives with a block of the app's present state is true as of when it was sent; read here for what it does not say. " + + "A message that starts with an block carries the app's present state as of when it was sent (\"Waiting on you\" there is the person); read here for what it does not say. " + "Name items by their ref in everything you say." func factoryFloorSchemaJSON() string { From fc2caf074494077dc1a60a6d94e9dbe26e17e1c8 Mon Sep 17 00:00:00 2001 From: ZeroPoint95 <329227198+ZeroPoint95@users.noreply.github.com> Date: Thu, 12 Nov 2026 01:07:34 -0500 Subject: [PATCH 3/9] Factory manager: review follow-ups in the engine An independent review found the first change fixed the manager only when it was opened by its own button. - The manager's conversation carries a mark beside its journal, so it is the factory's after a relaunch too, and an app that had read it as an ordinary chat is told when the mark is given. - The factory puts its manager right as it starts: one in a refused folder is made again and the old one stopped and archived, so a tab the app brings back never writes to it as the manager. - Opens are answered one at a time, the new manager is answered before the old one is put away, and the brief's policy read is bounded. - A replaced manager is the person's own chat again, titled "Factory manager (earlier)". - factory_floor says the factory's counts on what shipped and on an ask nothing answers. The docs say what the code does. - The real agents are asserted to satisfy the bounded-stop interfaces. Co-Authored-By: Claude --- cmd/codeaf/appfactory_foreman.go | 145 +++++++-- cmd/codeaf/appfactory_setup.go | 6 + cmd/codeaf/factory_chat_owner.go | 40 +++ cmd/codeaf/factory_foreman.go | 86 +++++- cmd/codeaf/factory_foreman_fresh_test.go | 281 ++++++++++++++++-- .../0-factory-manager-answers-stops-clears.md | 6 +- docs/design/app-v3/PROTOCOL.md | 44 ++- internal/manual/chat/factory.md | 6 +- internal/remote/stopbound_test.go | 5 + internal/session/tools_floor.go | 42 ++- internal/session/tools_floor_test.go | 25 +- 11 files changed, 597 insertions(+), 89 deletions(-) diff --git a/cmd/codeaf/appfactory_foreman.go b/cmd/codeaf/appfactory_foreman.go index aac54c8dfc..230a77cf8a 100644 --- a/cmd/codeaf/appfactory_foreman.go +++ b/cmd/codeaf/appfactory_foreman.go @@ -8,6 +8,7 @@ import ( "github.com/Agent-Field/codeaf/internal/appwire" "github.com/Agent-Field/codeaf/internal/factory" "github.com/Agent-Field/codeaf/internal/factory/store" + "github.com/Agent-Field/codeaf/internal/guard" ) // FactoryForemanOpen is factory.foreman_open: the foreman conversation of the @@ -32,27 +33,38 @@ func (b *appBackend) FactoryForemanFresh(ctx context.Context, p appwire.FactoryR return b.foremanOpened(ctx, p, foremanFresh) } -// foremanOpened answers the foreman a maker names, held open and the -// factory's. A foreman the maker REPLACED (a fresh start, or one moved out of -// a folder it may not work in) is stopped and put away once the new one is -// named, so nothing is left running behind a conversation no door opens. -func (b *appBackend) foremanOpened(ctx context.Context, p appwire.FactoryRef, maker func(*store.Store, string, string, func() (factory.Snapshot, error)) func(context.Context) (string, error)) (appwire.FactoryForeman, error) { +// foremanDoor is one way of naming the foreman: the open's or the fresh +// start's. +type foremanDoor func(st *store.Store, workspace, profileDir string, load func() (factory.Snapshot, error)) func(context.Context) (string, error) + +// foremanOpened answers the foreman a door names, held open and the +// factory's. +// +// ONE AT A TIME. Two opens that both found no foreman, or both found one in a +// refused folder, each made one: the second was the store's and the first was +// left held, the factory's by its flag, with no floor behind it and no door +// that would ever reach it again. The local factory's gate makes the naming +// and the hold one act, so a second caller meets the first one's foreman. +// +// THE NEW ONE IS ANSWERED BEFORE THE OLD ONE IS PUT AWAY. A foreman the door +// REPLACED (a fresh start, or one moved out of a folder it may not work in) is +// stopped and archived on a goroutine of its own once the new one is held: +// closing a conversation whose turn will not let go waits out its graces, and +// the app's call would time out behind them with the new manager already made +// and nobody told. +func (b *appBackend) foremanOpened(ctx context.Context, p appwire.FactoryRef, door foremanDoor) (appwire.FactoryForeman, error) { host, err := b.localHostOf(p.Factory) if err != nil { return appwire.FactoryForeman{}, err } - load := host.seam.Load - if load == nil { - load = factory.LocalSeam(host.floor, time.Now(), factoryPipelineOption(host.floor)).Load - } + gate := &b.local.foreman + gate.Lock() + defer gate.Unlock() before, _ := host.floor.Foreman() - transcript, err := maker(host.floor, host.workspace, host.profile, load)(ctx) + transcript, err := door(host.floor, host.workspace, host.profile, foremanLoad(host))(ctx) if err != nil { return appwire.FactoryForeman{}, appwire.Errorf(appwire.CodeConflict, "the factory's manager could not be opened: %s", err) } - if before != "" && !sameSessionFile(before, transcript) { - b.putAwayForeman(before) - } id := appAgentID(transcript) a, err := b.find(id) if err != nil { @@ -60,7 +72,8 @@ func (b *appBackend) foremanOpened(ctx context.Context, p appwire.FactoryRef, ma } // The factory's before anything is said of it, so the app never draws the // manager among the person's own chats for the moment between the two. - b.locked(func() { a.view.FactoryOwned = true }) + flipped := false + b.locked(func() { flipped, a.view.FactoryOwned = !a.view.FactoryOwned, true }) if b.tidied(id) { if _, err := b.Reopen(id); err != nil { return appwire.FactoryForeman{}, err @@ -69,21 +82,115 @@ func (b *appBackend) foremanOpened(ctx context.Context, p appwire.FactoryRef, ma if _, err := b.holdJobChat(id); err != nil { return appwire.FactoryForeman{}, err } + // A CONVERSATION THE APP ALREADY HELD OPEN IS TOLD IT IS THE FACTORY'S. The + // hold above says an agent only when it had to open it, and a tab brought + // back by a relaunch had opened this one as an ordinary chat: until the + // app heard otherwise it went on offering to archive the manager. + if flipped { + b.sayAgent(a) + } + if before != "" && !sameSessionFile(before, transcript) { + guard.Go("app/factory foreman put away", func() { b.putAwayForeman(before) }) + } return appwire.FactoryForeman{Factory: p.Factory, Agent: id}, nil } +// foremanLoad is the floor's read a foreman's making asks for its brief's +// policy lines: the host's own seam when it has one. +func foremanLoad(host *appFactoryHost) func() (factory.Snapshot, error) { + if load := host.seam.Load; load != nil { + return load + } + return factory.LocalSeam(host.floor, time.Now(), factoryPipelineOption(host.floor)).Load +} + +// settleForeman puts the factory's foreman right AS THE FACTORY STARTS, before +// anybody opens it. +// +// THE OPEN IS NOT THE ONLY ROAD TO A FOREMAN. The app brings back the tabs it +// had, and resumes a reply the last quit cut: both reach the conversation by +// its id and never through factory.foreman_open. So a foreman still working in +// a folder the factory refuses was written to in that folder again, and one +// made before the foreman's mark existed came back as one of the person's own +// chats, with Archive offered on it. Here, once per start: +// +// - a foreman in a refused folder is made again in its own, and the old one +// stopped and archived, so the tab that comes back holds an archived chat +// and the manager's button opens the new one; +// - any other foreman takes the mark, and the app is told it is the +// factory's if it had already read it as an ordinary chat. +// +// A store that names no foreman is left alone: one is never made by default. +func (b *appBackend) settleForeman(host *appFactoryHost) { + gate := &b.local.foreman + gate.Lock() + chat, err := host.floor.Foreman() + if err != nil || chat == "" { + gate.Unlock() + return + } + if _, err := os.Stat(chat); err != nil { + gate.Unlock() + return + } + if foremanFolderRefusal(host.floor, chat) == nil { + _ = markFactoryForeman(host.floor, chat) + gate.Unlock() + b.ownForeman(chat) + return + } + _, err = foremanMint(host.floor, host.workspace, host.profile, foremanLoad(host)) + gate.Unlock() + if err == nil { + b.putAwayForeman(chat) + } +} + +// ownForeman tells the app a conversation it already knows is the factory's, +// when this engine had read it as an ordinary chat. One the engine has not +// read yet needs nothing said: its mark is on disk and its first reading +// carries it ([factoryOwnedConversation]). +func (b *appBackend) ownForeman(transcript string) { + id := appAgentID(transcript) + var a *appAgent + flipped := false + b.locked(func() { + if a = b.agents[id]; a != nil && !a.view.FactoryOwned { + a.view.FactoryOwned, flipped = true, true + } + }) + if flipped { + b.sayAgent(a) + } +} + // putAwayForeman stops a replaced foreman's turn and archives its -// conversation: its history stays on disk and in Archived, and nothing of it -// runs on. It is best effort. The new foreman is already the factory's, and a -// conversation that is gone or cannot be closed is no reason to refuse it. +// conversation. It is no longer the factory's: its mark is gone and the app is +// told, so it reads as one of the person's own chats, in Archived with its +// history. It is best effort. The new foreman is already the factory's, and a +// conversation that is gone or will not close is no reason to refuse that one; +// what could not be archived is left open and is the person's to close. func (b *appBackend) putAwayForeman(transcript string) { if _, err := os.Stat(transcript); err != nil { return } id := appAgentID(transcript) - if _, err := b.find(id); err != nil { + a, err := b.find(id) + if err != nil { return } + b.locked(func() { + a.view.FactoryOwned = false + // The title its retiring gave it on disk ([foremanRetire]), for an + // agent this engine had already read under the manager's name. + if a.view.Title == foremanTitle || a.view.Title == foremanOldTitle { + a.view.Title = foremanEarlierTitle + } + }) _ = b.Stop(id) - _ = b.Archive([]string{id}) + if err := b.Archive([]string{id}); err != nil { + // Archive says the agent when it succeeds; a refusal still has to + // say it is the person's now. + b.sayAgent(a) + } } diff --git a/cmd/codeaf/appfactory_setup.go b/cmd/codeaf/appfactory_setup.go index c5cb95a9db..9e5000a013 100644 --- a/cmd/codeaf/appfactory_setup.go +++ b/cmd/codeaf/appfactory_setup.go @@ -150,6 +150,9 @@ type appLocalFactory struct { runner bool runnerDone chan struct{} closingDone chan struct{} + // foreman is held while the factory's foreman is named and held open, so + // two asks never each make one (appfactory_foreman.go). + foreman sync.Mutex // githubHTTP, rules, seamHook and parent are test seams set before the // first start. parent, when set, is the conversation config the runner // opens its stages from, as a terminal launch hands one to its worker. @@ -435,6 +438,9 @@ func (b *appBackend) startLocalLocked(ctx context.Context, resume bool) error { l.host, l.server = nil, nil return err } + // The foreman is put right as the factory starts, off this call: a tab the + // app brings back reaches it without ever opening it by its door. + guard.Go("app/factory foreman settle", func() { b.settleForeman(host) }) return nil } diff --git a/cmd/codeaf/factory_chat_owner.go b/cmd/codeaf/factory_chat_owner.go index abc27cf915..9574b93423 100644 --- a/cmd/codeaf/factory_chat_owner.go +++ b/cmd/codeaf/factory_chat_owner.go @@ -24,6 +24,10 @@ type factoryChatOwner struct { Root string `json:"root"` Job int `json:"job"` Repo string `json:"repo"` + // Foreman marks the floor's own conversation, which belongs to no job: + // Job is zero and Repo empty, so no job's reading of the marks takes it + // for one of its chats ([readFactoryOwner] answers jobs only). + Foreman bool `json:"foreman,omitempty"` } // Ownership lives beside the journal, not in a crew that can be replaced by a @@ -84,9 +88,45 @@ func factoryOwnedConversation(file string) bool { if _, ok := readFactoryOwner(file); ok { return true } + if factoryForemanMarked(file) { + return true + } return legacyFactoryChat(file, 0) } +// markFactoryForeman writes that file is the floor's own conversation, beside +// its journal where a job's chat keeps its mark. WITHOUT IT THE FOREMAN WAS +// THE FACTORY'S ONLY IN THE MEMORY OF THE ENGINE THAT LAST OPENED IT: after a +// relaunch the app read it back as one of the person's own chats, listed it +// among them, and offered to archive it. It is written only when it is not +// there, so an open costs one read. +func markFactoryForeman(st *store.Store, file string) error { + if st == nil || strings.TrimSpace(file) == "" || factoryForemanMarked(file) { + return nil + } + data, err := json.Marshal(factoryChatOwner{Root: chatKey(st.Root()), Foreman: true}) + if err != nil { + return err + } + return os.WriteFile(filepath.Join(filepath.Dir(file), factoryOwnerFile), data, 0o600) +} + +// unmarkFactoryForeman takes the mark off a foreman that was replaced: from +// then on it is an ordinary conversation of the person's, found in Archived +// with its history. Only a foreman's own mark is removed, never a job's. +func unmarkFactoryForeman(file string) { + if factoryForemanMarked(file) { + _ = os.Remove(filepath.Join(filepath.Dir(file), factoryOwnerFile)) + } +} + +// factoryForemanMarked reports whether file carries the foreman's mark. +func factoryForemanMarked(file string) bool { + var o factoryChatOwner + data, err := os.ReadFile(filepath.Join(filepath.Dir(file), factoryOwnerFile)) + return err == nil && json.Unmarshal(data, &o) == nil && o.Foreman && o.Root != "" +} + func ownedJobChats(root string, it factory.Item, ownerRoots []string) []string { var out []string markers, _ := filepath.Glob(filepath.Join(root, "*", "*", factoryOwnerFile)) diff --git a/cmd/codeaf/factory_foreman.go b/cmd/codeaf/factory_foreman.go index c05b944b0a..4fabe59a5a 100644 --- a/cmd/codeaf/factory_foreman.go +++ b/cmd/codeaf/factory_foreman.go @@ -49,6 +49,13 @@ const foremanHandle = "foreman" // was given; [foremanRetitle] moves it to [foremanTitle] on the next open. const foremanOldTitle = "foreman" +// foremanEarlierTitle is what a foreman that was REPLACED is titled from then +// on, when its title was still the manager's own. A tab the app brings back +// may still hold that conversation: under the manager's name it read as the +// manager, and a person wrote to it there while the manager's button opened +// another one. +const foremanEarlierTitle = "Factory manager (earlier)" + // foremanOpening is the brief's fixed part, said once so the brief, the test // and the manual quote the same words. It is kept free of the characters a // journal line escapes (angle brackets, double quotes), so the conversation's @@ -181,20 +188,26 @@ func withForeman(seam factory.Seam, st *store.Store, workspace, profileDir strin // in the store, so A SECOND ASK OPENS THE SAME CONVERSATION. load is the floor's // read, asked once at the making for the recipes' policy lines. // -// A FOREMAN WHOSE FOLDER IS THE CODEAF HOME IS MADE AGAIN, in a folder of its +// A FOREMAN WHOSE FOLDER THE FACTORY REFUSES IS MADE AGAIN, in a folder of its // own. A host that named no folder once gave the foreman the engine's working -// directory, which under the desk is the home itself: every save of that -// conversation then swept the whole home, and its tool calls waited behind a -// capture that never finished, so the manager answered nothing at all. No new -// foreman is made there ([factoryFolderRefusal]); this is the same refusal for -// the one a store already names. The old conversation is left on disk for its -// owner to put away ([appBackend.FactoryForemanOpen]). +// directory, which under the desk is the codeaf home itself: every save of +// that conversation then swept the whole home, and its tool calls waited behind +// a capture that never finished, so the manager answered nothing at all. No +// new foreman is made there ([factoryFolderRefusal]); this is the same refusal +// for the one a store already names, and it is that rule whole: the home, any +// folder above it, and any folder inside it that is not one of the factory's +// own areas. The old conversation is left on disk, off the `factory` team and +// no longer marked the factory's. The app stops and archives it +// ([appBackend.foremanOpened], [appBackend.settleForeman]); the terminal's +// door leaves it as it is, an ordinary conversation in the person's list. func foremanMaker(st *store.Store, workspace, profileDir string, load func() (factory.Snapshot, error)) func(context.Context) (string, error) { return func(context.Context) (string, error) { factoryReconcileOnce(profileDir, st) if chat, err := st.Foreman(); err == nil && chat != "" { if _, serr := os.Stat(chat); serr == nil && foremanFolderRefusal(st, chat) == nil { foremanRetitle(profileDir, chat) + // One made before the mark existed takes it on its next open. + _ = markFactoryForeman(st, chat) return chat, nil } } @@ -255,14 +268,14 @@ func foremanMint(st *store.Store, workspace, profileDir string, load func() (fac return "", err } transcript := place.Transcript() - var snap factory.Snapshot - if load != nil { - snap, _ = load() + if err := session.SeedConversation(transcript, where, foremanTitle, foremanBrief(foremanPolicyRead(load))); err != nil { + return "", err } - if err := session.SeedConversation(transcript, where, foremanTitle, foremanBrief(snap)); err != nil { + if err := markFactoryForeman(st, transcript); err != nil { return "", err } - if old, err := st.Foreman(); err == nil && old != "" { + old, _ := st.Foreman() + if old != "" { foremanLeaveTeam(profileDir, old) } if err := foremanJoinTeam(profileDir, transcript, where); err != nil { @@ -271,9 +284,58 @@ func foremanMint(st *store.Store, workspace, profileDir string, load func() (fac if err := st.SetForeman(transcript); err != nil { return "", err } + // Only once the store names the new one: until then the old one is still + // the foreman, and still the factory's. + if old != "" && !sameSessionFile(old, transcript) { + foremanRetire(old) + } return transcript, nil } +// foremanRetire makes a replaced foreman an ordinary conversation of the +// person's: its mark comes off, and a title that was still the manager's own +// says it is the earlier one. A title the person gave it is theirs and stays. +// It is best effort; the new foreman is the factory's either way. +func foremanRetire(transcript string) { + unmarkFactoryForeman(transcript) + dir := session.DirOf(transcript) + meta, err := session.LoadMeta(dir) + if err != nil { + return + } + if title := strings.TrimSpace(meta.Title); title == foremanTitle || title == foremanOldTitle { + meta.Title = foremanEarlierTitle + _ = session.SaveMeta(dir, meta) + } +} + +// foremanPolicyWait is how long a foreman's making waits for the floor's read +// before it is made without the recipes' policy lines. +const foremanPolicyWait = 5 * time.Second + +// foremanPolicyRead is the floor's read for the brief's policy lines, WAITED +// FOR A BOUNDED TIME. The read is the one factory_floor bounds for the same +// reason (internal/session's floorWithin): it reads disk and may wait on a +// lock, and a making that waited on it without limit was an open of the +// manager that never answered. A brief without the policy lines is still a +// foreman; a floor that could not be read in time says nothing here. +func foremanPolicyRead(load func() (factory.Snapshot, error)) factory.Snapshot { + if load == nil { + return factory.Snapshot{} + } + done := make(chan factory.Snapshot, 1) + go func() { + snap, _ := load() + done <- snap + }() + select { + case snap := <-done: + return snap + case <-time.After(foremanPolicyWait): + return factory.Snapshot{} + } +} + // foremanBrief is the foreman's opening note: [foremanOpening], then each // repository's policy lines as facts. A floor with no policy says none (the // emptiness law). diff --git a/cmd/codeaf/factory_foreman_fresh_test.go b/cmd/codeaf/factory_foreman_fresh_test.go index a1d1dcfbd6..abfc798eed 100644 --- a/cmd/codeaf/factory_foreman_fresh_test.go +++ b/cmd/codeaf/factory_foreman_fresh_test.go @@ -15,13 +15,62 @@ import ( "os" "path/filepath" "strings" + "sync" "testing" + "time" "github.com/Agent-Field/codeaf/internal/appwire" + "github.com/Agent-Field/codeaf/internal/factory/store" + "github.com/Agent-Field/codeaf/internal/remote" "github.com/Agent-Field/codeaf/internal/session" "github.com/Agent-Field/codeaf/internal/teams" ) +// THE APP'S STOP IS BOUNDED ONLY WHILE THE REAL CONVERSATION CAN BE: the app +// reaches a conversation through a remote agent, and a rename of the method +// on either side would restore the stop that did nothing, with every test on +// a fake still passing ([appStopByPerson]). +var _ appBoundedStop = (*remote.Agent)(nil) + +// foremanEventually waits for a thing the engine does off the call (putting +// the replaced foreman away, settling the foreman as the factory starts). +func foremanEventually(t *testing.T, what string, ok func() bool) { + t.Helper() + deadline := time.Now().Add(15 * time.Second) + for !ok() { + if time.Now().After(deadline) { + t.Fatalf("%s never happened", what) + } + time.Sleep(10 * time.Millisecond) + } +} + +// homeForeman makes a foreman the way the older build made it: working in dir +// (the codeaf home), named by the store, in the `factory` team, and WITHOUT +// the foreman's mark, which did not exist then. +func homeForeman(t *testing.T, floor *store.Store, profile, dir string) string { + t.Helper() + bucket, err := v3ProjectDir(dir) + if err != nil { + t.Fatal(err) + } + place, err := v3MintSession(bucket, dir, v3StampLaunchDir(v3LaunchDir(), dir), false) + if err != nil { + t.Fatal(err) + } + old := place.Transcript() + if err := session.SeedConversation(old, dir, foremanTitle, "the older brief"); err != nil { + t.Fatal(err) + } + if err := foremanJoinTeam(profile, old, dir); err != nil { + t.Fatal(err) + } + if err := floor.SetForeman(old); err != nil { + t.Fatal(err) + } + return old +} + // foremanHandleHolder is the conversation key holding `@foreman` in the // profile's teams, "" for none, and how many members hold it. func foremanHandleHolder(t *testing.T, profile string) (key string, holders int) { @@ -54,24 +103,7 @@ func TestAForemanWorkingInTheCodeafHomeIsMadeAgainInAFolderOfItsOwn(t *testing.T // The foreman as the older build made it: in the engine's own folder, // which under the desk is the codeaf home. - bucket, err := v3ProjectDir(state) - if err != nil { - t.Fatal(err) - } - place, err := v3MintSession(bucket, state, v3StampLaunchDir(v3LaunchDir(), state), false) - if err != nil { - t.Fatal(err) - } - old := place.Transcript() - if err := session.SeedConversation(old, state, foremanTitle, "the older brief"); err != nil { - t.Fatal(err) - } - if err := foremanJoinTeam(profile, old, state); err != nil { - t.Fatal(err) - } - if err := floor.SetForeman(old); err != nil { - t.Fatal(err) - } + old := homeForeman(t, floor, profile, state) maker := foremanMaker(floor, factoryHostWorkspace(state), profile, nil) made, err := maker(context.Background()) @@ -98,6 +130,23 @@ func TestAForemanWorkingInTheCodeafHomeIsMadeAgainInAFolderOfItsOwn(t *testing.T if _, err := os.Stat(old); err != nil { t.Fatalf("the conversation it replaced is gone from disk: %v", err) } + // THE MARK FOLLOWS THE FOREMAN: the new one is the factory's on disk, so a + // relaunch reads it back as the factory's, and the old one is the + // person's own chat again. + if !factoryOwnedConversation(made) { + t.Error("the foreman made again is not marked the factory's on disk") + } + if factoryOwnedConversation(old) { + t.Error("the conversation it replaced is still marked the factory's") + } + // AND IT NO LONGER READS AS THE MANAGER: a tab the app brings back may + // still hold it, and under the manager's name a person wrote to it there. + if was, err := session.LoadMeta(session.DirOf(old)); err != nil || was.Title != foremanEarlierTitle { + t.Errorf("the conversation it replaced is titled %q (%v); want %q", was.Title, err, foremanEarlierTitle) + } + if now, err := session.LoadMeta(session.DirOf(made)); err != nil || now.Title != foremanTitle { + t.Errorf("the foreman made again is titled %q (%v)", now.Title, err) + } // AND THE NEXT ASK OPENS THE SAME ONE, as every ask after a making does. if again, err := maker(context.Background()); err != nil || !sameSessionFile(again, made) { t.Fatalf("the next open answered %q, %v; want %q", again, err, made) @@ -135,6 +184,9 @@ func TestForemanFreshStartsTheManagerOver(t *testing.T) { if foremanFloorDoor(st, web, profile, first) != nil { t.Error("the replaced foreman still carries factory_floor") } + if !factoryOwnedConversation(fresh) || factoryOwnedConversation(first) { + t.Errorf("the mark did not follow the foreman: fresh %v, replaced %v", factoryOwnedConversation(fresh), factoryOwnedConversation(first)) + } if again, err := foremanMaker(st, web, profile, nil)(context.Background()); err != nil || !sameSessionFile(again, fresh) { t.Fatalf("the open after a fresh start answered %q, %v; want the fresh one", again, err) } @@ -160,9 +212,7 @@ func TestTheForemansBriefLetsItAnswerBeyondTheFloor(t *testing.T) { func TestTheFactoryManagersDoorReopensItAndFreshPutsTheOldOneAway(t *testing.T) { s := newLocalSetup(t) - s.backend.open = func(ask appOpenAsk) (*appConn, error) { - return &appConn{Conv: &fakeConversation{}, Workspace: ask.Workspace, Transcript: ask.Transcript, Release: func() {}}, nil - } + convs := foremanFakeOpens(s) st := s.start() ref := map[string]any{"factory": st.Factory} @@ -197,25 +247,196 @@ func TestTheFactoryManagersDoorReopensItAndFreshPutsTheOldOneAway(t *testing.T) if !s.backend.isHeld(fresh.Agent) || s.backend.tidied(fresh.Agent) { t.Fatal("the fresh manager is not held open") } - if !s.backend.tidied(first.Agent) || s.backend.isHeld(first.Agent) { - t.Fatal("the manager it replaced was left open") - } + // The old one is put away off the call, so the new one is answered first. + foremanEventually(t, "the replaced manager being stopped and archived", func() bool { + return s.backend.tidied(first.Agent) && !s.backend.isHeld(first.Agent) && convs.stops(first.Agent) > 0 + }) var after appwire.FactoryForeman s.must(appwire.FeatureFactoryForemanOpen, ref, &after) if after.Agent != fresh.Agent { t.Fatalf("the door after a fresh start answered %q; want %q", after.Agent, fresh.Agent) } - // AND EACH IS THE FACTORY'S, never one of the person's own chats. - for _, id := range []string{first.Agent, fresh.Agent} { + // THE NEW ONE IS THE FACTORY'S AND THE OLD ONE IS THE PERSON'S AGAIN, in + // Archived with its history, never hidden where no list shows it. + for id, want := range map[string]bool{fresh.Agent: true, first.Agent: false} { a, err := s.backend.find(id) if err != nil { t.Fatal(err) } - owned := false - s.backend.locked(func() { owned = a.view.FactoryOwned }) - if !owned { - t.Errorf("%s is not marked the factory's", id) + owned, title := false, "" + s.backend.locked(func() { owned, title = a.view.FactoryOwned, a.view.Title }) + if owned != want { + t.Errorf("%s: the factory's = %v, want %v", id, owned, want) + } + if wantTitle := map[bool]string{true: foremanTitle, false: foremanEarlierTitle}[want]; title != wantTitle { + t.Errorf("%s is titled %q; want %q", id, title, wantTitle) } } s.mustFail(appwire.FeatureFactoryForemanFresh, map[string]any{"factory": "factory_other"}, appwire.CodeNotFound) } + +// foremanConvs is the fake conversations a rig opened, by agent id, so a test +// can read whether one was stopped. +type foremanConvs struct { + mu sync.Mutex + by map[string]*fakeConversation +} + +func (c *foremanConvs) stops(agent string) int { + c.mu.Lock() + conv := c.by[agent] + c.mu.Unlock() + if conv == nil { + return 0 + } + conv.mu.Lock() + defer conv.mu.Unlock() + return conv.interrupted +} + +// foremanFakeOpens has the rig open every conversation as a fake that holds +// its turn open until it is stopped, so a manager can be put away mid-turn. +func foremanFakeOpens(s *localSetup) *foremanConvs { + convs := &foremanConvs{by: map[string]*fakeConversation{}} + s.backend.open = func(ask appOpenAsk) (*appConn, error) { + conv := &fakeConversation{} + convs.mu.Lock() + convs.by[appAgentID(ask.Transcript)] = conv + convs.mu.Unlock() + return &appConn{Conv: conv, Workspace: ask.Workspace, Transcript: ask.Transcript, Release: func() {}}, nil + } + return convs +} + +// TWO ASKS NEVER EACH MAKE A MANAGER. A double press of the manager's button +// once made two: the second was the store's, and the first was left held and +// the factory's with no floor behind it and no door that reached it again. +func TestManyOpensAtOnceMakeOneFactoryManager(t *testing.T) { + s := newLocalSetup(t) + foremanFakeOpens(s) + st := s.start() + const asks = 8 + answers := make([]string, asks) + errs := make([]error, asks) + var wg sync.WaitGroup + for i := range asks { + wg.Add(1) + go func() { + defer wg.Done() + out, err := s.backend.FactoryForemanOpen(context.Background(), appwire.FactoryRef{Factory: st.Factory}) + answers[i], errs[i] = out.Agent, err + }() + } + wg.Wait() + for i := range asks { + if errs[i] != nil || answers[i] == "" || answers[i] != answers[0] { + t.Fatalf("ask %d answered %q, %v; the first answered %q", i, answers[i], errs[i], answers[0]) + } + } + if _, holders := foremanHandleHolder(t, s.backend.local.host.profile); holders != 1 { + t.Fatalf("%d conversations hold @foreman, want 1", holders) + } + named, _ := s.backend.local.host.floor.Foreman() + if appAgentID(named) != answers[0] { + t.Fatalf("the store names %q, the asks were answered %q", appAgentID(named), answers[0]) + } +} + +// THE FACTORY PUTS ITS MANAGER RIGHT AS IT STARTS, not only when the manager's +// button is pressed. The app brings its tabs back by conversation id and +// resumes a reply the last quit cut, so the manager working in the codeaf home +// was written to there again without factory.foreman_open ever being called. +func TestAFactoryStartingMovesItsManagerOutOfTheCodeafHome(t *testing.T) { + s := newLocalSetup(t) + foremanFakeOpens(s) + floor, err := store.Open(v3FactoryRoot()) + if err != nil { + t.Fatal(err) + } + old := homeForeman(t, floor, factoryProfile(s.backend.profileDir), s.home) + oldID := appAgentID(old) + + st := s.start() + foremanEventually(t, "the manager being made again as the factory started", func() bool { + named, _ := s.backend.local.host.floor.Foreman() + return named != "" && !sameSessionFile(named, old) && s.backend.tidied(oldID) + }) + named, _ := s.backend.local.host.floor.Foreman() + meta, err := session.LoadMetaIn(session.DirOf(named), "") + if err != nil { + t.Fatal(err) + } + if want := filepath.Join(s.backend.local.host.floor.Root(), "chats", "foreman"); meta.Workspace != want { + t.Fatalf("the manager works in %q; want %q", meta.Workspace, want) + } + if !factoryOwnedConversation(named) || factoryOwnedConversation(old) { + t.Fatalf("the mark did not follow the manager: new %v, old %v", factoryOwnedConversation(named), factoryOwnedConversation(old)) + } + // AND ITS BUTTON OPENS THE NEW ONE, with nothing more made. + var opened appwire.FactoryForeman + s.must(appwire.FeatureFactoryForemanOpen, map[string]any{"factory": st.Factory}, &opened) + if opened.Agent != appAgentID(named) { + t.Fatalf("the door answered %q; want the manager the start made, %q", opened.Agent, appAgentID(named)) + } +} + +// A MANAGER MADE BEFORE ITS MARK EXISTED IS TOLD TO THE APP AS THE FACTORY'S. +// After a relaunch the app read such a manager back as one of the person's +// own chats and offered to archive it; opening it by its button set the flag +// in the engine's memory and told the app nothing, because the conversation +// was already open. +func TestAnAlreadyOpenManagerIsSaidToBeTheFactorysWhenItsDoorOpensIt(t *testing.T) { + s := newLocalSetup(t) + foremanFakeOpens(s) + heard := &foremanHeard{next: s.backend.emit} + s.backend.emit = heard + st := s.start() + ref := map[string]any{"factory": st.Factory} + var first appwire.FactoryForeman + s.must(appwire.FeatureFactoryForemanOpen, ref, &first) + a, err := s.backend.find(first.Agent) + if err != nil { + t.Fatal(err) + } + // As a relaunch before the mark left it: open, and an ordinary chat. + s.backend.locked(func() { a.view.FactoryOwned = false }) + said := func() int { return heard.owned(first.Agent) } + before := said() + var again appwire.FactoryForeman + s.must(appwire.FeatureFactoryForemanOpen, ref, &again) + if again.Agent != first.Agent { + t.Fatalf("the door answered %q; want %q", again.Agent, first.Agent) + } + foremanEventually(t, "the app being told the open manager is the factory's", func() bool { return said() > before }) +} + +// foremanHeard keeps what the engine told the app of its agents, and passes +// every event on. +type foremanHeard struct { + next appwire.Emitter + + mu sync.Mutex + changed []appwire.Agent +} + +func (h *foremanHeard) Emit(event string, data any) { + if changed, ok := data.(appwire.AgentChanged); ok { + h.mu.Lock() + h.changed = append(h.changed, changed.Agent) + h.mu.Unlock() + } + h.next.Emit(event, data) +} + +// owned is how many times the app was told agent is the factory's. +func (h *foremanHeard) owned(agent string) int { + h.mu.Lock() + defer h.mu.Unlock() + n := 0 + for _, a := range h.changed { + if a.ID == agent && a.FactoryOwned { + n++ + } + } + return n +} diff --git a/docs/changes/unreleased/0-factory-manager-answers-stops-clears.md b/docs/changes/unreleased/0-factory-manager-answers-stops-clears.md index 55b9237324..6785fb7879 100644 --- a/docs/changes/unreleased/0-factory-manager-answers-stops-clears.md +++ b/docs/changes/unreleased/0-factory-manager-answers-stops-clears.md @@ -5,10 +5,12 @@ pr: 0 surface: [factory, engine, app] invalidates: - "Was true: agents.stop only interrupted the turn, so a turn parked on a wait that never read its context stayed working until the app was quit. Is true now: the app's Stop is bounded. The stop carries its bound (Interrupt's `within`, in milliseconds) and the engine abandons a turn that has not let go ten seconds later; the turn ends stopped by the person and the next send opens an ordinary turn. A lent factory stage is interrupted but never abandoned from the app." - - "Was true: a Factory manager made by a build that gave it the engine's own folder kept working in the codeaf home, where every save swept the whole home and its factory_floor calls never returned. Is true now: factory.foreman_open makes such a manager again in `/chats/foreman`, and stops and archives the conversation it replaces." + - "Was true: a Factory manager made by a build that gave it the engine's own folder kept working in the codeaf home, where every save swept the whole home and its factory_floor calls never returned. Is true now: a manager in a folder the factory refuses for its chats is made again in `/chats/foreman` as the app's factory starts, and on factory.foreman_open if it had not been; the conversation it replaces is stopped and archived on a best effort and is the person's own chat from then on." + - "Was true: the Factory manager was the factory's only in the memory of the engine that last opened it, so after a relaunch the app read it as one of the person's own chats, listed it among them and offered Archive. Is true now: a mark beside its journal makes every reading answer it factory_owned, and an app that had read it as an ordinary chat is sent agent.changed when the mark is given." + - "Was true: two factory.foreman_open calls at once could each make a manager. Is true now: the manager is named and held open one call at a time, and the manager a call replaces is put away after the new one is answered." - "Was true: factory_floor waited on the factory with no bound and ignored the turn's context. Is true now: a read waits at most 20 seconds and a start, priority or selection 60 seconds, a stop returns it at once, and a start or priority that got no answer says it may still take effect." - "Was true: the Factory manager could be archived or closed, after which factory.foreman_open answered a conversation nobody could write to. Is true now: the open is the person's own and takes the manager out of Closed, and factory.foreman_fresh {factory} (hello feature factory.foreman_fresh) starts it over in a new conversation, stopping and archiving the old one." - - "Was true: factory_floor could not say how many repositories had work or what shipped before today, its head counted only new and waiting jobs, and the manager's brief told it to answer only from factory_floor. Is true now: `repos` counts jobs by repository and state, `shipped` takes `days` (up to 90) and counts merged pull requests, every table opens with the whole factory's counts by state, and the brief lets the manager answer from the app's present state sent with a message and from its other tools." + - "Was true: factory_floor could not say how many repositories had work or what shipped before today, its head counted only new and waiting jobs, and the manager's brief told it to answer only from factory_floor. Is true now: `repos` counts jobs by repository and state, `shipped` takes `days` (up to 90) and counts merged pull requests, every answer but `item` and `repos` says the whole factory's counts by state on its first line, and the brief lets the manager answer from the app's present state sent with a message and from its other tools." --- The desktop app's Factory manager answered no question, could not be stopped, diff --git a/docs/design/app-v3/PROTOCOL.md b/docs/design/app-v3/PROTOCOL.md index 91d204b43e..cb17846370 100644 --- a/docs/design/app-v3/PROTOCOL.md +++ b/docs/design/app-v3/PROTOCOL.md @@ -2268,15 +2268,32 @@ each through the same factory verb the job page's own Start and priority send, as the person. It never stops, ships, merges or posts. `repos` lists every repository with its jobs counted by state, `shipped` takes `days` (1 is today and the default, at most 90) and counts the pull requests merged, and every -table opens with the whole factory's counts by state. One read or act waits a -bounded time on the factory (20 s to read, 60 s to act) and then answers that -the factory did not, so a floor that never answers cannot hold the turn open. +answer but `item` and `repos` says the whole factory's counts by state on its +first line. One read or act waits a bounded time on the factory (20 s to read, +60 s to act) and then answers that the factory did not, so a floor that never +answers cannot hold the turn open. The open is the person's own: a Factory manager they closed or archived is -taken out of Closed and answered open. A manager whose conversation works in -the codeaf home (one made by a build that gave it the engine's own folder) is -made again in `/chats/foreman` on its next open, and the conversation it -replaces is stopped and archived. +taken out of Closed and answered open. Opens are answered one at a time, so two +asks at once are answered the same manager. + +The Factory manager's conversation is the factory's on disk (a mark beside its +journal), so `agents.open` and every other reading answer it `factory_owned` +after a relaunch too, and it is left out of `agents.list`. A manager made +before the mark existed takes it when the factory starts or when it is next +opened, and an app that had already read it as an ordinary chat is sent +`agent.changed` with `factory_owned: true`. + +A manager whose conversation works in a folder the factory refuses for its +chats (the codeaf home, a folder above it, or a folder inside it outside the +factory's own areas; one made by a build that gave it the engine's own folder) +is made again in `/chats/foreman`. This happens as the factory on this +machine starts, and on `factory.foreman_open` if it had not happened yet, so a +tab the app brings back never writes to the old one as the manager. The +conversation it replaces is stopped and archived after the new one is +answered, on a best effort, and is no longer the factory's: it reads +`factory_owned: false`, as one of the person's own chats in Archived, titled +"Factory manager (earlier)" when its title was still the manager's own. ### Starting the Factory manager over (`factory.foreman_fresh`) @@ -2284,11 +2301,14 @@ The hello feature `factory.foreman_fresh` announces the app's Clear for the Factory manager. `factory.foreman_fresh {factory}` answers `{factory, agent}` as the open does, but `agent` is a NEW conversation with only the manager's brief in it; it takes the manager's place (every later `factory.foreman_open` -answers it) and its `@foreman` handle. The conversation it replaces is stopped -and archived: its history stays on disk and nothing of it runs on. A manager -has this where another conversation has Archive, because it is the factory's -one conversation with one door: archived, that door opened a chat nobody could -write to. Only the factory this engine serves is accepted. +answers it) and its `@foreman` handle. It opens on the default model and +settings, not the old conversation's. The conversation it replaces is stopped +and archived after the new one is answered, on a best effort: its history +stays on disk, and it is one of the person's own chats from then on +(`factory_owned: false`). A manager has this where another conversation has +Archive, because it is the factory's one conversation with one door: archived, +that door opened a chat nobody could write to. Only the factory this engine +serves is accepted. ### factory.job_publish_anyway Hello feature `factory.publish_anyway` advertises this person action. Params: diff --git a/internal/manual/chat/factory.md b/internal/manual/chat/factory.md index 603be727a8..98daf8a13a 100644 --- a/internal/manual/chat/factory.md +++ b/internal/manual/chat/factory.md @@ -289,7 +289,7 @@ When you ask ("start the oldest p1", "make #12 urgent"), it starts that one job ## the Factory manager counts — how many jobs are running, how many repos are active, how many pull requests merged this week -Every `factory_floor` table opens with the whole factory's counts by state, such as `in the factory: 2 running, 1 waiting on the person, 40 new`, so a how-many question is answered by one read, whichever table was asked for. The counts are of the factory and never of the rows listed, which stop at 50. +Every `factory_floor` answer but `item` and `repos` says the whole factory's counts by state on its first line, such as `in the factory: 2 running, 1 waiting on the person, 40 new`, so a how-many question is answered by one read, whichever was asked for. An ask nothing answers, and a day nothing shipped, say the counts too. The counts are of the factory and never of the rows listed, which stop at 50. - `repos`: every repository with its jobs counted by state, the one with work in hand first: `acme/api · 6 open: 1 running, 1 waiting on the person, 4 new · 3 done`. The first line says how many repositories have jobs, how many have open jobs, and how many have a job running, queued, waiting or following a pull request. - `shipped` with `days`: jobs shipped over that many days, today included (1 is today and the default, at most 90), newest first, each with the day it shipped. The first line counts the pull requests merged among them. @@ -302,9 +302,9 @@ One `factory_floor` read waits at most 20 seconds on the factory, and a start or The app's Stop is bounded. It interrupts the turn, and a turn that has still not let go ten seconds later is let go of by the engine: it ends "Stopped by you" and the next message opens an ordinary turn. -A Factory manager whose conversation works in the codeaf home is made again in a folder of its own (`chats/foreman` under the factory) the next time it is opened, and the conversation it replaces is stopped and archived. One made by an older build worked in the home itself, where every save swept the whole home and its tool calls never finished. +A Factory manager whose conversation works in a folder the factory refuses for its chats (the codeaf home, a folder above it, or elsewhere inside it) is made again in a folder of its own, `chats/foreman` under the factory. The desktop app does this as its factory starts, and when the manager is next opened if it had not yet; the conversation it replaces is then stopped and archived, and is one of your own chats in Archived from then on, titled "Factory manager (earlier)". One made by an older build worked in the home itself, where every save swept the whole home and its tool calls never finished. In a terminal the replaced conversation is left as it is, in your list. -The Factory manager is cleared, not archived. `factory.foreman_fresh` starts it over: a new conversation with only its brief takes its place and its `@foreman` address, and the old one is stopped and archived with its history kept. Opening the Factory manager is your own open, so one you closed or archived comes back open. +The Factory manager is cleared, not archived. `factory.foreman_fresh` starts it over: a new conversation with only its brief takes its place and its `@foreman` address, on the default model and settings, and the old one is stopped and archived with its history kept, as one of your own chats. Opening the Factory manager is your own open, so one you closed or archived comes back open. The manager's conversation stays the factory's across a relaunch: it is not listed among your chats and has no Archive. ## the default steps — what a job runs with no factory.yaml, how many rounds, security pass diff --git a/internal/remote/stopbound_test.go b/internal/remote/stopbound_test.go index 3782b683d7..efc67dce6c 100644 --- a/internal/remote/stopbound_test.go +++ b/internal/remote/stopbound_test.go @@ -16,6 +16,11 @@ import ( // ([session.Agent.InterruptWithin]). These pin both ends of that: what the // surface sends, and which door the engine answers it with. +// THE REAL AGENT IS ONE THE SERVER CAN BOUND. The server asks for the second +// stage by an optional interface, so a rename on the session's side would +// leave every hosted stop unbounded again with these tests still passing. +var _ boundedStopper = (*session.Agent)(nil) + // boundedFakeAgent is a [fakeAgent] whose stop can carry its second stage. type boundedFakeAgent struct { *fakeAgent diff --git a/internal/session/tools_floor.go b/internal/session/tools_floor.go index 3cd0bef2b1..af486c3dab 100644 --- a/internal/session/tools_floor.go +++ b/internal/session/tools_floor.go @@ -130,7 +130,7 @@ const ( const factoryFloorDescription = "Read the whole factory, start or prioritise a job when the person asks, and propose what to take on by selecting items. " + "You are the factory's foreman: you read, rank, propose and sum up. Start a job (start) or set its priority (priority with item) only when the person asks for that job in this conversation; nothing here ships, merges or posts. " + "ask: running (jobs running or queued to run, with their step), queue (jobs not started yet, most urgent then oldest first), waiting (jobs that need the person), shipped (jobs shipped today, with their pull requests and changes, to sum up; set days to read further back), repos (every repository with its count of jobs in each state, for how many and where), risky, cheapest (ten, by estimate), oldest (ten), all, or item (one item in full: its read, facts, stages, question and body; set item). " + - "Every table opens with the factory's counts by state, so a how-many question is answered by one read. " + + "Every answer but item and repos says the factory's counts by state on its first line, so a how-many question is answered by one read. " + "start takes one job by the number in its ref and starts it as the job page's Start does. priority is p0 (urgent), p1 (high), p2 (medium) or p3 (low), set on item. " + "item, mark and unmark take items by the number in their ref (#4 is 4). Only a new item can be selected; selections stay until the person runs or unselects them. The fields are named mark and unmark; the person calls them select and unselect. " + "A message that starts with an block carries the app's present state as of when it was sent (\"Waiting on you\" there is the person); read here for what it does not say. " + @@ -413,18 +413,13 @@ func floorAnswer(snap factory.Snapshot, ask string, item, days int) string { if now.IsZero() { now = time.Now() } + open := floorOpen(snap) if ask == floorAskShipped { - return floorShipped(snap, now, days) + return floorWithCounts(floorShipped(snap, now, days), open) } if ask == floorAskRepos { return floorReposAnswer(snap) } - var open []factory.Item - for _, it := range snap.Items { - if it.State != factory.StateDismissed && (it.State != factory.StateShipped || floorFollowing(it)) { - open = append(open, it) - } - } if len(open) == 0 { return floorNothing } @@ -482,7 +477,7 @@ func floorAnswer(snap factory.Snapshot, ask string, item, days int) string { head = "every open item" } if len(rows) == 0 { - return floorNoneAsked + " (" + head + ")" + return floorWithCounts(floorNoneAsked+" ("+head+")", open) } var b strings.Builder b.WriteString(floorSummary(open, len(rows), head)) @@ -501,6 +496,35 @@ func floorAnswer(snap factory.Snapshot, ask string, item, days int) string { return b.String() } +// floorOpen is every item still the factory's work: not dismissed, and not +// shipped unless it is still following its pull request. +func floorOpen(snap factory.Snapshot) []factory.Item { + var open []factory.Item + for _, it := range snap.Items { + if it.State != factory.StateDismissed && (it.State != factory.StateShipped || floorFollowing(it)) { + open = append(open, it) + } + } + return open +} + +// floorWithCounts puts the factory's counts on the first line of an answer +// that is not a table of open items (what shipped, or an ask nothing +// answers), so those say how the factory stands too: "nothing is waiting" is +// worth more beside what is running. Nothing is added when nothing is open. +func floorWithCounts(text string, open []factory.Item) string { + counts := floorCounts(open) + if counts == "" { + return text + } + head, rest, more := strings.Cut(text, "\n") + head += " · in the factory: " + counts + if more { + return head + "\n" + rest + } + return head +} + // floorSummary is the table's head: what was asked, how many rows answer it, // and THE FLOOR'S COUNTS BY STATE, each left out when it is nothing. The // counts are of the whole floor and not of the rows, so "how many are diff --git a/internal/session/tools_floor_test.go b/internal/session/tools_floor_test.go index 01363c25f4..b7fab9da74 100644 --- a/internal/session/tools_floor_test.go +++ b/internal/session/tools_floor_test.go @@ -381,6 +381,26 @@ func TestFactoryFloorHeadCountsTheWholeFactoryWhateverWasAsked(t *testing.T) { } } +// AND AN ASK NOTHING ANSWERS STILL SAYS HOW THE FACTORY STANDS: "is anything +// waiting on me" with nothing waiting is answered beside what is running, and +// a day nothing shipped says so beside the same counts. +func TestFactoryFloorAnAskWithNoRowsStillCountsTheFactory(t *testing.T) { + door := floorLab() + door.snap.Items[4].State = factory.StateRunning // nothing waits on the person now + const counts = " · in the factory: 2 running, 4 new" + if text, _ := floorCall(t, door, `{"ask":"waiting"}`); text != "no items in the factory answer that ask (waiting on the person)"+counts { + t.Errorf("waiting = %q", text) + } + if text, _ := floorCall(t, door, `{"ask":"shipped"}`); text != "nothing shipped today"+counts { + t.Errorf("shipped = %q", text) + } + // A factory with nothing open adds nothing: no counts of nothing. + empty := &fakeFloorDoor{snap: factory.Snapshot{Now: door.snap.Now}} + if text, _ := floorCall(t, empty, `{"ask":"shipped"}`); text != "nothing shipped today" { + t.Errorf("an empty factory's shipped = %q", text) + } +} + // `repos` IS WHERE THE WORK IS: every repository with its jobs counted by // state, the one with work in hand first. "How many repos are active in our // factory" was the question the manager could not answer from a table of jobs. @@ -432,13 +452,14 @@ func TestFactoryFloorShippedReadsBackByDaysAndCountsMergedPullRequests(t *testin merged(32, 33, 303, 20*24*time.Hour), factory.Item{ID: 33, Num: 34, Repo: "acme/api", Kind: factory.KindIssue, Title: "shipped by hand", State: factory.StateShipped, DoneAt: now.Add(-26 * time.Hour), DoneHow: "done by hand"}) + const counts = " · in the factory: 1 running, 1 waiting on the person, 4 new" text, _ := floorCall(t, door, `{"ask":"shipped"}`) - if head, _, _ := strings.Cut(text, "\n"); head != "shipped today · 1 job · 1 pull request merged" { + if head, _, _ := strings.Cut(text, "\n"); head != "shipped today · 1 job · 1 pull request merged"+counts { t.Errorf("today's head = %q", head) } text, _ = floorCall(t, door, `{"ask":"shipped","days":7}`) lines := strings.Split(text, "\n") - if lines[0] != "shipped in the last 7 days · 3 jobs · 2 pull requests merged" { + if lines[0] != "shipped in the last 7 days · 3 jobs · 2 pull requests merged"+counts { t.Errorf("the week's head = %q", lines[0]) } // Newest first, each with the day it shipped, and nothing older than asked. From 8e19024f5323c8fc5c2fef52830b2843ca00d134 Mon Sep 17 00:00:00 2001 From: ZeroPoint95 <329227198+ZeroPoint95@users.noreply.github.com> Date: Thu, 12 Nov 2026 01:33:27 -0500 Subject: [PATCH 4/9] Factory manager: second review pass in the engine - The start-time settle is a chore of the factory host, so it never writes the floor after a close has begun; putting the replaced conversation away stays off it. - The store names the new manager before any mark is written, and a sweep at start retires a conversation a cut-short making left marked. - A replaced manager's new name is written through its conversation, so it still reads "Factory manager (earlier)" when it is opened again. - The brief and factory_floor name both blocks the app may put ahead of a message, afi-now and app-places. Co-Authored-By: Claude --- cmd/codeaf/appfactory_foreman.go | 71 +++++++++++++++++------ cmd/codeaf/appfactory_setup.go | 2 +- cmd/codeaf/factory_foreman.go | 47 +++++++++++++-- cmd/codeaf/factory_foreman_fresh_test.go | 74 +++++++++++++++++++++++- internal/manual/chat/factory.md | 2 +- internal/session/tools_floor.go | 2 +- 6 files changed, 168 insertions(+), 30 deletions(-) diff --git a/cmd/codeaf/appfactory_foreman.go b/cmd/codeaf/appfactory_foreman.go index 230a77cf8a..d62eca7d5c 100644 --- a/cmd/codeaf/appfactory_foreman.go +++ b/cmd/codeaf/appfactory_foreman.go @@ -118,32 +118,54 @@ func foremanLoad(host *appFactoryHost) func() (factory.Snapshot, error) { // stopped and archived, so the tab that comes back holds an archived chat // and the manager's button opens the new one; // - any other foreman takes the mark, and the app is told it is the -// factory's if it had already read it as an ordinary chat. +// factory's if it had already read it as an ordinary chat; +// - a conversation still marked the foreman's that the store no longer +// names (a making cut short) is retired and put away. // // A store that names no foreman is left alone: one is never made by default. +// +// IT WRITES THE FLOOR ONLY UNDER THE HOST. The naming is a chore of the host +// ([appFactoryHost.chore]), so it never starts once a close has begun and a +// close waits for it; putting the replaced conversations away is not, since a +// conversation that will not let go would hold the close for its graces, and +// what it writes is the person's chats, never the floor. func (b *appBackend) settleForeman(host *appFactoryHost) { + host.chore("app/factory foreman settle", func() { + away := b.settleForemanNow(host) + if len(away) > 0 { + guard.Go("app/factory foreman put away", func() { + for _, old := range away { + b.putAwayForeman(old) + } + }) + } + }) +} + +// settleForemanNow is the naming half of [appBackend.settleForeman], under the +// foreman's gate: it answers the conversations that were retired and are the +// caller's to put away. +func (b *appBackend) settleForemanNow(host *appFactoryHost) (away []string) { gate := &b.local.foreman gate.Lock() + defer gate.Unlock() + away = foremanSweep(host.floor, host.profile) chat, err := host.floor.Foreman() if err != nil || chat == "" { - gate.Unlock() - return + return away } if _, err := os.Stat(chat); err != nil { - gate.Unlock() - return + return away } if foremanFolderRefusal(host.floor, chat) == nil { _ = markFactoryForeman(host.floor, chat) - gate.Unlock() b.ownForeman(chat) - return + return away } - _, err = foremanMint(host.floor, host.workspace, host.profile, foremanLoad(host)) - gate.Unlock() - if err == nil { - b.putAwayForeman(chat) + if _, err := foremanMint(host.floor, host.workspace, host.profile, foremanLoad(host)); err == nil { + away = append(away, chat) } + return away } // ownForeman tells the app a conversation it already knows is the factory's, @@ -167,9 +189,16 @@ func (b *appBackend) ownForeman(transcript string) { // putAwayForeman stops a replaced foreman's turn and archives its // conversation. It is no longer the factory's: its mark is gone and the app is // told, so it reads as one of the person's own chats, in Archived with its -// history. It is best effort. The new foreman is already the factory's, and a -// conversation that is gone or will not close is no reason to refuse that one; -// what could not be archived is left open and is the person's to close. +// history, under the name [foremanEarlierTitle] when it still carried the +// manager's. It is best effort. The new foreman is already the factory's, and +// a conversation that is gone or will not close is no reason to refuse that +// one; what could not be archived is left open and is the person's to close. +// +// THE NAME IS WRITTEN THROUGH THE CONVERSATION. A title lives in the journal, +// and an open reads it from there: the name [foremanRetire] leaves in the +// folder's row alone was lost the first time the conversation was opened +// again, which is exactly when it matters, since a tab the app brought back +// may still hold this conversation and a person may write to it there. func (b *appBackend) putAwayForeman(transcript string) { if _, err := os.Stat(transcript); err != nil { return @@ -179,14 +208,18 @@ func (b *appBackend) putAwayForeman(transcript string) { if err != nil { return } + rename := false b.locked(func() { a.view.FactoryOwned = false - // The title its retiring gave it on disk ([foremanRetire]), for an - // agent this engine had already read under the manager's name. - if a.view.Title == foremanTitle || a.view.Title == foremanOldTitle { - a.view.Title = foremanEarlierTitle - } + rename = a.view.Title == foremanTitle || a.view.Title == foremanOldTitle || a.view.Title == foremanEarlierTitle }) + if rename { + if _, err := b.Rename(id, foremanEarlierTitle); err != nil { + // It could not be opened to be renamed (no provider, another + // window holds it): this engine still says the earlier name. + b.locked(func() { a.view.Title = foremanEarlierTitle }) + } + } _ = b.Stop(id) if err := b.Archive([]string{id}); err != nil { // Archive says the agent when it succeeds; a refusal still has to diff --git a/cmd/codeaf/appfactory_setup.go b/cmd/codeaf/appfactory_setup.go index 9e5000a013..deac4ef3fa 100644 --- a/cmd/codeaf/appfactory_setup.go +++ b/cmd/codeaf/appfactory_setup.go @@ -440,7 +440,7 @@ func (b *appBackend) startLocalLocked(ctx context.Context, resume bool) error { } // The foreman is put right as the factory starts, off this call: a tab the // app brings back reaches it without ever opening it by its door. - guard.Go("app/factory foreman settle", func() { b.settleForeman(host) }) + b.settleForeman(host) return nil } diff --git a/cmd/codeaf/factory_foreman.go b/cmd/codeaf/factory_foreman.go index 4fabe59a5a..22f807c4a7 100644 --- a/cmd/codeaf/factory_foreman.go +++ b/cmd/codeaf/factory_foreman.go @@ -60,7 +60,7 @@ const foremanEarlierTitle = "Factory manager (earlier)" // and the manual quote the same words. It is kept free of the characters a // journal line escapes (angle brackets, double quotes), so the conversation's // file holds it as written and can be searched for it. -const foremanOpening = "You are the foreman of this factory, its factory-wide manager. Every job, its state, its step and its pull request come through factory_floor: read it before you answer what is running, what waits, what is queued, what shipped or how many jobs a repository has, and never answer those from memory. A message from the person may start with a block tagged afi-now that the app writes: the time, the factory's counts, what waits on the person ('Waiting on you' is the person, never you), what is running, what was made today and what was noticed lately. It is the app's, not the person's words, and it was true when they sent the message: answer from it what it says, read factory_floor for what it does not, and never quote it back. Anything else the person asks, answer as any assistant would, with your other tools. Start a job or set its priority with factory_floor only when the person asks for it here, naming the job by its ref. Propose what to take on by selecting jobs. Never stop, ship, merge or post. Name jobs by their ref." +const foremanOpening = "You are the foreman of this factory, its factory-wide manager. Every job, its state, its step and its pull request come through factory_floor: read it before you answer what is running, what waits, what is queued, what shipped or how many jobs a repository has, and never answer those from memory. A message from the person may start with blocks the app writes. One tagged afi-now is the app's present state: the time, the factory's counts, what waits on the person ('Waiting on you' is the person, never you), what is running, what was made today and what was noticed lately. One tagged app-places says where things are in the app, for when the person asks where to find or do something. They are the app's, not the person's words, and were true when the message was sent: answer from them what they say, read factory_floor for what they do not, and never quote them back. Anything else the person asks, answer as any assistant would, with your other tools. Start a job or set its priority with factory_floor only when the person asks for it here, naming the job by its ref. Propose what to take on by selecting jobs. Never stop, ship, merge or post. Name jobs by their ref." // floorDoor is `factory_floor`'s door over the store, or nil. // @@ -271,9 +271,6 @@ func foremanMint(st *store.Store, workspace, profileDir string, load func() (fac if err := session.SeedConversation(transcript, where, foremanTitle, foremanBrief(foremanPolicyRead(load))); err != nil { return "", err } - if err := markFactoryForeman(st, transcript); err != nil { - return "", err - } old, _ := st.Foreman() if old != "" { foremanLeaveTeam(profileDir, old) @@ -281,17 +278,55 @@ func foremanMint(st *store.Store, workspace, profileDir string, load func() (fac if err := foremanJoinTeam(profileDir, transcript, where); err != nil { return "", err } + // THE STORE'S WORD COMES FIRST, THEN THE MARKS. The store is what makes a + // conversation the foreman; the mark only says so beside the journal. A + // making cut short after the store named the new one leaves marks a later + // open or start puts right ([markFactoryForeman] there, [foremanSweep]); + // one cut short before it leaves a conversation nobody names and nothing + // hides, which the person can see and delete. Marked first, that same + // conversation was hidden from every list with no door that opened it. if err := st.SetForeman(transcript); err != nil { return "", err } - // Only once the store names the new one: until then the old one is still - // the foreman, and still the factory's. + if err := markFactoryForeman(st, transcript); err != nil { + return "", err + } if old != "" && !sameSessionFile(old, transcript) { foremanRetire(old) } return transcript, nil } +// foremanSweep retires every conversation that still carries the foreman's +// mark and is not the one the store names: a making that was cut short +// between naming the new foreman and retiring the old one left the old one +// marked, so it was the factory's in every reading, in no list, and behind no +// door. They are found where a replaced foreman is always left, among the +// `factory` team's former members under the foreman's handle. It answers the +// conversations it retired, for their owner to put away. +func foremanSweep(st *store.Store, profileDir string) []string { + current, err := st.Foreman() + if err != nil { + return nil + } + f, err := teams.Load(profileDir) + if err != nil { + return nil + } + var retired []string + for _, t := range f.Teams { + for _, m := range t.FormerMembers { + file := strings.TrimSpace(m.File) + if file == "" || m.Handle != foremanHandle || sameSessionFile(file, current) || !factoryForemanMarked(file) { + continue + } + foremanRetire(file) + retired = append(retired, file) + } + } + return retired +} + // foremanRetire makes a replaced foreman an ordinary conversation of the // person's: its mark comes off, and a title that was still the manager's own // says it is the earlier one. A title the person gave it is theirs and stays. diff --git a/cmd/codeaf/factory_foreman_fresh_test.go b/cmd/codeaf/factory_foreman_fresh_test.go index abfc798eed..d9753f81ad 100644 --- a/cmd/codeaf/factory_foreman_fresh_test.go +++ b/cmd/codeaf/factory_foreman_fresh_test.go @@ -192,11 +192,49 @@ func TestForemanFreshStartsTheManagerOver(t *testing.T) { } } +// A MAKING CUT SHORT LEAVES NO CONVERSATION HIDDEN FOR GOOD. Between the +// store naming the new foreman and the old one being retired there is a +// moment a crash can fall into: the old one then still carries the mark, so +// every reading calls it the factory's, no list shows it and no door opens +// it. The sweep finds it where a replaced foreman is always left and retires +// it, and never touches the one the store names. +func TestAForemanLeftMarkedByAMakingCutShortIsRetired(t *testing.T) { + st, web, profile := talkLab(t) + first, err := foremanMaker(st, web, profile, nil)(context.Background()) + if err != nil { + t.Fatal(err) + } + fresh, err := foremanFresh(st, web, profile, nil)(context.Background()) + if err != nil { + t.Fatal(err) + } + if got := foremanSweep(st, profile); len(got) != 0 { + t.Fatalf("a finished making left %v for the sweep", got) + } + // As the crash leaves it: the old one's retiring never ran. + if err := markFactoryForeman(st, first); err != nil { + t.Fatal(err) + } + if !factoryOwnedConversation(first) { + t.Fatal("the old foreman is not marked; the premise is gone") + } + got := foremanSweep(st, profile) + if len(got) != 1 || !sameSessionFile(got[0], first) { + t.Fatalf("the sweep retired %v; want the old foreman alone", got) + } + if factoryOwnedConversation(first) || !factoryOwnedConversation(fresh) { + t.Fatalf("after the sweep: old marked %v, current marked %v", factoryOwnedConversation(first), factoryOwnedConversation(fresh)) + } + if again := foremanSweep(st, profile); len(again) != 0 { + t.Fatalf("a second sweep retired %v", again) + } +} + // THE BRIEF NO LONGER FENCES THE MANAGER INTO ONE TOOL. It was told to answer // "only from" factory_floor, so it could not use what the app tells it with // each message, nor answer anything a plain assistant can. func TestTheForemansBriefLetsItAnswerBeyondTheFloor(t *testing.T) { - for _, want := range []string{"factory_floor", "tagged afi-now", "is the person, never you", "your other tools", "only when the person asks", "Never stop, ship, merge or post"} { + for _, want := range []string{"factory_floor", "tagged afi-now", "tagged app-places", "is the person, never you", "your other tools", "only when the person asks", "Never stop, ship, merge or post"} { if !strings.Contains(foremanOpening, want) { t.Errorf("the brief lacks %q: %s", want, foremanOpening) } @@ -251,6 +289,12 @@ func TestTheFactoryManagersDoorReopensItAndFreshPutsTheOldOneAway(t *testing.T) foremanEventually(t, "the replaced manager being stopped and archived", func() bool { return s.backend.tidied(first.Agent) && !s.backend.isHeld(first.Agent) && convs.stops(first.Agent) > 0 }) + // ITS NEW NAME WENT THROUGH THE CONVERSATION, where an open reads a title + // from: left in the folder's row alone it read "Factory manager" again the + // first time the conversation was opened. + if got := convs.renamedTo(first.Agent); got != foremanEarlierTitle { + t.Errorf("the replaced manager's conversation was renamed %q; want %q", got, foremanEarlierTitle) + } var after appwire.FactoryForeman s.must(appwire.FeatureFactoryForemanOpen, ref, &after) if after.Agent != fresh.Agent { @@ -282,6 +326,20 @@ type foremanConvs struct { by map[string]*fakeConversation } +// renamedTo is the name the rig's conversation for agent was last given by +// the person's rename door, "" for none. +func (c *foremanConvs) renamedTo(agent string) string { + c.mu.Lock() + conv := c.by[agent] + c.mu.Unlock() + if conv == nil { + return "" + } + conv.mu.Lock() + defer conv.mu.Unlock() + return renamed[conv] +} + func (c *foremanConvs) stops(agent string) int { c.mu.Lock() conv := c.by[agent] @@ -348,7 +406,7 @@ func TestManyOpensAtOnceMakeOneFactoryManager(t *testing.T) { // was written to there again without factory.foreman_open ever being called. func TestAFactoryStartingMovesItsManagerOutOfTheCodeafHome(t *testing.T) { s := newLocalSetup(t) - foremanFakeOpens(s) + convs := foremanFakeOpens(s) floor, err := store.Open(v3FactoryRoot()) if err != nil { t.Fatal(err) @@ -372,6 +430,18 @@ func TestAFactoryStartingMovesItsManagerOutOfTheCodeafHome(t *testing.T) { if !factoryOwnedConversation(named) || factoryOwnedConversation(old) { t.Fatalf("the mark did not follow the manager: new %v, old %v", factoryOwnedConversation(named), factoryOwnedConversation(old)) } + // THE OLD ONE NO LONGER READS AS THE MANAGER, in the engine's word and in + // its own journal's: the tab the app brings back may still hold it. + foremanEventually(t, "the replaced manager being renamed", func() bool { return convs.renamedTo(oldID) == foremanEarlierTitle }) + was, err := s.backend.find(oldID) + if err != nil { + t.Fatal(err) + } + owned, title := true, "" + s.backend.locked(func() { owned, title = was.view.FactoryOwned, was.view.Title }) + if owned || title != foremanEarlierTitle { + t.Fatalf("the replaced manager reads %q, the factory's %v; want %q and the person's own", title, owned, foremanEarlierTitle) + } // AND ITS BUTTON OPENS THE NEW ONE, with nothing more made. var opened appwire.FactoryForeman s.must(appwire.FeatureFactoryForemanOpen, map[string]any{"factory": st.Factory}, &opened) diff --git a/internal/manual/chat/factory.md b/internal/manual/chat/factory.md index 98daf8a13a..f6e1b5a703 100644 --- a/internal/manual/chat/factory.md +++ b/internal/manual/chat/factory.md @@ -294,7 +294,7 @@ Every `factory_floor` answer but `item` and `repos` says the whole factory's cou - `repos`: every repository with its jobs counted by state, the one with work in hand first: `acme/api · 6 open: 1 running, 1 waiting on the person, 4 new · 3 done`. The first line says how many repositories have jobs, how many have open jobs, and how many have a job running, queued, waiting or following a pull request. - `shipped` with `days`: jobs shipped over that many days, today included (1 is today and the default, at most 90), newest first, each with the day it shipped. The first line counts the pull requests merged among them. -The Factory manager is not limited to `factory_floor`. The desktop app starts each message to it with an `` block of the app's present state (the time, the factory's counts, what waits on you, what is running, what was made today and what was noticed lately), the block Afi is sent. It answers from that for what the block says and reads the factory for the rest, and it answers anything else as any conversation does, with its other tools. +The Factory manager is not limited to `factory_floor`. The desktop app starts each message to it with an `` block of the app's present state (the time, the factory's counts, what waits on you, what is running, what was made today and what was noticed lately), the block Afi is sent, and its first message with an `` block saying where things are in the app. It answers from that for what the block says and reads the factory for the rest, and it answers anything else as any conversation does, with its other tools. ## the Factory manager did not answer, takes forever, stop does nothing, clear the Factory manager's conversation, archived Factory manager diff --git a/internal/session/tools_floor.go b/internal/session/tools_floor.go index af486c3dab..758beb0077 100644 --- a/internal/session/tools_floor.go +++ b/internal/session/tools_floor.go @@ -133,7 +133,7 @@ const factoryFloorDescription = "Read the whole factory, start or prioritise a j "Every answer but item and repos says the factory's counts by state on its first line, so a how-many question is answered by one read. " + "start takes one job by the number in its ref and starts it as the job page's Start does. priority is p0 (urgent), p1 (high), p2 (medium) or p3 (low), set on item. " + "item, mark and unmark take items by the number in their ref (#4 is 4). Only a new item can be selected; selections stay until the person runs or unselects them. The fields are named mark and unmark; the person calls them select and unselect. " + - "A message that starts with an block carries the app's present state as of when it was sent (\"Waiting on you\" there is the person); read here for what it does not say. " + + "A message may start with blocks the app writes: is the app's present state as of when it was sent (\"Waiting on you\" there is the person) and says where things are in the app; read here for what they do not say. " + "Name items by their ref in everything you say." func factoryFloorSchemaJSON() string { From a87bb40e47078fb27ef486173649ded4e591d5da Mon Sep 17 00:00:00 2001 From: ZeroPoint95 <329227198+ZeroPoint95@users.noreply.github.com> Date: Thu, 12 Nov 2026 02:04:18 -0500 Subject: [PATCH 5/9] Factory manager: tell the app when the manager is replaced A tab the app brought back went on showing the replaced manager with a box to write in. The engine now sends factory.foreman_changed {factory, agent, replaced} on every replacement so a window swaps that tab for the manager. The replaced conversation is renamed in its journal without being opened when nothing holds it (session.RetitleConversation): opening it in the folder it was moved out of, to write one line, was the cost the move exists to end. Co-Authored-By: Claude --- cmd/codeaf/appfactory_foreman.go | 30 ++++++-- cmd/codeaf/factory_foreman_fresh_test.go | 51 +++++++++++-- .../0-factory-manager-answers-stops-clears.md | 1 + docs/design/app-v3/PROTOCOL.md | 7 ++ internal/appwire/factory_foreman.go | 23 ++++++ internal/manual/chat/factory.md | 2 +- internal/session/talk_retitle_test.go | 74 +++++++++++++++++++ internal/session/talk_seed.go | 34 +++++++++ 8 files changed, 208 insertions(+), 14 deletions(-) create mode 100644 internal/session/talk_retitle_test.go diff --git a/cmd/codeaf/appfactory_foreman.go b/cmd/codeaf/appfactory_foreman.go index d62eca7d5c..b608acc231 100644 --- a/cmd/codeaf/appfactory_foreman.go +++ b/cmd/codeaf/appfactory_foreman.go @@ -9,6 +9,7 @@ import ( "github.com/Agent-Field/codeaf/internal/factory" "github.com/Agent-Field/codeaf/internal/factory/store" "github.com/Agent-Field/codeaf/internal/guard" + "github.com/Agent-Field/codeaf/internal/session" ) // FactoryForemanOpen is factory.foreman_open: the foreman conversation of the @@ -90,11 +91,19 @@ func (b *appBackend) foremanOpened(ctx context.Context, p appwire.FactoryRef, do b.sayAgent(a) } if before != "" && !sameSessionFile(before, transcript) { + b.sayForemanChanged(p.Factory, id, before) guard.Go("app/factory foreman put away", func() { b.putAwayForeman(before) }) } return appwire.FactoryForeman{Factory: p.Factory, Agent: id}, nil } +// sayForemanChanged tells every window the factory's manager was replaced: +// which conversation is the manager now, and which one no longer is, so a +// window still showing the old one swaps it ([appwire.EventFactoryForemanChanged]). +func (b *appBackend) sayForemanChanged(factory, agent, replaced string) { + b.emit.Emit(appwire.EventFactoryForemanChanged, appwire.FactoryForemanChanged{Factory: factory, Agent: agent, Replaced: appAgentID(replaced)}) +} + // foremanLoad is the floor's read a foreman's making asks for its brief's // policy lines: the host's own seam when it has one. func foremanLoad(host *appFactoryHost) func() (factory.Snapshot, error) { @@ -162,8 +171,9 @@ func (b *appBackend) settleForemanNow(host *appFactoryHost) (away []string) { b.ownForeman(chat) return away } - if _, err := foremanMint(host.floor, host.workspace, host.profile, foremanLoad(host)); err == nil { + if made, err := foremanMint(host.floor, host.workspace, host.profile, foremanLoad(host)); err == nil { away = append(away, chat) + b.sayForemanChanged(appLocalID, appAgentID(made), chat) } return away } @@ -214,11 +224,21 @@ func (b *appBackend) putAwayForeman(transcript string) { rename = a.view.Title == foremanTitle || a.view.Title == foremanOldTitle || a.view.Title == foremanEarlierTitle }) if rename { - if _, err := b.Rename(id, foremanEarlierTitle); err != nil { - // It could not be opened to be renamed (no provider, another - // window holds it): this engine still says the earlier name. - b.locked(func() { a.view.Title = foremanEarlierTitle }) + // A CONVERSATION THAT IS NOT OPEN IS NOT OPENED TO BE RENAMED. Its + // folder is the one it was moved out of, and assembling it there + // again to write one line is the thing this whole door exists to + // stop: its journal takes the line directly. One that is open (a tab + // holds it, or a turn is running) is renamed through its own door. + var err error + if b.isHeld(id) { + _, err = b.Rename(id, foremanEarlierTitle) + } else { + err = session.RetitleConversation(transcript, foremanEarlierTitle) } + // Whether or not the journal took it, this engine says the earlier + // name from here on. + _ = err + b.locked(func() { a.view.Title = foremanEarlierTitle }) } _ = b.Stop(id) if err := b.Archive([]string{id}); err != nil { diff --git a/cmd/codeaf/factory_foreman_fresh_test.go b/cmd/codeaf/factory_foreman_fresh_test.go index d9753f81ad..89d2203ef9 100644 --- a/cmd/codeaf/factory_foreman_fresh_test.go +++ b/cmd/codeaf/factory_foreman_fresh_test.go @@ -251,6 +251,8 @@ func TestTheForemansBriefLetsItAnswerBeyondTheFloor(t *testing.T) { func TestTheFactoryManagersDoorReopensItAndFreshPutsTheOldOneAway(t *testing.T) { s := newLocalSetup(t) convs := foremanFakeOpens(s) + heard := &foremanHeard{next: s.backend.emit} + s.backend.emit = heard st := s.start() ref := map[string]any{"factory": st.Factory} @@ -282,6 +284,11 @@ func TestTheFactoryManagersDoorReopensItAndFreshPutsTheOldOneAway(t *testing.T) if fresh.Agent == "" || fresh.Agent == first.Agent { t.Fatalf("a fresh start answered %q; want a new conversation", fresh.Agent) } + // EVERY WINDOW IS TOLD WHICH CONVERSATION IS THE MANAGER NOW, and only + // when one was replaced: the opens and the reopen above replaced nothing. + if got := heard.replacements(); len(got) != 1 || got[0] != (appwire.FactoryForemanChanged{Factory: st.Factory, Agent: fresh.Agent, Replaced: first.Agent}) { + t.Fatalf("the app was told of the replacements %+v; want one, of %s by %s", got, first.Agent, fresh.Agent) + } if !s.backend.isHeld(fresh.Agent) || s.backend.tidied(fresh.Agent) { t.Fatal("the fresh manager is not held open") } @@ -407,6 +414,8 @@ func TestManyOpensAtOnceMakeOneFactoryManager(t *testing.T) { func TestAFactoryStartingMovesItsManagerOutOfTheCodeafHome(t *testing.T) { s := newLocalSetup(t) convs := foremanFakeOpens(s) + heard := &foremanHeard{next: s.backend.emit} + s.backend.emit = heard floor, err := store.Open(v3FactoryRoot()) if err != nil { t.Fatal(err) @@ -430,9 +439,24 @@ func TestAFactoryStartingMovesItsManagerOutOfTheCodeafHome(t *testing.T) { if !factoryOwnedConversation(named) || factoryOwnedConversation(old) { t.Fatalf("the mark did not follow the manager: new %v, old %v", factoryOwnedConversation(named), factoryOwnedConversation(old)) } + // THE APP IS TOLD, so a tab it brought back on the old conversation is + // swapped for the manager instead of being written to. + if got := heard.replacements(); len(got) != 1 || got[0] != (appwire.FactoryForemanChanged{Factory: st.Factory, Agent: appAgentID(named), Replaced: oldID}) { + t.Fatalf("the app was told of the replacements %+v; want one, of %s by %s", got, oldID, appAgentID(named)) + } // THE OLD ONE NO LONGER READS AS THE MANAGER, in the engine's word and in - // its own journal's: the tab the app brings back may still hold it. - foremanEventually(t, "the replaced manager being renamed", func() bool { return convs.renamedTo(oldID) == foremanEarlierTitle }) + // its own journal's, where an open reads a name from. AND IT WAS NEVER + // OPENED TO BE RENAMED: its folder is the one it was moved out of. + foremanEventually(t, "the replaced manager's journal taking its new name", func() bool { + data, err := os.ReadFile(old) + return err == nil && strings.Contains(string(data), foremanEarlierTitle) + }) + convs.mu.Lock() + _, assembled := convs.by[oldID] + convs.mu.Unlock() + if assembled { + t.Error("the replaced manager was opened in its old folder to be put away") + } was, err := s.backend.find(oldID) if err != nil { t.Fatal(err) @@ -485,19 +509,30 @@ func TestAnAlreadyOpenManagerIsSaidToBeTheFactorysWhenItsDoorOpensIt(t *testing. type foremanHeard struct { next appwire.Emitter - mu sync.Mutex - changed []appwire.Agent + mu sync.Mutex + changed []appwire.Agent + replaced []appwire.FactoryForemanChanged } func (h *foremanHeard) Emit(event string, data any) { - if changed, ok := data.(appwire.AgentChanged); ok { - h.mu.Lock() - h.changed = append(h.changed, changed.Agent) - h.mu.Unlock() + h.mu.Lock() + switch said := data.(type) { + case appwire.AgentChanged: + h.changed = append(h.changed, said.Agent) + case appwire.FactoryForemanChanged: + h.replaced = append(h.replaced, said) } + h.mu.Unlock() h.next.Emit(event, data) } +// replacements is every time the app was told the manager was replaced. +func (h *foremanHeard) replacements() []appwire.FactoryForemanChanged { + h.mu.Lock() + defer h.mu.Unlock() + return append([]appwire.FactoryForemanChanged(nil), h.replaced...) +} + // owned is how many times the app was told agent is the factory's. func (h *foremanHeard) owned(agent string) int { h.mu.Lock() diff --git a/docs/changes/unreleased/0-factory-manager-answers-stops-clears.md b/docs/changes/unreleased/0-factory-manager-answers-stops-clears.md index 6785fb7879..368f03ecd6 100644 --- a/docs/changes/unreleased/0-factory-manager-answers-stops-clears.md +++ b/docs/changes/unreleased/0-factory-manager-answers-stops-clears.md @@ -7,6 +7,7 @@ invalidates: - "Was true: agents.stop only interrupted the turn, so a turn parked on a wait that never read its context stayed working until the app was quit. Is true now: the app's Stop is bounded. The stop carries its bound (Interrupt's `within`, in milliseconds) and the engine abandons a turn that has not let go ten seconds later; the turn ends stopped by the person and the next send opens an ordinary turn. A lent factory stage is interrupted but never abandoned from the app." - "Was true: a Factory manager made by a build that gave it the engine's own folder kept working in the codeaf home, where every save swept the whole home and its factory_floor calls never returned. Is true now: a manager in a folder the factory refuses for its chats is made again in `/chats/foreman` as the app's factory starts, and on factory.foreman_open if it had not been; the conversation it replaces is stopped and archived on a best effort and is the person's own chat from then on." - "Was true: the Factory manager was the factory's only in the memory of the engine that last opened it, so after a relaunch the app read it as one of the person's own chats, listed it among them and offered Archive. Is true now: a mark beside its journal makes every reading answer it factory_owned, and an app that had read it as an ordinary chat is sent agent.changed when the mark is given." + - "Was true: nothing told the app when the manager was replaced, so a tab it brought back kept showing the old conversation under the manager's name. Is true now: the event factory.foreman_changed {factory, agent, replaced} is sent on every replacement, and the replaced conversation is titled Factory manager (earlier) in its journal, without being opened when nothing holds it." - "Was true: two factory.foreman_open calls at once could each make a manager. Is true now: the manager is named and held open one call at a time, and the manager a call replaces is put away after the new one is answered." - "Was true: factory_floor waited on the factory with no bound and ignored the turn's context. Is true now: a read waits at most 20 seconds and a start, priority or selection 60 seconds, a stop returns it at once, and a start or priority that got no answer says it may still take effect." - "Was true: the Factory manager could be archived or closed, after which factory.foreman_open answered a conversation nobody could write to. Is true now: the open is the person's own and takes the manager out of Closed, and factory.foreman_fresh {factory} (hello feature factory.foreman_fresh) starts it over in a new conversation, stopping and archiving the old one." diff --git a/docs/design/app-v3/PROTOCOL.md b/docs/design/app-v3/PROTOCOL.md index cb17846370..ded629391c 100644 --- a/docs/design/app-v3/PROTOCOL.md +++ b/docs/design/app-v3/PROTOCOL.md @@ -1734,6 +1734,7 @@ and the reply is empty. | `deliverable.changed` | `DeliverableChanged` | | `devices.changed` | `DevicesResult` | | `entry.added` | `EntryAdded` | +| `factory.foreman_changed` | `FactoryForemanChanged` | | `factory.jobs_changed` | `FactoryJobsChanged` | | `factory.sessions_changed` | `Empty` | | `hello` | `Hello` | @@ -2295,6 +2296,12 @@ answered, on a best effort, and is no longer the factory's: it reads `factory_owned: false`, as one of the person's own chats in Archived, titled "Factory manager (earlier)" when its title was still the manager's own. +Whenever the engine replaces the manager (here, or by `factory.foreman_fresh`) +it sends the event `factory.foreman_changed {factory, agent, replaced}`: +`agent` is the manager now and `replaced` the conversation that no longer is. +A window showing `replaced` swaps it for `agent`; without the event a tab the +app brought back went on showing the old conversation under the manager's name. + ### Starting the Factory manager over (`factory.foreman_fresh`) The hello feature `factory.foreman_fresh` announces the app's Clear for the diff --git a/internal/appwire/factory_foreman.go b/internal/appwire/factory_foreman.go index af07e73118..137284cbf3 100644 --- a/internal/appwire/factory_foreman.go +++ b/internal/appwire/factory_foreman.go @@ -3,6 +3,7 @@ package appwire import ( "context" "encoding/json" + "reflect" ) // ── factory.foreman_open: the factory's own manager, as a conversation ──── @@ -58,6 +59,27 @@ func withFactoryForemanFreshFeature(hello Hello, e Engine) Hello { return hello } +// EventFactoryForemanChanged says the engine REPLACED a factory's manager: the +// conversation Replaced is the manager no longer, and Agent is. It is sent +// when the manager is started over (factory.foreman_fresh), and when one +// working in a folder the factory refuses is made again, which happens as the +// factory starts with nobody asking. Its data is [FactoryForemanChanged]. +// +// IT IS HOW A WINDOW HOLDING THE OLD CONVERSATION LEARNS IT IS HOLDING THE +// WRONG ONE. A tab the app brought back still showed the replaced manager +// under its name, with a box to write in; what was written there reached a +// conversation that could no longer read the factory, while the manager's own +// button opened another. The app swaps the tab for the manager on this event. +const EventFactoryForemanChanged = "factory.foreman_changed" + +// FactoryForemanChanged names the manager a factory has now and the +// conversation it replaced. Factory is the id factory.foreman_open takes. +type FactoryForemanChanged struct { + Factory string `json:"factory"` + Agent string `json:"agent"` + Replaced string `json:"replaced"` +} + // FactoryForemanOpener is the engine's half of factory.foreman_open. type FactoryForemanOpener interface { FactoryForemanOpen(ctx context.Context, p FactoryRef) (FactoryForeman, error) @@ -76,6 +98,7 @@ func withFactoryForemanOpenFeature(hello Hello, e Engine) Hello { func init() { methods[FeatureFactoryForemanOpen] = shaped[FactoryRef, FactoryForeman](foremanOpen) methods[FeatureFactoryForemanFresh] = shaped[FactoryRef, FactoryForeman](foremanFreshCall) + eventShapes[EventFactoryForemanChanged] = reflect.TypeFor[FactoryForemanChanged]() } func foremanFreshCall(e Engine, raw json.RawMessage) (*FactoryForeman, error) { diff --git a/internal/manual/chat/factory.md b/internal/manual/chat/factory.md index f6e1b5a703..122d4af187 100644 --- a/internal/manual/chat/factory.md +++ b/internal/manual/chat/factory.md @@ -302,7 +302,7 @@ One `factory_floor` read waits at most 20 seconds on the factory, and a start or The app's Stop is bounded. It interrupts the turn, and a turn that has still not let go ten seconds later is let go of by the engine: it ends "Stopped by you" and the next message opens an ordinary turn. -A Factory manager whose conversation works in a folder the factory refuses for its chats (the codeaf home, a folder above it, or elsewhere inside it) is made again in a folder of its own, `chats/foreman` under the factory. The desktop app does this as its factory starts, and when the manager is next opened if it had not yet; the conversation it replaces is then stopped and archived, and is one of your own chats in Archived from then on, titled "Factory manager (earlier)". One made by an older build worked in the home itself, where every save swept the whole home and its tool calls never finished. In a terminal the replaced conversation is left as it is, in your list. +A Factory manager whose conversation works in a folder the factory refuses for its chats (the codeaf home, a folder above it, or elsewhere inside it) is made again in a folder of its own, `chats/foreman` under the factory. The desktop app does this as its factory starts, and when the manager is next opened if it had not yet; the conversation it replaces is then stopped and archived, and is one of your own chats in Archived from then on, titled "Factory manager (earlier)". The app is told (`factory.foreman_changed`), and a tab still showing the old conversation is swapped for the manager. One made by an older build worked in the home itself, where every save swept the whole home and its tool calls never finished. In a terminal the replaced conversation is left as it is, in your list. The Factory manager is cleared, not archived. `factory.foreman_fresh` starts it over: a new conversation with only its brief takes its place and its `@foreman` address, on the default model and settings, and the old one is stopped and archived with its history kept, as one of your own chats. Opening the Factory manager is your own open, so one you closed or archived comes back open. The manager's conversation stays the factory's across a relaunch: it is not listed among your chats and has no Archive. diff --git a/internal/session/talk_retitle_test.go b/internal/session/talk_retitle_test.go new file mode 100644 index 0000000000..70d9d9e6d6 --- /dev/null +++ b/internal/session/talk_retitle_test.go @@ -0,0 +1,74 @@ +package session + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// A CONVERSATION NOBODY HAS OPEN CAN BE RENAMED WITHOUT BEING OPENED. +// +// The factory renames a manager it replaced, and that conversation's folder is +// the one it was moved out of: assembling it there again to write one line +// was the cost the move exists to end. The name goes on the journal as a +// title line, which is what the next open reads, so the name survives it. +func TestRetitleConversationNamesAJournalNobodyHolds(t *testing.T) { + path := filepath.Join(t.TempDir(), "transcript.jsonl") + if err := SeedConversation(path, t.TempDir(), "Factory manager", "the opening note"); err != nil { + t.Fatal(err) + } + if err := RetitleConversation(path, " Factory manager (earlier) "); err != nil { + t.Fatalf("RetitleConversation: %v", err) + } + replayed, err := replaySessionFile(path) + if err != nil { + t.Fatal(err) + } + if replayed.title != "Factory manager (earlier)" { + t.Fatalf("the next open reads the name %q; want the new one", replayed.title) + } + // Nothing of the conversation is rewritten: the note it opened with is + // still its first words. + data, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(data), "the opening note") || strings.Count(string(data), `"type":"session"`) != 1 { + t.Fatalf("the journal was rewritten:\n%s", data) + } +} + +// A JOURNAL SOMEBODY HOLDS IS REFUSED, AND SO IS A PATH WITH NO CONVERSATION: +// the held one is renamed through its own door, and this door never makes a +// conversation by naming one. +func TestRetitleConversationRefusesAHeldJournalAndAnEmptyPath(t *testing.T) { + path := filepath.Join(t.TempDir(), "transcript.jsonl") + if err := SeedConversation(path, t.TempDir(), "Factory manager", "the opening note"); err != nil { + t.Fatal(err) + } + held, _, err := openSessionFile(path, "", "", "") + if err != nil { + t.Fatal(err) + } + if err := RetitleConversation(path, "another name"); err == nil { + t.Error("a journal somebody holds was renamed under them") + } + if err := held.Close(); err != nil { + t.Fatal(err) + } + if replayed, err := replaySessionFile(path); err != nil || replayed.title != "Factory manager" { + t.Fatalf("the refused rename left the name %q (%v)", replayed.title, err) + } + + missing := filepath.Join(t.TempDir(), "transcript.jsonl") + if err := RetitleConversation(missing, "a name"); err == nil { + t.Error("a path with no conversation was renamed") + } + if _, err := os.Stat(missing); err == nil { + t.Error("renaming a path with no conversation made a journal there") + } + if err := RetitleConversation(path, " "); err == nil { + t.Error("an empty name was accepted") + } +} diff --git a/internal/session/talk_seed.go b/internal/session/talk_seed.go index 0526d9afb2..4b27e60e52 100644 --- a/internal/session/talk_seed.go +++ b/internal/session/talk_seed.go @@ -2,6 +2,7 @@ package session import ( "errors" + "os" "strings" ) @@ -48,6 +49,39 @@ func SeedConversation(path, cwd, title, note string) error { return nil } +// RetitleConversation gives a conversation NOBODY HAS OPEN the name title: one +// literal title line appended to its journal, which is where an open reads a +// name from, and the folder's row beside it. It is for a caller that must +// rename a conversation without assembling it (a workspace it should not +// open again, a provider it does not have). A journal somebody holds is +// refused, as any second claim on one is; that conversation is renamed +// through its own door ([Agent.RenameByPerson]). A path that holds no +// conversation is refused too: this names one, it never makes one. +func RetitleConversation(path, title string) error { + path, title = strings.TrimSpace(path), strings.Join(strings.Fields(title), " ") + if path == "" || title == "" { + return errors.New("a conversation is renamed by its file and a name") + } + if _, err := os.Stat(path); err != nil { + return errors.New("there is no conversation there to rename") + } + journal, replayed, err := openSessionFile(path, "", "", sessionIDOfFolder(path)) + if err != nil { + return err + } + defer journal.Close() + if !replayed.existed { + return errors.New("there is no conversation there to rename") + } + journal.appendNamedTitle(title, true) + dir := DirOf(path) + if meta, err := LoadMeta(dir); err == nil && strings.TrimSpace(meta.ID) != "" { + meta.Title, meta.LiteralName = title, true + _ = SaveMeta(dir, meta) + } + return nil +} + // sessionIDOfFolder is the id a seeded journal's header carries: its folder's // meta id when the journal sits in a session folder, and "" (a fresh id) when // it does not, which is [openSessionFile]'s own rule for a flat file. From 3e449f7d5281a785be0a9469504d6b6a1c1f186e Mon Sep 17 00:00:00 2001 From: ZeroPoint95 <329227198+ZeroPoint95@users.noreply.github.com> Date: Thu, 12 Nov 2026 06:23:13 -0500 Subject: [PATCH 6/9] Bounded stop test: wait for the abandoned line instead of reading it at once The clock's abandon closes the turn's events before it writes its journal line, so reading the journal the instant the events ended sometimes found nothing. It failed about one run in three under -race; it now passes 20 of 20 there. Co-Authored-By: Claude Opus 5.5 --- internal/session/abandon_test.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/internal/session/abandon_test.go b/internal/session/abandon_test.go index cdb4c93dbc..0149d603c1 100644 --- a/internal/session/abandon_test.go +++ b/internal/session/abandon_test.go @@ -171,6 +171,10 @@ func TestABoundedStopAbandonsATurnThatWouldNotLetGo(t *testing.T) { close(closed) }() waitUntilClosed(t, "the bounded stop letting go of the turn", closed, 10*time.Second) + // The clock's abandon closes the turn's events before it writes its + // journal line, so the line is waited for, not read the instant the + // events end. + waitFor(t, "the abandoned line in the journal", func() bool { return len(abandonedLines(t, journal)) > 0 }) if lines := abandonedLines(t, journal); len(lines) != 1 || lines[0].Reason != string(AbandonStopTimeout) { t.Fatalf("want one abandoned line saying %q, got %+v", AbandonStopTimeout, lines) } From e954594c852d60c96a5ede78140bdfe4ebf58122 Mon Sep 17 00:00:00 2001 From: ZeroPoint95 <329227198+ZeroPoint95@users.noreply.github.com> Date: Thu, 12 Nov 2026 06:24:01 -0500 Subject: [PATCH 7/9] Change entry: number it for #2608 Co-Authored-By: Claude Opus 5.5 --- ...ops-clears.md => 2608-factory-manager-answers-stops-clears.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename docs/changes/unreleased/{0-factory-manager-answers-stops-clears.md => 2608-factory-manager-answers-stops-clears.md} (100%) diff --git a/docs/changes/unreleased/0-factory-manager-answers-stops-clears.md b/docs/changes/unreleased/2608-factory-manager-answers-stops-clears.md similarity index 100% rename from docs/changes/unreleased/0-factory-manager-answers-stops-clears.md rename to docs/changes/unreleased/2608-factory-manager-answers-stops-clears.md From 0715d17c9051cc607fbf3a13aeba8ccde816e52e Mon Sep 17 00:00:00 2001 From: ZeroPoint95 <329227198+ZeroPoint95@users.noreply.github.com> Date: Sat, 14 Nov 2026 09:49:17 -0500 Subject: [PATCH 8/9] Change entry: set pr to 2608 Co-Authored-By: Claude Fable 5.2 --- .../unreleased/2608-factory-manager-answers-stops-clears.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/changes/unreleased/2608-factory-manager-answers-stops-clears.md b/docs/changes/unreleased/2608-factory-manager-answers-stops-clears.md index 368f03ecd6..fe39df9849 100644 --- a/docs/changes/unreleased/2608-factory-manager-answers-stops-clears.md +++ b/docs/changes/unreleased/2608-factory-manager-answers-stops-clears.md @@ -1,7 +1,7 @@ --- kind: fixed title: the Factory manager answers, stops, and is cleared rather than archived -pr: 0 +pr: 2608 surface: [factory, engine, app] invalidates: - "Was true: agents.stop only interrupted the turn, so a turn parked on a wait that never read its context stayed working until the app was quit. Is true now: the app's Stop is bounded. The stop carries its bound (Interrupt's `within`, in milliseconds) and the engine abandons a turn that has not let go ten seconds later; the turn ends stopped by the person and the next send opens an ordinary turn. A lent factory stage is interrupted but never abandoned from the app." From 69f29db7f17e48110b9319aadbb09fc71efb201b Mon Sep 17 00:00:00 2001 From: ZeroPoint95 <329227198+ZeroPoint95@users.noreply.github.com> Date: Sat, 14 Nov 2026 10:03:30 -0500 Subject: [PATCH 9/9] Factory manager: guard the policy read's goroutine The guard law (TestEveryGoroutineInTheGuardedTreeIsGuarded) refuses a bare go statement here. A read that panics now answers nothing, and the bounded wait returns an empty policy as it does for a slow read. Co-Authored-By: Claude Fable 5.2 --- cmd/codeaf/factory_foreman.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/cmd/codeaf/factory_foreman.go b/cmd/codeaf/factory_foreman.go index 22f807c4a7..9f1bf441d3 100644 --- a/cmd/codeaf/factory_foreman.go +++ b/cmd/codeaf/factory_foreman.go @@ -10,6 +10,7 @@ import ( "github.com/Agent-Field/codeaf/internal/factory" "github.com/Agent-Field/codeaf/internal/factory/store" + "github.com/Agent-Field/codeaf/internal/guard" "github.com/Agent-Field/codeaf/internal/session" "github.com/Agent-Field/codeaf/internal/teams" ) @@ -359,10 +360,11 @@ func foremanPolicyRead(load func() (factory.Snapshot, error)) factory.Snapshot { return factory.Snapshot{} } done := make(chan factory.Snapshot, 1) - go func() { + // Guarded: a read that panics answers nothing, and the wait below says so. + guard.Go("app/factory foreman policy read", func() { snap, _ := load() done <- snap - }() + }) select { case snap := <-done: return snap