feat(tool): govern background command job lifecycle (cap, memory floor, shutdown reaping)

cmd/ggcode/acp.go

@@ -6,6 +6,7 @@ import (
 	"os"
 	"os/signal"
 	"syscall"
+	"time"
 
 	"github.com/spf13/cobra"
 	"github.com/topcheer/ggcode/internal/acp"
@@ -92,7 +93,13 @@ func newACPCommand(cfgFile *string) *cobra.Command {
 			ctx, stop := signal.NotifyContext(context.Background(), syscall.SIGINT, syscall.SIGTERM)
 			defer stop()
 
-			return handler.Run(ctx)
+			// r71: reap managed background jobs when the ACP server exits so
+			// start_command children (detach=true included) do not orphan.
+			runErr := handler.Run(ctx)
+			if jm := registry.JobManager(); jm != nil {
+				jm.ShutdownAll(2 * time.Second)
+			}
+			return runErr
 		},
 	}
 

cmd/ggcode/pipe.go

@@ -77,6 +77,11 @@ func RunPipe(cfg *config.Config, cfgPath, prompt string, allowedTools, allowedDi
 	}
 	registry := core.Registry
 	core.StartBackgroundServices()
+	// r71: reap managed background jobs (detach=true included) when the pipe
+	// run ends so its children do not outlive the process as orphans.
+	if jm := registry.JobManager(); jm != nil {
+		defer jm.ShutdownAll(2 * time.Second)
+	}
 	defer core.Close()
 
 	// Load project memory file list (for path-triggered dynamic loading).

cmd/ggcode/root.go

@@ -955,6 +955,7 @@ func run(cfg *config.Config, cfgFile, resumeID string, bypass bool) error {
 			RemoteAgentsInfo: func() string { return remoteAgentsInfo },
 		}, task, agentType)
 	})
+	repl.SetJobManager(registry.JobManager())
 	repl.SetSubAgentManager(subMgr, prov, registry)
 	repl.SetAskUserTool(registry)
 	repl.SetCommandPane(registry, workingDir)

internal/agent/constraint_violation.go

@@ -126,12 +126,39 @@ func (s *constraintViolationState) recordReasoning(text string, iter int) {
 	}
 	s.currentIter = iter
 	extracted := cvExtractConstraints(text, iter)
+
+	// #2733: scope declarations have REPLACE semantics, not accumulate.
+	// Scope constraints are task-local commitments about where changes will
+	// land; a later declaration ("I'll limit changes to docs/") supersedes an
+	// earlier one ("I'll only modify auth/"). Accumulating them forms an
+	// implicit global AND -- after two different scope declarations, EVERY edit
+	// violates at least one of them, burning the cvMaxWarnings quota with
+	// false positives and silencing real violations. Avoid constraints are
+	// naturally additive and keep accumulating.
+	var newScopes []cvConstraint
+	for _, c := range extracted {
+		if c.constraintT == "scope" {
+			newScopes = append(newScopes, c)
+		}
+	}
+	if len(newScopes) > 0 {
+		kept := s.constraints[:0]
+		for _, existing := range s.constraints {
+			if existing.constraintT == "scope" {
+				continue // superseded by this turn's scope declaration(s)
+			}
+			kept = append(kept, existing)
+		}
+		s.constraints = kept
+	}
+
 	for _, c := range extracted {
 		if len(s.constraints) >= cvMaxTracked {
 			break
 		}
 		// Deduplicate: skip if we already track a constraint with the same
-		// pattern and type.
+		// pattern and type (covers re-declaring the same scope this turn --
+		// a re-declaration must not re-arm an identical superseded scope).
 		dup := false
 		for _, existing := range s.constraints {
 			if existing.constraintT == c.constraintT && existing.pattern == c.pattern {

internal/agent/duplicate_decl_check.go

@@ -300,8 +300,14 @@ func collectPythonDecls(src string) map[regexDeclKey]int {
 // idiom inside any function body) and is excluded (#2703 scenario 2).
 var jsFuncRe = regexp.MustCompile(`(?m)^(?:export\s+)?(?:default\s+)?(?:async\s+)?function\s+(\w+)\s*\(`)
 
-// jsClassRe matches top-level class declarations.
-var jsClassRe = regexp.MustCompile(`(?m)^(?:export\s+)?(?:default\s+)?(?:abstract\s+)?class\s+(\w+)\s*[\{<]`)
+// jsClassRe matches top-level class declarations. #2734: inheritance is the
+// dominant class form in real JS/TS (React components extend Component,
+// services extend Base) -- the old `[\{<]` only matched bare classes and
+// silently excluded `class Foo extends Bar {` / `implements`, making the
+// duplicate-class check dead code for those forms (old/new counts both 0,
+// no failure signal). Match an optional extends/implements heritage list
+// before the `{`/`<` terminator.
+var jsClassRe = regexp.MustCompile(`(?m)^(?:export\s+)?(?:default\s+)?(?:abstract\s+)?class\s+(\w+)(?:\s+extends\s+[\w.<>[\]]+)?(?:\s+implements\s+[\w.<>[\],\s]+)?\s*[\{<]`)
 
 // jsConstFuncRe matches top-level "const foo = (" arrow function declarations.
 // Indented const is a block-scoped local (the most common false-positive

internal/agent/lock_without_unlock_check.go

@@ -258,9 +258,29 @@ func simulateHeldLocks(fn *ast.FuncDecl, fset *token.FileSet) []lockWithoutUnloc
 			return
 		}
 		if _, isLock := lockMethodNames[sel.Sel.Name]; isLock {
-			if _, exists := held[recv]; !exists {
-				held[recv] = &simHeldEntry{lock: lockCall{receiver: recv, method: sel.Sel.Name, pos: call.Pos()}}
+			if prev, exists := held[recv]; exists {
+				// #2740: re-acquiring an already-held lock on the same
+				// receiver is a guaranteed self-deadlock (Go mutexes are not
+				// reentrant) even when the function is syntactically
+				// balanced. The header's failure mode #3 promises this
+				// detection; the old code silently returned. Only Lock/
+				// TryLock warn - RLock re-entry is legal (sync.RWMutex
+				// reader reentrancy) and stays silent per the issue's
+				// conservative guidance. Mark the held entry reported so the
+				// function-end check does not emit a second, misleading
+				// missing-unlock warning for the same anchor (#1099).
+				if !prev.reported && (sel.Sel.Name == "Lock" || sel.Sel.Name == "TryLock") {
+					prev.reported = true
+					instances = append(instances, lockWithoutUnlockInstance{
+						receiver: recv,
+						method:   sel.Sel.Name,
+						funcName: fn.Name.Name,
+						posStr:   fset.Position(call.Pos()).String() + " (double lock / non-reentrant re-acquire)",
+					})
+				}
+				return
 			}
+			held[recv] = &simHeldEntry{lock: lockCall{receiver: recv, method: sel.Sel.Name, pos: call.Pos()}}
 			return
 		}
 		if sel.Sel.Name == "Unlock" || sel.Sel.Name == "RUnlock" {

internal/agent/reproducer_lifecycle.go

@@ -57,9 +57,11 @@ import (
 const (
 	reproLifecycleMaxWarnings = 1 // max warnings per run
 
-	// reproducerFertilityWindow: how many iterations after a reproducer run
-	// we consider the agent "in the edit phase" and expect a re-run.
-	reproducerFertilityWindow = 8
+	reproducerRerunGraceIterations = 2 // iterations to wait after edit before warning
+
+	// commandTokenMinLen: minimum length of a command token to count for
+	// overlap matching (filters out short generic words).
+	commandTokenMinLen = 3
 )
 
 // reproducerLifecycleState tracks the reproduce->edit->rerun lifecycle.
@@ -126,6 +128,70 @@ var reproducerRunToolNames = map[string]bool{
 	"start_command": true,
 }
 
+// reproducerRerunMatches reports whether a run tool input qualifies as a
+// re-run of the reproducer itself (#2752). It qualifies if it matches the
+// reproducer script shape (e.g. `python3 repro.py`), or if it shares a
+// meaningful token overlap with the recorded reproducer snippet (covers
+// text-established reproducers whose snippet may be prose-like).
+func reproducerRerunMatches(inp, snippet string) bool {
+	if inp == "" {
+		return false
+	}
+	if reproducerCommandRe.MatchString(inp) {
+		return true
+	}
+	if snippet == "" {
+		return false
+	}
+	return reproCommandTokenOverlap(inp, snippet)
+}
+
+// reproCommandTokenOverlap checks whether the two command strings share a
+// distinctive script/path token (e.g. both reference `repro.py`).
+func reproCommandTokenOverlap(a, b string) bool {
+	tokensA := reproCommandTokens(a)
+	tokensB := reproCommandTokens(b)
+	if len(tokensA) == 0 || len(tokensB) == 0 {
+		return false
+	}
+	for ta := range tokensA {
+		if tokensB[ta] {
+			return true
+		}
+	}
+	return false
+}
+
+// reproCommandTokens splits a command string into lowercase tokens suitable
+// for overlap matching. Fields are additionally split on path separators so
+// `./cmd/reprogo/main.go` and `go run ./cmd/reprogo` share `reprogo`.
+// Generic shell verbs, flags, and common directory names are dropped so
+// overlap means script/argument identity rather than generic words.
+func reproCommandTokens(s string) map[string]bool {
+	generic := map[string]bool{
+		"and": true, "the": true, "run": true, "bash": true, "sh": true,
+		"python": true, "python3": true, "node": true, "ruby": true,
+		"cargo": true, "go": true, "test": true, "tests": true, "cd": true,
+		"echo": true, "make": true, "cmd": true, "src": true, "pkg": true,
+		"internal": true, "desktop": true, "main": true, "github.com": true,
+		"github": true, "www": true, "head": true, "git": true, "diff": true,
+	}
+	tokens := make(map[string]bool)
+	for _, field := range strings.Fields(strings.ToLower(s)) {
+		for _, comp := range strings.Split(field, "/") {
+			comp = strings.Trim(comp, "\"'`$();|&~.:")
+			if len(comp) < commandTokenMinLen || strings.HasPrefix(comp, "-") {
+				continue
+			}
+			if generic[comp] {
+				continue
+			}
+			tokens[comp] = true
+		}
+	}
+	return tokens
+}
+
 // observeToolCalls updates the lifecycle state based on the tools the agent
 // invoked this iteration.
 func (s *reproducerLifecycleState) observeToolCalls(iteration int, toolNames []string, toolInputs []string) {
@@ -157,9 +223,12 @@ func (s *reproducerLifecycleState) observeToolCalls(iteration int, toolNames []s
 			}
 		}
 
-		// Phase 3: detect re-run after edit.
+		// Phase 3: detect re-run of the reproducer itself after edit (#2752).
+		// A bare run_command (e.g. `git diff`, `ls`) must NOT discharge the
+		// re-run obligation: the command must either match the reproducer
+		// script shape or resemble the recorded reproducer snippet.
 		if s.editedAfterReproducer && !s.reranAfterEdit {
-			if reproducerRunToolNames[tn] {
+			if reproducerRunToolNames[tn] && reproducerRerunMatches(inp, s.reproducerSnippet) {
 				s.reranAfterEdit = true
 				debug.Log("agent", "reproducer-lifecycle: re-run after edit at iter %d", iteration)
 			}
@@ -189,12 +258,13 @@ func (s *reproducerLifecycleState) checkIncomplete(iteration int) string {
 	if s.warned {
 		return ""
 	}
-	// Only warn if: reproducer established, code edited after, NOT re-run,
-	// and we're past the fertility window from the edit.
+	// Only warn if: reproducer established, code edited after, and the
+	// reproducer itself has NOT been re-run. Wait a grace period after the
+	// edit so the agent has a chance to re-run it.
 	if !s.hasReproducer || !s.editedAfterReproducer || s.reranAfterEdit {
 		return ""
 	}
-	if iteration-s.editIteration < 2 {
+	if iteration-s.editIteration < reproducerRerunGraceIterations {
 		return "" // give the agent a chance to re-run
 	}
 

internal/agent/reversibility_check.go

@@ -91,9 +91,15 @@ func (r *reversibilityState) recordSafetySignal(toolName, args string) {
 			if hasCommandToken(tokens[1:], "test", "check") {
 				r.testsRan = true
 			}
-		case "test", "pytest":
-			// pytest as the command's first token IS the test command
+		case "pytest", "py.test", "vitest", "jest":
+			// Bare test-runner commands as the first token ARE test runs
 			// (#1194: `pytest -q scripts/` has no `test` token following).
+			// #2754: bare `test` is NOT in this list - it is the POSIX
+			// shell builtin (`test -f x`, `test -d dist && rm -rf dist`),
+			// a conditional, not a test run. Counting it flipped testsRan
+			// and silently disarmed the commit/push gate - the same
+			// false-verification family as #2255 ("build:" in a commit
+			// message) and #2552 (`make clean` counted as build).
 			r.testsRan = true
 		}
 	case "git_add", "git_commit":

internal/agent/zz_issue2733_test.go

@@ -0,0 +1,117 @@
+package agent
+
+import (
+	"strings"
+	"testing"
+)
+
+// zz_issue2733_test.go: regression tests for scope-constraint supersede
+// semantics (#2733). Scope declarations used to accumulate with no
+// replacement, forming an implicit AND: after declaring "only modify auth/"
+// and later "limit changes to docs/", any edit violated at least one of the
+// two constraints, burning the cvMaxWarnings=2 quota with false positives
+// while real violations went silent.
+
+func issue2733ScopeConstraints(s *constraintViolationState) []cvConstraint {
+	var out []cvConstraint
+	for _, c := range s.constraints {
+		if c.constraintT == "scope" {
+			out = append(out, c)
+		}
+	}
+	return out
+}
+
+// Test 1: the issue's exact reproduction -- a second, different scope
+// declaration must supersede the first, so an edit inside the CURRENT scope
+// (docs/) is no longer falsely flagged by the stale auth/ constraint (the
+// AND-ization that burned the whole warning quota with noise).
+func TestIssue2733LaterScopeSupersedesEarlier(t *testing.T) {
+	s := newConstraintViolationState()
+	s.recordReasoning("I'll only modify files in the auth/ directory.", 1)
+	s.recordReasoning("I'll limit changes to the docs/ folder.", 5)
+
+	scopes := issue2733ScopeConstraints(s)
+	if len(scopes) != 1 {
+		t.Fatalf("expected exactly 1 scope constraint after supersede, got %d (%+v)", len(scopes), scopes)
+	}
+	if scopes[0].pattern != "docs/" {
+		t.Fatalf("expected surviving scope pattern 'docs/', got %q", scopes[0].pattern)
+	}
+
+	// Edit inside the CURRENT (latest) scope: under the old accumulate
+	// semantics this violated the stale auth/ constraint -- a false positive
+	// that burned the quota and let real violations through silently.
+	if msg := s.checkToolCall("edit_file", map[string]any{"file_path": "docs/readme.md"}, 6); msg != "" {
+		t.Fatalf("edit inside current scope must not warn, got: %s", msg)
+	}
+	// Quota untouched: 0 warnings spent.
+	if s.warnings != 0 {
+		t.Fatalf("quota must be intact after in-scope edit, warnings=%d", s.warnings)
+	}
+
+	// A genuinely out-of-scope edit still fires exactly once.
+	msg := s.checkToolCall("edit_file", map[string]any{"file_path": "cmd/main.go"}, 7)
+	if msg == "" {
+		t.Fatal("real out-of-scope edit (cmd/main.go vs docs/) must warn")
+	}
+	if want := "docs/"; !strings.Contains(msg, want) {
+		t.Fatalf("warning should cite the current scope %q, got: %s", want, msg)
+	}
+}
+
+// Test 2: avoid constraints remain additive alongside the supersede rule --
+// an avoid declaration must survive a later scope declaration.
+func TestIssue2733AvoidStaysAdditive(t *testing.T) {
+	s := newConstraintViolationState()
+	s.recordReasoning("I won't modify the config/ directory.", 1)
+	s.recordReasoning("I'll limit changes to the docs/ folder.", 3)
+
+	var avoids []cvConstraint
+	for _, c := range s.constraints {
+		if c.constraintT == "avoid" {
+			avoids = append(avoids, c)
+		}
+	}
+	if len(avoids) != 1 {
+		t.Fatalf("avoid constraint must survive scope supersede, got %d", len(avoids))
+	}
+	// config/ is both avoided and outside docs/ -- the avoid arm must fire.
+	msg := s.checkToolCall("edit_file", map[string]any{"file_path": "config/app.yaml"}, 4)
+	if msg == "" || !strings.Contains(msg, "avoid") {
+		t.Fatalf("edit into avoided config/ must warn via avoid arm, got: %s", msg)
+	}
+}
+
+// Test 3: re-declaring the SAME scope must not duplicate the constraint
+// (dedup path still applies after supersede logic).
+func TestIssue2733SameScopeRedeclareNoDuplicate(t *testing.T) {
+	s := newConstraintViolationState()
+	s.recordReasoning("I'll only modify files in the auth/ directory.", 1)
+	s.recordReasoning("Staying within auth/ as planned.", 4)
+
+	scopes := issue2733ScopeConstraints(s)
+	if len(scopes) != 1 {
+		t.Fatalf("re-declared same scope must dedup, got %d scope constraints", len(scopes))
+	}
+	if scopes[0].iter != 4 {
+		t.Fatalf("surviving entry should be the latest declaration (iter=4), got iter=%d", scopes[0].iter)
+	}
+}
+
+// Test 4: supersede only drops older scopes when the new turn actually
+// declares a scope -- reasoning with no scope declarations (e.g. only an
+// avoid) must leave existing scope constraints intact.
+func TestIssue2733NoScopeDeclarationKeepsExisting(t *testing.T) {
+	s := newConstraintViolationState()
+	s.recordReasoning("I'll only modify files in the auth/ directory.", 1)
+	s.recordReasoning("I won't touch the vendor/ directory.", 2)
+
+	scopes := issue2733ScopeConstraints(s)
+	if len(scopes) != 1 || scopes[0].pattern != "auth/" {
+		t.Fatalf("scope without a competing declaration must survive, got %+v", scopes)
+	}
+	if msg := s.checkToolCall("edit_file", map[string]any{"file_path": "docs/readme.md"}, 3); msg == "" {
+		t.Fatal("out-of-scope edit must still warn when scope not superseded")
+	}
+}