Fix --skip-unchanged deadlocking a goal loop that is the only writer of its tree (#217 item 13)

GraphcodeKit/Sources/CLI/GraphcodeCommand.swift

@@ -126,10 +126,13 @@ public enum GraphcodeCommand: Equatable, Sendable {
                            budget burns per turn, not per hour. Reported, never
                            estimated — a loop whose backend reports nothing is never
                            stopped by a budget
-      --skip-unchanged     for --type goal: don't re-run the predicate while HEAD and
-                           the dirty file list are unchanged since its last failure.
-                           Only for predicates that depend on the tree — one that
-                           watches CI or a deploy would never be re-asked
+      --skip-unchanged     for --type goal: while the session is busy, don't re-run
+                           the predicate while HEAD and the dirty file list are
+                           unchanged since its last failure. An idle session gets
+                           one more predicate run and failure notice per unchanged
+                           tree, then polls stay quiet until the tree changes —
+                           the loop is the only writer of its own tree, so waiting
+                           on a change would wait on the loop itself
 
     UPDATE OPTIONS (node update; pass only what changes)
       --goal, --predicate, --prompt, --check, --model, --metric, --direction as above
@@ -751,6 +754,51 @@ extension GraphcodeCommand {
         + "a time-based one --prompt, and the backend must be able to host that type"
     }
   }
+
+  /// The one sentence both create and update print: what the flag actually skips, and
+  /// the one-notice-per-frozen-tree bound that keeps an idle loop from being stranded
+  /// or woken into an agent turn every poll.
+  static let skipUnchangedAdvice =
+    "warning: --skip-unchanged spares the predicate only while the session is busy; "
+    + "an idle session on an unchanged tree gets one more predicate run and one more "
+    + "failure notice, then polls stay quiet until the tree changes — the loop is the "
+    + "only writer of its own tree"
+
+  /// Advice printed at `node create` time for the flag combination a first-time user
+  /// reached for and got stranded by (issue #217 item 13): `--skip-unchanged` on a goal
+  /// loop with a predicate. The flag's name invites reading it as "the daemon handles
+  /// idle polls", when what it does is skip re-runs while the session is busy — the
+  /// loop itself is the only writer of the tree the skip was watching. The poller now
+  /// wakes an idle loop on an unchanged tree, so this is advisory rather than a
+  /// refusal, but the contract is worth saying out loud where the flag is typed.
+  public static func createWarnings(for draft: NodeDraft) -> [String] {
+    guard draft.loopType == .goalBased, let goal = draft.goal,
+      goal.skipsUnchangedWorkspace, goal.effectivePredicate != nil
+    else { return [] }
+    return [skipUnchangedAdvice]
+  }
+
+  /// The same advice for `node update --skip-unchanged true`, judged against the node
+  /// as the update will leave it — the flag only matters on a goal loop whose predicate
+  /// survives the update. Best-effort by design: a node the client cannot see at the
+  /// top level (a sub-graph child — issue #217 item 15) warns nobody rather than
+  /// warning wrongly.
+  public static func updateWarnings(
+    for update: NodeUpdate, currentNode: LoopNode?
+  ) -> [String] {
+    guard update.skipsUnchangedWorkspace == true, let node = currentNode,
+      node.loopType == .goalBased
+    else { return [] }
+    let predicateAfter: String?
+    if let raw = update.goalPredicate {
+      let trimmed = raw.trimmingCharacters(in: .whitespacesAndNewlines)
+      predicateAfter = trimmed.isEmpty ? nil : trimmed
+    } else {
+      predicateAfter = node.goal?.effectivePredicate
+    }
+    guard predicateAfter != nil else { return [] }
+    return [skipUnchangedAdvice]
+  }
 }
 
 /// The export/import verbs' parsing, split from the enum body the way `render` would

GraphcodeKit/Sources/Domain/GoalSpec.swift

@@ -58,12 +58,16 @@ public struct GoalSpec: Codable, Equatable, Sendable {
   /// never blow this bound. The budget is only as real as the reporting — the poller
   /// acts on what was reported, and stays silent about what wasn't.
   public var tokenBudget: Int?
-  /// When true, the poller skips re-running the predicate while the working tree is
-  /// unchanged (same `HEAD`, same dirty files) since the last failing run. Off by
-  /// default with reason: plenty of predicates watch things *outside* the tree — a CI
-  /// run, a deployed endpoint — and skipping those would wait on a change that never
-  /// comes. Opt in exactly when the predicate is a function of the tree (a test suite,
-  /// a lint) and expensive enough that docs/08's conservative-polling stance applies.
+  /// When true, the poller skips re-running the predicate while the session is busy
+  /// and the working tree is unchanged (same `HEAD`, same dirty files) since the last
+  /// failing run. An idle session on that unchanged tree gets one more predicate run
+  /// and one more failure relay — a goal loop is the only writer of its own tree and
+  /// only writes once woken, so gating the wake on a tree change would strand it —
+  /// after which polls stay quiet until the tree moves again. Off by default with
+  /// reason: plenty of predicates watch things *outside* the tree — a CI run, a
+  /// deployed endpoint — and the skip buys them least. Opt in when the predicate is a
+  /// function of the tree (a test suite, a lint) and expensive enough that docs/08's
+  /// conservative-polling stance applies.
   public var skipsUnchangedWorkspace: Bool
 
   public init(

GraphcodeKit/Sources/GraphStore.swift

@@ -127,6 +127,11 @@ public actor GraphStore {
   /// The failure tail last relayed to each node's session, so an unchanged failure is
   /// never repeated at it poll after poll.
   private var lastPredicateFeedback: [UUID: String] = [:]
+  /// The fingerprint whose unchanged tree has already bought an idle loop its one
+  /// re-awake (see `evaluateGoal`'s skip path). Cleared with the poller like the other
+  /// two caches; a new failing run at a new fingerprint naturally leaves it stale, and
+  /// stale here means one more notice the next freeze is allowed to spend.
+  private var reawakenedFingerprints: [UUID: String] = [:]
   /// `node send --follow-up` messages waiting for their target to finish its current
   /// turn — drained whenever the store settles (`drainAndBroadcast`) and on each
   /// presence poll. The content is in the target's memory log from the moment it was
@@ -2006,11 +2011,13 @@ public actor GraphStore {
 
   private func cancelGoalPoller(_ nodeID: UUID) {
     goalPollers.removeValue(forKey: nodeID)?.cancel()
-    // The caches ride the poller's lifecycle: a resolved node needs neither, and an
-    // update that changed the predicate must not skip the new command on the old
-    // tree's fingerprint or suppress its first failure as "already relayed".
+    // The caches ride the poller's lifecycle: a resolved node needs none of them, and
+    // an update that changed the predicate must not skip the new command on the old
+    // tree's fingerprint, suppress its first failure as "already relayed", or count
+    // the old tree's re-awake as spent.
     failedPredicateFingerprints.removeValue(forKey: nodeID)
     lastPredicateFeedback.removeValue(forKey: nodeID)
+    reawakenedFingerprints.removeValue(forKey: nodeID)
   }
 
   /// One poll. Called on the timer in production and directly from tests, so the
@@ -2056,9 +2063,36 @@ public actor GraphStore {
           command: Self.workspaceFingerprintCommand,
           workingDirectory: node.worktreeBinding?.worktreePath ?? graph.project.path))
       // Same tree the predicate already failed against — running it again buys the
-      // same answer at full price. A missing fingerprint (not a git repo, capture not
-      // wired) falls through to a real run: skipping is the optimisation, never the rule.
-      if let fingerprint, failedPredicateFingerprints[nodeID] == fingerprint { return }
+      // same answer at full price *while the session is busy*: its next write is what
+      // would change the tree, and until it does the answer cannot. A missing
+      // fingerprint (not a git repo, capture not wired) falls through to a real run:
+      // skipping is the optimisation, never the rule.
+      //
+      // An idle session flips the case, and there the skip is a deadlock: a goal loop
+      // is the only writer of its own tree, and it only writes once woken — so
+      // "waiting for the tree to change" waits on the loop that is asleep (issue #217
+      // item 13). Idle plus unchanged is therefore wake-worthy, once per frozen tree:
+      // the predicate runs again — the only path on which an external watcher's change
+      // is ever seen — and the relay below re-delivers the failure even if it reads
+      // the same as the last one, because the session that already heard it heard it
+      // before its turn left the tree unmoved. After that the skip holds again until
+      // the tree moves: re-delivering every poll would be a full agent turn a minute,
+      // the unbounded spend the failure-tail dedup exists to prevent.
+      if let fingerprint, failedPredicateFingerprints[nodeID] == fingerprint {
+        let presence: Presence?
+        if let onReadPresence {
+          presence = await onReadPresence(node, graph.project.path).presence
+        } else {
+          presence = node.presence?.presence
+        }
+        // A nil presence stays skipped: the relay only ever tells a session it can see
+        // idle, so falling through would pay the predicate's price for a wake that can
+        // never land. Such a loop's exits are its stall bound and its human.
+        guard presence == .idle else { return }
+        guard reawakenedFingerprints[nodeID] != fingerprint else { return }
+        reawakenedFingerprints[nodeID] = fingerprint
+        lastPredicateFeedback.removeValue(forKey: nodeID)
+      }
     }
 
     let outcome: PredicateOutcome

graphcode-cli/Sources/main.swift

@@ -163,6 +163,10 @@ do {
     attributed.createdBy =
       SurfaceRef.nodeID(
         fromZmxSessionName: ProcessInfo.processInfo.environment["ZMX_SESSION"] ?? "")
+    // On stderr so stdout stays the rendered graph a script may be parsing.
+    for warning in GraphcodeCommand.createWarnings(for: attributed) {
+      FileHandle.standardError.write(Data("\(warning)\n".utf8))
+    }
     // Named a composite, and the very same command is addressed at its sub-graph
     // instead — which is what makes the CLI able to build one at all. Without this
     // there was no surface anywhere that could put a loop inside a composite, so the
@@ -227,7 +231,18 @@ do {
     attributed.updatedBy = SurfaceRef.nodeID(
       fromZmxSessionName: ProcessInfo.processInfo.environment["ZMX_SESSION"] ?? "")
     try client.send(.openProject(path: projectPath))
-    _ = try client.waitForEvent { if case .graphChanged = $0 { return true } else { return false } }
+    let opened = try client.waitForEvent {
+      if case .graphChanged = $0 { return true } else { return false }
+    }
+    // The same advice `node create` prints — turning the flag on from `update` is the
+    // same surprise. Best-effort: the node must be visible at the top level.
+    if case .graphChanged(let graph) = opened {
+      for warning in GraphcodeCommand.updateWarnings(
+        for: attributed, currentNode: graph.nodes.first(where: { $0.id == nodeID }))
+      {
+        FileHandle.standardError.write(Data("\(warning)\n".utf8))
+      }
+    }
     try client.send(
       .graphCommand(
         projectPath: projectPath, command: .updateNode(nodeID, update: attributed)))

graphcode/Tests/PredicateFeedbackTests.swift

@@ -158,6 +158,113 @@ struct PredicateFeedbackTests {
     #expect(evaluated.value == 2)
   }
 
+  @Test
+  func anIdleLoopOnAnUnchangedTreeIsWokenOnceNotEveryPoll() async {
+    // The stranding from issue #217 item 13: a goal loop is the only writer of its
+    // tree, and it only writes once woken — so gating the wake on a tree change waits
+    // on the loop that is asleep. Idle plus unchanged is wake-worthy, but bounded:
+    // one re-delivery per frozen tree, because every further one is a full agent turn
+    // — the spend the failure-tail dedup exists to prevent.
+    let evaluated = LockIsolated(0)
+    let delivered = LockIsolated(0)
+    let graph = goalGraph(presence: .idle, skipsUnchanged: true)
+    let store = GraphStore(
+      graph: graph,
+      onCheckPredicate: { _ in
+        evaluated.withValue { $0 += 1 }
+        return PredicateOutcome(passed: false, outputTail: "no")
+      },
+      onDeliverMessage: { _, _, _ in
+        delivered.withValue { $0 += 1 }
+        return true
+      },
+      onCaptureScript: { _ in "tree-1" })
+
+    await store.evaluateGoal(graph.nodes[0].id)
+    await store.evaluateGoal(graph.nodes[0].id)
+    await store.evaluateGoal(graph.nodes[0].id)
+    await store.evaluateGoal(graph.nodes[0].id)
+
+    #expect(evaluated.value == 2)
+    #expect(delivered.value == 2)
+  }
+
+  @Test
+  func aTreeChangeBuysOneMoreWakeOnTheNextFreeze() async {
+    // The bound is per frozen tree, not per loop: each failing run at a new
+    // fingerprint leaves the idle loop one more notice it may be given if the tree
+    // freezes again — that is what keeps "skip until changed" from becoming "skip
+    // until stopped".
+    let fingerprint = LockIsolated("tree-1")
+    let evaluated = LockIsolated(0)
+    let delivered = LockIsolated(0)
+    let graph = goalGraph(presence: .idle, skipsUnchanged: true)
+    let store = GraphStore(
+      graph: graph,
+      onCheckPredicate: { _ in
+        evaluated.withValue { $0 += 1 }
+        return PredicateOutcome(passed: false, outputTail: "no")
+      },
+      onDeliverMessage: { _, _, _ in
+        delivered.withValue { $0 += 1 }
+        return true
+      },
+      onCaptureScript: { _ in fingerprint.value })
+
+    await store.evaluateGoal(graph.nodes[0].id)  // first failing run: told
+    await store.evaluateGoal(graph.nodes[0].id)  // idle on tree-1: its one wake
+    await store.evaluateGoal(graph.nodes[0].id)  // skip holds
+    fingerprint.setValue("tree-2")
+    await store.evaluateGoal(graph.nodes[0].id)  // tree moved: re-run; same tail, not told
+    await store.evaluateGoal(graph.nodes[0].id)  // idle on tree-2: one more wake
+    await store.evaluateGoal(graph.nodes[0].id)  // skip holds again
+
+    #expect(evaluated.value == 4)
+    #expect(delivered.value == 3)
+  }
+
+  @Test
+  func aPresencelessSessionStaysSkippedRatherThanPayingForAWakeThatCannotLand() async {
+    // The relay only ever tells a session it can see idle; with no presence reading
+    // no wake can land, so the skip holds instead of spending predicate runs on one.
+    // Such a loop's exits are its stall bound and its human.
+    let evaluated = LockIsolated(0)
+    let graph = goalGraph(presence: nil, skipsUnchanged: true)
+    let store = GraphStore(
+      graph: graph,
+      onCheckPredicate: { _ in
+        evaluated.withValue { $0 += 1 }
+        return PredicateOutcome(passed: false, outputTail: "no")
+      },
+      onCaptureScript: { _ in "tree-1" })
+
+    await store.evaluateGoal(graph.nodes[0].id)
+    await store.evaluateGoal(graph.nodes[0].id)
+
+    #expect(evaluated.value == 1)
+  }
+
+  @Test
+  func anIdleLoopOnAnUnchangedTreeStillNoticesThePredicatePassing() async {
+    // The other half of the same deadlock: with the tree frozen, the only way a
+    // CI-watching predicate ever goes green is on a poll the skip used to eat.
+    let evaluated = LockIsolated(0)
+    let graph = goalGraph(presence: .idle, skipsUnchanged: true)
+    let store = GraphStore(
+      graph: graph,
+      onCheckPredicate: { _ in
+        evaluated.withValue { $0 += 1 }
+        return PredicateOutcome(passed: evaluated.value > 1, outputTail: "no")
+      },
+      onDeliverMessage: { _, _, _ in true },
+      onCaptureScript: { _ in "tree-1" })
+
+    await store.evaluateGoal(graph.nodes[0].id)
+    await store.evaluateGoal(graph.nodes[0].id)
+
+    #expect(await store.graph.nodes[0].state == .succeeded)
+  }
+
   @Test
   func theCLIParsesSkipUnchanged() throws {
     let create = try GraphcodeCommand.parse([
@@ -179,4 +286,45 @@ struct PredicateFeedbackTests {
     }
     #expect(nodeUpdate.skipsUnchangedWorkspace == false)
   }
+
+  @Test
+  func theCreateWarningFiresOnlyForSkipUnchangedPairedWithAPredicate() throws {
+    let warned = try GraphcodeCommand.parse([
+      "node", "create", "/tmp/p", "--title", "S", "--type", "goal",
+      "--goal", "done", "--predicate", "make test", "--skip-unchanged",
+    ])
+    guard case .createNode(_, let warnedDraft, _) = warned else {
+      Issue.record("expected createNode, got \(warned)")
+      return
+    }
+    let warnings = GraphcodeCommand.createWarnings(for: warnedDraft)
+    #expect(warnings.count == 1)
+    #expect(warnings[0].contains("idle"))
+
+    let unwarned = try GraphcodeCommand.parse([
+      "node", "create", "/tmp/p", "--title", "S", "--type", "goal",
+      "--goal", "done", "--predicate", "make test",
+    ])
+    guard case .createNode(_, let unwarnedDraft, _) = unwarned else {
+      Issue.record("expected createNode, got \(unwarned)")
+      return
+    }
+    #expect(GraphcodeCommand.createWarnings(for: unwarnedDraft).isEmpty)
+  }
+
+  @Test
+  func theUpdateWarningFiresWhenSkipUnchangedIsTurnedOnForAPredicatedGoalLoop() throws {
+    let graph = goalGraph()
+    let parsed = try GraphcodeCommand.parse([
+      "node", "update", "/tmp/p", UUID().uuidString, "--skip-unchanged", "true",
+    ])
+    guard case .updateNode(_, _, let nodeUpdate) = parsed else {
+      Issue.record("expected updateNode, got \(parsed)")
+      return
+    }
+    #expect(
+      GraphcodeCommand.updateWarnings(for: nodeUpdate, currentNode: graph.nodes[0]).count == 1)
+    // Best-effort: a node the client cannot see (a sub-graph child) warns nobody.
+    #expect(GraphcodeCommand.updateWarnings(for: nodeUpdate, currentNode: nil).isEmpty)
+  }
 }