From 21e4bd621613f7a54b86ea0ecf2b9d5730c599eb Mon Sep 17 00:00:00 2001 From: ZeroPoint95 <329227198+ZeroPoint95@users.noreply.github.com> Date: Tue, 3 Nov 2026 13:29:21 -0500 Subject: [PATCH 1/4] Pairing QA fixes: failures, approvals, removed computers, devices loop (#2285) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * pairing: a failure says its sentence once, and a finished pairing drops its check number - B2: the failure sentence is the pairing's error only; it is no longer also appended as a status line, so the desktop sheet shows it once. - B3a: finish() clears Words with the code and link, so no check number stays on screen after pairing succeeds, fails or is cancelled. Co-Authored-By: Claude Opus 5.5 * sync: log why a chat could not be shared, once per chat and reason (T7) The sharing pass kept only that a chat failed and showed one sentence; nothing recorded the cause. Each failure now logs the chat and its error the first time that reason appears, so retries do not flood the log. Co-Authored-By: Claude Opus 5.5 * tui: an approval that timed out says it may still have arrived (T2) "no answer from your devices — try again" blamed devices that take no part and invited a retry the service would refuse. A timed-out answer now says the sync service did not answer and to check /devices first; a timed-out lookup says the sync service did not answer. The manual quotes both. Co-Authored-By: Claude Opus 5.5 * pair: the terminal approval keys sit on their own line (T9) `codeaf pair approve` printed the check number and "a / d" on one line, so the typed answer ran into the question. The keys now follow on their own line as "a approve · d decline:", matching the chat card. Co-Authored-By: Claude Opus 5.5 * manual: correct how a removed computer rejoins and where older chats are prepared (D1) - A removed computer comes back with `codeaf pair --replace`; plain `codeaf pair` does not bring it back. - Older chats are prepared in the background only by the terminal chat, not by the desktop app. Co-Authored-By: Claude Opus 5.5 * app: a removed computer reads the removal and "codeaf pair --replace", not "wireauth: device revoked" (B11) - Engine errors that are the revoked-device refusal reach the app as chatlist.Removed instead of the wire sentinel's text, so the chats area no longer shows "wireauth: device revoked". - Removed now says "run codeaf pair --replace to bring it back": plain `codeaf pair` does not bring a removed computer back (B1). Notices stored with the earlier wording are still recognised as a removal. - The manual quotes the new sentence. Co-Authored-By: Claude Opus 5.5 * sync: a failed continue logs its reason, and the app test pins the take failure's Detail (B5a) - The "continue phases … outcome=failed" line now ends with the error as reason="…", so chat.log names why a move failed. - appTakeError already carries the cause in Detail; the S5 take-failure test now checks it reaches the app over the wire and is not the generic sentence. Co-Authored-By: Claude Opus 5.5 * pairing: the code instruction says dashes are optional (F4) The engine already reads a join code with or without dashes and spaces (pair.ReadJoinCode, covered by TestReadCodeShapes). The instruction now says so: "and type 42-715-302 (dashes optional)". Copy pins and the desktop-sync manual quote follow. Co-Authored-By: Claude Opus 5.5 * manual: restore the fresh-computer --replace exception, scoped so it cannot contradict rejoining (D1 follow-up) A computer whose chats hold only startup headers still pairs without --replace when it first joins (identity.Joinable). A removed computer already holds the identity, so pair.adopt stops at "already joined" before that check and only --replace brings it back. The sentence now says both. Co-Authored-By: Claude Opus 5.5 * pairing: a removed computer pairs back from the app (B1) A removed computer keeps the identity and its revoked device key, so pairing it again ended "already paired; nothing changed" and left it removed. Only --replace minted a new key, and the app never sends it. - pair.Joining gains Revoked. When it is set and the grant is the same identity, adopt renews the device key as Replace does. With the same identity nothing local is given up; a different set of devices still follows the normal rules. - The app's use_code and request_link ask the directory whether this computer's own device is refused (ownDeviceRevoked) and set Revoked. - New TestB1AppRePairsARemovedComputer covers code and link re-pairing after removal (new key, same identity, chat kept, old key still removed, restart requested) and an unremoved computer still being told nothing changed. - The manual says the app brings a removed computer back on its own. Co-Authored-By: Claude Opus 5.5 * tui: the approval timeout says "sync", not "the sync service" (T2 follow-up) The word law (#1508) keeps "service" out of person-facing text, and TestNoPersonFacingStringInThisSurfaceSaysLane caught approve.go. Both timeout sentences now begin "sync did not answer in time", as the other sync sentences do, and the manual quotes them. Co-Authored-By: Claude Opus 5.5 * approve: an approval whose reply was lost still pairs, and its link stops asking (T1, T3) - After an approve times out or is refused as already answered, the door reads the request again. Approved for this device and listed in this computer's own fleet counts as success: the solo marker ends and the card closes as approved. - Still waiting means nothing arrived: the screen says the plain retry. Unknown keeps the "may still have arrived" sentence. - Reopening the link of an approval that landed says the device is already paired and ends solo, instead of asking to approve again. - The device record gets its own 8 s wait, and an answer's overall wait is 20 s, so a slow first write cannot spend the approve's time. - Tests: a slow relay that approves but loses the reply (both computers end up paired), one that never hears the approval (the retry pairs), and a reopened link after a lost approval. Co-Authored-By: Claude Opus 5.5 * devices: a roster read's own last-seen stamp no longer publishes devices.changed (B8b) A hosted relay stamps the asking computer's last seen on every request, so each roster read moved this computer's stamp and devices.changed went out. The app answers that event with another read, and the two kept each other going while a window was open: constant relay traffic, and the Computers page reloading every few seconds. publish now judges a change with this computer's last seen, and that of any computer online now, left out; neither is shown. The event still carries the current stamps, and an offline computer's new last seen or a change in who is online still publishes. Co-Authored-By: Claude Opus 5.5 * changes: entry for the pairing QA fixes Co-Authored-By: Claude Opus 5.5 * tui: a declined answer that timed out is the plain retry, not "check /devices" (T2 follow-up) Co-Authored-By: Claude Opus 5.5 * app: a removed computer's refusal names no chat and points at Pair a computer (B11 follow-up) Co-Authored-By: Claude Opus 5.5 * manual: rejoining keeps chats syncing and memories and settings in place (D1 follow-up) Co-Authored-By: Claude Opus 5.5 * changes: the lost-reply fix is the terminal screen's, and the app's removal remedy Co-Authored-By: Claude Opus 5.5 * changes: re-adding a removed computer is not finished (B1) Two-computer testing on 2026-11-03 still showed neither computer listing the other after re-pairing a removed one; the entry no longer says it comes back. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- cmd/codeaf/app.go | 5 + cmd/codeaf/app_pair.go | 6 +- cmd/codeaf/app_pair_copy.go | 5 +- cmd/codeaf/app_pair_e14_test.go | 2 +- cmd/codeaf/app_pair_rejoin_test.go | 114 +++++++++++++++ cmd/codeaf/app_relay_test.go | 28 +++- cmd/codeaf/app_take_recovery_test.go | 8 ++ cmd/codeaf/appdevices.go | 16 ++- cmd/codeaf/appdevices_binding_test.go | 11 ++ cmd/codeaf/appdevices_lastseen_test.go | 45 ++++++ cmd/codeaf/approvals.go | 73 +++++++++- cmd/codeaf/approvals_test.go | 134 ++++++++++++++++++ cmd/codeaf/pair.go | 18 +++ cmd/codeaf/pair_terminal.go | 6 +- cmd/codeaf/pairlink_test.go | 2 +- cmd/codeaf/review_fixes_test.go | 4 + .../unreleased/2285-pairing-qa-fixes.md | 18 +++ internal/chatlist/copy.go | 6 +- internal/chatlist/offer_test.go | 9 +- .../manual/chat/add-or-remove-a-computer.md | 10 +- internal/manual/chat/desktop-sync.md | 2 +- internal/pair/join.go | 8 ++ internal/syncsetup/copy.go | 5 + internal/syncsetup/take.go | 3 +- internal/tui3/approve.go | 33 ++++- internal/tui3/approve_test.go | 50 +++++++ internal/tui3/manual_devices_test.go | 2 +- 27 files changed, 597 insertions(+), 26 deletions(-) create mode 100644 cmd/codeaf/app_pair_rejoin_test.go create mode 100644 cmd/codeaf/appdevices_lastseen_test.go create mode 100644 docs/changes/unreleased/2285-pairing-qa-fixes.md diff --git a/cmd/codeaf/app.go b/cmd/codeaf/app.go index 0e80e4b1b6..49873e4187 100644 --- a/cmd/codeaf/app.go +++ b/cmd/codeaf/app.go @@ -52,6 +52,7 @@ import ( "github.com/Agent-Field/codeaf/internal/cellstore" "github.com/Agent-Field/codeaf/internal/clipboard" "github.com/Agent-Field/codeaf/internal/config" + "github.com/Agent-Field/codeaf/internal/directory" "github.com/Agent-Field/codeaf/internal/effort" "github.com/Agent-Field/codeaf/internal/enginehost" "github.com/Agent-Field/codeaf/internal/executor" @@ -4703,6 +4704,10 @@ func appEngineError(err error) error { if appMissingKey(err) { return &appwire.Error{Code: appwire.CodeEngine, Message: appNoProvider} } + // A removed computer reads the removal and its remedy, never the wire's sentinel. + if errors.Is(err, directory.ErrRevoked) { + return &appwire.Error{Code: appwire.CodeEngine, Message: appRemoved} + } return &appwire.Error{Code: appwire.CodeEngine, Message: redact.Secrets(plainWords(err.Error()))} } diff --git a/cmd/codeaf/app_pair.go b/cmd/codeaf/app_pair.go index 97046d3299..d7ae62048c 100644 --- a/cmd/codeaf/app_pair.go +++ b/cmd/codeaf/app_pair.go @@ -235,6 +235,7 @@ func (o *appPairOperation) finish(state, line string, err error, restart bool) { o.view.Question = nil o.view.Code = "" o.view.Link = "" + o.view.Words = "" o.view.ExpiresAt = time.Time{} if line != "" { o.view.Lines = append(o.view.Lines, strings.Split(line, "\n")...) @@ -265,7 +266,8 @@ func (o *appPairOperation) run(p appwire.PairStartParams) { case errors.Is(err, pair.ErrRefused): o.finish("cancelled", appPairDeclined, nil, false) default: - o.finish("failed", appPairFailure(err), errors.New(appPairFailure(err)), false) + // The failure sentence is the error alone; repeating it as a line showed it twice. + o.finish("failed", "", errors.New(appPairFailure(err)), false) } } func (o *appPairOperation) perform(p appwire.PairStartParams) (error, bool) { @@ -307,6 +309,7 @@ func (o *appPairOperation) perform(p appwire.PairStartParams) (error, bool) { return nil, false case "use_code": joining := pairJoiningAt(dir)(false) + joining.Revoked = ownDeviceRevoked(o.ctx, dir, route) if p.Name != "" { joining.Label = p.Name } @@ -321,6 +324,7 @@ func (o *appPairOperation) perform(p appwire.PairStartParams) (error, bool) { return nil, !joined.Already case "request_link": joining := linkJoining(dir)(route, false) + joining.Revoked = ownDeviceRevoked(o.ctx, dir, route) if p.Name != "" { joining.Label = p.Name } diff --git a/cmd/codeaf/app_pair_copy.go b/cmd/codeaf/app_pair_copy.go index 2d17a7d625..9e9ca86cf4 100644 --- a/cmd/codeaf/app_pair_copy.go +++ b/cmd/codeaf/app_pair_copy.go @@ -20,6 +20,9 @@ const ( appPairFollowed = "Paired. This computer now shares your current chats and keeps its saved keys." appPairListUnread = "Paired. Your computers could not be listed yet. Open codeaf again in a moment." appPairFailed = "This computer could not finish pairing. Try again." + // appRemoved answers any call refused because this computer was removed, + // so it names no chat; the app's own remedy is Pair a computer. + appRemoved = "This computer was removed by another of your devices. To bring it back, choose Pair a computer." ) func pairApprovalOptions() []appwire.PairOption { @@ -27,7 +30,7 @@ func pairApprovalOptions() []appwire.PairOption { } func appPairCodeLines(code string) []string { - return []string{"Pairing shares your chats with another computer.", fmt.Sprintf("On your other computer, choose Pair a computer, then Enter a code from the other computer, and type %s. This code lasts %d minutes.", code, int(pair.CodeValidFor/time.Minute))} + return []string{"Pairing shares your chats with another computer.", fmt.Sprintf("On your other computer, choose Pair a computer, then Enter a code from the other computer, and type %s (dashes optional). This code lasts %d minutes.", code, int(pair.CodeValidFor/time.Minute))} } func appPairApproveTitle(name string) string { return fmt.Sprintf("Pair with %s?", name) } func appPairWords(words string) string { diff --git a/cmd/codeaf/app_pair_e14_test.go b/cmd/codeaf/app_pair_e14_test.go index 8cfeb7d7d6..fac6e84b15 100644 --- a/cmd/codeaf/app_pair_e14_test.go +++ b/cmd/codeaf/app_pair_e14_test.go @@ -6,7 +6,7 @@ import ( ) func TestA3PairingInstructionsNameDesktopControls(t *testing.T) { - want := "On your other computer, choose Pair a computer, then Enter a code from the other computer, and type 42-715-302. This code lasts 10 minutes." + want := "On your other computer, choose Pair a computer, then Enter a code from the other computer, and type 42-715-302 (dashes optional). This code lasts 10 minutes." if got := appPairCodeLines("42-715-302")[1]; got != want { t.Fatalf("A3: instructions = %q, want %q", got, want) } diff --git a/cmd/codeaf/app_pair_rejoin_test.go b/cmd/codeaf/app_pair_rejoin_test.go new file mode 100644 index 0000000000..dd0b0c3715 --- /dev/null +++ b/cmd/codeaf/app_pair_rejoin_test.go @@ -0,0 +1,114 @@ +package main + +import ( + "context" + "errors" + "os" + "path/filepath" + "slices" + "strings" + "testing" + + "github.com/Agent-Field/codeaf/internal/appwire" + "github.com/Agent-Field/codeaf/internal/cell/celltest" + "github.com/Agent-Field/codeaf/internal/directory" + "github.com/Agent-Field/codeaf/internal/identity" + "github.com/Agent-Field/codeaf/internal/syncsetup" +) + +// B1: a computer removed from your devices pairs back from the app, by code and +// by link, with no --replace to know about. It keeps the identity and gets a new +// device key; one that was never removed is told nothing changed, not "Paired". +func TestB1AppRePairsARemovedComputer(t *testing.T) { + for _, tc := range []struct { + name string + link bool + removed bool + }{{"code", false, true}, {"link", true, true}, {"code, not removed", false, false}} { + t.Run(tc.name, func(t *testing.T) { + celltest.Available(t) + relay := e2Relay(t) + t.Setenv(syncsetup.URLVar, relay.URL) + dirA, dirB := t.TempDir(), t.TempDir() + id, err := identity.Ensure(dirA) + if err != nil { + t.Fatal(err) + } + if _, err := identity.Adopt(dirB, id, false); err != nil { + t.Fatal(err) + } + a, old := appDeviceSync(t, dirA), appDeviceSync(t, dirB) + for side, s := range map[string]*syncsetup.Sync{"A": a, "B": old} { + if err := s.Rename(context.Background(), side); err != nil { + t.Fatal(err) + } + } + if tc.removed { + if err := a.Dir.Revoke(context.Background(), old.Device.ID()); err != nil { + t.Fatal(err) + } + if _, err := old.Dir.List(context.Background()); !errors.Is(err, directory.ErrRevoked) { + t.Fatalf("the removed computer must be refused before re-pairing: %v", err) + } + } + kept := filepath.Join(dirB, "v3", "projects", "local", "saved", "meta.json") + if err := os.MkdirAll(filepath.Dir(kept), 0700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(kept, []byte(`{"id":"saved","title":"keep me"}`), 0600); err != nil { + t.Fatal(err) + } + _, _, wa := appDeviceBackend(t, dirA) + _, _, wb := appDeviceBackend(t, dirB) + + var joinID string + if tc.link { + request := machineCall[appwire.PairResult](t, wb, "pair.start", appwire.PairStartParams{Mode: "request_link", Name: "B"}).Pairing + waiting := e2PairWait(t, wb, request.ID, "waiting") + approve := machineCall[appwire.PairResult](t, wa, "pair.start", appwire.PairStartParams{Mode: "approve_link", Link: waiting.Link}).Pairing + q := e2PairWait(t, wa, approve.ID, "question") + machineCall[appwire.PairResult](t, wa, "pair.answer", appwire.PairAnswerParams{ID: approve.ID, Question: q.Question.ID, Option: "y"}) + e2PairWait(t, wa, approve.ID, "done") + joinID = request.ID + } else { + offer := machineCall[appwire.PairResult](t, wa, "pair.start", appwire.PairStartParams{Mode: "offer_code", Name: "A"}).Pairing + waiting := e2PairWait(t, wa, offer.ID, "waiting") + joinID = machineCall[appwire.PairResult](t, wb, "pair.start", appwire.PairStartParams{Mode: "use_code", Code: waiting.Code, Name: "B"}).Pairing.ID + q := e2PairWait(t, wa, offer.ID, "question") + machineCall[appwire.PairResult](t, wa, "pair.answer", appwire.PairAnswerParams{ID: offer.ID, Question: q.Question.ID, Option: "y"}) + e2PairWait(t, wa, offer.ID, "done") + } + done := e2PairWait(t, wb, joinID, "done") + if raw, err := os.ReadFile(kept); err != nil || !strings.Contains(string(raw), "keep me") { + t.Fatalf("re-pairing lost a local chat: %s %v", raw, err) + } + fresh := appDeviceSync(t, dirB) + if fresh.Identity.ID() != id.ID() { + t.Fatalf("re-pairing changed the identity: %s, want %s", fresh.Identity.ID(), id.ID()) + } + + if !tc.removed { + if fresh.Device.ID() != old.Device.ID() || done.Restart || !slices.Contains(done.Lines, appPairAlready) { + t.Fatalf("an unremoved computer must be told nothing changed: %+v", done) + } + return + } + if fresh.Device.ID() == old.Device.ID() { + t.Fatal("the removed computer kept its refused device key") + } + if slices.Contains(done.Lines, appPairAlready) || !done.Restart { + t.Fatalf("the removed computer was not paired back: %+v", done) + } + listing, err := fresh.Dir.List(context.Background()) + if err != nil { + t.Fatalf("the re-paired computer is still refused: %v", err) + } + if dev, ok := listing.Devices[fresh.Device.ID()]; !ok || dev.Revoked { + t.Fatalf("the new device is not in your devices: %+v", listing.Devices) + } + if !listing.Devices[old.Device.ID()].Revoked { + t.Fatal("the old device key must stay removed") + } + }) + } +} diff --git a/cmd/codeaf/app_relay_test.go b/cmd/codeaf/app_relay_test.go index 4a246a1daf..b7f86ce0c5 100644 --- a/cmd/codeaf/app_relay_test.go +++ b/cmd/codeaf/app_relay_test.go @@ -267,7 +267,7 @@ func TestM1M4M8M9M10AppPair(t *testing.T) { func assertAppPairCopyM9(t *testing.T, rec *appRecorder) { t.Helper() - allowed := regexp.MustCompile(`^(Pairing shares your chats with another computer\.|On your other computer, choose Pair a computer, then Enter a code from the other computer, and type [0-9-]+\. This code lasts 10 minutes\.|Waiting for approval on your other computer\. It should show: .+\.|Paired with .+\. Your chats can continue on both computers\.|Paired\. Your chats are kept\. Both computers' chats will appear as sharing finishes\.|Cancelled\. Nothing was paired\.|Pair with .+\?|Pair only if your other computer shows the same (three words|check number): .+\.|Pair|Don't pair|Open this pairing link on a computer you already use\.|Compare the check number on both computers: .+\.|Waiting for approval\. This link lasts [0-9]+ minutes\.)$`) + allowed := regexp.MustCompile(`^(Pairing shares your chats with another computer\.|On your other computer, choose Pair a computer, then Enter a code from the other computer, and type [0-9-]+ \(dashes optional\)\. This code lasts 10 minutes\.|Waiting for approval on your other computer\. It should show: .+\.|Paired with .+\. Your chats can continue on both computers\.|Paired\. Your chats are kept\. Both computers' chats will appear as sharing finishes\.|Cancelled\. Nothing was paired\.|Pair with .+\?|Pair only if your other computer shows the same (three words|check number): .+\.|Pair|Don't pair|Open this pairing link on a computer you already use\.|Compare the check number on both computers: .+\.|Waiting for approval\. This link lasts [0-9]+ minutes\.)$`) rec.mu.Lock() defer rec.mu.Unlock() for i, name := range rec.events { @@ -321,12 +321,38 @@ func TestAppPairEP4(t *testing.T) { if failed.Error == nil || failed.Error.Message != "That code did not work, or it timed out. Ask for a new one." { t.Fatalf("failure: %+v", failed) } + if slices.Contains(failed.Lines, failed.Error.Message) { + t.Fatalf("B2: failure sentence shown twice: %+v", failed) + } if got := appDeviceList(t, wa); len(got.Devices) != 1 { t.Fatal(got) } machineCall[appwire.PairResult](t, wa, "pair.cancel", appwire.IDParams{ID: offer.ID}) } +// B3a: a finished pairing keeps no check number on screen. +func TestAppPairFinishClearsCheckB3(t *testing.T) { + celltest.Available(t) + relay := e2Relay(t) + t.Setenv(syncsetup.URLVar, relay.URL) + _, rec, w := appDeviceBackend(t, t.TempDir()) + request := machineCall[appwire.PairResult](t, w, "pair.start", appwire.PairStartParams{Mode: "request_link", Name: "B"}).Pairing + if waiting := e2PairWait(t, w, request.ID, "waiting"); waiting.Words == "" { + t.Fatalf("no check number while waiting: %+v", waiting) + } + got := machineCall[appwire.PairResult](t, w, "pair.cancel", appwire.IDParams{ID: request.ID}).Pairing + if got.State != "cancelled" || got.Words != "" { + t.Fatalf("B3a: finished pairing kept its check number: %+v", got) + } + rec.mu.Lock() + defer rec.mu.Unlock() + for i, event := range rec.events { + if p := rec.data[i]; event == appwire.EventPairChanged && p.(appwire.PairResult).Pairing.State == "cancelled" && p.(appwire.PairResult).Pairing.Words != "" { + t.Fatalf("B3a: final event kept its check number: %+v", p) + } + } +} + // E-P1's link counterparts: the advertisement includes all four modes. func TestAppPairLinksEP1(t *testing.T) { celltest.Available(t) diff --git a/cmd/codeaf/app_take_recovery_test.go b/cmd/codeaf/app_take_recovery_test.go index 033366ca28..39a832c499 100644 --- a/cmd/codeaf/app_take_recovery_test.go +++ b/cmd/codeaf/app_take_recovery_test.go @@ -94,12 +94,20 @@ func TestAppTakeInstallFailureS5(t *testing.T) { if len(rows) != 1 { t.Fatalf("S5 rows before take: %+v", rows) } + logged := faultLog(t) armed.Store(true) _, failure := wire.call("agents.take", appwire.TakeParams{Cell: c.ID, Seen: rows[0].Seen}) _ = os.Chmod(parent, 0700) if !injected.Load() || failure == nil || failure.Message != appTakeFailed { t.Fatalf("S5 injection=%v failure=%+v", injected.Load(), failure) } + // B5a: the reason travels with the failure and into the continue log line. + if reason, _ := failure.Detail.(string); reason == "" || reason == appTakeFailed { + t.Fatalf("B5a: failed take carries no reason: %+v", failure) + } + if !strings.Contains(logged(), "outcome=failed reason=") { + t.Fatalf("B5a: continue log names no reason:\n%s", logged()) + } after, err := owner.Dir.Cell(t.Context(), c.ID) if err != nil { t.Fatal(err) diff --git a/cmd/codeaf/appdevices.go b/cmd/codeaf/appdevices.go index b70e35fcfd..9653f2c647 100644 --- a/cmd/codeaf/appdevices.go +++ b/cmd/codeaf/appdevices.go @@ -81,7 +81,7 @@ func (d *appDevices) publish(emit appwire.Emitter) { if err != nil || view.Mode == appwire.DevicesOn && !complete { return } - encoded, err := json.Marshal(appwire.DevicesResult{Devices: view}) + encoded, err := json.Marshal(appwire.DevicesResult{Devices: devicesCompared(view)}) if err != nil || string(encoded) == d.lastWire { return } @@ -89,6 +89,20 @@ func (d *appDevices) publish(emit appwire.Emitter) { emit.Emit(appwire.EventDevicesChanged, appwire.DevicesResult{Devices: view}) } +// devicesCompared is the view a change is judged by. A hosted relay stamps the +// asking computer's last seen on every request, so this computer's own stamp, +// and that of any computer online now, moves on each roster read; publishing +// it made the app read again, which stamped again (B8b). Neither is shown. +func devicesCompared(view appwire.Devices) appwire.Devices { + view.Devices = slices.Clone(view.Devices) + for i, row := range view.Devices { + if row.Self || row.Online != nil && *row.Online { + view.Devices[i].LastSeen = time.Time{} + } + } + return view +} + func (d *appDevices) close() { d.cancel() <-d.owned diff --git a/cmd/codeaf/appdevices_binding_test.go b/cmd/codeaf/appdevices_binding_test.go index 8f371426bb..c34f9fe2fe 100644 --- a/cmd/codeaf/appdevices_binding_test.go +++ b/cmd/codeaf/appdevices_binding_test.go @@ -233,3 +233,14 @@ func TestAppDevicesV1RelayChangeDropsOldRoster(t *testing.T) { return len(v.Devices) == 2 && v.Devices[0].Name == "new relay desk" && v.Devices[1].ID == partner.Device.ID() && !v.Devices[1].Removed }) } + +// B11: a call refused because this computer was removed says so in words, with +// the app's own remedy, never the wire's "wireauth: device revoked". Any call +// lands here, the roster included, so it does not speak of "this chat". +func TestB11RevokedReadSaysTheComputerWasRemoved(t *testing.T) { + err := appEngineError(errors.Join(errors.New("devices"), directory.ErrRevoked)) + var wire *appwire.Error + if !errors.As(err, &wire) || wire.Message != appRemoved || strings.Contains(wire.Message, "wireauth") || strings.Contains(wire.Message, "this chat") || !strings.Contains(wire.Message, "Pair a computer") { + t.Fatalf("revoked read = %v, want %q", err, appRemoved) + } +} diff --git a/cmd/codeaf/appdevices_lastseen_test.go b/cmd/codeaf/appdevices_lastseen_test.go new file mode 100644 index 0000000000..1a7ede86b1 --- /dev/null +++ b/cmd/codeaf/appdevices_lastseen_test.go @@ -0,0 +1,45 @@ +package main + +import ( + "testing" + "time" + + "github.com/Agent-Field/codeaf/internal/appwire" +) + +// B8b: a hosted relay stamps the asking computer's last seen on every request. +// That stamp, and an online computer's, must not count as a roster change, or +// each roster read publishes devices.changed and the app reads again. +func TestAppDevicesB8LastSeenStampIsNotAChange(t *testing.T) { + on, off := true, false + at := time.Date(2026, 11, 2, 12, 0, 0, 0, time.UTC) + roster := func(later time.Duration) appwire.Devices { + return appwire.Devices{Mode: appwire.DevicesOn, Devices: []appwire.Device{ + {ID: "self", Name: "Desk", Self: true, Online: &on, LastSeen: at.Add(later)}, + {ID: "online", Name: "Home", Online: &on, LastSeen: at.Add(later)}, + {ID: "offline", Name: "Office", Online: &off, LastSeen: at}, + }} + } + first, read := devicesCompared(roster(0)), devicesCompared(roster(5*time.Second)) + if !devicesSame(t, first, read) { + t.Fatal("B8b: a read that only moved this computer's and an online computer's last seen counts as a change") + } + if roster(5 * time.Second).Devices[0].LastSeen.IsZero() { + t.Fatal("devicesCompared changed the published view") + } + moved := roster(0) + moved.Devices[2].LastSeen = at.Add(time.Minute) + if devicesSame(t, first, devicesCompared(moved)) { + t.Fatal("an offline computer's new last seen is shown, so it must still count as a change") + } + flipped := roster(0) + flipped.Devices[1].Online = &off + if devicesSame(t, first, devicesCompared(flipped)) { + t.Fatal("a computer going offline must still count as a change") + } +} + +func devicesSame(t *testing.T, a, b appwire.Devices) bool { + t.Helper() + return string(mustJSON(t, a)) == string(mustJSON(t, b)) +} diff --git a/cmd/codeaf/approvals.go b/cmd/codeaf/approvals.go index 00baeb1eb0..9a8c4bba25 100644 --- a/cmd/codeaf/approvals.go +++ b/cmd/codeaf/approvals.go @@ -72,7 +72,12 @@ func (d *approvalsDoor) who() (pair.Approver, error) { } who, err := d.approver(route) if err == nil { - who.BeforeApprove = func(ctx context.Context) error { return ownDeviceRecord(ctx, d.dir, route) } + // The record gets its own wait, so a slow first write cannot spend the approve's. + who.BeforeApprove = func(ctx context.Context) error { + ctx, cancel := context.WithTimeout(ctx, approveStep) + defer cancel() + return ownDeviceRecord(ctx, d.dir, route) + } } return who, err } @@ -96,6 +101,13 @@ func (d *approvalsDoor) Pending(ctx context.Context, typed string) (tui3.Pending return tui3.PendingDevice{}, err } as, err := who.Look(ctx, ref) + if errors.Is(err, pair.ErrLinkDecided) && approvedHere(who, ref.Code, "") == requestApprovedHere { + // An approval whose reply was lost: the device is in, so this computer is shared. + if err := identity.EndSolo(d.dir); err != nil { + return tui3.PendingDevice{}, err + } + return tui3.PendingDevice{}, tui3.ErrAlreadyPaired + } if err != nil { return tui3.PendingDevice{}, told(err) } @@ -129,12 +141,69 @@ func (d *approvalsDoor) recall(p tui3.PendingDevice) (pair.Asking, error) { func (d *approvalsDoor) Approve(ctx context.Context, p tui3.PendingDevice) error { return d.answer(p, func(who pair.Approver, as pair.Asking) error { if err := who.Approve(ctx, as); err != nil { - return err + if err = settleApproval(who, as, err); err != nil { + return err + } } return identity.EndSolo(d.dir) }) } +// approveStep bounds each wait of an approval on its own: the device record, +// and the lookup after an answer whose reply was lost. +const approveStep = 8 * time.Second + +// settleApproval reads the request again after an approve that ran out of time +// or was refused as already answered. The relay may have let the device in +// while the reply was lost, or an earlier attempt may have, so an approved +// request for this device is success. A request still waiting says plainly that +// nothing arrived; anything else keeps the error it came with. +func settleApproval(who pair.Approver, as pair.Asking, err error) error { + if !errors.Is(err, context.DeadlineExceeded) && !errors.Is(err, pair.ErrLinkDecided) { + return err + } + switch approvedHere(who, as.Ref.Code, as.DeviceID()) { + case requestApprovedHere: + return nil + case requestWaiting: + if errors.Is(err, context.DeadlineExceeded) { + return tui3.ErrAnswerNotArrived + } + } + return err +} + +type requestOutcome int + +const ( + requestUnknown requestOutcome = iota + requestWaiting + requestApprovedHere +) + +// approvedHere reads where a request stands. Approved here means the relay +// approved it for this very device AND that device is in this computer's own +// fleet, so a link some other fleet answered never counts as this one sharing. +// An empty device is whichever device the request names. +func approvedHere(who pair.Approver, code, device string) requestOutcome { + ctx, cancel := context.WithTimeout(context.Background(), approveStep) + defer cancel() + r, err := who.Requests.GetRequest(ctx, code, 0) + switch { + case err != nil: + return requestUnknown + case r.State == directory.RequestPending: + return requestWaiting + case r.State != directory.RequestApproved || device != "" && r.Device != device: + return requestUnknown + } + listing, err := who.Dir.List(ctx) + if dev, ok := listing.Devices[r.Device]; err != nil || !ok || dev.Revoked { + return requestUnknown + } + return requestApprovedHere +} + func (d *approvalsDoor) Deny(ctx context.Context, p tui3.PendingDevice) error { return d.answer(p, func(who pair.Approver, as pair.Asking) error { return who.Deny(ctx, as) }) } diff --git a/cmd/codeaf/approvals_test.go b/cmd/codeaf/approvals_test.go index f486ad4097..96dfdbf77e 100644 --- a/cmd/codeaf/approvals_test.go +++ b/cmd/codeaf/approvals_test.go @@ -2,7 +2,10 @@ package main import ( "context" + "errors" "net/http" + "os" + "path/filepath" "regexp" "runtime" "testing" @@ -162,3 +165,134 @@ func TestApprovalsDoorTellsAStoppedComputerItWasStopped(t *testing.T) { t.Fatalf("the stopped computer's revoke said %v, want %q", err, chatlist.RemovedChat) } } + +// lostReply is a slow relay seen from the approving computer: with decide it +// lets the device in but the reply never comes back; without, the approval +// never reaches it at all. +type lostReply struct { + directory.Client + decide bool +} + +func (l lostReply) ApproveRequest(ctx context.Context, code string, a directory.Approval) error { + if l.decide { + if err := l.Client.ApproveRequest(ctx, code, a); err != nil { + return err + } + } + return context.DeadlineExceeded +} + +// withLostReply points the door's approver at a relay that loses replies. +func withLostReply(door *approvalsDoor, home string, decide bool) { + door.approver = func(route pair.Mailbox) (pair.Approver, error) { + who, err := linkApprover(home)(route) + who.Dir = lostReply{who.Dir, decide} + return who, err + } +} + +// pendingAt opens the new device's link on the first computer. +func (r *pairRig) pendingAt(t *testing.T, door *approvalsDoor) (string, tui3.PendingDevice, chan error) { + t.Helper() + screen, done := r.asking(t) + link := screen.waitFor(t, regexp.QuoteMeta(r.url)+`/p/\S+#\S+`)[0] + r.enrol(t, door, "this computer") + // A first computer that has never shared its identity is solo. + if err := os.WriteFile(filepath.Join(r.homeA, "identity-solo"), nil, 0o600); err != nil { + t.Fatal(err) + } + p, err := door.Pending(context.Background(), link) + if err != nil { + t.Fatal(err) + } + return link, p, done +} + +// T1: the relay lets the device in but the reply is lost. The door looks the +// request up, so the approval counts, the first computer stops being solo and +// both computers end up paired. +func TestApprovalsDoorApprovalWhoseReplyWasLostStillPairs(t *testing.T) { + rig := newPairRig(t) + door := rig.approvalsAt(rig.homeA) + _, p, done := rig.pendingAt(t, door) + if !identity.Solo(rig.homeA) { + t.Fatal("the first computer did not start solo") + } + withLostReply(door, rig.homeA, true) + if err := door.Approve(context.Background(), p); err != nil { + t.Fatalf("approve = %v, want the lost reply settled as approved", err) + } + if err := <-done; err != nil { + t.Fatalf("the new device did not join: %v", err) + } + if identity.Solo(rig.homeA) { + t.Fatal("the first computer stays solo, so it never shares") + } + rows, err := door.Devices(context.Background()) + if err != nil || len(rows) != 2 { + t.Fatalf("devices = %+v, %v; want both computers", rows, err) + } +} + +// An approval that never reached the relay is said as a plain retry, and the +// retry lets the device in. +func TestApprovalsDoorApprovalThatNeverArrivedCanBeRetried(t *testing.T) { + rig := newPairRig(t) + door := rig.approvalsAt(rig.homeA) + _, p, done := rig.pendingAt(t, door) + withLostReply(door, rig.homeA, false) + if err := door.Approve(context.Background(), p); !errors.Is(err, tui3.ErrAnswerNotArrived) { + t.Fatalf("approve = %v, want ErrAnswerNotArrived", err) + } + if !identity.Solo(rig.homeA) { + t.Fatal("nothing was approved, yet the first computer stopped being solo") + } + door.approver = linkApprover(rig.homeA) + if err := door.Approve(context.Background(), p); err != nil { + t.Fatalf("retry = %v", err) + } + if err := <-done; err != nil { + t.Fatalf("the new device did not join: %v", err) + } + if identity.Solo(rig.homeA) { + t.Fatal("the first computer stays solo after the retry") + } +} + +// T3: an approval landed in an earlier attempt that never heard back (here, a +// screen that closed). Opening the same link again says the device is already +// paired and ends solo, instead of asking to approve again. +func TestApprovalsDoorReopenedLinkOfALostApprovalEndsSolo(t *testing.T) { + rig := newPairRig(t) + door := rig.approvalsAt(rig.homeA) + link, p, done := rig.pendingAt(t, door) + earlier := rig.approvalsAt(rig.homeA) + if _, err := earlier.Pending(context.Background(), link); err != nil { + t.Fatal(err) + } + as, _ := earlier.recall(p) + who, err := earlier.who() + if err != nil { + t.Fatal(err) + } + if err := who.Approve(context.Background(), as); err != nil { + t.Fatal(err) + } + if err := <-done; err != nil { + t.Fatalf("the new device did not join: %v", err) + } + if !identity.Solo(rig.homeA) { + t.Fatal("setup: the earlier attempt should have left the marker") + } + if _, err := door.Pending(context.Background(), link); !errors.Is(err, tui3.ErrAlreadyPaired) { + t.Fatalf("reopened link = %v, want ErrAlreadyPaired", err) + } + if identity.Solo(rig.homeA) { + t.Fatal("the first computer stays solo after the reopened link") + } + // A retried answer on the old card settles the same way. + if err := door.Approve(context.Background(), p); err != nil { + t.Fatalf("retried approve = %v, want it settled as approved", err) + } +} diff --git a/cmd/codeaf/pair.go b/cmd/codeaf/pair.go index a7b00550d4..b883b3521b 100644 --- a/cmd/codeaf/pair.go +++ b/cmd/codeaf/pair.go @@ -80,6 +80,24 @@ func ownDeviceRecord(ctx context.Context, dir string, route pair.Mailbox) error return syncsetup.PutOwnDevice(ctx, client, id, dev, devname.Name(dir)) } +// ownDeviceRevoked says the directory refuses this computer's own device: it +// was removed, and pairing again must give it a new device key. +func ownDeviceRevoked(ctx context.Context, dir string, route pair.Mailbox) bool { + if identity.Solo(dir) { + return false + } + client, _, err := homeDirectory(dir, route) + if err != nil { + return false + } + dev, err := identity.Device(dir) + if err != nil { + return false + } + listing, err := client.List(ctx) + return errors.Is(err, directory.ErrRevoked) || err == nil && listing.Devices[dev.ID()].Revoked +} + // recordOwnDeviceOrSay is what a finished pairing does about its record: the // pairing itself succeeded, so a record that did not land is one line on the // screen and a clean exit — the next chat start writes the record anyway. diff --git a/cmd/codeaf/pair_terminal.go b/cmd/codeaf/pair_terminal.go index 62ae02b2be..54f57fc019 100644 --- a/cmd/codeaf/pair_terminal.go +++ b/cmd/codeaf/pair_terminal.go @@ -114,11 +114,15 @@ func saidYes(typed string) bool { return false } +// pairApproveKeys sits on its own line under the check number, so the typed +// answer does not run into the question. +const pairApproveKeys = "a approve · d decline: " + // approveLink uses the same deliberate approval key as the chat card. Code // pairing keeps its own yes/no question because its comparison is different. func (t *terminal) approveLink(ctx context.Context, question string) bool { t.stopProgress() - fmt.Fprint(t.out, question+" a / d ") + fmt.Fprint(t.out, question+"\n"+pairApproveKeys) typed, ok := t.lines.next(ctx) if !ok { fmt.Fprintln(t.out) diff --git a/cmd/codeaf/pairlink_test.go b/cmd/codeaf/pairlink_test.go index 944df50fa2..0df9516bbe 100644 --- a/cmd/codeaf/pairlink_test.go +++ b/cmd/codeaf/pairlink_test.go @@ -51,7 +51,7 @@ func TestPairByLinkEndToEnd(t *testing.T) { t.Fatal(err) } // The asking computer names its own operating system, so on a Mac it says Mac. - want := pair.WantsToJoinLine("laptop", runtime.GOOS) + "\n" + pair.CheckQuestion(check) + " a / d " + want := pair.WantsToJoinLine("laptop", runtime.GOOS) + "\n" + pair.CheckQuestion(check) + "\n" + pairApproveKeys if !strings.HasPrefix(out.String(), want) || !strings.Contains(out.String(), pair.ApprovedLine("laptop")) { t.Fatalf("the approving terminal printed:\n%s\nwant it to start:\n%s", out.String(), want) } diff --git a/cmd/codeaf/review_fixes_test.go b/cmd/codeaf/review_fixes_test.go index cb29f90220..74db581edb 100644 --- a/cmd/codeaf/review_fixes_test.go +++ b/cmd/codeaf/review_fixes_test.go @@ -257,6 +257,7 @@ func TestR1_6CompletedChatsAreNotRetried(t *testing.T) { }) }) t.Setenv(syncsetup.URLVar, relay.URL) + logged := faultLog(t) b := &appBackend{left: make(chan struct{}), emit: newAppRecorder()} round := r1PublicationRounds(t, b) appSyncWait(t, "initial publication", func() bool { return goodReads.Load() > 0 && badReads.Load() > 0 }) @@ -270,6 +271,9 @@ func TestR1_6CompletedChatsAreNotRetried(t *testing.T) { if got := badReads.Load(); got < 2 || got > 3 { t.Errorf("failed chat attempts over three intervals = %d, want 2..3 with backoff", got) } + if got := strings.Count(logged(), "sharing chat "+bad.ID+": "); got != 1 || !strings.Contains(logged(), "503") { + t.Errorf("T7: the failing chat's reason was logged %d times, want once with its cause:\n%s", got, logged()) + } } func TestR1_6SlowFailureWaitsAfterCompletion(t *testing.T) { diff --git a/docs/changes/unreleased/2285-pairing-qa-fixes.md b/docs/changes/unreleased/2285-pairing-qa-fixes.md new file mode 100644 index 0000000000..3f64ad72b4 --- /dev/null +++ b/docs/changes/unreleased/2285-pairing-qa-fixes.md @@ -0,0 +1,18 @@ +--- +kind: fixed +title: Pairing QA fixes for failures, approvals, removed computers and the devices roster +pr: 2285 +surface: [chat, engine, app, docs] +invalidates: + - "A failed app pairing put its failure sentence in both the view's words and its error, so the app showed it twice, and a finished pairing kept showing its check number; now the failure is said once, in the error, and a finished pairing clears its words." + - "A terminal approval that timed out said no answer from your devices, try again, even when the answer had been sent; now it says sync did not answer in time, and when an approval was already sent it adds that the answer may still have arrived and to check /devices before answering again." + - "An approval from the terminal chat screen whose reply was lost left the joining computer unpaired and the link still asking; now that screen settles the approval it already gave, and opening there a link this computer already approved says the device is already paired instead of failing. Approvals from the desktop app and codeaf pair approve are unchanged." + - "The terminal pairing question ended with a / d on the same line; now the keys are printed on their own line as a approve · d decline." + - "A removed computer's chat said run codeaf pair to bring it back, and the app showed wireauth: device revoked; now the chat says the computer was removed and to run codeaf pair --replace, and the app says it was removed and to choose Pair a computer. The earlier sentence is still recognised as a removal notice." + - "A removed computer could not pair back from the app because the join was treated as already joined; now a removed computer's own pairing from the app replaces its device key instead of reusing the revoked one. Two-computer testing still showed neither computer listing the other afterwards, so re-adding a removed computer is not finished." + - "Every devices roster read published devices.changed because this computer's own last-seen stamp moved, so the app re-read the roster in a loop; now a read whose only difference is last-seen stamps of this computer or online rows publishes nothing." + - "A chat that could not be shared, or a continue that failed, left no reason in the log; now sharing logs each chat and reason once, and a failed continue logs reason=." +--- + +The pairing code instruction now says dashes are optional, and the devices +manual page is corrected for how a removed computer rejoins. diff --git a/internal/chatlist/copy.go b/internal/chatlist/copy.go index 4f816b5a80..aed6142dee 100644 --- a/internal/chatlist/copy.go +++ b/internal/chatlist/copy.go @@ -35,7 +35,7 @@ const ( // cellsync table lists them). None names a code or a number of requests: // each says what is true for the person and what, if anything, to do. relayFull = "your sync space is full%s, so new turns stay on this computer; free space there and reopen this chat" - Removed = removedChatStart + "run codeaf pair to bring it back" + Removed = removedChatStart + "run codeaf pair --replace to bring it back" SlowDown = "sync is asking this computer to slow down; new turns stay here and go up as soon as it allows" tooManyNewIn = "too many new identities from this network today — try again in about %s" TooManyNew = "this network has started too many new identities today; sync begins when it allows more" @@ -387,7 +387,7 @@ func Moved(from string, elapsed time.Duration, hadRunning bool) string { // IsRemovedNotice accepts the wire spelling from earlier engines as well as // each current reading place, so removal keeps its recovery behavior. func IsRemovedNotice(text string) bool { - return text == Removed || text == RemovedChat || text == LegacyRemoved + return text == Removed || text == RemovedChat || text == LegacyRemoved || text == earlierRemoved } // LegacySyncNotice preserves the vocabulary an older surface recognizes. @@ -412,4 +412,6 @@ const ( RemovedChat = removedChatStart + "/pair brings it back" // LegacyRemoved is retained for old surfaces that recognize terminal guidance by text. LegacyRemoved = removedChatStart + "run `codeaf pair` to bring it back" + // earlierRemoved is Removed as engines before --replace wrote it into notices. + earlierRemoved = removedChatStart + "run codeaf pair to bring it back" ) diff --git a/internal/chatlist/offer_test.go b/internal/chatlist/offer_test.go index d73a42e3f5..416f6197f3 100644 --- a/internal/chatlist/offer_test.go +++ b/internal/chatlist/offer_test.go @@ -88,7 +88,7 @@ func TestFrozenSentences(t *testing.T) { ClockOff: "this computer's clock is off by more than 5 minutes", ReplacedGone: "your identity was replaced and sync has deleted the old one; pair this computer again (/pair on a computer that has the new one)", Replaced: "your chats are moving to a new identity; when that is done, pair this computer again (/pair on the computer that moved them)", - Removed: "this device was removed by another of your devices, so this chat stays here only — run codeaf pair to bring it back", + Removed: "this device was removed by another of your devices, so this chat stays here only — run codeaf pair --replace to bring it back", SlowDown: "sync is asking this computer to slow down; new turns stay here and go up as soon as it allows", TooManyNew: "this network has started too many new identities today; sync begins when it allows more", } { @@ -155,3 +155,10 @@ func TestTooManyNewNamesWhenTheDayFreesUp(t *testing.T) { } } } + +// B11: a notice an earlier engine stored with the old remedy is still a removal. +func TestEarlierRemovedNoticeIsStillARemoval(t *testing.T) { + if !IsRemovedNotice(earlierRemoved) || !IsRemovedNotice(Removed) || earlierRemoved == Removed { + t.Fatal("removal notices from before --replace are no longer recognised") + } +} diff --git a/internal/manual/chat/add-or-remove-a-computer.md b/internal/manual/chat/add-or-remove-a-computer.md index d9932270f9..2e14d47e01 100644 --- a/internal/manual/chat/add-or-remove-a-computer.md +++ b/internal/manual/chat/add-or-remove-a-computer.md @@ -46,7 +46,7 @@ The card goes away once you have two devices. After that, `/pair` or Settings ## Do older chats move after pairing? -Older chats move through an independent copy. Once paired, codeaf prepares chats active in the last 30 days in the background, one at a time; opening another older chat prepares its copy then. This build shows and writes to the copy. The original files stay untouched and remain usable in older codeaf builds. +Older chats move through an independent copy. Once paired, the terminal chat prepares chats active in the last 30 days in the background while it is open, one at a time; the desktop app does not. Opening another older chat prepares its copy then. This build shows and writes to the copy. The original files stay untouched and remain usable in older codeaf builds. ## Pairing a server with no browser — the link, token and check number @@ -88,7 +88,7 @@ usage: codeaf pair [--name ] | codeaf pair approve | codeaf ``` - `codeaf pair` with nothing after it is for the NEW device. It asks to join and shows a link. -- `codeaf pair approve ` is for a device that is already in. It shows who is asking and asks `a / d`. Only `a` approves; `d`, Esc, Enter and anything else decline. Pasting the link into the chat, or typing `/pair `, does the same thing on a screen. +- `codeaf pair approve ` is for a device that is already in. It shows who is asking and the check number, then asks on its own line: `a approve · d decline:`. Only `a` approves; `d`, Esc, Enter and anything else decline. Pasting the link into the chat, or typing `/pair `, does the same thing on a screen. - `--name ` is what your devices call this computer. See the page *Naming a device*. - `--via
` names the address of the sync service to go through when it is not the one this device already uses. Pass it to both `codeaf pair` and `codeaf pair approve`. The new device prints the full approve line with `--via` in it when one is needed. @@ -102,7 +102,7 @@ Moving chats and pairing are available by default on Linux, macOS and native Win The new device shows a check number of four digits. The device that approves shows the same four digits. Look at both screens. Approve only when they match. If they do not match, decline: somebody else may have used the link. -On the approving screen the number reads `Check number 4821. Approve only if the new device shows the same number.` In a terminal the question reads the same: `a / d`. Only `a` approves; `d`, Esc, Enter and anything else decline. Code pairing keeps its `y / n` question. +On the approving screen the number reads `Check number 4821. Approve only if the new device shows the same number.` In a terminal the question reads the same, with the keys on the line below: `a approve · d decline:`. Only `a` approves; `d`, Esc, Enter and anything else decline. Code pairing keeps its `y / n` question. ## Approve a new device — the approve screen, a and d, and decline @@ -124,7 +124,7 @@ a approve · d decline · esc later The line under the name says what system the new device runs (Mac, Linux, Windows, iPhone or iPad, Android, or unknown system), when it asked, and how many minutes are left. A request that ran out says `run out`, and the screen says `that request has run out — ask the new device for a new link`. -While your answer is on its way the screen says `approving…` or `declining…` and offers no keys but `esc close`. If your other devices do not answer in time it says `no answer from your devices — try again`, and `a` and `d` work again. When the request has run out, `a` and `d` are not offered at all. +While your answer is on its way the screen says `approving…` or `declining…` and offers no keys but `esc close`. If sync does not answer in time, codeaf looks the request up again. When your approval did arrive, the screen closes as approved. When it did not, it says `sync did not answer in time — try again`, and `a` and `d` work again. When codeaf cannot tell whether an approval arrived, it says `sync did not answer in time — your answer may still have arrived; check /devices before answering again`. A decline that times out says `sync did not answer in time — try again`, because a declined device never appears in `/devices`. Opening a link whose device you already let in says `that device is already paired — there is nothing left to approve`. If looking up the request times out it says `sync did not answer in time — try again`. When the request has run out, `a` and `d` are not offered at all. When you approve, your screen says: @@ -239,7 +239,7 @@ this device was removed by another of your devices, so this chat stays here only ## Can a removed computer rejoin — /devices, pairing again and retained local chats -`/devices` on a removed computer says the same line in place of the list. A terminal refusal uses `this device was removed by another of your devices, so this chat stays here only — run codeaf pair to bring it back`. Home says `removed by another of your devices — pair again to sync`. The terminal command `codeaf devices` gives its removal explanation once, without following it with `nothing is paired yet`. It appears the moment the other device removes this one, even if codeaf has been open for days, in the open chat and on the home screen alike, and once only. Your chats on this device are safe and stay on it. They no longer follow your other devices. To bring the device back, run `codeaf pair` here, then approve the link it shows from a device that is still in (the steps above). If this computer has chats with submitted turns, run `codeaf pair --replace` instead. Its existing chats stay on this computer but no longer sync; shared memories and settings are cleared. Approve the new link from a computer still in your devices. A computer that has only opened codeaf, even with a provider connected, and whose chats contain only startup headers needs no `--replace`; saved knowledge, automations, tracked work and unknown local files still require the explicit replacement road. An active, already-paired computer is refused with the names of its other devices; `codeaf devices` lists them. +`/devices` on a removed computer says the same line in place of the list. A terminal refusal uses `this device was removed by another of your devices, so this chat stays here only — run codeaf pair --replace to bring it back`. Home says `removed by another of your devices — pair again to sync`. The terminal command `codeaf devices` gives its removal explanation once, without following it with `nothing is paired yet`. It appears the moment the other device removes this one, even if codeaf has been open for days, in the open chat and on the home screen alike, and once only. Your chats on this device are safe and stay on it. They no longer follow your other devices. To bring the device back, run `codeaf pair --replace` here, then approve the link it shows from a device that is still in (the steps above); plain `codeaf pair` does not bring a removed device back. The desktop app's Pair a computer does this on its own. Rejoining keeps the same identity, so only the device key is replaced: existing chats stay here and sync again, and shared memories and settings are not cleared. Approve the new link from a computer still in your devices. The exception for a fresh computer does not apply here: a computer that has only opened codeaf, even with a provider connected, and whose chats contain only startup headers needs no `--replace` when it joins your devices for the first time, but a removed computer already holds your identity, so it always needs `--replace`; saved knowledge, automations, tracked work and unknown local files require the explicit replacement road in either case. An active, already-paired computer is refused with the names of its other devices; `codeaf devices` lists them. ## Pair again in the chat — replace confirmation keeps local chats diff --git a/internal/manual/chat/desktop-sync.md b/internal/manual/chat/desktop-sync.md index fe84d06451..f508adb9f7 100644 --- a/internal/manual/chat/desktop-sync.md +++ b/internal/manual/chat/desktop-sync.md @@ -85,7 +85,7 @@ Compare the engine's three words or check number on both computers, then answer its explicit question. Nothing is approved by pressing Enter without a choice. Choose Pair or Don't pair. The code's instruction says: `On your other computer, choose Pair a computer, then Enter a code from the other computer, and type -. This code lasts 10 minutes.` The approval names the other computer and + (dashes optional). This code lasts 10 minutes.` The approval names the other computer and its platform, for example `Pair with Abir (Windows)?`, including when both computers share a name. Cancel takes back the waiting code or ends the request. A wrong code shows the diff --git a/internal/pair/join.go b/internal/pair/join.go index 649b0449a6..c968555766 100644 --- a/internal/pair/join.go +++ b/internal/pair/join.go @@ -34,6 +34,9 @@ type Joining struct { Platform string // Replace lets a computer that already has different chats give them up. Replace bool + // Revoked says this computer's own device was removed. Rejoining the same + // identity then mints a new device key, as Replace does; nothing is given up. + Revoked bool // ProfileDir names the preferences being joined, when outside Home. ProfileDir string // SaveSyncURL is told the relay the other device syncs through, when it is @@ -141,6 +144,11 @@ func adoptWithDevice(j Joining, grant Grant, dev *identity.Dev) (Joined, error) if err != nil && !errors.Is(err, identity.ErrNone) { return Joined{}, err } + // A removed computer holds the identity under a key the relay refuses, so + // "already joined" would leave it removed. Only a new device key returns it. + if err == nil && j.Revoked && held.ID() == grant.Identity.ID() { + j.Replace = true + } if err == nil && !j.Replace && held.ID() == grant.Identity.ID() { return Joined{Already: true}, nil } diff --git a/internal/syncsetup/copy.go b/internal/syncsetup/copy.go index b85ab84714..afa478fcd9 100644 --- a/internal/syncsetup/copy.go +++ b/internal/syncsetup/copy.go @@ -3,6 +3,7 @@ package syncsetup import ( "context" "errors" + "log" "os" "path/filepath" "time" @@ -88,6 +89,10 @@ func (s *Sync) PublishLocalChats(ctx context.Context, now func() time.Time, say if errors.Is(err, errNoChatHead) { continue } + // The reason goes to the log once per chat and reason; the person sees one sentence. + if pending.err == nil || pending.err.Error() != err.Error() { + log.Printf("sharing chat %s: %v", id, err) + } pending.err = err if pending.backoff == 0 { pending.backoff = DefaultInterval diff --git a/internal/syncsetup/take.go b/internal/syncsetup/take.go index 447047b98a..05cf29ef73 100644 --- a/internal/syncsetup/take.go +++ b/internal/syncsetup/take.go @@ -3,6 +3,7 @@ package syncsetup import ( "context" "errors" + "fmt" "github.com/Agent-Field/codeaf/internal/fsreplace" "log" "os" @@ -172,7 +173,7 @@ func (c *Continuer) take(ctx context.Context, id string, seen *directory.LeaseEx defer func() { outcome := "done" if err != nil { - outcome = "failed" + outcome = fmt.Sprintf("failed reason=%q", err.Error()) } log.Printf("continue %s phases capture=%s directory=%s retirement=%s fetch=%s acquire=%s restore=%s hooks=%s filing=%s unpublished=%s dirty=%s kept_upload=%s admission=%s total=%s outcome=%s", id, phases["local capture"], phases["directory"], phases["retirement"], phases["fetch"], phases["acquire"], phases["restore"], phases["hooks"], phases["filing"], phases["unpublished check"], phases["dirty check"], phases["kept-branch upload"], phases["staging/preflight"], time.Since(started), outcome) diff --git a/internal/tui3/approve.go b/internal/tui3/approve.go index 096df9a746..d0e9613c4a 100644 --- a/internal/tui3/approve.go +++ b/internal/tui3/approve.go @@ -86,14 +86,26 @@ const ( approveKeys = "a approve · d decline · esc later" approveLoading = "looking up the request…" approveGone = "that request has run out — ask the new device for a new link" - approveSlow = "no answer from your devices — try again" + approveSlow = "sync did not answer in time — try again" + approveUnsure = "sync did not answer in time — your answer may still have arrived; check /devices before answering again" approveWorking = "approving…" approveDeclining = "declining…" approveDenied = "%s was declined." approveTimeout = 8 * time.Second - joinedFleetWord = "%s is now paired. Your chats can continue there." + // approveAnswerTimeout covers an answer's steps: the device record, the + // relay's decision, and the lookup when that reply is lost. + approveAnswerTimeout = 20 * time.Second + joinedFleetWord = "%s is now paired. Your chats can continue there." ) +// ErrAnswerNotArrived is an answer the door looked for after a timeout and +// found the request still waiting: nothing was decided, so trying again is safe. +var ErrAnswerNotArrived = errors.New("the answer did not arrive") + +// ErrAlreadyPaired is a link whose request this fleet already approved, as +// when an approval's reply was lost: there is nothing left to answer. +var ErrAlreadyPaired = errors.New("that device is already paired — there is nothing left to approve") + // linkShape is what link pairing is typed as (contract section 2): a short // code with an optional key, or a codeaf URL from any channel's hosted origin // (plus the retired host). The six-digit code of the older pairing never matches. @@ -208,7 +220,7 @@ func (m pendingMsg) land(a *app) tea.Cmd { return nil } if m.err != nil { - m.card.line = approveFailure(m.err) + m.card.line = approveFailure(m.err, false) return nil } m.card.req, m.card.line = &m.req, "" @@ -222,7 +234,7 @@ func (m decidedMsg) land(a *app) tea.Cmd { m.card.working = false switch { case m.err != nil: - m.card.line = approveFailure(m.err) + m.card.line = approveFailure(m.err, m.approved) case m.approved: a.pair.close() a.addMachine.grew() @@ -237,8 +249,17 @@ func (m decidedMsg) land(a *app) tea.Cmd { // approveFailure is the one sentence for a refused or failed answer. The door // already says what happened in words a person can act on, so they are shown // as they are; only a silent wait, which has no sentence of its own, gets one. -func approveFailure(err error) string { +// A wait on an approval that was sent may still have been decided by sync, +// and /devices shows whether it was. A decline never shows there, so its wait +// is the plain retry, as is one the door found still waiting. +func approveFailure(err error, approved bool) string { + if errors.Is(err, ErrAnswerNotArrived) { + return approveSlow + } if errors.Is(err, context.DeadlineExceeded) { + if approved { + return approveUnsure + } return approveSlow } return err.Error() @@ -321,7 +342,7 @@ func (c *approveCard) key(a *app, name string) (tea.Cmd, bool) { func (a *app) decide(c *approveCard, approve bool) tea.Cmd { door, req := a.approvals, *c.req return func() tea.Msg { - ctx, cancel := context.WithTimeout(context.Background(), approveTimeout) + ctx, cancel := context.WithTimeout(context.Background(), approveAnswerTimeout) defer cancel() call := door.Deny if approve { diff --git a/internal/tui3/approve_test.go b/internal/tui3/approve_test.go index ce0e676a37..21f33ae8d0 100644 --- a/internal/tui3/approve_test.go +++ b/internal/tui3/approve_test.go @@ -3,6 +3,7 @@ package tui3 import ( "context" "errors" + "fmt" "strings" "sync" "testing" @@ -383,3 +384,52 @@ func TestOpenDeviceListMarksADeviceAnotherComputerRevoked(t *testing.T) { t.Fatalf("a revoked computer shows online:\n%s", plain(frame(a))) } } + +// T2: a timed-out answer blames neither the devices nor the person, and says the +// answer may already have landed, so a retry is not offered as the remedy. +func TestApproveTimeoutSaysTheAnswerMayHaveArrived(t *testing.T) { + late := fmt.Errorf("answer: %w", context.DeadlineExceeded) + if got := approveFailure(late, true); got != approveUnsure { + t.Fatalf("answer timeout = %q, want %q", got, approveUnsure) + } + if got := approveFailure(late, false); got != approveSlow { + t.Fatalf("lookup timeout = %q, want %q", got, approveSlow) + } + for _, line := range []string{approveSlow, approveUnsure} { + if strings.Contains(line, "your devices") { + t.Errorf("%q blames devices that take no part", line) + } + } + refused := errors.New("that request was already answered") + if got := approveFailure(refused, true); got != refused.Error() { + t.Fatalf("door sentence = %q, want it as it is", got) + } +} + +// T2: a decline that timed out is the plain retry. A declined device never +// shows in /devices, so pointing there would leave nothing to check. +func TestDeclineTimeoutIsAPlainRetry(t *testing.T) { + a, _ := approveApp(t, &fakeApprovals{}) + card := &approveCard{req: &PendingDevice{Name: "laptop"}, working: true} + a.showCard(card) + late := fmt.Errorf("deny: %w", context.DeadlineExceeded) + decidedMsg{card: card, req: *card.req, approved: false, err: late}.land(a) + if card.line != approveSlow { + t.Fatalf("decline timeout = %q, want %q", card.line, approveSlow) + } + decidedMsg{card: card, req: *card.req, approved: true, err: late}.land(a) + if card.line != approveUnsure { + t.Fatalf("approve timeout = %q, want %q", card.line, approveUnsure) + } +} + +// T1: an answer the door found still waiting after a timeout was not decided, +// so it is the plain retry, not "may still have arrived". +func TestApproveThatDidNotArriveIsAPlainRetry(t *testing.T) { + if got := approveFailure(ErrAnswerNotArrived, true); got != approveSlow { + t.Fatalf("not arrived = %q, want %q", got, approveSlow) + } + if approveAnswerTimeout < 2*approveTimeout { + t.Fatalf("an answer's %v cannot cover the record, the decision and the lookup", approveAnswerTimeout) + } +} diff --git a/internal/tui3/manual_devices_test.go b/internal/tui3/manual_devices_test.go index 381297d80d..7d04541d6d 100644 --- a/internal/tui3/manual_devices_test.go +++ b/internal/tui3/manual_devices_test.go @@ -23,7 +23,7 @@ func TestEverySentenceOfTheDeviceScreensIsOnItsPage(t *testing.T) { addMachineHeading, addMachinePitch, addMachineStepRun, addMachineStepPut, addMachineShow, approveAsk, fmt.Sprintf(approveCheckWord, "4821"), approveKeys, approveGone, fmt.Sprintf(approveDenied, "laptop"), fmt.Sprintf(joinedFleetWord, "laptop"), - strings.Join([]string{devicesMoveKey, devicesNameKey, devicesGoneKey, devicesCloseKey}, " · "), fmt.Sprintf(devicesRemoveAsk, "desk"), fmt.Sprintf(devicesRevoked, "desk"), "seen 3h ago", "offline", approveWorking, approveDeclining, approveSlow, + strings.Join([]string{devicesMoveKey, devicesNameKey, devicesGoneKey, devicesCloseKey}, " · "), fmt.Sprintf(devicesRemoveAsk, "desk"), fmt.Sprintf(devicesRevoked, "desk"), "seen 3h ago", "offline", approveWorking, approveDeclining, approveSlow, approveUnsure, ErrAlreadyPaired.Error(), deviceBring, deviceNothing, devicePickAsk, devicePickLeav, chatlist.Device{Name: "desk"}.Mark(false, tokens.Plain.Glyph(tokens.GQueued)), chatlist.RemovedChat, }, From 065da00556d274be49ad3accd546f63bd0fdfbf8 Mon Sep 17 00:00:00 2001 From: Santosh kumar <29346072+santoshkumarradha@users.noreply.github.com> Date: Tue, 3 Nov 2026 14:39:01 -0500 Subject: [PATCH 2/4] A person's pause survives a lost heartbeat reply (#2330) * test(factory): expose hold lost with heartbeat reply Keep a red regression at the real HTTP Hold/heartbeat/worker seam. Drop one heartbeat reply only after the factory drains its hold. The 30-second check has already started, so job completion cannot explain the failure. The runner remains unpaused after 249 successful subsequent heartbeats. Leave production code and TestAPauseAndResumeReachTheRunner unchanged. The lease forgets Hold at lease/signals.go:115 before serve/workers.go:523 writes its reply. A transport failure gives worker/worker.go:538 no signal, so Runner.Hold/setPaused and the paused log are never reached. Subsequent heartbeats have no hold to redeliver. Repeating Hold(true) is also a no-op because lease/doors.go:276 already sees the factory's paused flag. Validation: the new regression fails with GOMAXPROCS=1 and -race. The original -race -count=200 loop reproduced four missing-pause failures and one separate resume failure. A CPU-loaded throwaway characterization observed 81 consecutive lost holds, all still running without a paused log. Co-Authored-By: Claude Opus 5.5 * test(factory): capture pause worker and factory reports Co-Authored-By: Claude Opus 5.5 * test(factory): require heartbeat hold redelivery and resume precedence Co-Authored-By: Claude Opus 5.5 * fix(factory): redeliver wanted hold on every heartbeat Read the current pause intent on every live drain. Resume queues a fresh lease and suppresses an older uncollected hold; ordinary drains and the wire vocabulary remain compatible. Co-Authored-By: Claude Opus 5.5 * test(factory): expose pause log lost when cancelled drive finishes Co-Authored-By: Claude Opus 5.5 * test(factory): retain worker release snapshots for pause diagnosis Under CPU load the natural pause test failed eight times in sixty runs: the hold arrived, reports carried only the check log, and Done successfully released the lease. The factory remained paused and resumable despite the raw running state. Co-Authored-By: Claude Opus 5.5 * fix(factory): save pause before the drive can cancel its context The held drive exits through settleSpend, which cancels its context. Publishing c.paused before entering move allowed that exit to reject the pause log write. Change the control within the admitted store write so the log survives and the worker flushes it before releasing its lease. Co-Authored-By: Claude Opus 5.5 * test(factory): format pause regressions and document live hold delivery Co-Authored-By: Claude Opus 5.5 * test(factory): bound concurrent hold and spending answer regression Co-Authored-By: Claude Opus 5.5 * test(factory): schedule spending answer before item-locked hold Co-Authored-By: Claude Opus 5.5 * test(factory): use runner clock for lock scheduling Co-Authored-By: Claude Opus 5.5 * test(factory): require atomic saved holds at step boundaries Co-Authored-By: Claude Opus 5.5 * fix(factory): persist holds outside the loop lock before cancelling Serialize pause transitions with round admission. Store callbacks never acquire the loop lock, so spending answers cannot deadlock with holds. Failed writes leave the control and execution unchanged. Co-Authored-By: Claude Opus 5.5 * test(factory): hold only the first save in the step boundary regression Co-Authored-By: Claude Opus 5.5 * test(factory): capture the first hold retention hook caller Instrument the existing failing boundary test before changing the fixture. Co-Authored-By: Claude Opus 5.5 * test(factory): block the hold save after its preliminary read The first-hook goroutine stack showed Hold at loop.go:2149 calling Store.Get and retainOutput at store.go:219, before setPaused acquires pauseMu. Arm the fixture through the runner clock inside setPaused so the blocker intercepts the pause write. Preserve every boundary, persistence, cancellation and failed-save assertion; remove temporary stack logging. Co-Authored-By: Claude Opus 5.5 * docs: record the hold boundary fixture correction Co-Authored-By: Claude Opus 5.5 * docs: keep the boundary fixture note with the existing hold change The changelog gate rejects PR zero in the existing hold entry. Fold this related fixture note into that entry instead of introducing a second rejected fragment; no pull request number has been assigned. Co-Authored-By: Claude Opus 5.5 * docs: changelog keeps to what a person sees Co-Authored-By: Claude Opus 5.5 * changelog: name the pull request Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- .../unreleased/2330-factory-hold-heartbeat.md | 17 ++ internal/factory/lease/hold_heartbeat_test.go | 42 ++++ internal/factory/lease/signals.go | 18 +- internal/factory/run/hold_spend_lock_test.go | 212 ++++++++++++++++++ internal/factory/run/loop.go | 50 +++-- .../factory/worker/hold_heartbeat_test.go | 76 +++++++ internal/factory/worker/pause_finish_test.go | 60 +++++ internal/factory/worker/worker_test.go | 51 ++++- internal/manual/chat/factory.md | 4 +- 9 files changed, 499 insertions(+), 31 deletions(-) create mode 100644 docs/changes/unreleased/2330-factory-hold-heartbeat.md create mode 100644 internal/factory/lease/hold_heartbeat_test.go create mode 100644 internal/factory/run/hold_spend_lock_test.go create mode 100644 internal/factory/worker/hold_heartbeat_test.go create mode 100644 internal/factory/worker/pause_finish_test.go diff --git a/docs/changes/unreleased/2330-factory-hold-heartbeat.md b/docs/changes/unreleased/2330-factory-hold-heartbeat.md new file mode 100644 index 0000000000..e7a0ce367d --- /dev/null +++ b/docs/changes/unreleased/2330-factory-hold-heartbeat.md @@ -0,0 +1,17 @@ +--- +kind: fixed +title: Factory holds survive lost heartbeat replies and finishing drives +pr: 2330 +surface: [factory] +invalidates: + - A hold used to be forgotten when its heartbeat reply was lost; live heartbeats now repeat the factory's current hold until the worker releases the lease. + - A cancelled drive could end before its pause log was saved; the pause is now written before the control lets the drive end. +--- + +Resume supersedes an older uncollected hold and still takes a fresh worker slot +through the queue. The heartbeat uses the existing hold signal so older workers +remain compatible. Holding an already held runner stays idempotent. + +Concurrent holds and spending answers complete without a lock inversion. Pause +writes finish before cancellation or admission of another paid round; a failed +write leaves the live control unchanged. diff --git a/internal/factory/lease/hold_heartbeat_test.go b/internal/factory/lease/hold_heartbeat_test.go new file mode 100644 index 0000000000..e68b5a77fc --- /dev/null +++ b/internal/factory/lease/hold_heartbeat_test.go @@ -0,0 +1,42 @@ +package lease_test + +import ( + "testing" + + "github.com/Agent-Field/codeaf/internal/factory/lease" +) + +func TestHeartbeatRepeatsWantedHoldAndResumeSupersedesIt(t *testing.T) { + f := setup(t) + it, held := f.running(t) + doors := lease.NewDoors(f.desk) + if err := doors.Hold(it.ID, true); err != nil { + t.Fatal(err) + } + for n := 0; n < 3; n++ { + signals, err := f.desk.DrainLive(held.ID, held.Worker) + if err != nil || len(signals) != 1 || signals[0].Verb != lease.SignalHold || !signals[0].Yes { + t.Fatalf("heartbeat %d lost wanted hold: %+v, %v", n, signals, err) + } + } + if err := doors.Hold(it.ID, false); err != nil { + t.Fatal(err) + } + for n := 0; n < 3; n++ { + signals, err := f.desk.DrainLive(held.ID, held.Worker) + if err != nil || len(signals) != 0 { + t.Fatalf("resumed heartbeat replayed hold: %+v, %v", signals, err) + } + } + // Resume must supersede even a hold that was never collected. + if err := doors.Hold(it.ID, true); err != nil { + t.Fatal(err) + } + if err := doors.Hold(it.ID, false); err != nil { + t.Fatal(err) + } + signals, err := f.desk.DrainLive(held.ID, held.Worker) + if err != nil || len(signals) != 0 { + t.Fatalf("resume kept an older hold: %+v, %v", signals, err) + } +} diff --git a/internal/factory/lease/signals.go b/internal/factory/lease/signals.go index 70f50c7142..5c06a3cee2 100644 --- a/internal/factory/lease/signals.go +++ b/internal/factory/lease/signals.go @@ -5,6 +5,7 @@ import ( "time" "github.com/Agent-Field/codeaf/internal/factory" + "github.com/Agent-Field/codeaf/internal/factory/run" "github.com/Agent-Field/codeaf/internal/factory/store" ) @@ -68,6 +69,7 @@ func (d *Desk) Drain(id, worker string) ([]Signal, error) { // DrainLive carries interactive verbs through a heartbeat. Publishing stays // queued until the finishing run releases its lease and a publish pull takes it. +// Holds repeat the stored pause intent until the worker releases its lease. func (d *Desk) DrainLive(id, worker string) ([]Signal, error) { return d.drain(id, worker, false) } @@ -93,14 +95,28 @@ func (d *Desk) drain(id, worker string, publishing bool) ([]Signal, error) { } } var kept []Signal + holdSent := false for _, s := range doc.Signals[held.Job] { + if !publishing && s.Verb == SignalHold { + // Resume queues the next lease. A queued older hold must not undo it. + if !holdSent && run.StoredPaused(it) { + out = append(out, Signal{Verb: SignalHold, Yes: true}) + holdSent = true + } + continue + } if carriedOut(s.Verb) && !publishing { kept = append(kept, s) } else { out = append(out, s) } } - if len(out) == 0 { + // Hold is desired state, not a one-shot notification. Reuse the existing + // signal vocabulary so older workers also retry a lost heartbeat reply. + if !publishing && !holdSent && run.StoredPaused(it) { + out = append(out, Signal{Verb: SignalHold, Yes: true}) + } + if len(out) == 0 && len(doc.Signals[held.Job]) == len(kept) { return false, nil } for _, s := range out { diff --git a/internal/factory/run/hold_spend_lock_test.go b/internal/factory/run/hold_spend_lock_test.go new file mode 100644 index 0000000000..55e679f7ed --- /dev/null +++ b/internal/factory/run/hold_spend_lock_test.go @@ -0,0 +1,212 @@ +package run + +import ( + "context" + "os" + "os/exec" + "runtime" + "strings" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/Agent-Field/codeaf/internal/factory" + "github.com/Agent-Field/codeaf/internal/factory/store" +) + +// A child process bounds the failure even when both runner doors deadlock. +func TestHoldSpendAnswerLockOrder(t *testing.T) { + if os.Getenv("CODEAF_HOLD_LOCK_CHILD") != "1" { + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + cmd := exec.CommandContext(ctx, os.Args[0], "-test.run=^TestHoldSpendAnswerLockOrder$", "-test.timeout=8s") + cmd.Env = append(os.Environ(), "CODEAF_HOLD_LOCK_CHILD=1") + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("Hold and SpendAnswer must complete without deadlocking: %v\n%s", err, out) + } + return + } + + st, err := store.Open(t.TempDir()) + if err != nil { + t.Fatal(err) + } + id, err := st.Add(context.Background(), factory.Item{Title: "hold and spending answer", Repo: "acme/api", State: factory.StateNeedsYou, QKind: factory.QKindSpendStopped, Question: "spend?", Stream: &factory.Stream{}}) + if err != nil { + t.Fatal(err) + } + r := New(Options{Store: st}) + lp := r.loop() + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + lp.ctls[id] = &loopCtl{id: id, ctx: ctx, cancel: cancel, pending: factory.QKindSpendStopped, answers: make(chan loopAnswer, 1)} + rev, err := st.Revision(id) + if err != nil { + t.Fatal(err) + } + + // Stop Hold just before its store write, then queue SpendAnswer first + // on lp.mu. Hold must be able to finish that write without the loop lock. + atWrite, advance := make(chan struct{}), make(chan struct{}) + r.opts.Clock = func() time.Time { + close(atWrite) + <-advance + return time.Now() + } + holdDone := make(chan error, 1) + go func() { holdDone <- r.Hold(id, true) }() + select { + case <-atWrite: + case <-time.After(time.Second): + t.Fatal("Hold never reached its write") + } + lp.mu.Lock() + answerDone := make(chan error, 1) + go func() { + _, err := r.SpendAnswer(id, rev, true, nil) + answerDone <- err + }() + waitBlocked := func(door string) { + t.Helper() + deadline := time.Now().Add(time.Second) + for { + stack := make([]byte, 1<<20) + stack = stack[:runtime.Stack(stack, true)] + for _, goroutine := range strings.Split(string(stack), "\n\n") { + if strings.Contains(goroutine, door) && strings.Contains(goroutine, "sync.(*Mutex).Lock") { + return + } + } + if time.Now().After(deadline) { + t.Fatalf("%s never waited for lp.mu\n%s", door, stack) + } + time.Sleep(time.Millisecond) + } + } + waitBlocked("(*Runner).SpendAnswer") + // Put the mutex into starvation mode so its first waiter goes first. + time.Sleep(20 * time.Millisecond) + close(advance) + waitBlocked("(*Runner).Hold") + lp.mu.Unlock() + for _, done := range []chan error{holdDone, answerDone} { + select { + case err := <-done: + // A saved hold can legitimately make the answer's revision stale. + if err != nil { + if _, stale := err.(*store.StaleError); !stale { + t.Fatal(err) + } + } + case <-time.After(time.Second): + t.Fatal("Hold and SpendAnswer deadlocked") + } + } +} + +func TestHoldPersistsBeforeStepBoundary(t *testing.T) { + for _, failWrite := range []bool{false, true} { + name := "saved" + if failWrite { + name = "failed write" + } + t.Run(name, func(t *testing.T) { + st, err := store.Open(t.TempDir()) + if err != nil { + t.Fatal(err) + } + id, err := st.Add(context.Background(), factory.Item{Title: "pause boundary", Repo: "acme/api", State: factory.StateRunning, Stream: &factory.Stream{}}) + if err != nil { + t.Fatal(err) + } + r := New(Options{Store: st}) + lp := r.loop() + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + roundCtx, roundCancel := context.WithCancel(ctx) + defer roundCancel() + c := &loopCtl{id: id, ctx: ctx, cancel: cancel, roundCancel: roundCancel} + lp.ctls[id] = c + atSave, save := make(chan struct{}), make(chan struct{}) + var savingHold atomic.Bool + // Hold reads the item before setPaused takes pauseMu. Arm only + // when setPaused samples its clock, so that read cannot block here. + r.opts.Clock = func() time.Time { + savingHold.Store(true) + return time.Now() + } + var firstSave sync.Once + st.SetOutputPolicy(func() (int, int) { + if !savingHold.Load() { + return 0, 0 + } + // A later write in the same hold passes through; only the first is held. + firstSave.Do(func() { + close(atSave) + <-save + if failWrite { + // Refuse the final rename after the callback has run. + if err := os.Rename(st.ItemPath(id), st.ItemPath(id)+".before"); err != nil { + return + } + _ = os.Mkdir(st.ItemPath(id), 0700) + } + }) + return 0, 0 + }) + holdDone := make(chan error, 1) + go func() { holdDone <- r.Hold(id, true) }() + select { + case <-atSave: + case <-time.After(time.Second): + t.Fatal("Hold never reached save") + } + boundary := make(chan bool, 1) + go func() { boundary <- lp.held(c) }() + select { + case <-boundary: + t.Error("drive passed its boundary before the hold was saved") + case <-time.After(30 * time.Millisecond): + } + if roundCtx.Err() != nil || ctx.Err() != nil { + t.Error("hold cancelled execution before its log was saved") + } + close(save) + select { + case err := <-holdDone: + if (err != nil) != failWrite { + t.Fatalf("Hold error = %v, failWrite = %v", err, failWrite) + } + case <-time.After(time.Second): + t.Fatal("Hold did not finish") + } + if failWrite { + if err := os.Remove(st.ItemPath(id)); err != nil { + t.Fatal(err) + } + if err := os.Rename(st.ItemPath(id)+".before", st.ItemPath(id)); err != nil { + t.Fatal(err) + } + } + select { + case allowed := <-boundary: + if allowed != failWrite { + t.Errorf("step allowed = %v, failWrite = %v", allowed, failWrite) + } + case <-time.After(time.Second): + t.Fatal("step boundary did not settle") + } + it, err := st.Get(id) + if err != nil { + t.Fatal(err) + } + if c.paused == failWrite || it.Stream.Paused == failWrite || logHas(it, sayPaused) == failWrite { + t.Errorf("hold did not commit control, stream and log together: paused=%v stream=%+v", c.paused, it.Stream) + } + if (roundCtx.Err() != nil) == failWrite { + t.Errorf("round cancellation does not match saved hold: %v", roundCtx.Err()) + } + }) + } +} diff --git a/internal/factory/run/loop.go b/internal/factory/run/loop.go index ae2aab739d..f5f6d2b456 100644 --- a/internal/factory/run/loop.go +++ b/internal/factory/run/loop.go @@ -107,6 +107,9 @@ type loopCtl struct { // step; askMu keeps one question at a time. wmu sync.Mutex askMu sync.Mutex + // pauseMu keeps pause writes and paid round admission in order without + // taking floorLoop.mu inside a store callback. + pauseMu sync.Mutex // reverify says the control runs the checks again, holding no bench. reverify bool // hearMu keeps one reading of the manager conversation at a time @@ -1373,6 +1376,8 @@ func shortfall(res factory.StageResult) string { // held ends a paused drive so its bench and worker lease can serve another // job. The saved stream keeps the interrupted step for the ordinary resume path. func (lp *floorLoop) held(c *loopCtl) bool { + c.pauseMu.Lock() + defer c.pauseMu.Unlock() lp.mu.Lock() paused := c.paused lp.mu.Unlock() @@ -1385,15 +1390,18 @@ func (lp *floorLoop) held(c *loopCtl) bool { func (lp *floorLoop) round(c *loopCtl, exec Executor, job Job) (res factory.StageResult, again bool, err error) { rctx, cancel := context.WithCancel(c.ctx) defer cancel() + c.pauseMu.Lock() lp.mu.Lock() if c.paused { // Paused before it began: the chat it was to carry on waits for run. c.carry = job.Resume lp.mu.Unlock() + c.pauseMu.Unlock() return res, true, nil } c.roundCancel = cancel lp.mu.Unlock() + c.pauseMu.Unlock() // The manager can spend while a shell/check stage is running too. Every // check executor owns this context. Chat agents use their live family meter // so cancellation still returns the round's precise cost and chat identity. @@ -2164,28 +2172,17 @@ func (r *Runner) Hold(id int, on bool) error { return r.setPaused(c, func(bool) bool { return on }) } -// setPaused moves the control to the paused state want answers for the one -// it is in, under the loop's lock, and says the word on the stream and into -// the manager's conversation only when that changed anything. +// setPaused saves a pause before exposing it to the drive. Round admission +// waits for the write, so a failed save leaves execution unchanged. func (r *Runner) setPaused(c *loopCtl, want func(was bool) bool) error { lp := r.loop() - id := c.id - paused, changed := func() (bool, bool) { - lp.mu.Lock() - defer lp.mu.Unlock() - next := want(c.paused) - if next == c.paused { - return next, false - } - c.paused = next - if next { - if c.roundCancel != nil { - c.roundCancel() - } - } - return next, true - }() - if !changed { + c.pauseMu.Lock() + lp.mu.Lock() + paused := want(c.paused) + unchanged := paused == c.paused + lp.mu.Unlock() + if unchanged { + c.pauseMu.Unlock() return nil } kind, word := EventResumed, sayResumed @@ -2193,14 +2190,23 @@ func (r *Runner) setPaused(c *loopCtl, want func(was bool) bool) error { kind, word = EventPaused, sayPaused } now := r.now() - if err := lp.move(c, "", kind, word, func(it *factory.Item) error { + if err := lp.write(c, func(it *factory.Item) error { it.Stream.Paused = paused loopSay(it, now, "said", word) return nil }); err != nil { + c.pauseMu.Unlock() return err } - lp.tell(id, word) + lp.mu.Lock() + c.paused = paused + if paused && c.roundCancel != nil { + c.roundCancel() + } + lp.mu.Unlock() + c.pauseMu.Unlock() + lp.emit(c.id, "", kind, word) + lp.tell(c.id, word) return nil } diff --git a/internal/factory/worker/hold_heartbeat_test.go b/internal/factory/worker/hold_heartbeat_test.go new file mode 100644 index 0000000000..476e4e4ff4 --- /dev/null +++ b/internal/factory/worker/hold_heartbeat_test.go @@ -0,0 +1,76 @@ +package worker_test + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "sync/atomic" + "testing" + "time" + + "github.com/Agent-Field/codeaf/internal/factory/api" +) + +func TestAHoldSurvivesALostHeartbeatReply(t *testing.T) { + var lost, subsequent atomic.Int32 + g, _ := compatRig(t, func(next http.Handler, _ *reportLog) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if !strings.HasSuffix(r.URL.Path, "/heartbeat") { + next.ServeHTTP(w, r) + return + } + captured := httptest.NewRecorder() + next.ServeHTTP(captured, r) + var beat api.WorkerHeartbeat + if err := json.Unmarshal(captured.Body.Bytes(), &beat); err != nil { + t.Error(err) + } + for _, signal := range beat.Signals { + if signal.Verb == "hold" && signal.Yes && lost.CompareAndSwap(0, 1) { + // The factory drained the hold, but its reply never reached the worker. + panic(http.ErrAbortHandler) + } + } + if lost.Load() == 1 { + subsequent.Add(1) + } + for key, values := range captured.Header() { + w.Header()[key] = values + } + w.WriteHeader(captured.Code) + _, _ = w.Write(captured.Body.Bytes()) + }) + }) + id := g.add(t, check("sleep 30")) + if err := g.floor.Launch(id); err != nil { + t.Fatal(err) + } + for deadline := time.Now().Add(5 * time.Second); ; time.Sleep(5 * time.Millisecond) { + it, _ := g.st.Get(id) + if logHas(it, "check: sleep 30") > 0 { + break + } + if time.Now().After(deadline) { + t.Fatal("the worker never started the check") + } + } + if err := g.floor.Hold(id, true); err != nil { + t.Fatal(err) + } + for deadline := time.Now().Add(5 * time.Second); ; time.Sleep(5 * time.Millisecond) { + it, err := g.st.Get(id) + if err != nil { + t.Fatal(err) + } + if logHas(it, "paused") > 0 { + if lost.Load() != 1 { + t.Fatal("the hold heartbeat reply was not lost") + } + return + } + if time.Now().After(deadline) { + t.Fatalf("hold was not delivered after a lost heartbeat reply: lost=%d subsequentHeartbeats=%d state=%s factoryPaused=%t runnerRunning=%t pausedLog=%d", lost.Load(), subsequent.Load(), it.State, it.Stream != nil && it.Stream.Paused, g.run.Running(id), logHas(it, "paused")) + } + } +} diff --git a/internal/factory/worker/pause_finish_test.go b/internal/factory/worker/pause_finish_test.go new file mode 100644 index 0000000000..ab7e75e610 --- /dev/null +++ b/internal/factory/worker/pause_finish_test.go @@ -0,0 +1,60 @@ +package worker_test + +import ( + "sync/atomic" + "testing" + "time" + + "github.com/Agent-Field/codeaf/internal/factory" + "github.com/Agent-Field/codeaf/internal/factory/run" +) + +func TestAPauseIsReportedWhenTheCancelledDriveFinishesFirst(t *testing.T) { + var delay atomic.Bool + g := newRigWith(t, nil, func(o *run.Options) { + o.Clock = func() time.Time { + if delay.CompareAndSwap(true, false) { + // Let the cancelled drive finish before the pause's store write. + time.Sleep(200 * time.Millisecond) + } + return time.Now() + } + }) + id := g.add(t, check("sleep 30"), factory.Stage{Name: "again", Kind: factory.StageCheck, Ask: "true", On: true}) + if err := g.floor.Launch(id); err != nil { + t.Fatal(err) + } + for deadline := time.Now().Add(5 * time.Second); ; time.Sleep(5 * time.Millisecond) { + it, _ := g.st.Get(id) + if logHas(it, "check: sleep 30") > 0 { + break + } + if time.Now().After(deadline) { + t.Fatal("the worker never started the check") + } + } + delay.Store(true) + if err := g.floor.Hold(id, true); err != nil { + t.Fatal(err) + } + for deadline := time.Now().Add(5 * time.Second); ; time.Sleep(5 * time.Millisecond) { + it, _ := g.st.Get(id) + if !g.run.Running(id) && logHas(it, "paused") == 1 { + if logHas(it, "check: true") != 0 { + t.Fatal("the second step started while held") + } + break + } + if time.Now().After(deadline) { + local, _ := g.mirror.Get(id) + t.Fatalf("pause lost as drive finished: factory=%+v worker=%+v running=%t reports=%q", it.Stream, local.Stream, g.run.Running(id), g.seen.all()) + } + } + if err := g.floor.Hold(id, false); err != nil { + t.Fatal(err) + } + // Stop the resumed job so this fixture leaves no long-running check. + if err := g.floor.Stop(id); err != nil { + t.Fatal(err) + } +} diff --git a/internal/factory/worker/worker_test.go b/internal/factory/worker/worker_test.go index a27e50cfc2..e45d74727e 100644 --- a/internal/factory/worker/worker_test.go +++ b/internal/factory/worker/worker_test.go @@ -2,6 +2,9 @@ package worker_test import ( "context" + "encoding/json" + "fmt" + "net/http" "net/http/httptest" "os" "path/filepath" @@ -31,6 +34,8 @@ type rig struct { run *run.Runner // record is the factory's record, where the service writes approvals. record *record.Record + mirror *store.Store + seen *reportLog } func newRig(t *testing.T, localLease ...func(string, api.WorkerLease)) rig { @@ -62,7 +67,29 @@ func newRigWith(t *testing.T, keepLocal func(string, api.WorkerLease), tune func t.Fatal(err) } handler := serve.New(seam, serve.Config{Address: "http://test", Store: st, Record: history, PullWait: 50 * time.Millisecond}) - server := httptest.NewServer(handler) + seen := &reportLog{} + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { + body := "" + if req.Method == http.MethodPost { + body = peek(req) + } + captured := httptest.NewRecorder() + handler.ServeHTTP(captured, req) + if isReport(req) || strings.HasSuffix(req.URL.Path, "/done") { + seen.add(fmt.Sprintf("%s status=%d request=%s", req.URL.Path, captured.Code, body)) + } + if strings.HasSuffix(req.URL.Path, "/heartbeat") && strings.Contains(captured.Body.String(), `"verb"`) { + var beat api.WorkerHeartbeat + if err := json.Unmarshal(captured.Body.Bytes(), &beat); err == nil { + seen.add(fmt.Sprintf("heartbeat signals=%+v", beat.Signals)) + } + } + for key, values := range captured.Header() { + w.Header()[key] = values + } + w.WriteHeader(captured.Code) + _, _ = w.Write(captured.Body.Bytes()) + })) t.Cleanup(func() { server.Close(); handler.Close() }) mirror, err := store.Open(t.TempDir()) @@ -96,17 +123,25 @@ func newRigWith(t *testing.T, keepLocal func(string, api.WorkerLease), tune func defer mu.Unlock() creds[slot] = w.Credential() }, - Attach: api.WorkerAttach{Machine: "laptop", Name: "laptop (pid 123)", Handle: "me", TakesUnassigned: map[string]bool{lease.EveryRepo: true}}, - Runner: r, - Mirror: mirror, - LocalLease: keepLocal, + Attach: api.WorkerAttach{Machine: "laptop", Name: "laptop (pid 123)", Handle: "me", TakesUnassigned: map[string]bool{lease.EveryRepo: true}}, + Runner: r, + Mirror: mirror, + LocalLease: func(base string, held api.WorkerLease) { + if held.ID == "" { + local, err := mirror.Get(held.Job) + seen.add(fmt.Sprintf("worker release job=%d state=%s stream=%+v error=%v", held.Job, local.State, local.Stream, err)) + } + if keepLocal != nil { + keepLocal(base, held) + } + }, Slots: 2, ReportEvery: 20 * time.Millisecond, BeatEvery: 20 * time.Millisecond, }) }() t.Cleanup(func() { cancel(); <-done }) - return rig{base: server.URL, st: st, floor: client.Open(server.URL, "").Seam(), doors: seam, run: r, record: history} + return rig{base: server.URL, st: st, floor: client.Open(server.URL, "").Seam(), doors: seam, run: r, record: history, mirror: mirror, seen: seen} } func TestWorkerLocalReceiptFollowsPullHeartbeatAndRelease(t *testing.T) { @@ -293,7 +328,9 @@ func TestAPauseAndResumeReachTheRunner(t *testing.T) { break } if time.Now().After(deadline) { - t.Fatal("the worker's runner never said paused") + it, _ := g.st.Get(id) + local, _ := g.mirror.Get(id) + t.Fatalf("the worker's runner never said paused: factory=%+v worker=%+v running=%t reports=%q", it.Stream, local.Stream, g.run.Running(id), g.seen.all()) } } if err := g.floor.Hold(id, false); err != nil { diff --git a/internal/manual/chat/factory.md b/internal/manual/chat/factory.md index 45367dfa70..13d6265f6a 100644 --- a/internal/manual/chat/factory.md +++ b/internal/manual/chat/factory.md @@ -460,7 +460,9 @@ continues the job. Repeated spending reports count once and cannot lower a round conversation when it can be reopened. It takes a free Jobs at once slot, or waits in the ordinary queue until a slot is available. No model round starts while it waits. Completed steps are kept and are not run again. The activity -says `paused`, then `resumed`; both actions record who made them. +says `paused`, then `resumed`; both actions record who made them. If a heartbeat +reply is lost, the factory repeats its current hold on later heartbeats. Resume +supersedes an older hold still waiting to be collected. Only the job's person may pause or resume it. A job waiting for you, finished or publishing cannot be paused. Pause releases its Jobs at once slot after From 79c8a9417870185602afdb300f69577bda85d9aa Mon Sep 17 00:00:00 2001 From: ZeroPoint95 <329227198+ZeroPoint95@users.noreply.github.com> Date: Tue, 3 Nov 2026 15:41:24 -0500 Subject: [PATCH 3/4] Pairing follow-ups: app and pair approve settle a lost reply; chat rows say a take is running (#2314) * pairing: the app and codeaf pair approve settle a lost approval reply too The terminal chat screen's T1/T3 fix now covers the desktop app's approve_link and codeaf pair approve: an approve that timed out or was refused as already answered is looked up with settleApproval, so a device the relay let in counts and the approver ends solo; a reopened link this computer already approved says already paired and ends solo instead of failing as already answered. Co-Authored-By: Claude Opus 5.5 * app: chat rows say when a take of that chat is running Co-Authored-By: Claude Opus 5.5 * changes: one entry for the pairing follow-ups; #2285's entry keeps what ships there Co-Authored-By: Claude Opus 5.5 * changes: number the pairing follow-ups entry Co-Authored-By: Claude Opus 5.5 * app: a finished take never says taking:false on a chat still elsewhere The take stopped counting before it held the inventory lock, so a follower emission in that gap sent the old row with taking:false, which a client that stopped waiting reads as a take that did not move the chat. It now stops counting under the lock, and a failed reconcile republishes only a cached row that already says the chat is here. The protocol text states the guarantee the code keeps; the change entry lists its docs surface. Co-Authored-By: Claude Opus 5.5 * app: a take that acquired the chat says it runs until a row says the chat is here An error after the chat was acquired, or a presence refresh after a failed reconcile, still republished the row from before the take with taking:false. The take now records the fence it acquired, and every row older than that keeps taking:true until a row says the chat is here or a later fence holds it. The scheduling-dependent lock test is replaced by deterministic ones; the protocol and change entry say when taking ends, and a reopened link counts any approval for this account's devices. Co-Authored-By: Claude Opus 5.5 * app: republishing a take's taking leaves agents where the take put them Republishing cached rows for taking also re-placed their local agents, so after a failed reconcile the agent a take had just put here went back to the other computer. That republish now changes rows only. A chat that leaves the list drops its landed take. The round trip checks taking only on the row that says the chat is here, since a lagging row may still say it. The protocol and change entry say a here row can carry taking while a take runs, and that a lost approval reply did let the device join. Co-Authored-By: Claude Opus 5.5 * app: a presence change leaves a landed take's agent where the take put it When a take acquired the chat but its reconcile read failed, the cache still said the chat was elsewhere, and the next presence-only frame re-placed the agent the take had put here back on the other computer. That refresh now skips re-placing agents on rows whose take landed and no row has yet confirmed. It skips only those rows rather than every row: presence changes the offline sentence in `line` and `takeover_line`, which an agent's location carries, and nothing else pushes that update to a following client. Co-Authored-By: Claude Opus 5.5 * app: a reading from before a landed take leaves the agent here readChats re-placed every row's agent, so a follower reading newer than the cache but older than the take moved the agent agents.take put here back elsewhere. It now skips rows with a landed take, as presence does; a row saying the chat is here clears the landed take first and still places the agent. agents.take is now driven through its acquired-then-failed paths with the copy and open stood in (no engine needed): opening the moved chat fails, and settleTake's reading fails followed by a stale reading. Co-Authored-By: Claude Opus 5.5 * app: only a directory reading drops a landed take whose chat left the list publishTaking and the presence refresh mark taking on cached rows, which can be empty while the follower runs; pruning there forgot a take that acquired the chat, so a later reading from before it moved the agent back. Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- cmd/codeaf/app_chats.go | 110 +++- cmd/codeaf/app_pair.go | 20 +- cmd/codeaf/app_take.go | 38 +- cmd/codeaf/app_take_inflight_test.go | 523 ++++++++++++++++++ cmd/codeaf/app_take_test.go | 3 + cmd/codeaf/approvals_test.go | 220 +++++++- cmd/codeaf/pairlink.go | 22 +- .../unreleased/2314-pairing-followups.md | 9 + docs/design/app-v3/PROTOCOL.md | 10 +- internal/appwire/chats.go | 1 + 10 files changed, 922 insertions(+), 34 deletions(-) create mode 100644 cmd/codeaf/app_take_inflight_test.go create mode 100644 docs/changes/unreleased/2314-pairing-followups.md diff --git a/cmd/codeaf/app_chats.go b/cmd/codeaf/app_chats.go index 61a397a9e4..bd5435fef9 100644 --- a/cmd/codeaf/app_chats.go +++ b/cmd/codeaf/app_chats.go @@ -24,6 +24,8 @@ import ( type appChats struct { start sync.Once byAgent map[string]appwire.ChatRow + taking map[string]int + landed map[string]uint64 take sync.Mutex op sync.Mutex mu sync.Mutex @@ -352,13 +354,16 @@ func (b *appBackend) readChats(parent context.Context, s *syncsetup.Sync, feed d } out = append(out, row) } + b.chats.markTaking(out, true) cacheFacts := func() { b.chats.mu.Lock() defer b.chats.mu.Unlock() b.chats.facts = facts } cacheFacts() - b.publishChats(out, version, emit) + // markTaking has already cleared a landed take on a row saying the chat is + // here, so only a row from before the take keeps its agent where it is. + b.publishChatRows(out, version, emit, func(r appwire.ChatRow) bool { return !b.chats.landedTake(r.Cell) }) return appwire.ChatsResult{Chats: out, Version: version}, nil } @@ -371,6 +376,97 @@ func (c *appChats) newerChats(version uint64) (appwire.ChatsResult, bool) { return appwire.ChatsResult{}, false } +// A take is in flight from when agents.take asks for the chat until it ends, +// so a client that stopped waiting can tell a move still copying from one that +// never happened. A take that acquired the chat keeps saying so on every row +// from before it until a row says the chat is here or a later fence holds it. +// Only a directory reading (prune) can say a chat left the list: the cache +// may be empty or stale, so rows from it never drop a landed take. +func (c *appChats) markTaking(rows []appwire.ChatRow, prune bool) { + c.mu.Lock() + defer c.mu.Unlock() + listed := make(map[string]bool, len(rows)) + for i := range rows { + r := &rows[i] + listed[r.Cell] = true + fence, landed := c.landed[r.Cell] + if landed && (r.Here || r.Seen.Fence > fence) { + delete(c.landed, r.Cell) + landed = false + } + r.Taking = c.taking[r.Cell] > 0 || landed + } + if !prune { + return + } + // A chat that left the list is no longer anyone's take. + for id := range c.landed { + if !listed[id] { + delete(c.landed, id) + } + } +} + +func (c *appChats) landedTake(id string) bool { + c.mu.Lock() + defer c.mu.Unlock() + _, ok := c.landed[id] + return ok +} + +func (c *appChats) landTake(id string, fence uint64) { + c.mu.Lock() + defer c.mu.Unlock() + if c.landed == nil { + c.landed = make(map[string]uint64) + } + c.landed[id] = fence +} + +func (c *appChats) countTake(id string, delta int) { + c.mu.Lock() + defer c.mu.Unlock() + if c.taking == nil { + c.taking = make(map[string]int) + } + c.taking[id] += delta + if c.taking[id] <= 0 { + delete(c.taking, id) + } +} + +// The take's own start and end change no directory version, so its row is +// emitted again from the cache. +func (b *appBackend) publishTaking() { + c := &b.chats + c.op.Lock() + defer c.op.Unlock() + cached := func() ([]appwire.ChatRow, uint64, bool) { + c.mu.Lock() + defer c.mu.Unlock() + return slices.Clone(c.rows), c.version, !c.closed && c.source != nil + } + rows, version, following := cached() + if !following { + return + } + c.markTaking(rows, false) + // Only taking changed, so the cache's older locations stay off the agents + // a take may already have placed here. + b.publishChatRows(rows, version, true, nil) +} + +// A finished take stops counting under the inventory lock, and this reading +// clears its taking with the row that says the chat is here. +func (b *appBackend) settleTake(ctx context.Context, s *syncsetup.Sync, id string) error { + c := &b.chats + c.op.Lock() + defer c.op.Unlock() + c.countTake(id, -1) + _, err := b.readChats(ctx, s, c.feed, true) + return err +} + func appChatPresence(row *appwire.ChatRow, fact chatlist.Row, state dirwatch.State, known map[string]bool) { row.Online = nil if fact.DeviceID != "" { @@ -385,6 +481,11 @@ func appChatPresence(row *appwire.ChatRow, fact chatlist.Row, state dirwatch.Sta // Inventory operations hold op through caching and emission. A list refreshes // the cache silently; only the follower compares against rows actually emitted. func (b *appBackend) publishChats(rows []appwire.ChatRow, version uint64, emit bool) { + b.publishChatRows(rows, version, emit, func(appwire.ChatRow) bool { return true }) +} + +// locate says which rows re-place their local agents; nil re-places none. +func (b *appBackend) publishChatRows(rows []appwire.ChatRow, version uint64, emit bool, locate func(appwire.ChatRow) bool) { old, current := b.chats.cacheRows(rows, version, emit) if !current || !emit { return @@ -399,7 +500,7 @@ func (b *appBackend) publishChats(rows []appwire.ChatRow, version uint64, emit b if !exists || !reflect.DeepEqual(prev, r) { b.emit.Emit(appwire.EventChatChanged, appwire.ChatChanged{Chat: r, Version: version}) } - if r.LocalAgent != "" { + if locate != nil && r.LocalAgent != "" && locate(r) { var a *appAgent b.locked(func() { a = b.agents[r.LocalAgent] }) if a != nil { @@ -494,7 +595,10 @@ func (b *appBackend) refreshChatInventory(ctx context.Context, feed dirwatch.Fol r := &rows[i] appChatPresence(r, c.facts[r.Cell], state, known) } - b.publishChats(rows, version, true) + c.markTaking(rows, false) + // Presence still refreshes local agents' lines, but a cached row from + // before a landed take would move its agent back elsewhere. + b.publishChatRows(rows, version, true, func(r appwire.ChatRow) bool { return !c.landedTake(r.Cell) }) } return true } diff --git a/cmd/codeaf/app_pair.go b/cmd/codeaf/app_pair.go index d7ae62048c..ab6d85076b 100644 --- a/cmd/codeaf/app_pair.go +++ b/cmd/codeaf/app_pair.go @@ -15,6 +15,7 @@ import ( "github.com/Agent-Field/codeaf/internal/guard" "github.com/Agent-Field/codeaf/internal/identity" "github.com/Agent-Field/codeaf/internal/pair" + "github.com/Agent-Field/codeaf/internal/tui3" ) const appJoinBusy = "Finish the work on this computer before joining another computer." @@ -346,7 +347,7 @@ func (o *appPairOperation) perform(p appwire.PairStartParams) (error, bool) { if err != nil { return err, false } - who, err := linkApprover(dir)(route) + who, err := appLinkApprover(dir)(route) if err != nil { return err, false } @@ -355,6 +356,15 @@ func (o *appPairOperation) perform(p appwire.PairStartParams) (error, bool) { return nil } asking, err := who.Look(o.ctx, ref) + if errors.Is(err, pair.ErrLinkDecided) && approvedHere(who, ref.Code, "") == requestApprovedHere { + // An approval whose reply was lost: the device is in, so this computer is shared. + if err := identity.EndSolo(dir); err != nil { + return err, false + } + record() + o.line(tui3.ErrAlreadyPaired.Error()) + return nil, false + } if err != nil { return err, false } @@ -371,7 +381,9 @@ func (o *appPairOperation) perform(p appwire.PairStartParams) (error, bool) { return pair.ErrRefused, false } if err := who.Approve(o.ctx, asking); err != nil { - return err, false + if err = settleApproval(who, asking, err); err != nil { + return err, false + } } if err := identity.EndSolo(dir); err != nil { return err, false @@ -382,6 +394,10 @@ func (o *appPairOperation) perform(p appwire.PairStartParams) (error, bool) { } return errors.New("unknown pairing mode"), false } + +// appLinkApprover is this computer as the device that approves a link from the app. +var appLinkApprover = linkApprover + func (o *appPairOperation) Show(code *pair.Code, lines string) { o.update(func(p *appwire.Pairing) { p.State = "waiting" diff --git a/cmd/codeaf/app_take.go b/cmd/codeaf/app_take.go index 1d341a555c..3e05c87d57 100644 --- a/cmd/codeaf/app_take.go +++ b/cmd/codeaf/app_take.go @@ -61,6 +61,15 @@ func appTakeWords(words string) string { } return words } + +// Tests stand in for the copy that acquires the chat and for opening it here. +var ( + appTakeSeen = func(ctx context.Context, s *syncsetup.Sync, id string, seen *directory.LeaseExpectation) (syncsetup.Continued, error) { + return chatContinuer(s, nil).TakeSeen(ctx, id, seen) + } + appTakeOpen = (*appBackend).OpenAgent +) + func (b *appBackend) TakeAgent(p appwire.TakeParams) (appwire.Agent, error) { if p.Cell == "" || p.Seen.Device == "" { return appwire.Agent{}, appwire.Errorf(appwire.CodeBadParams, "a chat and its seen computer are required") @@ -71,6 +80,15 @@ func (b *appBackend) TakeAgent(p appwire.TakeParams) (appwire.Agent, error) { if err := b.pairingWorkRefusal(); err != nil { return appwire.Agent{}, err } + b.chats.countTake(p.Cell, 1) + b.publishTaking() + taking := true + defer func() { + if taking { + b.chats.countTake(p.Cell, -1) + b.publishTaking() + } + }() b.chats.take.Lock() defer b.chats.take.Unlock() s, ok, err := syncsetup.Open(b.relayHome()) @@ -129,10 +147,12 @@ func (b *appBackend) TakeAgent(p appwire.TakeParams) (appwire.Agent, error) { return appwire.Agent{}, err } } - got, err := chatContinuer(s, nil).TakeSeen(ctx, p.Cell, seen) + got, err := appTakeSeen(ctx, s, p.Cell, seen) if err != nil { return appwire.Agent{}, appTakeError(err) } + // The chat is this computer's from here on, even if opening it fails. + b.chats.landTake(p.Cell, got.Taken.Driving.Fence) // The continuer's already-here shortcut may observe a local acquisition // that happened after our preflight read. It must not accept older consent. if got.AlreadyHere && (p.Seen.Device != s.Device.ID() || !seen.Matches(directory.Cell{Head: got.Taken.Driving.Head, Lease: directory.Lease{Device: s.Device.ID(), Fence: got.Taken.Driving.Fence}})) { @@ -164,7 +184,7 @@ func (b *appBackend) TakeAgent(p appwire.TakeParams) (appwire.Agent, error) { location.Seen.Head = "" } b.located(a, location) - agent, err := b.OpenAgent(id) + agent, err := appTakeOpen(b, id) if err != nil { return appwire.Agent{}, appTakeError(err) } @@ -181,18 +201,16 @@ func (b *appBackend) TakeAgent(p appwire.TakeParams) (appwire.Agent, error) { } b.located(a, location) agent.Location = location + taking = false // Acquisition may have been observed before installation and admission. // Reconcile the local id even when no later directory version will arrive. - reconcileInventory := func() error { - b.chats.op.Lock() - defer b.chats.op.Unlock() - _, err := b.readChats(ctx, s, b.chats.feed, true) - return err - } - err = reconcileInventory() + err = b.settleTake(ctx, s, p.Cell) if err != nil { - // The take has completed. Inventory recovery belongs to its follower. + // The take has completed. Inventory recovery belongs to its follower, + // whose reading that shows the chat here clears the row's taking. b.startChatInventory() } + // A newer cached version skips the reading's own taking. + b.publishTaking() return agent, nil } diff --git a/cmd/codeaf/app_take_inflight_test.go b/cmd/codeaf/app_take_inflight_test.go new file mode 100644 index 0000000000..a3a587d30a --- /dev/null +++ b/cmd/codeaf/app_take_inflight_test.go @@ -0,0 +1,523 @@ +package main + +import ( + "context" + "errors" + "math" + "slices" + "sync/atomic" + "testing" + "time" + + "github.com/Agent-Field/codeaf/internal/appwire" + "github.com/Agent-Field/codeaf/internal/cell" + "github.com/Agent-Field/codeaf/internal/cell/celltest" + "github.com/Agent-Field/codeaf/internal/cellsync" + "github.com/Agent-Field/codeaf/internal/devname" + "github.com/Agent-Field/codeaf/internal/directory" + "github.com/Agent-Field/codeaf/internal/dirwatch" + "github.com/Agent-Field/codeaf/internal/handoff" + "github.com/Agent-Field/codeaf/internal/identity" + "github.com/Agent-Field/codeaf/internal/session" + "github.com/Agent-Field/codeaf/internal/syncsetup" +) + +// A client that stopped waiting for agents.take reads the row's taking flag: +// set while the take runs, cleared when it ends without moving the chat. +func TestAppTakeInFlightRowClearsWhenTheTakeFails(t *testing.T) { + celltest.Available(t) + appDevicesRelay(t) + aHome, bHome := t.TempDir(), t.TempDir() + id, err := identity.Ensure(aHome) + if err != nil { + t.Fatal(err) + } + if _, err := identity.Adopt(bHome, id, false); err != nil { + t.Fatal(err) + } + a, b := appDeviceSync(t, aHome), appDeviceSync(t, bHome) + for _, s := range []*syncsetup.Sync{a, b} { + if err := syncsetup.PutOwnDevice(t.Context(), s.Dir, s.Identity, s.Device, devname.Name(s.Home)); err != nil { + t.Fatal(err) + } + } + c, err := cell.CreateIn(t.TempDir(), cell.Options{Class: cell.FilesOnly}) + if err != nil { + t.Fatal(err) + } + if _, err := a.Dir.Create(t.Context(), c.ID, directory.CellInit{Head: "saved-head"}); err != nil { + t.Fatal(err) + } + _, rec, wire := appDeviceBackend(t, bHome) + var row appwire.ChatRow + appSyncWait(t, "the other computer's chat is listed", func() bool { + for _, r := range machineCall[appwire.ChatsResult](t, wire, "chats.list", struct{}{}).Chats { + if r.Cell == c.ID && r.CanTake { + row = r + return true + } + } + return false + }) + if row.Taking { + t.Fatalf("no take has started: %+v", row) + } + stale := row.Seen + stale.Head = "another-head" + if _, refusal := wire.call("agents.take", appwire.TakeParams{Cell: row.Cell, Seen: stale}); refusal == nil || refusal.Code != appwire.CodeConflict { + t.Fatalf("stale take: %+v", refusal) + } + started := rec.waitFor(t, 5*time.Second, func(name string, data any) bool { + return name == appwire.EventChatChanged && data.(appwire.ChatChanged).Chat.Cell == c.ID && data.(appwire.ChatChanged).Chat.Taking + }) + heard, ended := -1, appwire.ChatRow{} + rec.waitFor(t, 5*time.Second, func(name string, data any) bool { + heard++ + changed, ok := data.(appwire.ChatChanged) + if heard <= started || name != appwire.EventChatChanged || !ok || changed.Chat.Cell != c.ID || changed.Chat.Taking { + return false + } + ended = changed.Chat + return true + }) + if ended.Here { + t.Fatalf("a failed take left the chat here: %+v", ended) + } + for _, r := range machineCall[appwire.ChatsResult](t, wire, "chats.list", struct{}{}).Chats { + if r.Cell == c.ID && r.Taking { + t.Fatalf("the list still says a take is running: %+v", r) + } + } +} + +// A take that acquired the chat keeps saying it runs on every row from before +// it, whatever ends the call: a client that stopped waiting reads taking:false +// on a chat still elsewhere as a take that did not move it. +func TestAppTakeInFlightRowsFromBeforeALandedTakeStillSayItRuns(t *testing.T) { + const id = "chat" + var c appChats + mark := func(row appwire.ChatRow) bool { + rows := []appwire.ChatRow{row} + c.markTaking(rows, true) + return rows[0].Taking + } + before := appwire.ChatRow{Cell: id, Seen: appwire.Seen{Device: "other", Fence: 4}} + if mark(before) { + t.Fatal("no take has started") + } + c.countTake(id, 1) + c.landTake(id, 5) + c.countTake(id, -1) + if !mark(before) { + t.Fatal("a row from before the landed take says it ended") + } + if !mark(appwire.ChatRow{Cell: id, Seen: appwire.Seen{Device: "this", Fence: 5}}) { + t.Fatal("the taken fence not yet installed here says the take ended") + } + if mark(appwire.ChatRow{Cell: id, Here: true, Seen: appwire.Seen{Device: "this", Fence: 5}}) { + t.Fatal("the row that says the chat is here still says a take runs") + } + if mark(before) { + t.Fatal("a take already seen here kept counting") + } + // A later take elsewhere ends it too, so the flag never sticks. + c.landTake(id, 5) + if mark(appwire.ChatRow{Cell: id, Seen: appwire.Seen{Device: "other", Fence: 6}}) || mark(before) { + t.Fatal("a chat another computer took again still says this take runs") + } + // Cached rows can be empty or stale, so a chat missing from them keeps + // its landed take. + c.landTake(id, 5) + c.markTaking(nil, false) + c.markTaking([]appwire.ChatRow{{Cell: "another"}}, false) + if !c.landedTake(id) { + t.Fatal("cached rows without the chat dropped its landed take") + } + // A chat that leaves a directory reading takes its landed take with it. + c.markTaking([]appwire.ChatRow{{Cell: "another"}}, true) + if mark(before) { + t.Fatal("a chat that left the list came back saying an old take runs") + } +} + +// The cached row republished after a landed take whose reading failed still +// shows the chat elsewhere, so it still says the take runs. +func TestAppTakeInFlightCacheAfterALandedTakeStillSaysItRuns(t *testing.T) { + celltest.Available(t) + appDevicesRelay(t) + aHome, bHome := t.TempDir(), t.TempDir() + id, err := identity.Ensure(aHome) + if err != nil { + t.Fatal(err) + } + if _, err := identity.Adopt(bHome, id, false); err != nil { + t.Fatal(err) + } + a, b := appDeviceSync(t, aHome), appDeviceSync(t, bHome) + for _, s := range []*syncsetup.Sync{a, b} { + if err := syncsetup.PutOwnDevice(t.Context(), s.Dir, s.Identity, s.Device, devname.Name(s.Home)); err != nil { + t.Fatal(err) + } + } + c, err := cell.CreateIn(t.TempDir(), cell.Options{Class: cell.FilesOnly}) + if err != nil { + t.Fatal(err) + } + if _, err := a.Dir.Create(t.Context(), c.ID, directory.CellInit{Head: "saved-head"}); err != nil { + t.Fatal(err) + } + backend, rec, wire := appDeviceBackend(t, bHome) + var row appwire.ChatRow + appSyncWait(t, "the other computer's chat is listed", func() bool { + for _, r := range machineCall[appwire.ChatsResult](t, wire, "chats.list", struct{}{}).Chats { + if r.Cell == c.ID && r.CanTake { + row = r + return true + } + } + return false + }) + backend.chats.countTake(c.ID, 1) + backend.publishTaking() + backend.chats.landTake(c.ID, row.Seen.Fence+1) + // The take placed its agent here; the cache still holds the row from + // before it, naming that agent. + placed := backend.agent("taken-here") + here := &appwire.Location{Here: true, Cell: c.ID} + backend.located(placed, here) + backend.chats.mu.Lock() + for i := range backend.chats.rows { + if backend.chats.rows[i].Cell == c.ID { + backend.chats.rows[i].LocalAgent = placed.view.ID + } + } + backend.chats.mu.Unlock() + backend.chats.countTake(c.ID, -1) + rec.mu.Lock() + from := len(rec.data) + rec.mu.Unlock() + backend.publishTaking() + backend.locked(func() { + if placed.view.Location != here { + t.Errorf("republishing taking moved the taken agent back: %+v", placed.view.Location) + } + }) + listed := false + for _, r := range machineCall[appwire.ChatsResult](t, wire, "chats.list", struct{}{}).Chats { + if r.Cell == c.ID { + listed = true + if !r.Taking { + t.Fatal("the list read after a landed take says it ended with the chat elsewhere") + } + } + } + if !listed { + t.Fatal("the chat left the list") + } + rec.mu.Lock() + defer rec.mu.Unlock() + for _, data := range rec.data[from:] { + if changed, ok := data.(appwire.ChatChanged); ok && changed.Chat.Cell == c.ID && !changed.Chat.Here && !changed.Chat.Taking { + t.Fatalf("a landed take was announced as one that did not move the chat: %+v", changed.Chat) + } + } +} + +// A watch that has announced no newer version than the cache, so its next +// frame is presence only. +type presenceOnlyFeed struct{ changes chan struct{} } + +func (f presenceOnlyFeed) State() dirwatch.State { return dirwatch.State{Up: true} } +func (f presenceOnlyFeed) Changes() <-chan struct{} { return f.changes } +func (presenceOnlyFeed) Probe() {} +func (presenceOnlyFeed) Close() {} + +// A take that acquired the chat but whose reconcile read failed leaves the +// cache saying the chat is elsewhere; a device coming or going must not move +// the agent the take put here back there. +func TestAppTakeInFlightPresenceAfterALandedTakeLeavesTheAgentHere(t *testing.T) { + celltest.Available(t) + appDevicesRelay(t) + aHome, bHome := t.TempDir(), t.TempDir() + id, err := identity.Ensure(aHome) + if err != nil { + t.Fatal(err) + } + if _, err := identity.Adopt(bHome, id, false); err != nil { + t.Fatal(err) + } + a, b := appDeviceSync(t, aHome), appDeviceSync(t, bHome) + for _, s := range []*syncsetup.Sync{a, b} { + if err := syncsetup.PutOwnDevice(t.Context(), s.Dir, s.Identity, s.Device, devname.Name(s.Home)); err != nil { + t.Fatal(err) + } + } + c, err := cell.CreateIn(t.TempDir(), cell.Options{Class: cell.FilesOnly}) + if err != nil { + t.Fatal(err) + } + if _, err := a.Dir.Create(t.Context(), c.ID, directory.CellInit{Head: "saved-head"}); err != nil { + t.Fatal(err) + } + backend, _, wire := appDeviceBackend(t, bHome) + var row appwire.ChatRow + appSyncWait(t, "the other computer's chat is listed", func() bool { + for _, r := range machineCall[appwire.ChatsResult](t, wire, "chats.list", struct{}{}).Chats { + if r.Cell == c.ID && r.CanTake { + row = r + return true + } + } + return false + }) + // This computer already holds a local copy, so the cached row names its + // agent while saying the chat is elsewhere. + placed := backend.agent("taken-here") + backend.chats.mu.Lock() + for i := range backend.chats.rows { + if backend.chats.rows[i].Cell == c.ID { + backend.chats.rows[i].LocalAgent = placed.view.ID + } + } + backend.chats.mu.Unlock() + // agents.take acquires the chat and places the agent here; settleTake + // stops counting it, and its reading fails, leaving the cache as it was. + backend.chats.countTake(c.ID, 1) + backend.chats.landTake(c.ID, row.Seen.Fence+1) + here := &appwire.Location{Here: true, Cell: c.ID} + backend.located(placed, here) + backend.chats.countTake(c.ID, -1) + + if !backend.refreshChatInventory(t.Context(), presenceOnlyFeed{changes: make(chan struct{})}) { + t.Fatal("the presence refresh stopped the follower") + } + backend.locked(func() { + if placed.view.Location != here { + t.Errorf("a presence change moved the taken agent back: %+v", placed.view.Location) + } + }) +} + +// A watch that has announced a directory version newer than any cache, so its +// next frame reads the rows again. +type newerVersionFeed struct{ presenceOnlyFeed } + +func (newerVersionFeed) State() dirwatch.State { + return dirwatch.State{Up: true, Version: math.MaxUint64} +} + +// appLandedTakeFixture lists another computer's chat here and has agents.take +// acquire it without an engine: the copy answers with the chat's own folder at +// the next fence, and open stands in for opening the moved chat. +func appLandedTakeFixture(t *testing.T, open func(*appBackend, string) (appwire.Agent, error)) (*appBackend, *appRecorder, *actionWire, *atomic.Bool, appwire.ChatRow) { + t.Helper() + celltest.Available(t) + _, _, down := appDevicesRelay(t) + aHome, bHome := t.TempDir(), t.TempDir() + id, err := identity.Ensure(aHome) + if err != nil { + t.Fatal(err) + } + if _, err := identity.Adopt(bHome, id, false); err != nil { + t.Fatal(err) + } + a, b := appDeviceSync(t, aHome), appDeviceSync(t, bHome) + for _, s := range []*syncsetup.Sync{a, b} { + if err := syncsetup.PutOwnDevice(t.Context(), s.Dir, s.Identity, s.Device, devname.Name(s.Home)); err != nil { + t.Fatal(err) + } + } + c, err := cell.CreateIn(t.TempDir(), cell.Options{Class: cell.FilesOnly}) + if err != nil { + t.Fatal(err) + } + if err := session.SaveMeta(c.Root, session.Meta{ID: c.ID, Title: "moved", Workspace: t.TempDir()}); err != nil { + t.Fatal(err) + } + if _, err := a.Dir.Create(t.Context(), c.ID, directory.CellInit{Head: "saved-head"}); err != nil { + t.Fatal(err) + } + backend, rec, wire := appDeviceBackend(t, bHome) + var row appwire.ChatRow + appSyncWait(t, "the other computer's chat is listed", func() bool { + for _, r := range machineCall[appwire.ChatsResult](t, wire, "chats.list", struct{}{}).Chats { + if r.Cell == c.ID && r.CanTake { + row = r + return true + } + } + return false + }) + seen, opened := appTakeSeen, appTakeOpen + t.Cleanup(func() { appTakeSeen, appTakeOpen = seen, opened }) + appTakeSeen = func(_ context.Context, _ *syncsetup.Sync, id string, _ *directory.LeaseExpectation) (syncsetup.Continued, error) { + if id != c.ID { + t.Errorf("took %s, want %s", id, c.ID) + } + return syncsetup.Continued{Device: "this computer", Taken: handoff.Taken{Cell: c, Driving: cellsync.Driving{Cell: c, Fence: row.Seen.Fence + 1, Head: "saved-head"}}}, nil + } + appTakeOpen = open + return backend, rec, wire, down, row +} + +// appNoEndedTake fails on any chat.changed row from index from on that shows +// the chat elsewhere without saying the take runs. +func appNoEndedTake(t *testing.T, rec *appRecorder, from int, id string) { + t.Helper() + rec.mu.Lock() + defer rec.mu.Unlock() + for _, data := range rec.data[from:] { + if changed, ok := data.(appwire.ChatChanged); ok && changed.Chat.Cell == id && !changed.Chat.Here && !changed.Chat.Taking { + t.Fatalf("a take that acquired the chat was announced as one that did not move it: %+v", changed.Chat) + } + } +} + +// agents.take acquired the chat but opening it here failed: every row after +// still says the take runs while the chat shows elsewhere. +func TestAppTakeAcquiredThenOpenFailsKeepsSayingItRuns(t *testing.T) { + backend, rec, wire, _, row := appLandedTakeFixture(t, func(*appBackend, string) (appwire.Agent, error) { + return appwire.Agent{}, errors.New("the moved chat would not open") + }) + rec.mu.Lock() + from := len(rec.data) + rec.mu.Unlock() + if _, refusal := wire.call("agents.take", appwire.TakeParams{Cell: row.Cell, Seen: row.Seen}); refusal == nil { + t.Fatal("a take whose chat would not open succeeded") + } + if !backend.chats.landedTake(row.Cell) { + t.Fatal("a take that acquired the chat recorded no landed take") + } + appNoEndedTake(t, rec, from, row.Cell) + for _, r := range machineCall[appwire.ChatsResult](t, wire, "chats.list", struct{}{}).Chats { + if r.Cell == row.Cell && !r.Here && !r.Taking { + t.Fatalf("the list read after the take says it ended with the chat elsewhere: %+v", r) + } + } + appNoEndedTake(t, rec, from, row.Cell) +} + +// agents.take acquired and opened the chat but its follow-up reading failed: +// the rows still say the take runs, and a later reading from before the take +// leaves the agent here. +func TestAppTakeAcquiredThenReadingFailsKeepsTheAgentHere(t *testing.T) { + var down *atomic.Bool + backend, rec, wire, relayDown, row := appLandedTakeFixture(t, func(b *appBackend, id string) (appwire.Agent, error) { + // The relay drops once the chat is open, so settleTake's reading fails. + down.Store(true) + return appwire.Agent{ID: id}, nil + }) + down = relayDown + rec.mu.Lock() + from := len(rec.data) + rec.mu.Unlock() + got := machineCall[appwire.TakeResult](t, wire, "agents.take", appwire.TakeParams{Cell: row.Cell, Seen: row.Seen}).Agent + if got.Location == nil || !got.Location.Here { + t.Fatalf("the taken agent is not here: %+v", got.Location) + } + appNoEndedTake(t, rec, from, row.Cell) + here := func(when string) { + t.Helper() + var a *appAgent + backend.locked(func() { a = backend.agents[got.ID] }) + if a == nil { + t.Fatalf("%s: the taken agent is gone", when) + } + backend.locked(func() { + if a.view.Location == nil || !a.view.Location.Here { + t.Errorf("%s: the taken agent moved back: %+v", when, a.view.Location) + } + }) + } + here("after the take") + // The follower's next reading is no older than the cache but older than + // the take: it still shows the chat elsewhere. + down.Store(false) + if !backend.refreshChatInventory(t.Context(), newerVersionFeed{presenceOnlyFeed{changes: make(chan struct{})}}) { + t.Fatal("the refresh stopped the follower") + } + if !backend.chats.landedTake(row.Cell) { + t.Fatal("a reading from before the take cleared the landed take") + } + here("after a reading from before the take") + appNoEndedTake(t, rec, from, row.Cell) +} + +// agents.take acquired the chat while the follower's cache was empty, and +// opening it failed: republishing the empty cache keeps the landed take, so a +// later reading from before the take still says it runs and leaves the agent +// the take placed here. +func TestAppTakeAcquiredWithAnEmptyCacheKeepsTheLandedTake(t *testing.T) { + backend, rec, wire, _, row := appLandedTakeFixture(t, func(*appBackend, string) (appwire.Agent, error) { + return appwire.Agent{}, errors.New("the moved chat would not open") + }) + // The follower runs, but its first reading failed and cached nothing. + // Its watch goes quiet so no reading of its own refills the cache; the + // test drives the next reading itself. + backend.chats.mu.Lock() + cancel, done := backend.chats.cancel, backend.chats.done + backend.chats.mu.Unlock() + if cancel == nil || done == nil { + t.Fatal("the chat follower is not running") + } + cancel() + <-done + backend.chats.mu.Lock() + backend.chats.rows, backend.chats.version = nil, 0 + backend.chats.mu.Unlock() + rec.mu.Lock() + from := len(rec.data) + rec.mu.Unlock() + if _, refusal := wire.call("agents.take", appwire.TakeParams{Cell: row.Cell, Seen: row.Seen}); refusal == nil { + t.Fatal("a take whose chat would not open succeeded") + } + if !backend.chats.landedTake(row.Cell) { + t.Fatal("republishing an empty cache dropped the landed take") + } + placed := func() *appAgent { + var found *appAgent + backend.locked(func() { + for _, a := range backend.agents { + if a.view.Location != nil && a.view.Location.Cell == row.Cell { + found = a + } + } + }) + return found + } + a := placed() + if a == nil { + t.Fatal("the take placed no agent for the chat") + } + backend.locked(func() { + if !a.view.Location.Here { + t.Fatalf("the taken agent is not here: %+v", a.view.Location) + } + }) + // The follower's next reading is newer than the cache but older than the + // take: it still shows the chat elsewhere. + if !backend.refreshChatInventory(t.Context(), newerVersionFeed{presenceOnlyFeed{changes: make(chan struct{})}}) { + t.Fatal("the refresh stopped the follower") + } + listed := false + backend.chats.mu.Lock() + rows := slices.Clone(backend.chats.rows) + backend.chats.mu.Unlock() + for _, r := range rows { + if r.Cell == row.Cell { + listed = true + if !r.Here && !r.Taking { + t.Fatalf("a reading from before the take says it ended with the chat elsewhere: %+v", r) + } + } + } + if !listed { + t.Fatal("the reading did not list the taken chat") + } + appNoEndedTake(t, rec, from, row.Cell) + backend.locked(func() { + if a.view.Location == nil || !a.view.Location.Here { + t.Errorf("a reading from before the take moved the agent back: %+v", a.view.Location) + } + }) +} diff --git a/cmd/codeaf/app_take_test.go b/cmd/codeaf/app_take_test.go index 4bf81eb4a7..c4455cfc1a 100644 --- a/cmd/codeaf/app_take_test.go +++ b/cmd/codeaf/app_take_test.go @@ -381,6 +381,9 @@ func appTakeRoundTrip(t *testing.T, daemon bool, missingWorkspace ...bool) { if r.Seen.Fence > taken.Location.Seen.Fence { t.Fatalf("F1 take returned fence %d, next row has %d", taken.Location.Seen.Fence, r.Seen.Fence) } + if r.Here && r.Seen.Fence == taken.Location.Seen.Fence && r.Taking { + t.Fatalf("a finished take still reads as running: %+v", r) + } return r.Seen.Fence == taken.Location.Seen.Fence } } diff --git a/cmd/codeaf/approvals_test.go b/cmd/codeaf/approvals_test.go index 96dfdbf77e..7303f6903a 100644 --- a/cmd/codeaf/approvals_test.go +++ b/cmd/codeaf/approvals_test.go @@ -8,15 +8,20 @@ import ( "path/filepath" "regexp" "runtime" + "slices" + "strings" + "sync/atomic" "testing" "time" + "github.com/Agent-Field/codeaf/internal/appwire" "github.com/Agent-Field/codeaf/internal/chatlist" "github.com/Agent-Field/codeaf/internal/devname" "github.com/Agent-Field/codeaf/internal/directory" "github.com/Agent-Field/codeaf/internal/identity" "github.com/Agent-Field/codeaf/internal/pair" "github.com/Agent-Field/codeaf/internal/pairbox" + "github.com/Agent-Field/codeaf/internal/syncsetup" "github.com/Agent-Field/codeaf/internal/tui3" ) @@ -192,16 +197,49 @@ func withLostReply(door *approvalsDoor, home string, decide bool) { } } -// pendingAt opens the new device's link on the first computer. -func (r *pairRig) pendingAt(t *testing.T, door *approvalsDoor) (string, tui3.PendingDevice, chan error) { +// soloAsking starts the new device asking and returns its link, with the first +// computer enrolled and solo. +func (r *pairRig) soloAsking(t *testing.T) (string, chan error) { t.Helper() screen, done := r.asking(t) link := screen.waitFor(t, regexp.QuoteMeta(r.url)+`/p/\S+#\S+`)[0] - r.enrol(t, door, "this computer") + r.enrol(t, r.approvalsAt(r.homeA), "this computer") // A first computer that has never shared its identity is solo. if err := os.WriteFile(filepath.Join(r.homeA, "identity-solo"), nil, 0o600); err != nil { t.Fatal(err) } + return link, done +} + +// approvedEarlier approves link from the first computer without ending solo, as +// an earlier attempt that never heard back did. +func (r *pairRig) approvedEarlier(t *testing.T, link string, done chan error) { + t.Helper() + earlier := r.approvalsAt(r.homeA) + p, err := earlier.Pending(context.Background(), link) + if err != nil { + t.Fatal(err) + } + as, _ := earlier.recall(p) + who, err := earlier.who() + if err != nil { + t.Fatal(err) + } + if err := who.Approve(context.Background(), as); err != nil { + t.Fatal(err) + } + if err := <-done; err != nil { + t.Fatalf("the new device did not join: %v", err) + } + if !identity.Solo(r.homeA) { + t.Fatal("setup: the earlier attempt should have left the marker") + } +} + +// pendingAt opens the new device's link on the first computer. +func (r *pairRig) pendingAt(t *testing.T, door *approvalsDoor) (string, tui3.PendingDevice, chan error) { + t.Helper() + link, done := r.soloAsking(t) p, err := door.Pending(context.Background(), link) if err != nil { t.Fatal(err) @@ -267,32 +305,180 @@ func TestApprovalsDoorReopenedLinkOfALostApprovalEndsSolo(t *testing.T) { rig := newPairRig(t) door := rig.approvalsAt(rig.homeA) link, p, done := rig.pendingAt(t, door) - earlier := rig.approvalsAt(rig.homeA) - if _, err := earlier.Pending(context.Background(), link); err != nil { - t.Fatal(err) + rig.approvedEarlier(t, link, done) + if _, err := door.Pending(context.Background(), link); !errors.Is(err, tui3.ErrAlreadyPaired) { + t.Fatalf("reopened link = %v, want ErrAlreadyPaired", err) } - as, _ := earlier.recall(p) - who, err := earlier.who() + if identity.Solo(rig.homeA) { + t.Fatal("the first computer stays solo after the reopened link") + } + // A retried answer on the old card settles the same way. + if err := door.Approve(context.Background(), p); err != nil { + t.Fatalf("retried approve = %v, want it settled as approved", err) + } +} + +// approveLosingReply runs `codeaf pair approve ` on the first computer, +// typing a, through a relay that loses its reply. +func (r *pairRig) approveLosingReply(link string, decide bool) (*pairScreenText, error) { + out := &pairScreenText{} + d := r.door(r.homeA, strings.NewReader("a\n"), out) + d.approver = func(route pair.Mailbox) (pair.Approver, error) { + who, err := linkApprover(r.homeA)(route) + who.Dir = lostReply{who.Dir, decide} + return who, err + } + return out, d.run(context.Background(), []string{link}) +} + +// T1 for `codeaf pair approve`: the lost reply is looked up, so the approval +// counts and the first computer stops being solo. +func TestPairApproveWhoseReplyWasLostStillPairs(t *testing.T) { + rig := newPairRig(t) + link, done := rig.soloAsking(t) + out, err := rig.approveLosingReply(link, true) if err != nil { - t.Fatal(err) + t.Fatalf("approve = %v, want the lost reply settled as approved", err) } - if err := who.Approve(context.Background(), as); err != nil { - t.Fatal(err) + if !strings.Contains(out.String(), pair.ApprovedLine("laptop")) { + t.Fatalf("the approving terminal printed:\n%s", out.String()) } if err := <-done; err != nil { t.Fatalf("the new device did not join: %v", err) } + if identity.Solo(rig.homeA) { + t.Fatal("the first computer stays solo, so it never shares") + } +} + +// `codeaf pair approve` whose approval never reached the relay says to run it +// again, and running it again lets the device in. +func TestPairApproveThatNeverArrivedCanBeRetried(t *testing.T) { + rig := newPairRig(t) + link, done := rig.soloAsking(t) + if _, err := rig.approveLosingReply(link, false); err == nil || err.Error() != pairAnswerNotArrived { + t.Fatalf("approve = %v, want %q", err, pairAnswerNotArrived) + } if !identity.Solo(rig.homeA) { - t.Fatal("setup: the earlier attempt should have left the marker") + t.Fatal("nothing was approved, yet the first computer stopped being solo") } - if _, err := door.Pending(context.Background(), link); !errors.Is(err, tui3.ErrAlreadyPaired) { - t.Fatalf("reopened link = %v, want ErrAlreadyPaired", err) + if _, err := rig.approveWith(t, []string{link}, "a\n"); err != nil { + t.Fatalf("retry = %v", err) + } + if err := <-done; err != nil { + t.Fatalf("the new device did not join: %v", err) + } + if identity.Solo(rig.homeA) { + t.Fatal("the first computer stays solo after the retry") + } +} + +// T3 for `codeaf pair approve`: the same link again, after an approval that +// never heard back, says the device is already paired and ends solo. +func TestPairApproveReopenedLinkOfALostApprovalEndsSolo(t *testing.T) { + rig := newPairRig(t) + link, done := rig.soloAsking(t) + rig.approvedEarlier(t, link, done) + out, err := rig.approveWith(t, []string{link}, "") + if err != nil || !strings.Contains(out.String(), tui3.ErrAlreadyPaired.Error()) { + t.Fatalf("reopened link = %v, printed:\n%s\nwant it to say already paired", err, out.String()) } if identity.Solo(rig.homeA) { t.Fatal("the first computer stays solo after the reopened link") } - // A retried answer on the old card settles the same way. - if err := door.Approve(context.Background(), p); err != nil { - t.Fatalf("retried approve = %v, want it settled as approved", err) +} + +// appApprovingAt is the app on the first computer, with its link approver +// pointed at a relay that loses replies while lose is set. +func (r *pairRig) appApprovingAt(t *testing.T, lose *atomic.Int32) *actionWire { + t.Helper() + t.Setenv(syncsetup.URLVar, r.url) + old := appLinkApprover + appLinkApprover = func(dir string) func(pair.Mailbox) (pair.Approver, error) { + return func(route pair.Mailbox) (pair.Approver, error) { + who, err := old(dir)(route) + if n := lose.Load(); n != 0 { + who.Dir = lostReply{who.Dir, n > 0} + } + return who, err + } + } + t.Cleanup(func() { appLinkApprover = old }) + _, _, w := appDeviceBackend(t, r.homeA) + return w +} + +// appApprove opens link in the app's Pair a computer and answers yes. +func appApprove(t *testing.T, w *actionWire, link, state string) appwire.Pairing { + t.Helper() + op := machineCall[appwire.PairResult](t, w, "pair.start", appwire.PairStartParams{Mode: "approve_link", Link: link}).Pairing + q := e2PairWait(t, w, op.ID, "question") + machineCall[appwire.PairResult](t, w, "pair.answer", appwire.PairAnswerParams{ID: op.ID, Question: q.Question.ID, Option: "y"}) + return e2PairWait(t, w, op.ID, state) +} + +const ( + appLoseNothing = 0 + appLoseReply = 1 // the relay decides but the reply is lost + appLoseAnswer = -1 // the approval never reaches the relay +) + +// T1 for the app: the lost reply is looked up, so the pairing is done, not +// failed, and the first computer stops being solo. +func TestAppApprovalWhoseReplyWasLostStillPairs(t *testing.T) { + rig := newPairRig(t) + link, done := rig.soloAsking(t) + var lose atomic.Int32 + lose.Store(appLoseReply) + w := rig.appApprovingAt(t, &lose) + appApprove(t, w, link, "done") + if err := <-done; err != nil { + t.Fatalf("the new device did not join: %v", err) + } + if identity.Solo(rig.homeA) { + t.Fatal("the first computer stays solo, so it never shares") + } +} + +// An app approval that never reached the relay fails as a plain retry, and the +// retry lets the device in. +func TestAppApprovalThatNeverArrivedCanBeRetried(t *testing.T) { + rig := newPairRig(t) + link, done := rig.soloAsking(t) + var lose atomic.Int32 + lose.Store(appLoseAnswer) + w := rig.appApprovingAt(t, &lose) + if got := appApprove(t, w, link, "failed"); got.Error == nil || got.Error.Message != appPairFailed { + t.Fatalf("approve = %+v, want %q", got.Error, appPairFailed) + } + if !identity.Solo(rig.homeA) { + t.Fatal("nothing was approved, yet the first computer stopped being solo") + } + lose.Store(appLoseNothing) + appApprove(t, w, link, "done") + if err := <-done; err != nil { + t.Fatalf("the new device did not join: %v", err) + } + if identity.Solo(rig.homeA) { + t.Fatal("the first computer stays solo after the retry") + } +} + +// T3 for the app: opening the link again, after an approval that never heard +// back, is done with already paired and ends solo, instead of failing as +// already answered. +func TestAppReopenedLinkOfALostApprovalEndsSolo(t *testing.T) { + rig := newPairRig(t) + link, done := rig.soloAsking(t) + rig.approvedEarlier(t, link, done) + var lose atomic.Int32 + w := rig.appApprovingAt(t, &lose) + op := machineCall[appwire.PairResult](t, w, "pair.start", appwire.PairStartParams{Mode: "approve_link", Link: link}).Pairing + got := e2PairWait(t, w, op.ID, "done") + if !slices.Contains(got.Lines, tui3.ErrAlreadyPaired.Error()) { + t.Fatalf("reopened link = %+v, want it to say already paired", got) + } + if identity.Solo(rig.homeA) { + t.Fatal("the first computer stays solo after the reopened link") } } diff --git a/cmd/codeaf/pairlink.go b/cmd/codeaf/pairlink.go index e36677a16b..db523cab0e 100644 --- a/cmd/codeaf/pairlink.go +++ b/cmd/codeaf/pairlink.go @@ -25,8 +25,13 @@ import ( "github.com/Agent-Field/codeaf/internal/pair" "github.com/Agent-Field/codeaf/internal/reqsign" "github.com/Agent-Field/codeaf/internal/syncsetup" + "github.com/Agent-Field/codeaf/internal/tui3" ) +// pairAnswerNotArrived is an approval that timed out with the request still +// waiting: nothing was decided, so the same command can simply run again. +const pairAnswerNotArrived = "sync did not answer in time and nothing was decided — run the same command again" + // linkTimeout outlasts one held poll so the answer, not the client, ends a wait. const linkTimeout = directory.MaxWait + 10*time.Second @@ -174,6 +179,15 @@ func (d pairDoor) approve(ctx context.Context, relay, typed string) error { return err } asking, err := who.Look(ctx, ref) + if errors.Is(err, pair.ErrLinkDecided) && approvedHere(who, ref.Code, "") == requestApprovedHere { + // An approval whose reply was lost: the device is in, so this computer is shared. + if err := identity.EndSolo(d.joining(false).Home); err != nil { + return err + } + d.recordOwnDeviceOrSay(ctx, route) + d.term.say(tui3.ErrAlreadyPaired.Error()) + return nil + } if err != nil { return err } @@ -182,7 +196,13 @@ func (d pairDoor) approve(ctx context.Context, relay, typed string) error { return d.decline(ctx, who, asking) } if err := who.Approve(ctx, asking); err != nil { - return err + err = settleApproval(who, asking, err) + if errors.Is(err, tui3.ErrAnswerNotArrived) { + return errors.New(pairAnswerNotArrived) + } + if err != nil { + return err + } } if err := identity.EndSolo(d.joining(false).Home); err != nil { return err diff --git a/docs/changes/unreleased/2314-pairing-followups.md b/docs/changes/unreleased/2314-pairing-followups.md new file mode 100644 index 0000000000..eb8e7cf518 --- /dev/null +++ b/docs/changes/unreleased/2314-pairing-followups.md @@ -0,0 +1,9 @@ +--- +kind: fixed +title: App and pair approve settle a lost approval reply; chat rows say when a take is running +pr: 2314 +surface: [engine, app, docs] +invalidates: + - "An approval from the desktop app or codeaf pair approve whose reply was lost still let the device join, but the approving computer reported a failure, stayed solo and never shared its chats, and reopening the link said that request was already answered; now both settle the approval they already gave, as the terminal chat screen does, and reopening a link that was already approved says the device is already paired. An approval that never arrived says to try again." + - "Was true: a `chats.list` row or `chat.changed` event looked the same while `agents.take` was still copying a chat as when no take had happened, so an app that stopped waiting could not tell the two apart. Is true now: every row carries `taking`, true while `agents.take` runs for that chat; a take that ends without moving the chat emits the row again with `taking:false`, and once a take has acquired the chat, rows that still show it elsewhere keep `taking:true` until a row says the chat is here." +--- diff --git a/docs/design/app-v3/PROTOCOL.md b/docs/design/app-v3/PROTOCOL.md index 89701999c7..435ed625bf 100644 --- a/docs/design/app-v3/PROTOCOL.md +++ b/docs/design/app-v3/PROTOCOL.md @@ -1414,7 +1414,15 @@ and rows omit the unknown time. Existing records are not backfilled by startup. `local_agent` is the app's saved/open folder id, resolved independently from `cell`. Remote-only chats have no local agent. Cell locations include the observed computer, fence and optional saved `head` in opaque `seen`, plus `can_take` and the engine's -`takeover_line` when known. Older chats working in the home folder stay local. +`takeover_line` when known. `taking` is true while `agents.take` runs for that +chat, including while it waits behind another take, so a row that says the chat +is here can still say it; a followed inventory emits the row again when the take +starts and when it ends without moving the chat. Once a take has acquired the +chat, rows that still show it elsewhere keep `taking:true` until a row says the +chat is here or a later take holds it, even when opening the moved chat fails. +A client that stopped waiting for a take reads `taking:false` +on a chat still elsewhere as a take that did not move it. +Older chats working in the home folder stay local. Copy `cell` and `seen` verbatim to `agents.take`; never open an agent using the remote cell id. It returns the adopted local Agent through the normal open path. Stale observations answer `conflict`, `detail:{kind:"holder_changed"}` and the diff --git a/internal/appwire/chats.go b/internal/appwire/chats.go index cf6438fd73..93f24b64d5 100644 --- a/internal/appwire/chats.go +++ b/internal/appwire/chats.go @@ -26,6 +26,7 @@ type ChatRow struct { Crew string `json:"crew,omitempty"` Seen Seen `json:"seen"` CanTake bool `json:"can_take"` + Taking bool `json:"taking"` Line string `json:"line,omitempty"` TakeoverLine string `json:"takeover_line,omitempty"` } From d5f07a356f3c0cf115ddbef8828dd91675e9a829 Mon Sep 17 00:00:00 2001 From: Santosh kumar <29346072+santoshkumarradha@users.noreply.github.com> Date: Tue, 3 Nov 2026 21:36:03 -0500 Subject: [PATCH 4/4] Plugins: desktop protocol over the connect layer (#2331) * Expose plugin connections through the desktop protocol * Expose plugin connection dates and provider state * fix: prevent cancelled plugin saves and scope asks by agent Keep account lookup and credential lock waits outside the flow mutex. Serialize only the final credential replacement with cancellation so an abandoned browser attempt cannot save later. Scope connection ask ids by agent across plugin, chat and Inbox events. * manual: the desktop page is called Connectors Co-Authored-By: Claude Opus 5.5 * fix: report whether connector keys were checked Preserve unset key variable names in connection refusals. Expose catalog checkability and whether a connection probe passed without guessing vendor endpoints. Co-Authored-By: Claude Opus 5.5 * docs: name the pull request in the plugins change entry Co-Authored-By: Claude Opus 5.5 --------- Co-authored-by: Claude Opus 5.5 --- cmd/codeaf/app.go | 54 ++- cmd/codeaf/appaccount.go | 1 + cmd/codeaf/appinbox.go | 2 + cmd/codeaf/appinbox_test.go | 8 +- cmd/codeaf/appplugins.go | 270 +++++++++++ cmd/codeaf/appplugins_facts.go | 147 ++++++ cmd/codeaf/appplugins_test.go | 445 ++++++++++++++++++ cmd/codeaf/appretry.go | 1 + cmd/codeaf/appstate.go | 13 + .../unreleased/2331-plugins-protocol.md | 35 ++ docs/design/app-v3/PROTOCOL.md | 103 ++++ internal/appwire/plugins.go | 139 ++++++ internal/appwire/plugins_test.go | 119 +++++ internal/appwire/server.go | 1 + internal/appwire/setup.go | 45 +- internal/appwire/wire.go | 22 +- internal/connect/auth.go | 100 ++-- internal/connect/auth_test.go | 50 ++ internal/connect/connect.go | 2 + internal/connect/featured.go | 15 + internal/connect/key.go | 32 +- internal/connect/key_failure.go | 40 ++ internal/connect/keyref_test.go | 2 +- internal/connect/mcp.go | 1 + internal/connect/mcp_auth.go | 7 +- internal/connect/mcp_auth_test.go | 42 ++ internal/connect/mcp_fake_test.go | 1 + internal/connect/mcp_tools.go | 7 + internal/connect/plugins.go | 167 +++++++ internal/connect/plugins_test.go | 213 +++++++++ internal/connect/store.go | 50 +- internal/manual/chat/accounts.md | 25 + internal/provider/dispatch.go | 9 +- internal/provider/phase.go | 22 +- internal/provider/plan_doors_test.go | 7 +- internal/provider/routefailure.go | 8 + internal/provider/routefailure_test.go | 8 + internal/session/connect.go | 4 +- internal/session/question.go | 4 +- internal/session/questionlifecycle_test.go | 41 ++ internal/session/tools_connect.go | 4 + 41 files changed, 2159 insertions(+), 107 deletions(-) create mode 100644 cmd/codeaf/appplugins.go create mode 100644 cmd/codeaf/appplugins_facts.go create mode 100644 cmd/codeaf/appplugins_test.go create mode 100644 docs/changes/unreleased/2331-plugins-protocol.md create mode 100644 internal/appwire/plugins.go create mode 100644 internal/appwire/plugins_test.go create mode 100644 internal/connect/featured.go create mode 100644 internal/connect/key_failure.go create mode 100644 internal/connect/plugins.go create mode 100644 internal/connect/plugins_test.go diff --git a/cmd/codeaf/app.go b/cmd/codeaf/app.go index 49873e4187..482b654a2e 100644 --- a/cmd/codeaf/app.go +++ b/cmd/codeaf/app.go @@ -552,11 +552,14 @@ func appConnOf(client *remote.Client, release func(), assembled bool) *appConn { // appBackend is the [appwire.Engine] over this process's conversations. type appBackend struct { - chats appChats - pair appPair - drives *driveBook - sync appSync - syncPending map[string][]appSyncFact + pluginPauses map[string]appPluginPause + pluginOllama appPluginOllama + pluginAuth map[string]*appPluginAttempt + chats appChats + pair appPair + drives *driveBook + sync appSync + syncPending map[string][]appSyncFact // devices observes existing sync traffic without starting a roster watch. devices *appDevices @@ -1976,14 +1979,18 @@ func (b *appBackend) hold(a *appAgent, conn *appConn, workspace string) appwire. return view } -// appAskID is the stable id of one ask: the engine's own question key. -func appAskID(q session.Question) string { - return string(q.Kind) + ":" + q.Token() +// appAskID scopes connection questions to their agent because plugin completion events carry only this id. +func appAskID(agent string, q session.Question) string { + id := string(q.Kind) + ":" + q.Token() + if q.Kind == session.QuestionConnect { + return agent + "/" + id + } + return id } // appAskOf is the engine's question as the app's ask. func appAskOf(agent string, q session.Question) appwire.Ask { - ask := appwire.Ask{ID: appAskID(q), Agent: agent, Kind: appwire.AskQuestion, Title: strings.TrimSpace(q.Head), Detail: strings.TrimSpace(q.Reason)} + ask := appwire.Ask{ID: appAskID(agent, q), Agent: agent, Kind: appwire.AskQuestion, Title: strings.TrimSpace(q.Head), Detail: strings.TrimSpace(q.Reason)} if q.Kind == session.QuestionConsent { ask.Kind, ask.Subkind = appwire.AskApproval, appwire.SubkindPermission ask.Irreversible = q.Stakes == session.StakesIrreversible @@ -2141,7 +2148,7 @@ func (b *appBackend) questionEvent(a *appAgent, id string, live *appLive, event q.ConsentScopes, q.AlwaysAsk = nil, "critical" } } - askID := appAskID(q) + askID := appAskID(id, q) switch event.Kind { case session.EventQuestion: if q.Subject.Kind == session.SubjectCall { @@ -2176,6 +2183,9 @@ func (b *appBackend) questionEvent(a *appAgent, id string, live *appLive, event return } live.box.Event(appwire.EventAskOpened, appwire.AskOpened{Ask: ask}) + if q.Kind == session.QuestionConnect { + live.box.Event(appwire.EventPluginsNeeded, appwire.PluginsNeeded{Agent: id, Plugin: q.Subject.Name, Why: q.Reason, AskID: askID}) + } if moved { live.box.Event(appwire.EventAgentChanged, appwire.AgentChanged{Agent: view}) } @@ -2236,6 +2246,9 @@ func (b *appBackend) questionEvent(a *appAgent, id string, live *appLive, event } } live.box.Event(appwire.EventAskClosed, appwire.AskClosed{Agent: id, ID: askID, By: by, Option: option, At: at}) + if q.Kind == session.QuestionConnect { + live.box.Event(appwire.EventPluginsNeededDone, appwire.PluginsNeededDone{AskID: askID}) + } receipt := appwire.Entry{ID: appReceiptEntryID(askID), Agent: id, Kind: appwire.EntryReceipt, At: at, Text: words} if scope != "" { receipt.Text = "Always allowed · " + scope + " · you · " + at.Local().Format("15:04") @@ -2291,6 +2304,7 @@ func (b *appBackend) died(a *appAgent, live *appLive) { mine := false var view appwire.Agent var asks []appwire.Ask + pluginAsks := map[string]bool{} b.locked(func() { if mine = a.live == live; !mine { return @@ -2300,6 +2314,11 @@ func (b *appBackend) died(a *appAgent, live *appLive) { a.view.State = appwire.StateStopped view = a.view asks = live.asks + for id, q := range live.questions { + if q.Kind == session.QuestionConnect { + pluginAsks[id] = true + } + } live.asks, live.questions = nil, map[string]session.Question{} }) if !mine { @@ -2307,6 +2326,9 @@ func (b *appBackend) died(a *appAgent, live *appLive) { } at := b.now() for _, ask := range asks { + if pluginAsks[ask.ID] { + live.box.Event(appwire.EventPluginsNeededDone, appwire.PluginsNeededDone{AskID: ask.ID}) + } live.box.Event(appwire.EventAskClosed, appwire.AskClosed{Agent: view.ID, ID: ask.ID, By: appwire.ClosedByStopped, At: at}) live.box.Event(appwire.EventEntryAdded, appwire.EntryAdded{Entry: appwire.Entry{ ID: appReceiptEntryID(ask.ID), Agent: view.ID, Kind: appwire.EntryReceipt, At: at, Text: appReceipt(appwire.ClosedByStopped, "", nil, at), @@ -3384,6 +3406,7 @@ func (b *appBackend) answerAskScope(id, askID, option, text, scope string) error refused = appwire.Errorf(appwire.CodeNotFound, "the ask %s is not open; it may have been answered elsewhere", askID) return } + q = b.connectedPluginQuestion(q) key, refused = appValidateQuestionAnswer(askID, q, option, text, scope) if refused != nil { return @@ -4477,11 +4500,16 @@ func (b *appBackend) Close() error { var warmed chan struct{} var balances *appBalances var stopModelNotices func() + var pluginAttempts []*appPluginAttempt b.locked(func() { if already = b.closing; already { return } b.closing = true + for id, attempt := range b.pluginAuth { + pluginAttempts = append(pluginAttempts, attempt) + delete(b.pluginAuth, id) + } if b.left != nil { close(b.left) } @@ -4509,6 +4537,12 @@ func (b *appBackend) Close() error { if already { return nil } + for _, attempt := range pluginAttempts { + attempt.cancel() + if attempt.flow != nil { + attempt.flow.Cancel() + } + } b.closePair() if b.stopSyncedSettings != nil { b.stopSyncedSettings() diff --git a/cmd/codeaf/appaccount.go b/cmd/codeaf/appaccount.go index 420914acdc..6d98353add 100644 --- a/cmd/codeaf/appaccount.go +++ b/cmd/codeaf/appaccount.go @@ -270,6 +270,7 @@ func (b *appBackend) providerConnected() bool { // providerChanged says provider.changed for one provider, with key carried // only when this process made the key itself and the app has never seen it. func (b *appBackend) providerChanged(p appProvider, key string) { + b.locked(func() { delete(b.pluginPauses, p.id) }) view := b.providerView(p, appSourcesNow(), appPersistedRows()) b.emit.Emit(appwire.EventProviderChanged, appwire.ProviderChanged{Provider: view, Key: key}) } diff --git a/cmd/codeaf/appinbox.go b/cmd/codeaf/appinbox.go index 5c83dee49c..229b53f991 100644 --- a/cmd/codeaf/appinbox.go +++ b/cmd/codeaf/appinbox.go @@ -410,6 +410,8 @@ func (b *appBackend) AnswerInbox(id, option, text, why, scope string) (time.Time if money, found := a.live.money[ask]; found { _, refused = appValidateMoneyAnswer(money.scope, ask, option, words, a.live.running) } else if question, found := a.live.questions[ask]; found { + question = b.connectedPluginQuestion(question) + q.Options = appAskOf(agent, question).Options _, refused = appValidateQuestionAnswer(ask, question, option, words, scope) } }) diff --git a/cmd/codeaf/appinbox_test.go b/cmd/codeaf/appinbox_test.go index 7c09871133..361b952806 100644 --- a/cmd/codeaf/appinbox_test.go +++ b/cmd/codeaf/appinbox_test.go @@ -239,7 +239,7 @@ func TestAppInboxRefusesInvalidAnswersBeforeHolding(t *testing.T) { rec.waitFor(t, 5*time.Second, func(_ string, data any) bool { _, ok := data.(appwire.InboxAddedEvent); return ok }) id, option, want := appInboxID(agent.ID, "ask:9"), "sqlite", appwire.CodeNotFound if strings.HasPrefix(scenario, "empty ") { - id, option, want = appInboxID(agent.ID, appAskID(q)), "", appwire.CodeBadParams + id, option, want = appInboxID(agent.ID, appAskID(agent.ID, q)), "", appwire.CodeBadParams } switch scenario { case "closed": @@ -313,7 +313,7 @@ func TestAppInboxValidatesLandingAndFuelAnswers(t *testing.T) { q := session.Question{ID: 9, Kind: tc.kind, Head: "What next?", Options: session.AnswerOptions(tc.kind)} conv.raise(session.Event{Kind: session.EventQuestion, Question: &q}) rec.waitFor(t, 5*time.Second, func(_ string, data any) bool { _, ok := data.(appwire.InboxAddedEvent); return ok }) - id := appInboxID(agent.ID, appAskID(q)) + id := appInboxID(agent.ID, appAskID(agent.ID, q)) until, err := b.AnswerInbox(id, tc.option, tc.text, tc.why, "") undo := b.UndoInbox(id) if tc.valid { @@ -329,7 +329,7 @@ func TestAppInboxValidatesLandingAndFuelAnswers(t *testing.T) { if done, _, err := b.ListInbox(appwire.InboxDone, ""); err != nil || len(done) != 0 { t.Errorf("validation wrote Done: %+v %v", done, err) } - if asks, err := b.ListAsks(agent.ID); err != nil || len(asks) != 1 || asks[0].ID != appAskID(q) { + if asks, err := b.ListAsks(agent.ID); err != nil || len(asks) != 1 || asks[0].ID != appAskID(agent.ID, q) { t.Errorf("validation changed asks: %+v %v", asks, err) } a := mustFind(t, b, agent.ID) @@ -358,7 +358,7 @@ func TestAppInboxRetainsFreeTextAnswers(t *testing.T) { q := session.Question{ID: 9, Kind: kind, Head: "What next?", Options: session.AnswerOptions(kind)} conv.raise(session.Event{Kind: session.EventQuestion, Question: &q}) rec.waitFor(t, 5*time.Second, func(_ string, data any) bool { _, ok := data.(appwire.InboxAddedEvent); return ok }) - id := appInboxID(agent.ID, appAskID(q)) + id := appInboxID(agent.ID, appAskID(agent.ID, q)) until, err := b.AnswerInbox(id, "", "use the existing reader", "", "") if err != nil || until.IsZero() { t.Fatalf("text-only answer = %v %v", until, err) diff --git a/cmd/codeaf/appplugins.go b/cmd/codeaf/appplugins.go new file mode 100644 index 0000000000..02307a1dd8 --- /dev/null +++ b/cmd/codeaf/appplugins.go @@ -0,0 +1,270 @@ +package main + +import ( + "context" + "errors" + "strings" + "time" + + "github.com/Agent-Field/codeaf/internal/appwire" + "github.com/Agent-Field/codeaf/internal/config" + "github.com/Agent-Field/codeaf/internal/connect" + "github.com/Agent-Field/codeaf/internal/guard" + "github.com/Agent-Field/codeaf/internal/session" +) + +type appPluginAttempt struct { + cancel context.CancelFunc + flow *connect.Flow +} + +func (b *appBackend) pluginManager() (*connect.Manager, error) { + m := v3Connect(b.crewProfile()) + if m == nil { + return nil, appwire.Errorf(appwire.CodeEngine, "connections could not be read") + } + return m, nil +} +func (b *appBackend) ListPlugins() (appwire.PluginsResult, error) { + m, err := b.pluginManager() + if err != nil { + return appwire.PluginsResult{}, err + } + providers, err := b.ListProviders() + if err != nil { + return appwire.PluginsResult{}, err + } + if providers == nil { + providers = []appwire.Provider{} + } + views := make([]appwire.PluginProvider, 0, len(providers)) + for _, provider := range providers { + view := appwire.PluginProvider{Provider: provider} + b.pluginProviderFacts(&view) + views = append(views, view) + } + featured, words := connect.Featured() + return appwire.PluginsResult{Plugins: m.Plugins(), Providers: views, Featured: featured, FeaturedWords: words}, nil +} +func (b *appBackend) DescribePlugin(id string) (connect.PluginDescription, error) { + m, err := b.pluginManager() + if err != nil { + return connect.PluginDescription{}, err + } + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + d, err := m.DescribePlugin(ctx, id) + if err != nil { + return d, appEngineError(err) + } + return d, nil +} +func (b *appBackend) ConnectPluginBrowser(p appwire.PluginBrowserParams) (appwire.PluginBrowserResult, error) { + m, err := b.pluginManager() + if err != nil { + return appwire.PluginBrowserResult{}, err + } + ctx, cancel := context.WithTimeout(context.Background(), appSignInWait) + attempt := &appPluginAttempt{cancel: cancel} + var refused error + b.locked(func() { + if b.closing { + refused = appwire.Errorf(appwire.CodeEngine, "the engine is closing") + return + } + if b.pluginAuth == nil { + b.pluginAuth = map[string]*appPluginAttempt{} + } + if b.pluginAuth[p.ID] != nil { + refused = appwire.Errorf(appwire.CodeBusy, "a sign-in is already waiting") + return + } + b.pluginAuth[p.ID] = attempt + }) + if refused != nil { + cancel() + return appwire.PluginBrowserResult{}, refused + } + flow, err := m.BeginAuth(ctx, p.ID, p.Answer) + if err != nil { + cancel() + b.locked(func() { + if b.pluginAuth[p.ID] == attempt { + delete(b.pluginAuth, p.ID) + } + }) + return appwire.PluginBrowserResult{}, appEngineError(err) + } + active := false + b.locked(func() { + if b.pluginAuth[p.ID] == attempt { + attempt.flow = flow + active = true + } + }) + if !active { + flow.Cancel() + cancel() + return appwire.PluginBrowserResult{}, appwire.Errorf(appwire.CodeEngine, "sign-in was cancelled") + } + guard.Go("app/plugin-sign-in", func() { + defer cancel() + defer flow.Cancel() + status, waitErr := flow.Wait(ctx) + current := false + b.locked(func() { + if b.pluginAuth[p.ID] == attempt { + current = true + delete(b.pluginAuth, p.ID) + } + }) + if !current { + return + } + changed := appwire.PluginsChanged{ID: p.ID} + if waitErr != nil { + changed.Error = connect.SignInFailureReason(waitErr) + } else { + b.notePluginConnected(p.ID, status.Account) + } + b.emit.Emit(appwire.EventPluginsChanged, changed) + }) + // The surface opens this link, as it does for providers.sign_in_start. + return appwire.PluginBrowserResult{State: "waiting", Link: flow.URL()}, nil +} +func (b *appBackend) CancelPlugin(id string) error { + var attempt *appPluginAttempt + b.locked(func() { + attempt = b.pluginAuth[id] + delete(b.pluginAuth, id) + }) + if attempt != nil { + attempt.cancel() + if attempt.flow != nil { + attempt.flow.Cancel() + } + } + return nil +} +func (b *appBackend) ConnectPluginKey(p appwire.PluginKeyParams) (appwire.PluginKeyResult, error) { + m, err := b.pluginManager() + if err != nil { + return appwire.PluginKeyResult{}, err + } + key := strings.TrimSpace(p.Key) + if p.KeyEnv != "" { + name := strings.TrimPrefix(strings.TrimSpace(p.KeyEnv), "$") + if !connect.ValidKeyEnv(name) { + return appwire.PluginKeyResult{Error: "shape", Words: "Name an environment variable such as STRIPE_KEY"}, nil + } + key = "$" + name + } + if p.Answer != "" { + key = strings.TrimSpace(p.Answer) + " " + key + } + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + status, err := m.ConnectKey(ctx, p.ID, key) + if err != nil { + var failure *connect.KeyFailure + if errors.As(err, &failure) { + return appwire.PluginKeyResult{Error: failure.Kind, Words: failure.Words}, nil + } + return appwire.PluginKeyResult{}, appEngineError(err) + } + b.notePluginConnected(p.ID, status.Account) + b.emit.Emit(appwire.EventPluginsChanged, appwire.PluginsChanged{ID: p.ID}) + return appwire.PluginKeyResult{Account: status.Account, Checked: status.Checked}, nil +} +func (b *appBackend) SetPluginCapability(p appwire.PluginCapabilityParams) error { + m, err := b.pluginManager() + if err != nil { + return err + } + if err = m.SetCapabilityState(p.ID, p.Capability, p.State); err != nil { + return appEngineError(err) + } + b.emit.Emit(appwire.EventPluginsChanged, appwire.PluginsChanged{ID: p.ID}) + return nil +} +func (b *appBackend) DisconnectPlugin(p appwire.PluginDisconnectParams) error { + m, err := b.pluginManager() + if err != nil { + return err + } + if p.Account != "" { + found := false + for _, s := range m.Services() { + if s.ID == p.ID && s.Account == p.Account { + found = true + } + } + if !found { + return appwire.Errorf(appwire.CodeNotFound, "that account is not connected") + } + } + _ = b.CancelPlugin(p.ID) + if err = m.Disconnect(p.ID); err != nil { + return appEngineError(err) + } + b.emit.Emit(appwire.EventPluginsChanged, appwire.PluginsChanged{ID: p.ID}) + return nil +} +func (b *appBackend) notePluginConnected(id, account string) { + var doors []interface{ NoteConnected(string, string) } + b.locked(func() { + for _, a := range b.agents { + if a.live != nil { + if door, ok := a.live.conn.Conv.(interface{ NoteConnected(string, string) }); ok { + doors = append(doors, door) + } + } + } + }) + for _, door := range doors { + door.NoteConnected(id, account) + } +} + +// setPlanPaused changes the same source rows used by the terminal's provider settings. +func (b *appBackend) setPlanPaused(policies map[string]string) error { + rows := config.PersistedSources(b.crewProfile()) + for id, policy := range policies { + if policy != config.PlanPausedWait && policy != config.PlanPausedUseMeter { + return appwire.Errorf(appwire.CodeBadParams, "when_plan_paused is wait or use pay-as-you-go") + } + found := false + for _, row := range rows { + if row.ID == id { + found = true + } + } + if !found { + return appwire.Errorf(appwire.CodeNotFound, "that provider is not connected") + } + } + err := config.UpdateSources(b.crewProfile(), func(rows []config.PersistedSource) []config.PersistedSource { + for i := range rows { + if policy, ok := policies[rows[i].ID]; ok { + rows[i].PlanPaused = policy + } + } + return rows + }) + if err != nil { + return appEngineError(err) + } + return nil +} + +// connectedPluginQuestion accepts a completed connection from either chat or Inbox. +func (b *appBackend) connectedPluginQuestion(q session.Question) session.Question { + if q.Kind != session.QuestionConnect || q.Subject.Name == "" { + return q + } + if m := v3Connect(b.crewProfile()); m != nil && m.Connected(q.Subject.Name) { + q.Options = session.AnswerOptions(session.QuestionConnect) + q.Input = session.InputShape{} + } + return q +} diff --git a/cmd/codeaf/appplugins_facts.go b/cmd/codeaf/appplugins_facts.go new file mode 100644 index 0000000000..00cdc25276 --- /dev/null +++ b/cmd/codeaf/appplugins_facts.go @@ -0,0 +1,147 @@ +package main + +import ( + "context" + "encoding/json" + "io" + "net" + "net/http" + "net/url" + "strings" + "time" + + "github.com/Agent-Field/codeaf/internal/appwire" + "github.com/Agent-Field/codeaf/internal/codexauth" + "github.com/Agent-Field/codeaf/internal/config" + "github.com/Agent-Field/codeaf/internal/env" + "github.com/Agent-Field/codeaf/internal/guard" + "github.com/Agent-Field/codeaf/internal/modelsource" + "github.com/Agent-Field/codeaf/internal/provider" + "github.com/Agent-Field/codeaf/internal/session" +) + +type appPluginPause struct { + reset string + until time.Time +} +type appPluginOllama struct { + address string + checked time.Time + probing bool + running *bool +} + +func (b *appBackend) pluginProviderFacts(view *appwire.PluginProvider) { + profile := b.crewProfile() + rows := config.PersistedSources(profile) + sources := config.ResolveSources(profile, config.APIKeyAt(profile), config.DefaultBaseURL) + handed := false + if credentials := config.AppCredentialSource(); credentials != nil { + _, handed = credentials.Key(view.ID) + } + if view.ID == modelsource.DefaultID { + switch config.APIKeySourceAt(profile) { + case config.APIKeySourceOpenRouter: + view.KeyEnv = "OPENROUTER_API_KEY" + case config.APIKeySourceOpenAI: + view.KeyEnv = "OPENAI_API_KEY" + } + } else if service, ok := sources.ByID(view.ID); ok && !handed { + // Match the credential precedence used by config.sourceKeyFromRow. + if name := strings.TrimSpace(service.Source.KeyEnv); name != "" && strings.TrimSpace(env.Value(name)) != "" { + view.KeyEnv = name + } else { + for _, row := range rows { + if row.ID == view.ID && strings.TrimSpace(row.Key) == "" && strings.TrimSpace(env.Value(row.KeyEnv)) != "" { + view.KeyEnv = row.KeyEnv + } + } + } + } + if view.ID == "codex" { + if tokens, err := codexauth.Load(profile); err == nil { + view.Account = tokens.Email + } + } + b.locked(func() { + if pause, ok := b.pluginPauses[view.ID]; ok { + if !pause.until.IsZero() && !time.Now().Before(pause.until) { + delete(b.pluginPauses, view.ID) + } else { + view.PlanPaused = &appwire.PluginPlanPaused{ResetsAt: pause.reset} + } + } + }) + if view.ID == "ollama" { + b.pluginOllamaFacts(view) + } +} + +// pluginPlanPhase retains the same refusal the terminal reports, without parsing its display sentence. +func (b *appBackend) pluginPlanPhase(news session.PhaseNews) { + if news.SourceID == "" { + return + } + b.locked(func() { + if news.Phase == provider.PhasePlanPaused { + if b.pluginPauses == nil { + b.pluginPauses = map[string]appPluginPause{} + } + _, until := provider.RouteFailureOf(&provider.PlanPauseError{Reset: news.PlanReset}) + b.pluginPauses[news.SourceID] = appPluginPause{reset: news.PlanReset, until: until} + } else if (news.Phase == provider.PhaseWriting || news.Phase == provider.PhaseThinking) && news.Door != "" && news.Door != "pay-as-you-go" { + delete(b.pluginPauses, news.SourceID) + } + }) +} + +// pluginOllamaFacts returns the last local probe; network I/O never holds up plugins.list. +func (b *appBackend) pluginOllamaFacts(view *appwire.PluginProvider) { + address := strings.TrimSuffix(strings.TrimRight(view.Address, "/"), "/v1") + parsed, err := url.Parse(address) + if err != nil || (parsed.Scheme != "http" && parsed.Scheme != "https") { + return + } + host := parsed.Hostname() + if host != "localhost" && !net.ParseIP(host).IsLoopback() { + return + } + start := false + b.locked(func() { + if b.closing { + return + } + probe := &b.pluginOllama + if probe.address != address { + *probe = appPluginOllama{address: address} + } + view.Running = probe.running + if !probe.probing && (probe.checked.IsZero() || time.Since(probe.checked) >= 5*time.Second) { + probe.probing, start = true, true + } + }) + if !start { + return + } + guard.Go("app/plugin-ollama-probe", func() { + ctx, cancel := context.WithTimeout(context.Background(), 200*time.Millisecond) + defer cancel() + request, err := http.NewRequestWithContext(ctx, http.MethodGet, address+"/api/tags", nil) + running := false + if err == nil { + client := &http.Client{CheckRedirect: func(*http.Request, []*http.Request) error { return http.ErrUseLastResponse }} + if response, err := client.Do(request); err == nil { + var listing struct { + Models *[]json.RawMessage `json:"models"` + } + running = response.StatusCode == http.StatusOK && json.NewDecoder(io.LimitReader(response.Body, 1<<20)).Decode(&listing) == nil && listing.Models != nil + _ = response.Body.Close() + } + } + b.locked(func() { + if b.pluginOllama.address == address { + b.pluginOllama.running, b.pluginOllama.checked, b.pluginOllama.probing = &running, time.Now(), false + } + }) + }) +} diff --git a/cmd/codeaf/appplugins_test.go b/cmd/codeaf/appplugins_test.go new file mode 100644 index 0000000000..ecee01c14d --- /dev/null +++ b/cmd/codeaf/appplugins_test.go @@ -0,0 +1,445 @@ +package main + +import ( + "encoding/json" + "fmt" + "github.com/Agent-Field/codeaf/internal/appwire" + "github.com/Agent-Field/codeaf/internal/codexauth" + "github.com/Agent-Field/codeaf/internal/config" + "github.com/Agent-Field/codeaf/internal/connect" + "github.com/Agent-Field/codeaf/internal/modelsource" + "github.com/Agent-Field/codeaf/internal/provider" + "github.com/Agent-Field/codeaf/internal/session" + "io" + "net/http" + "net/http/httptest" + "path/filepath" + "runtime" + "strings" + "testing" + "time" +) + +func TestAppPluginsShareStoreAndQuestionAnswer(t *testing.T) { + conv := &fakeConversation{} + b, rec, agent := workBackend(t, conv) + b.profileDir = t.TempDir() + m := v3Connect(b.profileDir) + if err := m.SetCapabilityState("google", "mail-read", connect.StateOff); err != nil { + t.Fatal(err) + } + d, err := b.DescribePlugin("google") + if err != nil || d.Capabilities[0].State != connect.StateOff { + t.Fatalf("describe = %+v %v", d, err) + } + q := session.ConnectQuestion("connect-1", "Google", false, "", false) + q.Subject.Name = "google" + conv.onAnswer = func(answer session.Answer) { + conv.raise(session.Event{Kind: session.EventQuestionAnswered, Question: &q, Answer: &answer}) + } + conv.raise(session.Event{Kind: session.EventQuestion, Question: &q}) + i := rec.waitFor(t, 5*time.Second, appAskEvent(appwire.EventPluginsNeeded)) + rec.mu.Lock() + needed := rec.data[i].(appwire.PluginsNeeded) + rec.mu.Unlock() + if needed.Plugin != "google" || needed.AskID != appAskID(agent.ID, q) || needed.Agent != agent.ID { + t.Fatalf("needed = %+v", needed) + } + if err := b.AnswerAsk(agent.ID, needed.AskID, "2", ""); err != nil { + t.Fatal(err) + } + rec.waitFor(t, 5*time.Second, appAskEvent(appwire.EventPluginsNeededDone)) + asks, err := b.ListAsks(agent.ID) + if err != nil || len(asks) != 0 { + t.Fatalf("asks after not now: %+v %v", asks, err) + } +} + +func TestAppPluginsListKeyShapeAndProviderPolicy(t *testing.T) { + b := newFakeBackend(newAppRecorder(), nil) + defer b.Close() + b.profileDir = t.TempDir() + list, err := b.ListPlugins() + if err != nil { + t.Fatal(err) + } + if len(list.Plugins) != len(v3Connect(b.profileDir).Services()) || len(list.Featured) != 4 || list.FeaturedWords["slack"].What != "Channels, threads and DMs" { + t.Fatalf("list: %+v", list) + } + answer, err := b.ConnectPluginKey(appwire.PluginKeyParams{ID: "stripe", Key: "two keys"}) + if err != nil || answer.Error != "shape" { + t.Fatalf("key shape: %+v %v", answer, err) + } + if err := b.SetPluginCapability(appwire.PluginCapabilityParams{ID: "google", Capability: "mail-send", State: connect.StateYes}); err != nil { + t.Fatal(err) + } + if got := v3Connect(b.profileDir).CapabilityState("google", "mail-send"); got != connect.StateYes { + t.Fatalf("capability store: %s", got) + } + if err := config.WriteSources(b.profileDir, []config.PersistedSource{{ID: "z-ai", Written: "Z.ai"}}); err != nil { + t.Fatal(err) + } + settings, err := b.SetSettings(appwire.SettingsPatch{WhenPlanPaused: map[string]string{"z-ai": config.PlanPausedUseMeter}}) + if err != nil || settings.WhenPlanPaused["z-ai"] != config.PlanPausedUseMeter { + t.Fatalf("policy: %+v %v", settings.WhenPlanPaused, err) + } + rows := config.PersistedSources(b.profileDir) + if len(rows) != 1 || rows[0].PlanPaused != config.PlanPausedUseMeter { + t.Fatalf("source store: %+v", rows) + } +} + +func TestAppPluginsListKeyCheckability(t *testing.T) { + b := newFakeBackend(newAppRecorder(), nil) + defer b.Close() + b.profileDir = t.TempDir() + list, err := b.ListPlugins() + if err != nil { + t.Fatal(err) + } + keys, checkable := 0, 0 + for _, plugin := range list.Plugins { + if plugin.Kind != "key" { + continue + } + keys++ + raw, err := json.Marshal(plugin) + if err != nil { + t.Fatal(err) + } + want := `"key_checkable":false` + if plugin.ID == "breezy" || plugin.ID == "monaco" { + want = `"key_checkable":true` + checkable++ + } + if !strings.Contains(string(raw), want) { + t.Errorf("%s: missing %s", plugin.ID, want) + } + } + if keys != 99 || checkable != 2 { + t.Fatalf("keys = %d, checkable = %d; want 99 and 2", keys, checkable) + } +} + +func TestAppPluginsKeyResultReportsOnlyPassedProbes(t *testing.T) { + for _, tt := range []struct { + name, id, key, failure string + status, calls int + checked bool + }{ + {name: "unchecked", id: "stripe", key: "fixture-key"}, + {name: "checked", id: "breezy", key: "fixture-key", status: 200, calls: 1, checked: true}, + {name: "refused", id: "breezy", key: "fixture-key", status: 401, calls: 1, failure: "refused"}, + {name: "no answer", id: "breezy", key: "fixture-key", calls: 1, failure: "no_answer"}, + {name: "shape", id: "breezy", key: "two keys", failure: "shape"}, + } { + t.Run(tt.name, func(t *testing.T) { + calls := 0 + original := http.DefaultTransport + http.DefaultTransport = roundTripFunc(func(r *http.Request) (*http.Response, error) { + calls++ + if r.URL.String() != "https://api.breezy.hr/v3/companies" { + t.Errorf("unexpected request: %s", r.URL) + } + if tt.status == 0 { + return nil, fmt.Errorf("fixture: no answer") + } + return &http.Response{StatusCode: tt.status, Status: http.StatusText(tt.status), Header: http.Header{}, Body: io.NopCloser(strings.NewReader(`{}`))}, nil + }) + t.Cleanup(func() { http.DefaultTransport = original }) + b := newFakeBackend(newAppRecorder(), nil) + defer b.Close() + b.profileDir = t.TempDir() + answer, err := b.ConnectPluginKey(appwire.PluginKeyParams{ID: tt.id, Key: tt.key}) + if err != nil || answer.Error != tt.failure { + t.Fatalf("answer = %+v, err = %v; want failure %q", answer, err, tt.failure) + } + raw, err := json.Marshal(answer) + if err != nil { + t.Fatal(err) + } + if want := fmt.Sprintf(`"checked":%t`, tt.checked); !strings.Contains(string(raw), want) { + t.Errorf("answer = %s; want %s", raw, want) + } + if calls != tt.calls { + t.Errorf("probe requests = %d; want %d", calls, tt.calls) + } + if got := v3Connect(b.profileDir).Connected(tt.id); got != (tt.failure == "") { + t.Errorf("connected = %t; failure = %q", got, tt.failure) + } + }) + } +} + +func TestAppPluginsInboxAnswersConnectedKey(t *testing.T) { + conv := &fakeConversation{} + b, rec, agent := workBackend(t, conv) + b.profileDir = t.TempDir() + q := session.ConnectQuestion("connect-key", "Stripe", true, "", true) + q.Subject.Name = "stripe" + conv.onAnswer = func(answer session.Answer) { + conv.raise(session.Event{Kind: session.EventQuestionAnswered, Question: &q, Answer: &answer}) + } + conv.raise(session.Event{Kind: session.EventQuestion, Question: &q}) + rec.waitFor(t, 5*time.Second, appAskEvent(appwire.EventPluginsNeeded)) + // The bundled Stripe entry declares no probe, so this makes no network call. + result, err := b.ConnectPluginKey(appwire.PluginKeyParams{ID: "stripe", Key: "fake-plugin-key"}) + if err != nil || result.Error != "" { + t.Fatalf("connect: %+v %v", result, err) + } + if _, err := b.AnswerInbox(appInboxID(agent.ID, appAskID(agent.ID, q)), "1", "", "", ""); err != nil { + t.Fatal(err) + } + rec.waitFor(t, 7*time.Second, appAskEvent(appwire.EventPluginsNeededDone)) + conv.mu.Lock() + defer conv.mu.Unlock() + if len(conv.answers) != 1 || conv.answers[0].Key != "1" || conv.answers[0].Change != "" { + t.Fatalf("answer: %+v", conv.answers) + } +} + +func TestAppPluginsQuestionDoneWhenConversationDies(t *testing.T) { + conv := &fakeConversation{} + _, rec, agent := workBackend(t, conv) + q := session.ConnectQuestion("connect-dies", "Google", false, "", false) + q.Subject.Name = "google" + conv.raise(session.Event{Kind: session.EventQuestion, Question: &q}) + rec.waitFor(t, 5*time.Second, appAskEvent(appwire.EventPluginsNeeded)) + conv.mu.Lock() + end := conv.endLane + conv.mu.Unlock() + end() + i := rec.waitFor(t, 2*time.Second, appAskEvent(appwire.EventPluginsNeededDone)) + rec.mu.Lock() + defer rec.mu.Unlock() + if got := rec.data[i].(appwire.PluginsNeededDone).AskID; got != appAskID(agent.ID, q) { + t.Fatalf("%s closed %s", agent.ID, got) + } +} + +func TestAppPluginsRoundTwoPauseFacts(t *testing.T) { + b := newFakeBackend(newAppRecorder(), nil) + defer b.Close() + b.profileDir = t.TempDir() + reset := time.Now().Add(time.Hour).UTC().Format(time.RFC3339) + b.phaseNews(session.PhaseNews{Phase: provider.PhasePlanPaused, SourceID: "z-ai", PlanReset: reset}) + list, err := b.ListPlugins() + if err != nil { + t.Fatal(err) + } + raw, _ := json.Marshal(list) + if !strings.Contains(string(raw), `"plan_paused":{"resets_at":"`+reset+`"}`) { + t.Fatalf("missing plan pause: %s", raw) + } + b.phaseNews(session.PhaseNews{Phase: provider.PhaseWriting, SourceID: "z-ai", Door: "pay-as-you-go"}) + list, _ = b.ListPlugins() + raw, _ = json.Marshal(list) + if !strings.Contains(string(raw), `"plan_paused"`) { + t.Fatal("metered overflow erased plan pause") + } + b.phaseNews(session.PhaseNews{Phase: provider.PhaseWriting, SourceID: "z-ai", Door: "coding plan"}) + list, _ = b.ListPlugins() + raw, _ = json.Marshal(list) + if strings.Contains(string(raw), `"plan_paused"`) { + t.Fatalf("successful plan still paused: %s", raw) + } + b.phaseNews(session.PhaseNews{Phase: provider.PhasePlanPaused, SourceID: "z-ai", PlanReset: time.Now().Add(-time.Hour).UTC().Format(time.RFC3339)}) + list, _ = b.ListPlugins() + raw, _ = json.Marshal(list) + if strings.Contains(string(raw), `"plan_paused"`) { + t.Fatal("past reset still paused") + } +} + +func TestAppPluginsOllamaProbeDoesNotBlockList(t *testing.T) { + entered, release := make(chan struct{}), make(chan struct{}) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/api/tags" { + t.Errorf("probe path = %s", r.URL.Path) + } + close(entered) + <-release + _, _ = w.Write([]byte(`{"models":[]}`)) + })) + defer server.Close() + defer close(release) + b, _, _ := appProviderTest(t) + b.vendored = appStubbed("ollama", func(source *modelsource.Source) { source.Address = server.URL + "/v1" }) + read := func() appwire.PluginProvider { + list, err := b.ListPlugins() + if err != nil { + t.Fatal(err) + } + for _, p := range list.Providers { + if p.ID == "ollama" { + return p + } + } + t.Fatal("missing Ollama") + return appwire.PluginProvider{} + } + view := read() + select { + case <-entered: + case <-time.After(time.Second): + t.Fatal("probe did not start") + } + if view.Running != nil { + t.Fatal("probe blocked list or invented result") + } + deadline := time.Now().Add(time.Second) + for view.Running == nil && time.Now().Before(deadline) { + runtime.Gosched() + view = read() + } + if view.Running == nil || *view.Running { + t.Fatal("stalled Ollama did not time out as not running") + } +} + +func TestAppPluginsProviderAccountAndKeyVariable(t *testing.T) { + b, _, profile := appProviderTest(t) + t.Setenv("PLUGIN_PLAN_KEY", "fake-plugin-provider-key") + if err := config.WriteSources(profile, []config.PersistedSource{{ID: "z-ai", Written: "z-ai", KeyEnv: "PLUGIN_PLAN_KEY", Door: "coding-plan"}}); err != nil { + t.Fatal(err) + } + if err := codexauth.Save(profile, codexauth.Tokens{Email: "person@example.test", AccessToken: "fake-access", RefreshToken: "fake-refresh"}); err != nil { + t.Fatal(err) + } + list, err := b.ListPlugins() + if err != nil { + t.Fatal(err) + } + seen := 0 + for _, p := range list.Providers { + if p.ID == "z-ai" { + seen++ + if p.KeyEnv != "PLUGIN_PLAN_KEY" { + t.Fatalf("key variable = %q", p.KeyEnv) + } + } + if p.ID == "codex" { + seen++ + if p.Account != "person@example.test" { + t.Fatalf("account = %q", p.Account) + } + } + if p.ConnectedAt != "" { + t.Fatalf("invented provider date = %q", p.ConnectedAt) + } + } + if seen != 2 { + t.Fatalf("metadata providers = %d", seen) + } + raw, _ := json.Marshal(list) + if strings.Contains(string(raw), "fake-plugin-provider-key") || strings.Contains(string(raw), "fake-access") { + t.Fatal("secret leaked in provider facts") + } +} + +func TestAppPluginsDefaultProviderKeyVariable(t *testing.T) { + b, _, _ := appProviderTest(t) + t.Setenv("OPENAI_API_KEY", "sk-or-fake-plugin-fallback") + list, err := b.ListPlugins() + if err != nil { + t.Fatal(err) + } + for _, p := range list.Providers { + if p.ID == "openrouter" { + if p.KeyEnv != "OPENAI_API_KEY" { + t.Fatalf("key_env = %q, want OPENAI_API_KEY", p.KeyEnv) + } + return + } + } + t.Fatal("missing OpenRouter") +} + +func TestAppPluginsOllamaRunning(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { _, _ = w.Write([]byte(`{"models":[]}`)) })) + defer server.Close() + b, _, _ := appProviderTest(t) + b.vendored = appStubbed("ollama", func(source *modelsource.Source) { source.Address = server.URL + "/v1" }) + deadline := time.Now().Add(time.Second) + for time.Now().Before(deadline) { + list, err := b.ListPlugins() + if err != nil { + t.Fatal(err) + } + for _, p := range list.Providers { + if p.ID == "ollama" && p.Running != nil { + if !*p.Running { + t.Fatal("Ollama answered but running=false") + } + return + } + } + runtime.Gosched() + } + t.Fatal("Ollama probe never became running") +} + +func TestAppPluginsAskIDsAreUniqueAcrossAgents(t *testing.T) { + rec := newAppRecorder() + work1, work2 := t.TempDir(), t.TempDir() + conv1, conv2 := &fakeConversation{}, &fakeConversation{} + conversations := map[string]*fakeConversation{work1: conv1, work2: conv2} + b := newFakeBackend(rec, func(ask appOpenAsk) (*appConn, error) { + return &appConn{Conv: conversations[ask.Workspace], Workspace: ask.Workspace, Transcript: ask.Transcript, Release: func() {}}, nil + }) + defer b.Close() + b.state = &appState{path: filepath.Join(t.TempDir(), appStateName)} + b.mint = func(work string) (string, error) { return filepath.Join(work, "transcript.jsonl"), nil } + first, err := mustCreate(t, b, appwire.CreateAsk{Workspace: work1}) + if err != nil { + t.Fatal(err) + } + second, err := mustCreate(t, b, appwire.CreateAsk{Workspace: work2}) + if err != nil { + t.Fatal(err) + } + if first.ID == second.ID { + t.Fatal("fixture must create distinct agents") + } + q := session.ConnectQuestion("connect-1", "Google", false, "", false) + q.Subject.Name = "google" + askIDs := make([]string, 0, 2) + for _, item := range []struct { + agent appwire.Agent + conv *fakeConversation + }{{first, conv1}, {second, conv2}} { + item.conv.onAnswer = func(answer session.Answer) { + item.conv.raise(session.Event{Kind: session.EventQuestionAnswered, Question: &q, Answer: &answer}) + } + item.conv.raise(session.Event{Kind: session.EventQuestion, Question: &q}) + i := rec.waitFor(t, time.Second, func(event string, data any) bool { + needed, ok := data.(appwire.PluginsNeeded) + return ok && needed.Agent == item.agent.ID + }) + rec.mu.Lock() + askIDs = append(askIDs, rec.data[i].(appwire.PluginsNeeded).AskID) + rec.mu.Unlock() + } + if askIDs[0] == askIDs[1] { + t.Errorf("two agents emitted the same ask_id: %q", askIDs[0]) + } + for i, item := range []struct { + agent appwire.Agent + conv *fakeConversation + }{{first, conv1}, {second, conv2}} { + if i == 0 { + if err := b.AnswerAskWith(item.agent.ID, askIDs[i], appwire.AskAnswer{Option: "2"}); err != nil { + t.Fatal(err) + } + } else { + item.conv.mu.Lock() + end := item.conv.endLane + item.conv.mu.Unlock() + end() + } + rec.waitFor(t, time.Second, func(event string, data any) bool { + done, ok := data.(appwire.PluginsNeededDone) + return ok && done.AskID == askIDs[i] + }) + } +} diff --git a/cmd/codeaf/appretry.go b/cmd/codeaf/appretry.go index 07762f906b..21bb61c1a0 100644 --- a/cmd/codeaf/appretry.go +++ b/cmd/codeaf/appretry.go @@ -56,6 +56,7 @@ func (b *appBackend) listenPhases() func() { // phaseNews says a conversation's wait before its reply is asked again. func (b *appBackend) phaseNews(news session.PhaseNews) { + b.pluginPlanPhase(news) // Only the reply a person is reading is said: a task's own request and the // errands beside a reply (its naming, its memory) have their own pages. if news.Subject != "" || news.Role != lane.RoleTalk { diff --git a/cmd/codeaf/appstate.go b/cmd/codeaf/appstate.go index dc075a9e17..b91981954d 100644 --- a/cmd/codeaf/appstate.go +++ b/cmd/codeaf/appstate.go @@ -430,6 +430,14 @@ func (b *appBackend) Settings() (appwire.Settings, error) { if rung := config.DefaultEffortAt(profile); rung.Valid() { settings.Effort = rung.String() } + settings.WhenPlanPaused = map[string]string{} + for _, row := range config.PersistedSources(b.crewProfile()) { + policy := row.PlanPaused + if policy == "" { + policy = config.PlanPausedWait + } + settings.WhenPlanPaused[row.ID] = policy + } settingsExtra(&settings, file, profile) return settings, nil } @@ -445,6 +453,11 @@ func (b *appBackend) SetSettings(patch appwire.SettingsPatch) (appwire.Settings, return appwire.Settings{}, err } } + if patch.WhenPlanPaused != nil { + if err := b.setPlanPaused(patch.WhenPlanPaused); err != nil { + return appwire.Settings{}, err + } + } apply := func(key, raw string) error { row, ok := config.NewSettings(config.SettingsOptions{ProfileDir: profile}).Row(key) if !ok { diff --git a/docs/changes/unreleased/2331-plugins-protocol.md b/docs/changes/unreleased/2331-plugins-protocol.md new file mode 100644 index 0000000000..80ee58c2ec --- /dev/null +++ b/docs/changes/unreleased/2331-plugins-protocol.md @@ -0,0 +1,35 @@ +--- +pr: 2331 +kind: added +title: Desktop plugin methods share terminal connections and capability settings +surface: [engine, app, docs] +invalidates: + - "The desktop protocol had no plugin connection methods. Engines advertising plugins now expose the existing connect catalog, browser and key connections, capabilities and disconnect actions." +--- + +Seven plugin methods use the existing account and capability stores. +Connect questions emit plugin events with the same ask id used by chat and +Inbox. The desktop answers through asks.answer after connecting, without +starting a second sign-in. Provider plan-pause settings update existing source +rows through settings.set. + +Featured copy is engine data. Short blurbs derive from catalog descriptions, +and connected tool-server lists retain the server's own titles and read flags. + +Round 2 exposes provider account emails, credential variable names and observed +plan pauses with vendor reset values. A bounded background probe reports local +Ollama availability. Plugin connections now save connection and key dates in +the existing credential store; older entries and provider dates remain absent. +Connection questions use short catalog labels, with the fact and key supplied +separately on the wire. The providers.list response remains unchanged. + +Cancelling browser sign-in returns while an account lookup is pending and prevents +that attempt from saving credentials. Browser connections also recheck cancellation +inside the credential commit after waiting for its lock. Plugin ask ids include +the owning agent, so answering or stopping one agent cannot dismiss another +agent's connection request with the same session question key. + +Key connections now return `checked`, true only after a catalog probe passes. +Listed connectors expose `key_checkable`; saving a key without a catalog probe +does not verify it. An unset key environment variable keeps its name in the +refusal instead of replacing that name with `[key]`. diff --git a/docs/design/app-v3/PROTOCOL.md b/docs/design/app-v3/PROTOCOL.md index 435ed625bf..37cbed0702 100644 --- a/docs/design/app-v3/PROTOCOL.md +++ b/docs/design/app-v3/PROTOCOL.md @@ -1680,3 +1680,106 @@ manager chat or composer (and discards the returned step chats). They require a desktop update before pairing with this engine for jobs without a manager. The hello feature is capability advertisement, not client negotiation; it does not by itself make those older desktops compatible. + +## plugins methods + +The optional `plugins` hello feature enables these methods. They use +`internal/connect`, the same account and capability stores as terminal +`/connect`. Provider connection methods and the `Provider` type stay unchanged. + +| method | params | result | +|---|---|---| +| `plugins.list` | `{}` | `PluginsResult` | +| `plugins.describe` | `in.PluginID` | `PluginDescription` | +| `plugins.connect_browser` | `in.PluginBrowserParams` | `PluginBrowserResult` | +| `plugins.cancel` | `in.PluginID` | `Empty` | +| `plugins.connect_key` | `in.PluginKeyParams` | `PluginKeyResult` | +| `plugins.set_capability` | `in.PluginCapabilityParams` | `Empty` | +| `plugins.disconnect` | `in.PluginDisconnectParams` | `Empty` | + +`plugins.list` returns `{plugins, providers, featured, featured_words}`. +Each plugin has `id, name, kind, category, blurb, connected, key_checkable`, and optionally +`account, key_env, expired, connected_at, key_saved_at, ask: {label, answers?}, +key_hint: {words, url?}`. +Kind is `browser`, `key` or `server`. Category uses the catalog's spelling. +`key_checkable` is true only for key connectors with a catalog probe. +Blurb shortens the existing catalog words. Counts come from the returned +catalog. `featured` contains ordered ids; `featured_words[id]` contains +`{what, does}`. + +Plugin dates are UTC RFC 3339 instants saved in the existing credential store. +`connected_at` records the first saved connection; replacing a key preserves it +and updates `key_saved_at`. Browser token refresh does not change either date. +Older entries retain absent dates; disconnect removes them. +`ask.label` is the catalog's short field label (for example, "Workspace"), +not the terminal's instruction to paste the fact and key together. The protocol +accepts the fact in `answer` and the credential in `key` or `key_env` separately. + +Only `plugins.list` extends each existing provider view with optional +`account, key_env, connected_at, running, plan_paused: {resets_at?}`. +`providers.list` is unchanged. Account comes from ChatGPT's stored email; +key_env names the environment variable actually supplying the credential. +Provider connection dates have no stored source and remain absent. + +`running` is only for loopback Ollama addresses. A background GET to `/api/tags` +has a 200 ms deadline, accepts an Ollama model-list response and follows no +redirects. The list returns immediately; `running` is absent until the first +probe finishes, then true or false, cached for five seconds. Remote Ollama +addresses leave it absent. Fetch the list again to read a completed probe. + +`plan_paused` comes from subscription refusal phase news, identified by source. +`resets_at` preserves the vendor's reset value, including readable clock text; +it is absent when the vendor gives none. The refusal is remembered for this +engine connection, expires when its reset instant can be parsed, and clears +when the plan answers again. Metered overflow does not clear a paused plan. +A generic quota/rate limit is not evidence of a paused subscription. No account +quota polling or durable pause history is added. + +`plugins.describe {id}` returns +`{capabilities, scopes_words, tools?, notes?, accounts?, key_note?}`. +Capabilities contain `{id, phrase, covers, acts, state, tools}`. State is +`yes`, `ask` or `off`; the same `SetCapabilityState` store serves chat's +Always answer and `plugins.set_capability {id, capability, state}`. +Off removes that capability's tools when the next session builds its tools. +Connected servers' `tools` come from the existing MCP listing, as +`{name, title, read_only}`. Tool names grouped under a capability also form +its `covers` text. + +`plugins.connect_browser {id, answer?}` answers +`{state: "waiting", link}`. The desk opens `link` in the browser. +Completion emits `plugins.changed`; `error` carries a safe sign-in failure. +`plugins.cancel {id}` abandons an unfinished attempt. Closing the engine +also cancels pending attempts. + +`plugins.connect_key {id, key?, key_env?, answer?}` accepts one key or one +environment variable name. `answer` supplies the workspace when needed. +It returns `{checked: bool, account?}` on success, or +`{checked: false, error: "shape"|"refused"|"no_answer", words}` as a successful wire answer. +`checked` is true only when a probe ran and passed; a key saved without a +catalog probe returns false and is first tested on real use. +A shape failure reaches no vendor, a refusal retains at most 120 characters, +and a missing answer saves nothing. Key values never appear in replies. +`plugins.disconnect {id, account?}` uses the existing disconnect path. +The optional account must match the stored account. + +`settings.set` accepts `when_plan_paused: {[provider]: policy}`, where policy +is `wait` or `use pay-as-you-go`. It changes the existing persisted source +rows. `settings.get` and `settings.changed` return the map in +`settings.when_plan_paused`. An unknown provider is refused. + +| event | data | +|---|---| +| `plugins.changed` | `PluginsChanged`: `{id, error?}` | +| `plugins.needed` | `PluginsNeeded`: `{agent, plugin, why, ask_id}` | +| `plugins.needed_done` | `PluginsNeededDone`: `{ask_id}` | + +A connect question reaches the app on the existing `WatchQuestions` lane. +`plugins.needed` identifies that same question, also listed by `asks.list`. +After connecting through the plugin methods, the desk calls +`asks.answer {agent, id: ask_id, option: "1"}` to continue the turn without +a second sign-in. `option: "2"` means Not now and saves no refusal. +Answering in either chat or Inbox resolves the same question. +An answer or withdrawal emits `plugins.needed_done`. + +The current account store holds one account per plugin, including Slack. +It has no plugin call history; a Lately section remains absent. diff --git a/internal/appwire/plugins.go b/internal/appwire/plugins.go new file mode 100644 index 0000000000..79931abae4 --- /dev/null +++ b/internal/appwire/plugins.go @@ -0,0 +1,139 @@ +package appwire + +import ( + "reflect" + "strings" + + "github.com/Agent-Field/codeaf/internal/connect" +) + +const ( + FeaturePlugins = "plugins" + EventPluginsChanged = "plugins.changed" + EventPluginsNeeded = "plugins.needed" + EventPluginsNeededDone = "plugins.needed_done" +) + +type PluginsResult struct { + Plugins []connect.Plugin `json:"plugins"` + Providers []PluginProvider `json:"providers"` + Featured []string `json:"featured"` + FeaturedWords map[string]connect.FeaturedWords `json:"featured_words"` +} + +// PluginProvider extends the provider view only on the plugins page. +type PluginProvider struct { + Provider + Account string `json:"account,omitempty"` + KeyEnv string `json:"key_env,omitempty"` + ConnectedAt string `json:"connected_at,omitempty"` + Running *bool `json:"running,omitempty"` + PlanPaused *PluginPlanPaused `json:"plan_paused,omitempty"` +} +type PluginPlanPaused struct { + ResetsAt string `json:"resets_at,omitempty"` +} + +type PluginID struct { + ID string `json:"id"` +} +type PluginBrowserParams struct { + ID string `json:"id"` + Answer string `json:"answer,omitempty"` +} +type PluginBrowserResult struct { + State string `json:"state" enum:"waiting"` + Link string `json:"link"` +} +type PluginKeyParams struct { + ID string `json:"id"` + Key string `json:"key,omitempty"` + KeyEnv string `json:"key_env,omitempty"` + Answer string `json:"answer,omitempty"` +} +type PluginKeyResult struct { + Checked bool `json:"checked"` + Account string `json:"account,omitempty"` + Error string `json:"error,omitempty" enum:"shape,refused,no_answer"` + Words string `json:"words,omitempty"` +} +type PluginCapabilityParams struct { + ID string `json:"id"` + Capability string `json:"capability"` + State connect.CapabilityState `json:"state" enum:"yes,ask,off"` +} +type PluginDisconnectParams struct { + ID string `json:"id"` + Account string `json:"account,omitempty"` +} +type Plugins interface { + ListPlugins() (PluginsResult, error) + DescribePlugin(string) (connect.PluginDescription, error) + ConnectPluginBrowser(PluginBrowserParams) (PluginBrowserResult, error) + CancelPlugin(string) error + ConnectPluginKey(PluginKeyParams) (PluginKeyResult, error) + SetPluginCapability(PluginCapabilityParams) error + DisconnectPlugin(PluginDisconnectParams) error +} +type PluginsChanged struct { + ID string `json:"id"` + Error string `json:"error,omitempty"` +} +type PluginsNeeded struct { + Agent string `json:"agent"` + Plugin string `json:"plugin"` + Why string `json:"why"` + AskID string `json:"ask_id"` +} +type PluginsNeededDone struct { + AskID string `json:"ask_id"` +} + +func pluginMethod[P any, R any](id func(P) string, call func(Plugins, P) (R, error)) method { + return typed(func(e Engine, p P) (*R, error) { + plugins, ok := e.(Plugins) + if !ok { + return nil, Errorf(CodeUnknownMethod, "plugins are unavailable") + } + if id != nil { + if _, err := required("id", id(p)); err != nil { + return nil, err + } + } + out, err := call(plugins, p) + return &out, err + }) +} +func init() { + methods["plugins.list"] = pluginMethod(nil, func(e Plugins, _ struct{}) (PluginsResult, error) { return e.ListPlugins() }) + methods["plugins.describe"] = pluginMethod(func(p PluginID) string { return p.ID }, func(e Plugins, p PluginID) (connect.PluginDescription, error) { return e.DescribePlugin(p.ID) }) + methods["plugins.connect_browser"] = pluginMethod(func(p PluginBrowserParams) string { return p.ID }, func(e Plugins, p PluginBrowserParams) (PluginBrowserResult, error) { return e.ConnectPluginBrowser(p) }) + methods["plugins.cancel"] = pluginMethod(func(p PluginID) string { return p.ID }, func(e Plugins, p PluginID) (Empty, error) { return Empty{}, e.CancelPlugin(p.ID) }) + methods["plugins.connect_key"] = pluginMethod(func(p PluginKeyParams) string { return p.ID }, func(e Plugins, p PluginKeyParams) (PluginKeyResult, error) { + if strings.TrimSpace(p.Key) != "" && strings.TrimSpace(p.KeyEnv) != "" { + return PluginKeyResult{}, Errorf(CodeBadParams, "supply key or key_env, not both") + } + return e.ConnectPluginKey(p) + }) + methods["plugins.set_capability"] = pluginMethod(func(p PluginCapabilityParams) string { return p.ID }, func(e Plugins, p PluginCapabilityParams) (Empty, error) { + if _, err := required("capability", p.Capability); err != nil { + return Empty{}, err + } + switch p.State { + case connect.StateYes, connect.StateAsk, connect.StateOff: + default: + return Empty{}, Errorf(CodeBadParams, "state is yes, ask or off") + } + return Empty{}, e.SetPluginCapability(p) + }) + methods["plugins.disconnect"] = pluginMethod(func(p PluginDisconnectParams) string { return p.ID }, func(e Plugins, p PluginDisconnectParams) (Empty, error) { return Empty{}, e.DisconnectPlugin(p) }) + eventShapes[EventPluginsChanged] = reflect.TypeFor[PluginsChanged]() + eventShapes[EventPluginsNeeded] = reflect.TypeFor[PluginsNeeded]() + eventShapes[EventPluginsNeededDone] = reflect.TypeFor[PluginsNeededDone]() +} +func withPluginsFeature(h Hello, e Engine) Hello { + if _, ok := e.(Plugins); ok { + return withFeature(h, FeaturePlugins) + } + return h +} diff --git a/internal/appwire/plugins_test.go b/internal/appwire/plugins_test.go new file mode 100644 index 0000000000..e5930411dc --- /dev/null +++ b/internal/appwire/plugins_test.go @@ -0,0 +1,119 @@ +package appwire + +import ( + "encoding/json" + "github.com/Agent-Field/codeaf/internal/connect" + "strings" + "testing" +) + +func TestPluginsMethodsOptional(t *testing.T) { + for _, name := range []string{"plugins.list", "plugins.describe", "plugins.connect_browser", "plugins.cancel", "plugins.connect_key", "plugins.set_capability", "plugins.disconnect"} { + m, ok := methods[name] + if !ok { + t.Errorf("missing %s", name) + continue + } + _, err := m.call(&fakeEngine{}, json.RawMessage(`{"id":"google"}`)) + if wireCode(t, err) != CodeUnknownMethod { + t.Errorf("%s: %v", name, err) + } + } +} + +func TestPluginsPlanPausedSettingsPatch(t *testing.T) { + patch, err := decodeSettingsPatch(json.RawMessage(`{"when_plan_paused":{"z-ai":"use pay-as-you-go"}}`)) + if err != nil || patch.WhenPlanPaused["z-ai"] != "use pay-as-you-go" { + t.Fatalf("patch: %+v %v", patch, err) + } + for _, raw := range []string{`{"when_plan_paused":null}`, `{"when_plan_paused":{"z-ai":"spend"}}`} { + if _, err := decodeSettingsPatch(json.RawMessage(raw)); err == nil { + t.Fatalf("accepted %s", raw) + } + } +} + +type pluginsEngine struct { + fakeEngine + calls []string +} + +func (e *pluginsEngine) ListPlugins() (PluginsResult, error) { + e.calls = append(e.calls, "list") + return PluginsResult{Plugins: []connect.Plugin{}, Providers: []PluginProvider{}, Featured: []string{"google"}, FeaturedWords: map[string]connect.FeaturedWords{"google": {What: "Gmail and Calendar", Does: "Read mail"}}}, nil +} +func (e *pluginsEngine) DescribePlugin(id string) (connect.PluginDescription, error) { + e.calls = append(e.calls, id) + return connect.PluginDescription{Capabilities: []connect.PluginCapability{}, ScopesWords: []string{}}, nil +} +func (e *pluginsEngine) ConnectPluginBrowser(p PluginBrowserParams) (PluginBrowserResult, error) { + e.calls = append(e.calls, p.ID+":"+p.Answer) + return PluginBrowserResult{State: "waiting", Link: "https://example.test/sign-in"}, nil +} +func (e *pluginsEngine) CancelPlugin(id string) error { e.calls = append(e.calls, id); return nil } +func (e *pluginsEngine) ConnectPluginKey(p PluginKeyParams) (PluginKeyResult, error) { + e.calls = append(e.calls, p.ID+":"+p.KeyEnv+":"+p.Answer) + return PluginKeyResult{Error: "shape", Words: "one key"}, nil +} +func (e *pluginsEngine) SetPluginCapability(p PluginCapabilityParams) error { + e.calls = append(e.calls, p.ID+":"+p.Capability+":"+string(p.State)) + return nil +} +func (e *pluginsEngine) DisconnectPlugin(p PluginDisconnectParams) error { + e.calls = append(e.calls, p.ID+":"+p.Account) + return nil +} + +func TestPluginsDispatchValidationAndFeature(t *testing.T) { + e := &pluginsEngine{} + w := startGraphWire(t, e) + found := false + for _, f := range features(w.next()) { + found = found || f == FeaturePlugins + } + if !found { + t.Fatal("hello does not advertise plugins") + } + for _, tt := range []struct{ name, raw, want string }{ + {"plugins.list", `{}`, "list"}, + {"plugins.describe", `{"id":"google"}`, "google"}, + {"plugins.connect_browser", `{"id":"datadog","answer":"us1"}`, "datadog:us1"}, + {"plugins.cancel", `{"id":"google"}`, "google"}, + {"plugins.connect_key", `{"id":"stripe","key_env":"STRIPE_KEY","answer":"workspace"}`, "stripe:STRIPE_KEY:workspace"}, + {"plugins.set_capability", `{"id":"google","capability":"mail-read","state":"off"}`, "google:mail-read:off"}, + {"plugins.disconnect", `{"id":"slack","account":"workspace"}`, "slack:workspace"}, + } { + if _, err := methods[tt.name].call(e, json.RawMessage(tt.raw)); err != nil { + t.Fatalf("%s: %v", tt.name, err) + } + if got := e.calls[len(e.calls)-1]; got != tt.want { + t.Fatalf("%s forwarded %q", tt.name, got) + } + } + calls := len(e.calls) + for _, tt := range []struct{ name, raw string }{ + {"plugins.describe", `{}`}, + {"plugins.set_capability", `{"id":"google","capability":"mail-read","state":"always"}`}, + {"plugins.set_capability", `{"id":"google","state":"off"}`}, + {"plugins.connect_key", `{"id":"stripe","key":"secret","key_env":"STRIPE_KEY"}`}, + } { + if _, err := methods[tt.name].call(e, json.RawMessage(tt.raw)); wireCode(t, err) != CodeBadParams { + t.Fatalf("accepted %s: %v", tt.raw, err) + } + } + if len(e.calls) != calls { + t.Fatal("invalid request reached the engine") + } + if h := withPluginsFeature(Hello{}, &fakeEngine{}); len(h.Features) != 0 { + t.Fatal("unsupported engine advertises plugins") + } + raw, err := SchemaJSON() + if err != nil { + t.Fatal(err) + } + for _, word := range []string{"plugins.needed", "plugins.needed_done", "plugins.changed", "featured_words", "key_env", "ask_id"} { + if !strings.Contains(string(raw), word) { + t.Errorf("schema lacks %s", word) + } + } +} diff --git a/internal/appwire/server.go b/internal/appwire/server.go index 051bc1337e..b52f76ee1b 100644 --- a/internal/appwire/server.go +++ b/internal/appwire/server.go @@ -202,6 +202,7 @@ func (s *Server) Serve(ctx context.Context, in io.Reader, hello Hello, engine En hello = withSlashCatalogFeature(hello, engine) hello = withSyncFeatures(hello, engine) hello = withSkillsFeatures(hello, engine) + hello = withPluginsFeature(hello, engine) hello = withHealthFeature(hello, engine) hello = withManualCorpusFeature(hello, engine) hello = withMachinePolicyFeatures(hello, engine) diff --git a/internal/appwire/setup.go b/internal/appwire/setup.go index def16cd30a..4708491316 100644 --- a/internal/appwire/setup.go +++ b/internal/appwire/setup.go @@ -2,6 +2,7 @@ package appwire import ( "encoding/json" + "github.com/Agent-Field/codeaf/internal/config" "math" "strings" ) @@ -198,23 +199,24 @@ type ProviderParams struct { // optional, and a field present is changed. A money field sent as null means // none. [decodeSettingsPatch] reads exactly these fields and refuses any other. type SettingsPatchWire struct { - MemoryEnabled *bool `json:"memory_enabled,omitempty"` - DefaultModel *string `json:"default_model,omitempty"` - AsksBefore *string `json:"asks_before,omitempty"` - WorkFolder *string `json:"work_folder,omitempty"` - Effort *string `json:"effort,omitempty"` - BudgetUSD *float64 `json:"budget_usd,omitempty" nullable:"true"` - DayCapUSD *float64 `json:"day_cap_usd,omitempty" nullable:"true"` - TaskCapUSD *float64 `json:"task_cap_usd,omitempty" nullable:"true"` - ProposedTaskClockS *int `json:"proposed_task_clock_s,omitempty"` - IdleCloseDays *int `json:"idle_close_days,omitempty"` - Appearance *string `json:"appearance,omitempty" enum:"system,light,dark"` - TextSize *string `json:"text_size,omitempty" enum:"small,medium,large"` - OpenAtLogin *bool `json:"open_at_login,omitempty"` - DockBadge *bool `json:"dock_badge,omitempty"` - ChecksModel *string `json:"checks_model,omitempty"` - PlannerModel *string `json:"planner_model,omitempty"` - FallbackModel *string `json:"fallback_model,omitempty"` + WhenPlanPaused map[string]string `json:"when_plan_paused,omitempty"` + MemoryEnabled *bool `json:"memory_enabled,omitempty"` + DefaultModel *string `json:"default_model,omitempty"` + AsksBefore *string `json:"asks_before,omitempty"` + WorkFolder *string `json:"work_folder,omitempty"` + Effort *string `json:"effort,omitempty"` + BudgetUSD *float64 `json:"budget_usd,omitempty" nullable:"true"` + DayCapUSD *float64 `json:"day_cap_usd,omitempty" nullable:"true"` + TaskCapUSD *float64 `json:"task_cap_usd,omitempty" nullable:"true"` + ProposedTaskClockS *int `json:"proposed_task_clock_s,omitempty"` + IdleCloseDays *int `json:"idle_close_days,omitempty"` + Appearance *string `json:"appearance,omitempty" enum:"system,light,dark"` + TextSize *string `json:"text_size,omitempty" enum:"small,medium,large"` + OpenAtLogin *bool `json:"open_at_login,omitempty"` + DockBadge *bool `json:"dock_badge,omitempty"` + ChecksModel *string `json:"checks_model,omitempty"` + PlannerModel *string `json:"planner_model,omitempty"` + FallbackModel *string `json:"fallback_model,omitempty"` } type setupResult struct { @@ -305,6 +307,15 @@ func decodeSettingsPatch(raw json.RawMessage) (SettingsPatch, error) { for name := range fields { var err error switch name { + case "when_plan_paused": + if err = json.Unmarshal(fields[name], &patch.WhenPlanPaused); err != nil || patch.WhenPlanPaused == nil { + return SettingsPatch{}, Errorf(CodeBadParams, "when_plan_paused must map provider ids to wait or use pay-as-you-go") + } + for id, policy := range patch.WhenPlanPaused { + if strings.TrimSpace(id) == "" || (policy != config.PlanPausedWait && policy != config.PlanPausedUseMeter) { + return SettingsPatch{}, Errorf(CodeBadParams, "when_plan_paused must map provider ids to wait or use pay-as-you-go") + } + } case "default_model": err = text(name, &patch.DefaultModel) case "asks_before": diff --git a/internal/appwire/wire.go b/internal/appwire/wire.go index 41c929c1af..f06dbbf1d3 100644 --- a/internal/appwire/wire.go +++ b/internal/appwire/wire.go @@ -679,16 +679,17 @@ type MemorySettings struct { } type Settings struct { - Memory MemorySettings `json:"memory"` - DefaultModel string `json:"default_model"` - BudgetUSD *float64 `json:"budget_usd"` - AsksBefore string `json:"asks_before"` - DayCapUSD *float64 `json:"day_cap_usd"` - WorkFolder string `json:"work_folder"` - TaskCapUSD *float64 `json:"task_cap_usd"` - ProposedTaskClockS int `json:"proposed_task_clock_s"` - IdleCloseDays int `json:"idle_close_days"` - Effort string `json:"effort,omitempty" enum:"low,medium,high"` + WhenPlanPaused map[string]string `json:"when_plan_paused"` + Memory MemorySettings `json:"memory"` + DefaultModel string `json:"default_model"` + BudgetUSD *float64 `json:"budget_usd"` + AsksBefore string `json:"asks_before"` + DayCapUSD *float64 `json:"day_cap_usd"` + WorkFolder string `json:"work_folder"` + TaskCapUSD *float64 `json:"task_cap_usd"` + ProposedTaskClockS int `json:"proposed_task_clock_s"` + IdleCloseDays int `json:"idle_close_days"` + Effort string `json:"effort,omitempty" enum:"low,medium,high"` // The General rows the window draws by (Wave 8): kept here so every window // reads the same answer. Appearance string `json:"appearance" enum:"system,light,dark"` @@ -712,6 +713,7 @@ type Settings struct { // money figure sent as null is cleared, which the settings that always have a // figure refuse. type SettingsPatch struct { + WhenPlanPaused map[string]string MemoryEnabled *bool DefaultModel *string BudgetUSD *Figure diff --git a/internal/connect/auth.go b/internal/connect/auth.go index b3d8ca9a1f..5bbb55f70e 100644 --- a/internal/connect/auth.go +++ b/internal/connect/auth.go @@ -88,12 +88,14 @@ type Flow struct { plug Plug url string cancel context.CancelFunc + ctx context.Context done chan flowResult - mu sync.Mutex - settled bool - status Status - err error + mu sync.Mutex + settled bool + committed bool + status Status + err error } // flowResult is what the browser round-trip eventually produced. @@ -188,6 +190,7 @@ func (m *Manager) BeginAuth(ctx context.Context, id, answer string) (*Flow, erro plug: plug, url: doorURL, cancel: cancel, + ctx: runContext, done: make(chan flowResult, 1), } if refusal, refuses := plug.(refusing); refuses { @@ -254,44 +257,81 @@ func (f *Flow) Wait(ctx context.Context) (Status, error) { // Cancel abandons a connection nobody is going to finish — the person pressed // escape, or closed the tab and came back. It releases the loopback address so // the next attempt can have it. -func (f *Flow) Cancel() { f.cancel() } +func (f *Flow) Cancel() { + f.mu.Lock() + defer f.mu.Unlock() + f.cancel() + if !f.settled && !f.committed { + f.settled = true + f.status = Status{Service: f.plug.Service()} + f.err = context.Canceled + } +} + +// commit serializes the atomic credential replacement with cancellation, after slow work and lock waits finish. +func (f *Flow) commit(write func() error) error { + f.mu.Lock() + defer f.mu.Unlock() + if err := f.ctx.Err(); err != nil { + return err + } + if err := write(); err != nil { + return err + } + f.committed = true + return nil +} // settle records the outcome once: it asks whose account this is, writes the // connection to the store, and fixes the answer every later [Flow.Wait] gets. func (f *Flow) settle(ctx context.Context, result flowResult) (Status, error) { f.mu.Lock() - defer f.mu.Unlock() if f.settled { - return f.status, f.err + status, err := f.status, f.err + f.mu.Unlock() + return status, err } - f.settled = true - f.cancel() + f.mu.Unlock() + defer f.cancel() - service := f.plug.Service() - if result.err != nil { - f.status, f.err = Status{Service: service}, result.err - return f.status, f.err + waitContext := ctx + ctx, cancel := context.WithCancel(f.ctx) + stop := context.AfterFunc(waitContext, cancel) + if waitContext.Err() != nil { + cancel() } - if result.keys == nil { - f.status = Status{Service: service} - f.err = fmt.Errorf("connect %s: the service granted nothing", service.Name) - return f.status, f.err + defer stop() + defer cancel() + service := f.plug.Service() + status, err := Status{Service: service}, result.err + if err == nil && result.keys == nil { + err = fmt.Errorf("connect %s: the service granted nothing", service.Name) } - - entry := stored{Keys: result.keys, Scopes: granted(result.keys, service.Scopes)} - // THE IDENTITY PROBE MAY FAIL WITHOUT FAILING THE CONNECTION. The keys - // work; only the label is missing, and a missing label renders as - // nothing. - if account, err := f.plug.Account(ctx, staticClient(ctx, result.keys)); err == nil { - entry.Account = account + if err == nil { + entry := stored{Keys: result.keys, Scopes: granted(result.keys, service.Scopes)} + // The identity probe may fail without failing the connection; only its label is lost. + if account, probeErr := f.plug.Account(ctx, staticClient(ctx, result.keys)); probeErr == nil { + entry.Account = account + } + err = waitContext.Err() + if err == nil { + err = ctx.Err() + } + if err == nil { + err = f.manager.store.putConnected(ctx, service.ID, entry, f.commit) + } + if err != nil { + err = fmt.Errorf("connect %s: %w", service.Name, err) + } else { + status = Status{Service: service, Connected: true, Account: entry.Account} + } } - if err := f.manager.store.put(service.ID, entry); err != nil { - f.status = Status{Service: service} - f.err = fmt.Errorf("connect %s: %w", service.Name, err) - return f.status, f.err + f.mu.Lock() + defer f.mu.Unlock() + if !f.settled { + f.settled, f.status, f.err = true, status, err } - f.status = Status{Service: service, Connected: true, Account: entry.Account} - return f.status, nil + return f.status, f.err } // granted is what the person actually agreed to, for the record kept beside the diff --git a/internal/connect/auth_test.go b/internal/connect/auth_test.go index d040251011..58723e2e1f 100644 --- a/internal/connect/auth_test.go +++ b/internal/connect/auth_test.go @@ -4,6 +4,7 @@ import ( "context" "crypto/sha256" "encoding/base64" + "errors" "io" "net/http" "net/url" @@ -545,3 +546,52 @@ func TestSameKeys(t *testing.T) { t.Error("nil must only equal nil") } } + +func TestCancelDuringAccountLookupReturnsWithoutSaving(t *testing.T) { + onFreePort(t) + entered, release := make(chan struct{}), make(chan struct{}) + mux := http.NewServeMux() + mux.HandleFunc("/token", func(w http.ResponseWriter, r *http.Request) { + writeJSON(t, w, map[string]any{"access_token": "access-one", "token_type": "Bearer"}) + }) + mux.HandleFunc("/gmail/v1/users/me/profile", func(w http.ResponseWriter, r *http.Request) { + close(entered) + <-release + writeJSON(t, w, map[string]any{"emailAddress": "me@example.com"}) + }) + fakeService(t, mux) + defer close(release) + manager, _ := testManager(t) + flow, err := manager.BeginAuth(context.Background(), "google", "") + if err != nil { + t.Fatal(err) + } + drive(t, flow, "the-code") + waited := make(chan error, 1) + go func() { _, err := flow.Wait(context.Background()); waited <- err }() + select { + case <-entered: + case <-time.After(time.Second): + t.Fatal("account lookup did not start") + } + cancelled := make(chan struct{}) + go func() { flow.Cancel(); close(cancelled) }() + select { + case <-cancelled: + case <-time.After(100 * time.Millisecond): + t.Error("Cancel blocked on account lookup") + } + // Release the fake even on the buggy path so the waiter cannot leak. + release <- struct{}{} + select { + case err := <-waited: + if !errors.Is(err, context.Canceled) { + t.Errorf("Wait = %v, want cancellation", err) + } + case <-time.After(time.Second): + t.Fatal("Wait did not finish") + } + if manager.Connected("google") { + t.Error("cancelled attempt saved credentials") + } +} diff --git a/internal/connect/connect.go b/internal/connect/connect.go index 2fdddcb20b..e68588f443 100644 --- a/internal/connect/connect.go +++ b/internal/connect/connect.go @@ -141,6 +141,8 @@ type Status struct { Service // Connected reports that codeaf holds usable keys for this service. Connected bool + // Checked reports that this key connection's probe ran and passed. + Checked bool // Account is the address the keys belong to. THE EMPTINESS LAW: an // account we do not know is empty, never a placeholder — a screen that // renders nothing is honest, one that renders "unknown" is not. diff --git a/internal/connect/featured.go b/internal/connect/featured.go new file mode 100644 index 0000000000..36d5ecdaa9 --- /dev/null +++ b/internal/connect/featured.go @@ -0,0 +1,15 @@ +package connect + +type FeaturedWords struct { + What string `json:"what"` + Does string `json:"does"` +} + +func Featured() ([]string, map[string]FeaturedWords) { + return []string{"google", "slack", "notion", "sentry"}, map[string]FeaturedWords{ + "google": {"Gmail and Calendar", "Agents search and read your mail, see your calendar and add events. Sending mail and inviting people ask first."}, + "slack": {"Channels, threads and DMs", "Agents search and read your Slack across the workspaces you sign in to. Sending a message asks first."}, + "notion": {"Pages, databases and search", "Notion's own tools. Agents read and search your workspace; anything that changes a page asks first."}, + "sentry": {"Issues, events and releases", "Sentry's own tools. Agents read the errors behind a bug and check a release; anything that acts asks first."}, + } +} diff --git a/internal/connect/key.go b/internal/connect/key.go index be5aa1c376..bd7efa0607 100644 --- a/internal/connect/key.go +++ b/internal/connect/key.go @@ -44,7 +44,7 @@ type keyService interface { sign(base http.RoundTripper, key string) http.RoundTripper // check proves a key against the cheap read the catalog names, or says // nothing when it names none. - check(ctx context.Context, client *http.Client) error + check(ctx context.Context, client *http.Client) (bool, error) } var _ keyService = (*keyPlug)(nil) @@ -121,9 +121,9 @@ func (p *keyPlug) sign(base http.RoundTripper, key string) http.RoundTripper { // alternative — guessing at an address that might be free and might charge, and // might delete something — is worse than letting the first real call be the one // that says the key is wrong, which it will say in the service's own words. -func (p *keyPlug) check(ctx context.Context, client *http.Client) error { +func (p *keyPlug) check(ctx context.Context, client *http.Client) (bool, error) { if p.probe.address == "" { - return nil + return false, nil } method := p.probe.method if method == "" { @@ -131,18 +131,18 @@ func (p *keyPlug) check(ctx context.Context, client *http.Client) error { } request, err := http.NewRequestWithContext(ctx, method, p.probe.address, nil) if err != nil { - return err + return false, err } response, err := client.Do(request) if err != nil { - return err + return false, err } defer func() { _ = response.Body.Close() }() body, _ := io.ReadAll(io.LimitReader(response.Body, maxResponseBytes)) if accepted(p.probe.accepts, response.StatusCode) { - return nil + return true, nil } - return fmt.Errorf("%s: %s", response.Status, apiMessage(body)) + return false, fmt.Errorf("%s: %s", response.Status, apiMessage(body)) } // accepted reads the answers the catalog counts as yes, falling back to the two @@ -238,15 +238,15 @@ func (m *Manager) ConnectKey(ctx context.Context, id string, key string) (Status return Status{}, fmt.Errorf("%s is connected in a browser, not with a key", service.Name) } if strings.TrimSpace(key) == "" { - return Status{}, fmt.Errorf("%s needs a key", service.Name) + return Status{}, keyFailure("shape", fmt.Errorf("%s needs a key", service.Name), "") } blank, key, err := holder.read(key) if err != nil { - return Status{}, err + return Status{}, keyFailure("shape", err, "") } address, err := holder.address(blank) if err != nil { - return Status{}, err + return Status{}, keyFailure("shape", err, "") } // A NAME IS NOT A KEY, AND IT IS NOT STORED AS ONE. What a person pasted is // read once, here, and lands in whichever of the two fields it means @@ -263,17 +263,19 @@ func (m *Manager) ConnectKey(ctx context.Context, id string, key string) (Status // with an answer in it — go and set it — and it is said before anything // is written down, because a connection to a name that resolves to // nothing is a row that says "connected" over a request that cannot go. - return Status{}, err + return Status{}, keyFailure("shape", err, "") } - if err := holder.check(ctx, client); err != nil { - return Status{}, fmt.Errorf("%s did not accept that key: %w", service.Name, err) + checked, err := holder.check(ctx, client) + if err != nil { + secret, _ := entry.secret(service.Name) + return Status{}, keyFailure("refused", fmt.Errorf("%s did not accept that key: %w", service.Name, err), secret) } - if err := m.store.put(service.ID, entry); err != nil { + if err := m.store.putConnected(ctx, service.ID, entry, nil); err != nil { return Status{}, fmt.Errorf("connect %s: %w", service.Name, err) } service.Address = address // Account stays empty. See [keyPlug.Account]. - return Status{Service: service, Connected: true, KeyEnv: entry.KeyEnv}, nil + return Status{Service: service, Connected: true, KeyEnv: entry.KeyEnv, Checked: checked}, nil } // keyClient is the account-bearing client for one key entry. diff --git a/internal/connect/key_failure.go b/internal/connect/key_failure.go new file mode 100644 index 0000000000..5dacc1d9bc --- /dev/null +++ b/internal/connect/key_failure.go @@ -0,0 +1,40 @@ +package connect + +import ( + "errors" + "net" + "strings" +) + +// KeyFailure is a connection answer that a surface can render beside its field. +type KeyFailure struct { + Kind string + Words string +} + +func (e *KeyFailure) Error() string { return e.Words } + +func keyFailure(kind string, err error, secret string) error { + var network net.Error + if errors.As(err, &network) { + kind = "no_answer" + } + words := err.Error() + if secret != "" { + words = strings.ReplaceAll(words, secret, "[key]") + } + if kind == "no_answer" { + words = "The service did not answer · nothing was saved" + } + runes := []rune(words) + if len(runes) > 120 { + words = string(runes[:120]) + } + return &KeyFailure{Kind: kind, Words: words} +} + +// ValidKeyEnv accepts the same names as the terminal's key reference reader. +func ValidKeyEnv(name string) bool { + _, ok := envReference("$" + name) + return ok +} diff --git a/internal/connect/keyref_test.go b/internal/connect/keyref_test.go index 37e5725943..4d0c9bcdd3 100644 --- a/internal/connect/keyref_test.go +++ b/internal/connect/keyref_test.go @@ -211,7 +211,7 @@ func TestConnectingToAnUnsetVariableStoresNothing(t *testing.T) { if err == nil { t.Fatal("connecting to an unset variable succeeded") } - if !strings.Contains(err.Error(), "$NOTHING_IS_SET_HERE") { + if err.Error() != "Example reads its key from $NOTHING_IS_SET_HERE, and nothing is set there" { t.Errorf("the refusal does not name the variable: %q", err) } if _, ok, _ := manager.store.get("example"); ok { diff --git a/internal/connect/mcp.go b/internal/connect/mcp.go index 5717bf0554..1ba867d8f5 100644 --- a/internal/connect/mcp.go +++ b/internal/connect/mcp.go @@ -65,6 +65,7 @@ const authMCP = "mcp" // MCPTool is one tool a connected service offers, in the shape a belt needs: // flat, already decided, nothing left to interpret. type MCPTool struct { + Title string // Name is what the tool is called at the far end, and what // [Manager.MCPCall] must be given back to run it. Name string diff --git a/internal/connect/mcp_auth.go b/internal/connect/mcp_auth.go index 538bce6187..97eb0ff7bc 100644 --- a/internal/connect/mcp_auth.go +++ b/internal/connect/mcp_auth.go @@ -323,6 +323,7 @@ func (m *Manager) beginToolServer(ctx context.Context, plug *toolServer, answer manager: m, plug: plug, cancel: cancel, + ctx: runContext, done: make(chan flowResult, 1), } loopback := &mcpLoopback{answers: make(chan mcpAnswer, 1)} @@ -342,7 +343,7 @@ func (m *Manager) beginToolServer(ctx context.Context, plug *toolServer, answer ready := make(chan string, 1) go func() { - status, err := m.connectToolServer(runContext, plug, answer, redirect, loopback, ready) + status, err := m.connectToolServer(runContext, plug, answer, redirect, loopback, ready, flow.commit) // The callback listener must be released before this flow reports done, so // a reconnect that follows at once finds the registered loopback port free // and keeps its identity instead of falling through to a fresh redirect and @@ -401,7 +402,7 @@ func reconnectAddresses(record mcpRegistration, fallback []string) []string { // connectToolServer is the whole trip, from the first question to the keys on // disk. It runs on its own goroutine and reports through [Flow.finish]. -func (m *Manager) connectToolServer(ctx context.Context, plug *toolServer, given, redirect string, loopback *mcpLoopback, ready chan<- string) (Status, error) { +func (m *Manager) connectToolServer(ctx context.Context, plug *toolServer, given, redirect string, loopback *mcpLoopback, ready chan<- string, commit func(func() error) error) (Status, error) { filled, err := plug.at(given) if err != nil { return Status{}, err @@ -475,7 +476,7 @@ func (m *Manager) connectToolServer(ctx context.Context, plug *toolServer, given Scopes: granted(keys, found.scopes), Blank: given, } - if err := m.store.put(service.ID, entry); err != nil { + if err := m.store.putConnected(ctx, service.ID, entry, commit); err != nil { return Status{}, err } forgetTools(service.ID) diff --git a/internal/connect/mcp_auth_test.go b/internal/connect/mcp_auth_test.go index 7e0ca12f2d..a9023f7d1b 100644 --- a/internal/connect/mcp_auth_test.go +++ b/internal/connect/mcp_auth_test.go @@ -493,3 +493,45 @@ func TestAFinishedFlowReleasesItsCallbackListenerBeforeItReportsDone(t *testing. t.Fatal("Flow.Wait did not return after the listener was released") } } + +func TestCancelledToolServerDoesNotSaveAfterCredentialsLock(t *testing.T) { + for _, stage := range []string{"credentials lock", "final commit"} { + t.Run(stage, func(t *testing.T) { + fake := startFakeToolServer(t, fakeShape{}) + manager, _ := withToolServer(t, fake) + entered, release := make(chan struct{}), make(chan struct{}) + hold := func() { close(entered); <-release } + if stage == "credentials lock" { + manager.store.afterLoad = hold + } else { + manager.store.beforeCommit = hold + } + flow, err := manager.BeginAuth(context.Background(), "example", "") + if err != nil { + t.Fatal(err) + } + openInBrowser(t, flow) + select { + case <-entered: + case <-time.After(time.Second): + close(release) + flow.Cancel() + t.Fatal("credential commit did not start") + } + flow.Cancel() + close(release) + select { + case <-flow.done: + case <-time.After(time.Second): + t.Fatal("cancelled connection did not finish") + } + status, err := flow.Wait(context.Background()) + if err == nil || status.Connected { + t.Errorf("cancelled Wait = %+v, %v", status, err) + } + if manager.Connected("example") { + t.Error("cancelled browser connection saved after credentials lock") + } + }) + } +} diff --git a/internal/connect/mcp_fake_test.go b/internal/connect/mcp_fake_test.go index ae3879df36..d11072221a 100644 --- a/internal/connect/mcp_fake_test.go +++ b/internal/connect/mcp_fake_test.go @@ -287,6 +287,7 @@ func (f *fakeToolServer) tools(t *testing.T) *mcp.Server { server.AddTool(&mcp.Tool{ Name: "find_pages", + Title: "Find pages", Description: "Search the pages in this account.", Annotations: &mcp.ToolAnnotations{ReadOnlyHint: true}, InputSchema: json.RawMessage(`{"type":"object","properties":{"query":{"type":"string"}}}`), diff --git a/internal/connect/mcp_tools.go b/internal/connect/mcp_tools.go index eb175a434d..2a1dda8f9d 100644 --- a/internal/connect/mcp_tools.go +++ b/internal/connect/mcp_tools.go @@ -201,11 +201,18 @@ func mcpClient() *mcp.Client { func asTool(tool *mcp.Tool) MCPTool { out := MCPTool{ Name: strings.TrimSpace(tool.Name), + Title: strings.TrimSpace(tool.Title), Description: strings.TrimSpace(tool.Description), Schema: schemaOf(tool.InputSchema), } if tool.Annotations != nil { out.ReadOnly = tool.Annotations.ReadOnlyHint + if out.Title == "" { + out.Title = strings.TrimSpace(tool.Annotations.Title) + } + } + if out.Title == "" { + out.Title = out.Name } return out } diff --git a/internal/connect/plugins.go b/internal/connect/plugins.go new file mode 100644 index 0000000000..10aafd61c8 --- /dev/null +++ b/internal/connect/plugins.go @@ -0,0 +1,167 @@ +package connect + +import ( + "context" + "fmt" + "sort" + "strings" + "unicode" + "unicode/utf8" +) + +type Plugin struct { + ConnectedAt string `json:"connected_at,omitempty"` + KeySavedAt string `json:"key_saved_at,omitempty"` + ID string `json:"id"` + Name string `json:"name"` + Kind string `json:"kind" enum:"browser,key,server"` + Category string `json:"category"` + Blurb string `json:"blurb"` + Connected bool `json:"connected"` + Account string `json:"account,omitempty"` + KeyEnv string `json:"key_env,omitempty"` + KeyCheckable bool `json:"key_checkable"` + Expired bool `json:"expired,omitempty"` + Ask *PluginAsk `json:"ask,omitempty"` + KeyHint *PluginKeyHint `json:"key_hint,omitempty"` +} +type PluginAsk struct { + Label string `json:"label"` + Answers []string `json:"answers,omitempty"` +} +type PluginKeyHint struct { + Words string `json:"words"` + URL string `json:"url,omitempty"` +} +type PluginCapability struct { + ID string `json:"id"` + Phrase string `json:"phrase"` + Covers string `json:"covers"` + Acts bool `json:"acts"` + State CapabilityState `json:"state" enum:"yes,ask,off"` + Tools []string `json:"tools"` +} +type PluginTool struct { + Name string `json:"name"` + Title string `json:"title"` + ReadOnly bool `json:"read_only"` +} +type PluginDescription struct { + Capabilities []PluginCapability `json:"capabilities"` + ScopesWords []string `json:"scopes_words"` + Tools []PluginTool `json:"tools,omitempty"` + Notes string `json:"notes,omitempty"` + Accounts []string `json:"accounts,omitempty"` + KeyNote string `json:"key_note,omitempty"` +} + +// PluginBlurb shortens the catalog's own words without a second row catalog. +func PluginBlurb(s Service) string { + if _, tail, ok := strings.Cut(s.Blurb, "'s own tools — "); ok { + words, _, _ := strings.Cut(tail, " — ") + r, n := utf8.DecodeRuneInString(words) + return string(unicode.ToUpper(r)) + words[n:] + } + if s.Auth == AuthKey { + line := "Your account" + if _, tail, ok := strings.Cut(s.Blurb, " account at "); ok { + host, _, _ := strings.Cut(tail, ", with a key") + line += " at " + host + } + return line + " · with your key" + } + return s.Blurb +} + +func (m *Manager) Plugins() []Plugin { + out := []Plugin{} + for _, s := range m.Services() { + kind := s.Auth + if m.MCPService(s.ID) { + kind = "server" + } + p := Plugin{ID: s.ID, Name: s.Name, Kind: kind, Category: s.Category, Blurb: PluginBlurb(s.Service), Connected: s.Connected, Account: s.Account, KeyEnv: s.KeyEnv} + if plug, err := m.plug(s.ID); err == nil { + if keyed, ok := plug.(*keyPlug); ok { + p.KeyCheckable = keyed.probe.address != "" + } + } + if s.Blank != "" { + p.Ask = &PluginAsk{Label: s.Blank, Answers: s.Answers} + } else if s.KeyAsk != "" { + if plug, err := m.plug(s.ID); err == nil { + if keyed, ok := plug.(*keyPlug); ok && keyed.blank.label != "" { + p.Ask = &PluginAsk{Label: keyed.blank.label} + } + } + } + if s.KeyHint != "" { + p.KeyHint = &PluginKeyHint{Words: "Where's my key?", URL: s.KeyHint} + } + if record, ok, _ := m.store.get(s.ID); ok { + p.ConnectedAt, p.KeySavedAt = record.ConnectedAt, record.KeySavedAt + if record.Keys != nil && !record.Keys.Valid() && record.Keys.RefreshToken == "" { + p.Expired = true + p.Connected = false + } + } + out = append(out, p) + } + return out +} + +func (m *Manager) DescribePlugin(ctx context.Context, id string) (PluginDescription, error) { + plug, err := m.plug(id) + if err != nil { + return PluginDescription{}, err + } + s := plug.Service() + d := PluginDescription{Capabilities: []PluginCapability{}, ScopesWords: []string{}} + for _, scope := range s.Scopes { + switch scope { + case "https://www.googleapis.com/auth/gmail.modify": + d.ScopesWords = append(d.ScopesWords, "Read, change and send your Gmail") + case "https://www.googleapis.com/auth/calendar.events": + d.ScopesWords = append(d.ScopesWords, "See and edit your calendar events") + default: + d.ScopesWords = append(d.ScopesWords, scope) + } + } + if _, tail, ok := strings.Cut(s.Blurb, "signed in in your browser."); ok { + d.Notes = strings.TrimSpace(tail) + } + toolCaps := map[string][]string{} + capabilityMu.RLock() + for tool, capability := range capabilityRegistry[normalize(id)].tools { + toolCaps[capability] = append(toolCaps[capability], tool) + } + capabilityMu.RUnlock() + if m.MCPService(id) && m.Connected(id) { + tools, err := m.MCPTools(ctx, id) + if err != nil { + return d, err + } + for _, tool := range tools { + d.Tools = append(d.Tools, PluginTool{Name: tool.Name, Title: tool.Title, ReadOnly: tool.ReadOnly}) + cap := MCPToolCapability(tool) + toolCaps[cap] = append(toolCaps[cap], tool.Name) + } + } + for _, cap := range m.Capabilities(id) { + names := toolCaps[cap.ID] + if names == nil { + names = []string{} + } + sort.Strings(names) + d.Capabilities = append(d.Capabilities, PluginCapability{ID: cap.ID, Phrase: cap.Phrase, Covers: strings.Join(names, " · "), Acts: cap.Acts, State: m.CapabilityState(id, cap.ID), Tools: names}) + } + if record, ok, _ := m.store.get(s.ID); ok { + if record.Account != "" { + d.Accounts = []string{record.Account} + } + if record.KeyEnv != "" { + d.KeyNote = fmt.Sprintf("Read from $%s on this computer · codeaf never shows the key itself", record.KeyEnv) + } + } + return d, nil +} diff --git a/internal/connect/plugins_test.go b/internal/connect/plugins_test.go new file mode 100644 index 0000000000..402a8c760c --- /dev/null +++ b/internal/connect/plugins_test.go @@ -0,0 +1,213 @@ +package connect + +import ( + "context" + "encoding/json" + "errors" + "golang.org/x/oauth2" + "net/http" + "strings" + "testing" + "time" +) + +func TestPluginBlurb(t *testing.T) { + for _, tt := range []struct { + service Service + want string + }{ + {Service{Blurb: "Linear's own tools — your issues, projects and cycles — signed in in your browser."}, "Your issues, projects and cycles"}, + {Service{Blurb: "Datadog's own tools — your metrics, logs and monitors — signed in in your browser. Say which site first."}, "Your metrics, logs and monitors"}, + {Service{Auth: AuthKey, Blurb: catalogBlurb("Stripe", "https://api.stripe.com", blank{})}, "Your account at api.stripe.com · with your key"}, + } { + if got := PluginBlurb(tt.service); got != tt.want { + t.Errorf("got %q want %q", got, tt.want) + } + } +} + +func TestPluginDescribeUsesCapabilityStore(t *testing.T) { + m, err := NewManager(t.TempDir(), map[string]ClientCredential{"google": {ID: "fake", Secret: "fake"}}) + if err != nil { + t.Fatal(err) + } + d, err := m.DescribePlugin(context.Background(), "google") + if err != nil { + t.Fatal(err) + } + if len(d.Capabilities) != 4 || len(d.ScopesWords) != 2 || len(d.Capabilities[0].Tools) != 2 { + t.Fatalf("describe: %+v", d) + } + if err = m.SetCapabilityState("google", "mail-read", StateOff); err != nil { + t.Fatal(err) + } + d, err = m.DescribePlugin(context.Background(), "google") + if err != nil || d.Capabilities[0].State != StateOff { + t.Fatalf("store not shared: %+v %v", d, err) + } +} + +func TestPluginDescriptionListsServerTools(t *testing.T) { + fake := startFakeToolServer(t, fakeShape{}) + m, _ := withToolServer(t, fake) + connectFake(t, m) + d, err := m.DescribePlugin(context.Background(), "example") + if err != nil { + t.Fatal(err) + } + if len(d.Tools) != 4 { + t.Fatalf("tools: %+v", d.Tools) + } + for _, tool := range d.Tools { + if tool.Name == "find_pages" && (!tool.ReadOnly || tool.Title != "Find pages") { + t.Fatalf("tool: %+v", tool) + } + } +} + +func TestPluginCancelCannotSaveQueuedBrowserSuccess(t *testing.T) { + m, _ := testManager(t) + f := &Flow{ctx: context.Background(), manager: m, plug: google{}, cancel: func() {}, done: make(chan flowResult, 1)} + f.done <- flowResult{keys: &oauth2.Token{AccessToken: "fake", RefreshToken: "fake"}} + f.Cancel() + if _, err := f.Wait(context.Background()); err == nil { + t.Fatal("cancelled sign-in was saved") + } + if m.Connected("google") { + t.Fatal("cancelled sign-in connected") + } +} + +func TestPluginKeyAnswersSaveOnlySuccess(t *testing.T) { + const secret = "private-key-that-must-not-escape" + for _, kind := range []string{"shape", "refused", "no_answer", "success"} { + t.Run(kind, func(t *testing.T) { + calls := 0 + m, plug, _ := keyManager(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + calls++ + if kind == "no_answer" { + conn, _, err := w.(http.Hijacker).Hijack() + if err != nil { + t.Error(err) + return + } + _ = conn.Close() + return + } + if kind == "refused" { + w.WriteHeader(http.StatusUnauthorized) + _, _ = w.Write([]byte(secret + strings.Repeat(" refused", 30))) + return + } + _, _ = w.Write([]byte("{}")) + }), nil) + plug.probe = probe{address: plug.base + "/me"} + key := secret + if kind == "shape" { + key = "two keys" + } + _, err := m.ConnectKey(context.Background(), "example", key) + if kind == "success" { + if err != nil || !m.Connected("example") { + t.Fatalf("success: %v", err) + } + return + } + var failure *KeyFailure + if !errors.As(err, &failure) || failure.Kind != kind { + t.Fatalf("answer: %v", err) + } + if len([]rune(failure.Words)) > 120 || strings.Contains(failure.Words, secret) || m.Connected("example") { + t.Fatalf("unsafe answer: %+v", failure) + } + if kind == "shape" && calls != 0 { + t.Fatal("wrong shape reached the vendor") + } + }) + } +} + +type pluginCanceledAccount struct { + Plug + cancel context.CancelFunc +} + +func (p pluginCanceledAccount) Account(context.Context, *http.Client) (string, error) { + p.cancel() + return "", context.Canceled +} + +func TestPluginCancelledIdentityProbeSavesNothing(t *testing.T) { + m, _ := testManager(t) + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + f := &Flow{ctx: context.Background(), manager: m, plug: pluginCanceledAccount{Plug: google{}, cancel: cancel}, cancel: func() {}, done: make(chan flowResult, 1)} + f.done <- flowResult{keys: &oauth2.Token{AccessToken: "fake", RefreshToken: "fake"}} + if _, err := f.Wait(ctx); err == nil { + t.Fatal("cancelled identity probe saved a connection") + } + if m.Connected("google") { + t.Fatal("cancelled sign-in connected") + } +} + +func TestPluginSavedDatesAndShortAsk(t *testing.T) { + m, _, dir := keyManager(t, http.NotFoundHandler(), func(p *keyPlug) { + p.blank = blank{name: "workspace", label: "Workspace"} + p.base += "/{{.workspace}}" + p.service.KeyAsk = "Give the workspace and then the key, one space between them." + }) + before := time.Now().UTC().Truncate(time.Second) + if _, err := m.ConnectKey(context.Background(), "example", "team fake-key"); err != nil { + t.Fatal(err) + } + reopened, err := NewManager(dir, nil) + if err != nil { + t.Fatal(err) + } + raw, _ := json.Marshal(reopened.Plugins()[0]) + var got map[string]any + _ = json.Unmarshal(raw, &got) + for _, field := range []string{"connected_at", "key_saved_at"} { + value, _ := got[field].(string) + at, err := time.Parse(time.RFC3339Nano, value) + if err != nil || at.Before(before) || at.After(time.Now()) { + t.Fatalf("%s = %v", field, got[field]) + } + } + if got["ask"].(map[string]any)["label"] != "Workspace" { + t.Fatalf("ask = %v", got["ask"]) + } + if err := reopened.Disconnect("example"); err != nil { + t.Fatal(err) + } + raw, _ = json.Marshal(reopened.Plugins()[0]) + _ = json.Unmarshal(raw, &got) + if strings.Contains(string(raw), "connected_at") || strings.Contains(string(raw), "key_saved_at") { + t.Fatalf("disconnected dates: %s", raw) + } +} + +func TestPluginBrowserDatesAndLegacyAbsence(t *testing.T) { + m, _ := testManager(t) + f := &Flow{ctx: context.Background(), manager: m, plug: google{}, cancel: func() {}, done: make(chan flowResult, 1)} + f.done <- flowResult{keys: &oauth2.Token{AccessToken: "fake", RefreshToken: "fake"}} + if _, err := f.Wait(context.Background()); err != nil { + t.Fatal(err) + } + for _, p := range m.Plugins() { + if p.ID == "google" { + if _, err := time.Parse(time.RFC3339Nano, p.ConnectedAt); err != nil || p.KeySavedAt != "" { + t.Fatalf("browser dates = %+v", p) + } + } + } + if err := m.store.put("google", stored{Keys: &oauth2.Token{RefreshToken: "old"}, Scopes: wholeGrant()}); err != nil { + t.Fatal(err) + } + for _, p := range m.Plugins() { + if p.ID == "google" && (p.ConnectedAt != "" || p.KeySavedAt != "") { + t.Fatalf("invented old dates = %+v", p) + } + } +} diff --git a/internal/connect/store.go b/internal/connect/store.go index 5b947a5776..db5d67c7e5 100644 --- a/internal/connect/store.go +++ b/internal/connect/store.go @@ -1,12 +1,14 @@ package connect import ( + "context" "encoding/json" "fmt" "os" "path/filepath" "strings" "sync" + "time" "github.com/Agent-Field/codeaf/internal/filelock" "github.com/Agent-Field/codeaf/internal/home" @@ -21,6 +23,8 @@ const StoreFileName = "credentials.json" // stored is one service's entry in the file. type stored struct { + ConnectedAt string `json:"connected_at,omitempty"` + KeySavedAt string `json:"key_saved_at,omitempty"` // Account is the address the keys belong to, absent when the service // never told us. See the emptiness law on [Status]. Account string `json:"account,omitempty"` @@ -141,6 +145,8 @@ type store struct { // afterLoad is a test seam for holding two independent stores between the // read and replace that have to become one cross-process critical section. afterLoad func() + // beforeCommit lets tests pause immediately before the atomic file replacement. + beforeCommit func() } // newStore names the file under profileDir, or under codeaf's state root when @@ -188,6 +194,23 @@ func (s *store) put(id string, entry stored) error { }) } +// putConnected stamps a successful connection without inventing dates for old entries. +func (s *store) putConnected(ctx context.Context, id string, entry stored, commit func(func() error) error) error { + return s.replaceContext(ctx, id, func(entries map[string]stored) { + now := time.Now().UTC().Format(time.RFC3339Nano) + previous, exists := entries[id] + if exists { + entry.ConnectedAt = previous.ConnectedAt + } else { + entry.ConnectedAt = now + } + if entry.Auth == AuthKey { + entry.KeySavedAt = now + } + entries[id] = entry + }, commit) +} + // remove forgets one service's entry, preserving everything else in the file. // Removing an entry that is not there is a success, not an error. func (s *store) remove(id string) error { @@ -199,6 +222,10 @@ func (s *store) remove(id string) error { // replace is the single writer every change goes through: the read, the one // edit, the temporary file and the rename described on [store]. func (s *store) replace(id string, edit func(map[string]stored)) error { + return s.replaceContext(context.Background(), id, edit, nil) +} + +func (s *store) replaceContext(ctx context.Context, id string, edit func(map[string]stored), commit func(func() error) error) error { s.mu.Lock() defer s.mu.Unlock() @@ -223,6 +250,9 @@ func (s *store) replace(id string, edit func(map[string]stored)) error { if s.afterLoad != nil { s.afterLoad() } + if err := ctx.Err(); err != nil { + return err + } edit(entries) encoded, err := json.MarshalIndent(entries, "", " ") @@ -251,9 +281,25 @@ func (s *store) replace(id string, edit func(map[string]stored)) error { if err := temporary.Close(); err != nil { return fmt.Errorf("save connection %s: %w", id, err) } - if err := os.Rename(temporaryPath, s.path); err != nil { - return fmt.Errorf("save connection %s: %w", id, err) + if s.beforeCommit != nil { + s.beforeCommit() + } + write := func() error { + if err := ctx.Err(); err != nil { + return err + } + return os.Rename(temporaryPath, s.path) } + var commitErr error + if commit != nil { + commitErr = commit(write) + } else { + commitErr = write() + } + if commitErr != nil { + return fmt.Errorf("save connection %s: %w", id, commitErr) + } + removeTemporary = false return nil } diff --git a/internal/manual/chat/accounts.md b/internal/manual/chat/accounts.md index 3a85e4c44e..0a57fe5a28 100644 --- a/internal/manual/chat/accounts.md +++ b/internal/manual/chat/accounts.md @@ -581,3 +581,28 @@ affordance that answers as a failure. And accounts already connected on the far machine keep working: their tools are on the belt as usual. + +## Desktop Connectors and connected accounts + +The desktop engine exposes Connectors when its feature list includes `plugins`. +Connectors reads the same accounts as terminal `/connect`, with the catalog's +categories and descriptions. AI models use the existing provider connections. + +Each account capability has the same Yes, Ask or Off setting in both places. +Answering Always in a chat writes that setting too. Off leaves the capability's +tools out of the next session. + +Browser connections return a sign-in link for the desktop to open. Cancelling +abandons the pending connection. A key connection accepts a pasted key or the +name of an environment variable. A rejected or unanswered check saves nothing, +and the desktop never receives the key back. + +When a connector has no catalog check, codeaf saves the key without checking it, +and the first real use will tell you whether it works. + +An agent's connect question also appears in the desktop's waiting asks. +Connecting the account and answering that question continues its turn. +Not now saves no refusal, so a later need can ask again. + +The account store currently holds one account per connector, including Slack. +It does not record connection dates or recent connector calls. diff --git a/internal/provider/dispatch.go b/internal/provider/dispatch.go index 5344abe45a..698e79bbc3 100644 --- a/internal/provider/dispatch.go +++ b/internal/provider/dispatch.go @@ -967,6 +967,11 @@ func (c *Client) send(ctx context.Context, request *ai.Request, knobs callKnobs, // never reaches the endpoint walk or model chain: it either takes the one // configured metered door, once, or returns its typed pause immediately. if refusal.Kind == refusalPlanPaused { + detail := planPauseDetail(refusal.Reset, c.config.PlanOverflowDoor) + now := c.clock() + postPhase(PhaseNews{Phase: PhasePlanPaused, Detail: detail, Since: now, At: now, + Model: c.modelFor(request), SourceID: c.config.SourceID, PlanReset: refusal.Reset, + Role: RoleFrom(ctx), Session: SessionFrom(ctx), Subject: NodeFrom(ctx)}) useOverflow := c.config.OverflowOnPlanPause && strings.TrimSpace(c.config.PlanOverflow) != "" && !usingOverflow && claimPlanOverflow(ctx) @@ -981,9 +986,7 @@ func (c *Client) send(ctx context.Context, request *ai.Request, knobs callKnobs, pacedSince = time.Time{} continue } - detail := planPauseDetail(refusal.Reset, c.config.PlanOverflowDoor) - now := c.clock() - notePhase(ctx, c.modelFor(request), PhasePlanPaused, detail, now, now, "") + return nil, &PlanPauseError{ Reset: refusal.Reset, OverflowDoor: c.config.PlanOverflowDoor, Cause: lastErr, } diff --git a/internal/provider/phase.go b/internal/provider/phase.go index e862cf9059..6223898419 100644 --- a/internal/provider/phase.go +++ b/internal/provider/phase.go @@ -186,6 +186,9 @@ const PhaseWindow = 15 * time.Second // said at the seam rather than at the surface: no reader has to invent a figure // to have something to print. type PhaseNews struct { + // SourceID identifies the account behind this request; PlanReset is the vendor reset value. + SourceID string + PlanReset string // Phase is what is happening. An empty phase is the end of the story — // posted when a turn stops, so a surface stops drawing a clock for work // that is over. @@ -377,9 +380,10 @@ type phaseClock struct { // mu is here because a raced request has TWO stream loops and one story. // Both arms hold the same clock; only the one the person is hearing drives // it, and the handover happens on a third goroutine (hedge.go's flip). - mu sync.Mutex - model string - role lanes.Role + mu sync.Mutex + model string + sourceID string + role lanes.Role // session is the conversation these requests belong to, carried so that an // engine serving many windows can put this clock on the right one // (roles.go's [WithSession]). It is empty in a build where there is only @@ -428,11 +432,12 @@ func (c *Client) newPhaseClock(ctx context.Context, model string) *phaseClock { return nil } return &phaseClock{ - model: strings.TrimSpace(model), - role: RoleFrom(ctx), - session: SessionFrom(ctx), - subject: NodeFrom(ctx), - now: c.clock, + model: strings.TrimSpace(model), + sourceID: c.config.SourceID, + role: RoleFrom(ctx), + session: SessionFrom(ctx), + subject: NodeFrom(ctx), + now: c.clock, } } @@ -586,6 +591,7 @@ func (p *phaseClock) say(detail string, now time.Time) { Door: p.door, Detail: detail, Model: p.model, + SourceID: p.sourceID, Role: p.role, Session: p.session, Subject: p.subject, diff --git a/internal/provider/plan_doors_test.go b/internal/provider/plan_doors_test.go index 9997f87f00..da990236e2 100644 --- a/internal/provider/plan_doors_test.go +++ b/internal/provider/plan_doors_test.go @@ -25,7 +25,7 @@ func TestAnExhaustedPlanWaitsAndSpendsNothing(t *testing.T) { told := listen(t) client, err := NewClient(Config{ - APIKey: "test-key", BaseURL: plan.URL(), Model: "glm-5.3-flash", Direct: true, + SourceID: "z-ai", APIKey: "test-key", BaseURL: plan.URL(), Model: "glm-5.3-flash", Direct: true, BillingDoor: "coding plan", PlanOverflow: metered.URL(), PlanOverflowDoor: "pay-as-you-go", }) if err != nil { @@ -54,7 +54,7 @@ func TestAnExhaustedPlanWaitsAndSpendsNothing(t *testing.T) { t.Fatalf("plan-pause evidence = %+v, want a distinct temporary pause", evidence) } paused, ok := told.find(PhasePlanPaused) - if !ok || paused.Detail != "resets at 18:30 UTC · /connect can switch to pay-as-you-go" { + if !ok || paused.SourceID != "z-ai" || paused.PlanReset != "18:30 UTC" || paused.Detail != "resets at 18:30 UTC · /connect can switch to pay-as-you-go" { t.Fatalf("paused phase = %+v, found %t", paused, ok) } } @@ -118,6 +118,9 @@ func TestOverflowGoesToTheMeteredDoorAndSaysSo(t *testing.T) { plan.Requests()[0].Agent != DirectUserAgent || metered.Requests()[0].Agent != DirectUserAgent { t.Fatalf("requests: plan=%d metered=%d", len(plan.Requests()), len(metered.Requests())) } + if _, ok := told.find(PhasePlanPaused); !ok { + t.Fatal("overflow hid the plan pause") + } writing, ok := told.find(PhaseWriting) if !ok || writing.Door != "pay-as-you-go" { t.Fatalf("writing phase = %+v, found %t", writing, ok) diff --git a/internal/provider/routefailure.go b/internal/provider/routefailure.go index aa3aadf3b4..33c396860e 100644 --- a/internal/provider/routefailure.go +++ b/internal/provider/routefailure.go @@ -136,6 +136,14 @@ func parseReset(raw string) time.Time { if at, err := time.Parse(time.RFC3339, raw); err == nil { return at } + if clock, err := time.Parse("15:04 UTC", raw); err == nil { + now := time.Now().UTC() + at := time.Date(now.Year(), now.Month(), now.Day(), clock.Hour(), clock.Minute(), 0, 0, time.UTC) + if at.Before(now) { + at = at.Add(24 * time.Hour) + } + return at + } for _, layout := range []string{"15:04", "3:04pm", "3pm"} { if clock, err := time.Parse(layout, strings.ToLower(raw)); err == nil { now := time.Now() diff --git a/internal/provider/routefailure_test.go b/internal/provider/routefailure_test.go index 95ac464cc9..d4c740a050 100644 --- a/internal/provider/routefailure_test.go +++ b/internal/provider/routefailure_test.go @@ -71,3 +71,11 @@ func TestRouteFailureOnTheWire(t *testing.T) { t.Errorf("nil: %q", got) } } + +func TestRouteFailureReadsUTCPlanReset(t *testing.T) { + before := time.Now().UTC() + _, at := RouteFailureOf(&PlanPauseError{Reset: "18:30 UTC"}) + if at.IsZero() || at.Hour() != 18 || at.Minute() != 30 || at.Location() != time.UTC || at.Before(before) || at.After(before.Add(24*time.Hour)) { + t.Fatalf("UTC reset = %v", at) + } +} diff --git a/internal/session/connect.go b/internal/session/connect.go index a900a1f1ab..2d52f4fbc8 100644 --- a/internal/session/connect.go +++ b/internal/session/connect.go @@ -265,6 +265,7 @@ func (a *Agent) service(id string) (connectStatus, bool) { // whether it needs a typed answer — a key, or the one thing the service's // address is missing. type connectAsk struct { + service string answers chan connectAnswer needsKey bool // name is the account in the words a person owns it by, and secret says what @@ -318,7 +319,7 @@ func (a *Agent) ResolveConnect(id string, approve bool) { if !waiting { return } - if ask.needsKey { + if ask.needsKey && (a.connect == nil || !a.connect.Connected(ask.service)) { approve = false } // Buffered to one and read at most once, so this never blocks and never @@ -384,6 +385,7 @@ func (a *Agent) askConnect(ctx context.Context, service connectStatus) (connectA a.connectSeq++ id := "connect-" + strconv.FormatUint(a.connectSeq, 10) ask := connectAsk{ + service: service.ID, answers: make(chan connectAnswer, 1), needsKey: service.keyed() || service.asks(), name: strings.TrimSpace(service.Name), diff --git a/internal/session/question.go b/internal/session/question.go index c2a32cc9a7..0295d79df7 100644 --- a/internal/session/question.go +++ b/internal/session/question.go @@ -2700,7 +2700,9 @@ const ConsentFallbackReason = "it will not run this without your word" // to one of those is read as a decline (connect.go) and a chip that means no // while reading yes is worse than no chip. func (a *Agent) connectQuestion(id string, ask connectAsk) Question { - return a.said(QuestionConnect, id, ConnectQuestion(id, ask.name, ask.needsKey, "", ask.secret)) + q := ConnectQuestion(id, ask.name, ask.needsKey, "", ask.secret) + q.Subject.Name = ask.service + return a.said(QuestionConnect, id, q) } // ConnectQuestion is that object, and it is ONE BUILDER for the two roads it diff --git a/internal/session/questionlifecycle_test.go b/internal/session/questionlifecycle_test.go index 60f86c3919..702d00ee5e 100644 --- a/internal/session/questionlifecycle_test.go +++ b/internal/session/questionlifecycle_test.go @@ -418,3 +418,44 @@ func TestTheStampSaysItsDayWhenItIsNotToday(t *testing.T) { t.Fatalf("a stamp from today says more than its hour") } } + +func TestPluginQuestionUsesAccountConnectedByAnotherSurface(t *testing.T) { + agent, _ := questionSession(t, "cccc3333cccc3333", nil) + hub := &fakeHub{keyService: true} + agent.connect = hub + events := watched(agent) + go func() { + for range events { + } + }() + asks, stop := agent.WatchQuestions() + defer stop() + done := make(chan string, 1) + go func() { + _, failed := agent.connectService(context.Background(), connectStatus{ID: "stripe", Name: "Stripe", Auth: "key"}) + done <- failed + }() + raised := waitForAsk(t, asks, EventQuestion) + if raised.Question.Subject.Name != "stripe" { + t.Fatalf("missing plugin id: %+v", raised.Question) + } + hub.mu.Lock() + hub.keyConnected = true + hub.mu.Unlock() + if err := agent.ResolveQuestion(Answer{Kind: QuestionConnect, Ref: raised.Question.Ref, Key: "1"}); err != nil { + t.Fatal(err) + } + select { + case failed := <-done: + if failed != "" { + t.Fatal(failed) + } + case <-time.After(5 * time.Second): + t.Fatal("connected plugin left the turn waiting") + } + hub.mu.Lock() + defer hub.mu.Unlock() + if hub.key != "" || hub.begins != 0 { + t.Fatal("the connection was attempted twice") + } +} diff --git a/internal/session/tools_connect.go b/internal/session/tools_connect.go index b7c3a2e1ea..4c568cd2c3 100644 --- a/internal/session/tools_connect.go +++ b/internal/session/tools_connect.go @@ -317,6 +317,10 @@ func (a *Agent) connectService(ctx context.Context, service connectStatus) (acco return "", connectAnswerFailure(service.Name, answer) } + if fresh, known := a.service(service.ID); known && fresh.Connected { + return fresh.Account, "" + } + // An account opened with a key has no page to send anybody to: the person // has already done the only step there is, and the whole of what is left is // one call that either works or says why not.