Skip to content

Commit 6f264e2

Browse files
8888Ben Pittman
authored andcommitted
fix: add agentBeadID determinism test and capacity check comment (gt-27k)
Address PR gastownhall#3677 review feedback: 1. Add TestAgentBeadID_Deterministic regression test that constructs Managers from the same rig path (including after cwd change) and asserts agentBeadID returns identical strings — locks in the gt-lph fix against future refactors of NewManager. 2. Add comment at CheckDoltServerCapacity documenting the intentional behavior change: the old skip-on-error path (return nil when workspace.Find failed) was silently removed, and errors now propagate as fail-closed — the right behavior for gt-lfc0d.
1 parent 9e15734 commit 6f264e2

2 files changed

Lines changed: 66 additions & 0 deletions

File tree

internal/polecat/manager.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -290,6 +290,12 @@ func (m *Manager) CheckDoltHealth() error {
290290
// Fails closed if the check errors — a server that can't report capacity is likely
291291
// already under stress (gt-lfc0d).
292292
func (m *Manager) CheckDoltServerCapacity() error {
293+
// NOTE: Prior to gt-lph, this method called workspace.Find to locate townRoot,
294+
// which could fail and silently skip the capacity check (return nil). Now that
295+
// m.townRoot is computed deterministically at Manager construction, errors from
296+
// HasConnectionCapacity always propagate — this is intentional. A server that
297+
// can't report capacity is likely under stress, and silently passing was a
298+
// latent bug that allowed connection storms under load (gt-lfc0d).
293299
hasCapacity, active, err := doltserver.HasConnectionCapacity(m.townRoot)
294300
if err != nil {
295301
// Fail closed: if we can't check capacity, the server may be overloaded.

internal/polecat/manager_test.go

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -215,6 +215,66 @@ func TestAssigneeID(t *testing.T) {
215215
}
216216
}
217217

218+
// TestAgentBeadID_Deterministic verifies that agentBeadID returns the same string
219+
// on repeated calls regardless of process working directory. Regression test for
220+
// gt-lph: the old implementation called workspace.Find on each invocation, which
221+
// could resolve differently depending on cwd, causing non-deterministic IDs across
222+
// Manager instances for the same rig path.
223+
func TestAgentBeadID_Deterministic(t *testing.T) {
224+
townRoot := t.TempDir()
225+
rigPath := filepath.Join(townRoot, "myrig")
226+
if err := os.MkdirAll(rigPath, 0755); err != nil {
227+
t.Fatalf("mkdir rig: %v", err)
228+
}
229+
230+
r := &rig.Rig{
231+
Name: "myrig",
232+
Path: rigPath,
233+
}
234+
235+
// Construct two Managers from the same rig path — they must produce
236+
// identical agentBeadIDs regardless of construction context.
237+
m1 := NewManager(r, git.NewGit(rigPath), nil)
238+
m2 := NewManager(r, git.NewGit(rigPath), nil)
239+
240+
id1a := m1.agentBeadID("Toast")
241+
id1b := m1.agentBeadID("Toast")
242+
id2 := m2.agentBeadID("Toast")
243+
244+
// Same Manager, repeated calls — must be identical.
245+
if id1a != id1b {
246+
t.Errorf("agentBeadID not stable across calls: %q vs %q", id1a, id1b)
247+
}
248+
249+
// Different Manager, same rig — must be identical.
250+
if id1a != id2 {
251+
t.Errorf("agentBeadID differs across Managers for same rig: %q vs %q", id1a, id2)
252+
}
253+
254+
// Verify the ID is non-empty and contains expected components.
255+
if id1a == "" {
256+
t.Fatal("agentBeadID returned empty string")
257+
}
258+
259+
// Change process working directory and construct a third Manager —
260+
// the ID must still match (the old bug: workspace.Find resolved
261+
// differently from different cwds).
262+
origDir, err := os.Getwd()
263+
if err != nil {
264+
t.Fatalf("Getwd: %v", err)
265+
}
266+
if err := os.Chdir(townRoot); err != nil {
267+
t.Fatalf("Chdir to townRoot: %v", err)
268+
}
269+
defer func() { _ = os.Chdir(origDir) }()
270+
271+
m3 := NewManager(r, git.NewGit(rigPath), nil)
272+
id3 := m3.agentBeadID("Toast")
273+
if id1a != id3 {
274+
t.Errorf("agentBeadID differs after cwd change: %q (original) vs %q (after chdir)", id1a, id3)
275+
}
276+
}
277+
218278
// Note: State persistence tests removed - state is now derived from beads assignee field.
219279
// Integration tests should verify beads-based state management.
220280

0 commit comments

Comments
 (0)