-
Notifications
You must be signed in to change notification settings - Fork 7
fix: prevent pre-dispatch false positives from substring and non-completion matches #807
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 | ||||
|---|---|---|---|---|---|---|
|
|
@@ -2566,15 +2566,27 @@ check_task_already_done() { | |||||
| fi | ||||||
|
|
||||||
| # Check 2: Are there merged commits referencing this task ID? | ||||||
| # Look for commit messages containing the task ID (e.g., "t135.4", "(t135.4)") | ||||||
| # Only search the last 500 commits for performance | ||||||
| # IMPORTANT: Use word-boundary matching to prevent t020 matching t020.6. | ||||||
| # Escaped task_id for regex: dots become literal dots. | ||||||
| local escaped_task_regex | ||||||
| escaped_task_regex=$(printf '%s' "$task_id" | sed 's/\./\\./g') | ||||||
| # grep -w uses word boundaries but dots aren't word chars, so for subtask IDs | ||||||
| # like t020.1 we need a custom boundary: task_id followed by non-digit or EOL. | ||||||
| # This prevents t020 from matching t020.1, t020.2, etc. | ||||||
| local boundary_pattern="${task_id}([^.0-9]|$)" | ||||||
|
|
||||||
| local commit_count=0 | ||||||
| commit_count=$(git -C "$project_root" log --oneline -500 --all --grep="$task_id" 2>/dev/null | wc -l | tr -d ' ') || true | ||||||
| commit_count=$(git -C "$project_root" log --oneline -500 --all --grep="$task_id" 2>/dev/null \ | ||||||
| | grep -cE "$boundary_pattern" 2>/dev/null) || true | ||||||
| if [[ "$commit_count" -gt 0 ]]; then | ||||||
| # Verify at least one commit looks like a completion (not just "claim" or "blocked") | ||||||
| # Verify at least one commit looks like a REAL completion: | ||||||
| # Must have a PR merge reference "(#NNN)" AND the exact task ID. | ||||||
| # Exclude: "add tNNN", "claim tNNN", "mark tNNN blocked", "queue tNNN" | ||||||
| local completion_evidence="" | ||||||
| completion_evidence=$(git -C "$project_root" log --oneline -500 --all --grep="$task_id" 2>/dev/null \ | ||||||
| | grep -iE "(feat|fix|refactor|chore|perf|test).*${task_id}|${task_id}.*(\(#[0-9]+\)|PR #[0-9]+|merged)" \ | ||||||
| | grep -E "$boundary_pattern" \ | ||||||
| | grep -iE "\(#[0-9]+\)|PR #[0-9]+ merged" \ | ||||||
| | grep -ivE "add ${task_id}|claim ${task_id}|mark ${task_id}|queue ${task_id}|blocked" \ | ||||||
|
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. Similar to the issue with
Suggested change
|
||||||
| | head -1) || true | ||||||
| if [[ -n "$completion_evidence" ]]; then | ||||||
| log_info "Pre-dispatch check: $task_id has completion evidence: $completion_evidence" >&2 | ||||||
|
|
@@ -2583,10 +2595,11 @@ check_task_already_done() { | |||||
| fi | ||||||
|
|
||||||
| # Check 3: Does a merged PR exist for this task? | ||||||
| # Only check if gh CLI is available and authenticated (cached check) | ||||||
| # Only check if gh CLI is available and authenticated (cached check). | ||||||
| # Use exact task ID in title search to prevent substring matches. | ||||||
| if command -v gh &>/dev/null && check_gh_auth 2>/dev/null; then | ||||||
| local pr_count=0 | ||||||
| pr_count=$(gh pr list --repo "$project_root" --state merged --search "$task_id in:title" --limit 1 --json number --jq 'length' 2>/dev/null) || true | ||||||
| pr_count=$(gh pr list --repo "$project_root" --state merged --search "\"$task_id\" in:title" --limit 1 --json number --jq 'length' 2>/dev/null) || true | ||||||
| if [[ "$pr_count" -gt 0 ]]; then | ||||||
| log_info "Pre-dispatch check: $task_id has a merged PR on GitHub" >&2 | ||||||
| return 0 | ||||||
|
|
||||||
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.
The
boundary_patternis constructed using the unescaped$task_id. If the task ID contains special regex characters (like a dot int020.1), it will be misinterpreted bygrep -E, leading to incorrect matches. Theescaped_task_regexvariable was correctly created for this purpose but is not used.Using the escaped variable will fix the bug and also resolve a ShellCheck violation (SC2034), adhering to the repository's style guide.
References
escaped_task_regexvariable would trigger a ShellCheck warning (SC2034). Using it as suggested resolves this violation. (link)