Repository navigation
Clear the stale Needs input badge when Claude's permission is decided in the terminal #15170
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -767,6 +767,92 @@ struct FeedCoordinatorTests { | |||
| #expect(attention.events.first?.hookEventName == .permissionRequest) | ||||
| } | ||||
|
|
||||
| /// Claude Code keeps its PermissionRequest hook waiting after the user | ||||
| /// answers the prompt in the terminal or the auto-mode classifier decides, | ||||
| /// so the Feed request (and the "Needs input" overlay it owns) outlived the | ||||
| /// decision until the hook timed out, beside the agent's own "Running". | ||||
| /// A later hook from the same agent proves the decision was made elsewhere. | ||||
| @Test func laterClaudeHookRetiresPermissionDecidedOutsideFeed() async { | ||||
| defer { Self.resetFeedCoordinatorTestHooks() } | ||||
| let requestId = "claude-decided-in-terminal-request" | ||||
| let sessionId = "claude-decided-in-terminal-session" | ||||
| let ingested = DispatchSemaphore(value: 0) | ||||
| await MainActor.run { | ||||
| FeedCoordinator.shared.install(store: WorkstreamStore(ringCapacity: 10)) | ||||
| FeedCoordinatorTestHooks.afterBlockingEventIngested = { _, ingestedRequestId in | ||||
| if ingestedRequestId == requestId { ingested.signal() } | ||||
| } | ||||
| } | ||||
|
|
||||
| let permission = WorkstreamEvent( | ||||
| sessionId: sessionId, | ||||
| hookEventName: .permissionRequest, | ||||
| source: "claude", | ||||
| cwd: "/tmp", | ||||
| toolName: "Write", | ||||
| toolInputJSON: #"{"file_path":"/tmp/memory.md"}"#, | ||||
| requestId: requestId, | ||||
| extraFieldsJSON: #"{"_hook_sent_at_ms":2000}"# | ||||
| ) | ||||
| let done = DispatchSemaphore(value: 0) | ||||
| let resultBox = IngestResultBox() | ||||
| let startedAt = ContinuousClock.now | ||||
| DispatchQueue.global(qos: .userInitiated).async { | ||||
| resultBox.value = FeedCoordinator.shared.ingestBlocking(event: permission, waitTimeout: 5) | ||||
| done.signal() | ||||
| } | ||||
| guard waitForFeedTestSignal(ingested, timeout: .now() + 2) == .success else { | ||||
| Issue.record("the blocking PermissionRequest was never ingested") | ||||
| return | ||||
| } | ||||
|
|
||||
| func deliver(_ event: WorkstreamEvent) { | ||||
| // Acknowledged (non-decision) ingress commits before returning. | ||||
| let delivered = DispatchSemaphore(value: 0) | ||||
| DispatchQueue.global(qos: .userInitiated).async { | ||||
| _ = FeedCoordinator.shared.ingestBlocking(event: event, waitTimeout: 1) | ||||
| delivered.signal() | ||||
| } | ||||
| #expect(waitForFeedTestSignal(delivered, timeout: .now() + 2) == .success) | ||||
| } | ||||
| // The tool's own PreToolUse is sent before its permission request. | ||||
| deliver(WorkstreamEvent( | ||||
| sessionId: sessionId, hookEventName: .preToolUse, source: "claude", | ||||
| toolName: "Write", extraFieldsJSON: #"{"_hook_sent_at_ms":1990}"# | ||||
| )) | ||||
| // A subagent's tool proves nothing about the main agent's prompt. | ||||
| deliver(WorkstreamEvent( | ||||
| sessionId: sessionId, hookEventName: .preToolUse, source: "claude", | ||||
| toolName: "Bash", extraFieldsJSON: #"{"_hook_sent_at_ms":3000,"agent_id":"subagent-1"}"# | ||||
| )) | ||||
| #expect( | ||||
| FeedCoordinator.shared.isAwaitingDecision(requestId: requestId), | ||||
| "earlier or other-agent hooks must not retire a live permission request" | ||||
| ) | ||||
|
|
||||
| // The user answered in the terminal; Claude moved on to the next tool. | ||||
| deliver(WorkstreamEvent( | ||||
| sessionId: sessionId, hookEventName: .preToolUse, source: "claude", | ||||
| toolName: "Bash", extraFieldsJSON: #"{"_hook_sent_at_ms":3000}"# | ||||
| )) | ||||
| guard waitForFeedTestSignal(done, timeout: .now() + 2) == .success else { | ||||
| Issue.record("a later PreToolUse must retire the permission request decided in the terminal") | ||||
| return | ||||
| } | ||||
| #expect(startedAt.duration(to: .now) < .seconds(4)) | ||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win Remove the elapsed-duration assertion. Line 842 asserts Proposed fix- #expect(startedAt.duration(to: .now) < .seconds(4))Also remove As per coding guidelines: "An assertion on a measured wall-clock duration, or a hard absolute latency ceiling on shared CI." 📝 Committable suggestion
Suggested change
🤖 Prompt for AI AgentsSource: Coding guidelines |
||||
| guard case .unavailable = resultBox.value else { | ||||
| Issue.record("the superseded hook must return no decision, got \(String(describing: resultBox.value))") | ||||
| return | ||||
| } | ||||
| let status = await MainActor.run { | ||||
| FeedCoordinator.shared.store.items.first { $0.kind == .permissionRequest }?.status | ||||
| } | ||||
| guard case .expired = status else { | ||||
| Issue.record("the superseded permission card must stop being actionable") | ||||
| return | ||||
| } | ||||
| } | ||||
|
|
||||
| @Test func blockingDecisionEventPredicateCoversEveryDecisionKind() { | ||||
| // The three blocking-decision kinds must all surface attention… | ||||
| #expect(FeedCoordinator.isBlockingDecisionEvent(.permissionRequest)) | ||||
|
|
||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the wall-clock assertion on
_hook_sent_at_ms.Line 119 reads
Date(). Line 140 then assertssentAtMs >= startedAtMs. The test guidelines ban readingDate()in an assertion. The comparison also mixes two clocks: the CLI process clock and the test clock. An NTP step between the two reads makes a correct build fail. Assert on presence and a positive value instead.Proposed fix
As per coding guidelines: "Reading
Date()/Date.now/ ... in an assertion."📝 Committable suggestion
🤖 Prompt for AI Agents
Source: Coding guidelines