Skip to content

Import closed issues as terminal tasks (#502)#732

Open
iamnbutler wants to merge 1 commit intomainfrom
tasks/gh-iamnbutler-tasks-issue-502--ec378aa6
Open

Import closed issues as terminal tasks (#502)#732
iamnbutler wants to merge 1 commit intomainfrom
tasks/gh-iamnbutler-tasks-issue-502--ec378aa6

Conversation

@iamnbutler
Copy link
Copy Markdown
Owner

Summary

  • Fixes GitHub polling can miss issues closed before first poll #502: issue_to_task() previously skipped all non-open issues, causing issues closed before the first poll (or between poll intervals) to never be imported.
  • Closed issues are now imported with a terminal state: Completed (default) or Cancelled (if closed as "not planned").
  • Skip/ignore label filtering still applies to closed issues — they won't be imported if they match tasks/skip or a configured ignore label.

Test plan

  • Existing scheduler tests pass (38/38)
  • New test: closed_issue_imported_as_completed — verifies default closed → Completed
  • New test: closed_issue_not_planned_imported_as_cancelled — verifies NotPlanned → Cancelled
  • New test: closed_issue_with_skip_label_not_imported — verifies skip label still respected
  • Full workspace cargo check passes

🤖 Generated with Claude Code

Fixes #502: issues closed before the first poll or between poll intervals
were silently dropped because issue_to_task() returned None for non-open
issues. Now closed issues are imported with a terminal state (Completed
or Cancelled based on closure reason) so they are tracked as "seen".

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR correctly solves the silent-drop problem for issues closed before or between polls. The approach — import closed issues as terminal tasks immediately — is the right design: it ensures deduplication works and avoids re-importing the same issue on future polls. Label guards (skip/ignore) run before the closed-state branch, preserving existing filter behavior. Tests cover the two main cases and the skip label invariant.

One important inconsistency to fix before shipping: issue_to_task maps Unknown closure reason to Completed (via _ fallthrough), while reconcile_task maps it to Cancelled. This means the terminal state of an issue with no state_reason differs depending on whether it was already closed at first poll time or observed transitioning during polling. See the inline comment for the fix.

Build: cargo test --workspace passes. cargo clippy --workspace -- -D warnings has one pre-existing error in crates/models/src/work_queue.rs (should_implement_trait for WorkType::from_str) — not introduced by this PR.


References:

Reviewed by PR / Review

Comment on lines +123 to +129
if is_closed {
task.state = match issue.classify_closure() {
Some(ClosureReason::NotPlanned) => TaskState::Cancelled,
_ => TaskState::Completed,
};
return Some(task);
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[IMPORTANT] — Correctness: Unknown closure reason is mapped inconsistently with reconcile_task.

The _ arm here catches ClosureReason::Unknown (issued closed with no state_reason) and maps it to Completed. But reconcile_task (line 297) maps UnknownCancelled:

// reconcile_task
ClosureReason::NotPlanned | ClosureReason::Unknown => TaskState::Cancelled,

This means the terminal state of a task depends purely on timing: the same issue closed with no state_reason becomes Completed if it was already closed at first poll, but Cancelled if it was observed transitioning during reconciliation.

To stay consistent with the reconcile path:

if is_closed {
    task.state = match issue.classify_closure() {
        Some(ClosureReason::PrMerged | ClosureReason::ManualCompletion) => TaskState::Completed,
        _ => TaskState::Cancelled, // NotPlanned, Unknown, and the unreachable None
    };
    return Some(task);
}

A test for the Unknown case (closed with no state_reason) would lock in the expected behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

GitHub polling can miss issues closed before first poll

1 participant