feat(integrations): execute GitHub issue import + task traceability (#565)#609
Conversation
Add external_url + auto_close_github_issue columns to the v2 tasks table (both _init_database and _ensure_schema_upgrades), persist github_issue_number through create(), and add get_by_github_issue_number() (duplicate-import protection) and update_auto_close() helpers.
Backend for executing the GitHub issue import:
- core/github_issues_service: get_issue() + close_issue()
- POST /api/v2/integrations/github/import (dedupe via github_issue_number,
labels footer in description) and PATCH /issues/{number}/close
- tasks_v2: expose github_issue_number/external_url/auto_close_github_issue on
TaskResponse, accept auto_close toggle on PATCH, fire fire-and-forget issue
close on the DONE transition (failures swallowed)
- tests disable the shared rate limiter to avoid ip:testclient bucket bleed
- integrationsApi.importIssues/closeIssue + tasksApi.updateGitHubSettings - GitHubIssueBadge component (links a task to its source issue) - TaskBoardView: real import flow with in-progress state + summary banner (N created · M skipped) and board refresh - TaskDetailModal: GitHub badge + 'close issue when DONE' checkbox (optimistic) - Task type extended with github_issue_number/external_url/auto_close_github_issue
Address cross-family (codex) review: - P1: move GitHub issue auto-close from the HTTP PATCH path into core tasks.update_status (the single DONE chokepoint), so agent/batch (via runtime.complete_run) and CLI completions also close the linked issue. Fire-and-forget on a daemon thread, fully guarded — mirrors the existing blockers/conductor webhook dispatch pattern. - P2: de-duplicate imports on the full issue URL (get_by_external_url) instead of the bare issue number, so the same number in a different connected repo is still importable. Import now fetches then de-dupes by html_url.
…se (#565) Address second codex review pass: - Persist auto_close_github_issue before the status transition in update_task so a single PATCH {status: DONE, auto_close_github_issue: true} closes the issue (the core DONE dispatch now reads the fresh flag). - Run the auto-close thread non-daemon so a short-lived CLI process waits for the GitHub close at exit (bounded by the ~15s client timeout) instead of abandoning it. The server response still returns without joining.
…ide-effects (#565) Third codex review pass: - P1: auto-close now targets the task's source repo parsed from external_url, not the workspace's current connection — completing an old imported task after reconnecting to a different repo no longer closes the wrong issue. - P2: import is two-phase (fetch+dedupe all, then create) so a mid-batch fetch failure leaves no partial tasks behind. - P2: update_task pre-validates the status transition before any mutation, so a rejected PATCH no longer persists the auto-close flag (and illegal transitions now return a proper 400).
…565) Fourth codex review pass: - De-dupe issue numbers within a single /import payload so a repeated number creates one task (was: one per occurrence). - Reject pull-request numbers in get_issue (NotAnIssueError -> 422), matching list_issues which already excludes PRs — a PR can no longer be imported as a task. - Remove the unused standalone PATCH /issues/{n}/close endpoint + its frontend client. It was dead code and, lacking task context, would close the wrong repo's issue after a reconnect. Auto-close runs through the core dispatch, which correctly targets each task's source repo (external_url).
Fifth codex review pass: - Auto-close is now context-aware (mirrors WebhookNotificationService): in the server it schedules on the running event loop (no thread to join at shutdown); only in sync/CLI contexts does it use a non-daemon thread so the close completes before the process exits. Bounded by a 10s timeout so a hung GitHub call never stalls a CLI for long. Resolves the daemon-vs-completion tension from the prior pass. - Add a UNIQUE index on tasks(workspace_id, external_url) so duplicate-import protection is atomic: concurrent imports (double-submit / two tabs) can no longer both create a task for the same issue. The import catches the IntegrityError and records the loser as skipped.
Sixth codex review pass: - close_issue: posting the completion comment is now best-effort. A locked issue or comment-disabled repo no longer aborts before the close — the close (the operation that matters) always proceeds. - update_task: enabling auto-close on a task that is ALREADY DONE now closes the issue immediately (new tasks.autoclose_if_done helper), instead of silently doing nothing until some future re-completion.
#565) Seventh codex review pass: - The late opt-in close (for already-DONE tasks) now only fires when the PATCH is NOT also changing status. A combined {status: READY, auto_close: true} request reopens the task without closing the issue — previously it closed the issue while reopening the task (opposite of intent). Rebuttal (out of scope): auto-close resolves the PAT from the single machine-wide GIT_GITHUB credential (the #555/#563 model). After reconnecting a workspace to a different repo, closing an older imported task whose repo needs a different PAT may 403/404 (logged, best-effort). Per-repo credential storage is a broader change than #565; documented as a Known Limitation.
…ure (#565) Eighth codex review pass: - get_issue now raises IssueNotFoundError on a 404 (distinct from upstream GitHubConnectError), and the import maps it to 404 instead of 502 — a stale/typo'd issue number is reported as a client error, not a server failure. - TaskBoardView: the post-import board refresh (mutate) runs after the import resolves and outside its try/catch, so a transient /api/v2/tasks refresh error no longer flips a successful import into a 'Failed to import' banner.
Ninth codex review pass: - get_issue: a 404 on the issue endpoint now probes the repo to distinguish a genuinely missing issue (-> IssueNotFoundError -> 404) from a broken integration (renamed/deleted repo or rotated token -> connect/auth error), so a real auth/repo failure is no longer reported to the user as a stale issue number. The probe only runs on the 404 path. - update_task: apply the status transition BEFORE persisting the auto-close flag, so a concurrently-rejected transition (now a 409) leaves no persisted side effect. The explicit late-opt-in close now fires on a newly-enabled flag (false->true) when the task is DONE — double-fire-safe with the core DONE-transition close.
Tenth codex review pass: - Import phase 2 now rolls back already-created tasks if a create fails with an unexpected DB error (e.g. OperationalError on a locked DB), preserving the all-or-nothing contract. (The URL-unique index still makes a retry idempotent.) - A malformed saved repo slug (parse_repo ValueError) now returns 409 from /import, consistent with the browse endpoint, instead of a 500.
…back) (#565) Eleventh codex review pass: - Persist the auto-close flag BEFORE the status transition again, so the core DONE dispatch reads the value requested in THIS PATCH: {status: DONE, opt-out} no longer closes the issue (and {status: DONE, opt-in} still does). - Restore the pass-9 atomicity guarantee differently: if the transition is rejected by a concurrent change, roll the flag back to its original value (409), so a failed request leaves no side effect. - Late opt-in (false->true on an already-DONE task, no transition) still closes explicitly; double-fire and re-close are avoided by gating on (newly-enabled AND no transition in this request).
Twelfth codex review pass: - Import failures now render inline INSIDE the import modal (new importError prop) and keep it open, so the error is visible and the selection is preserved for a retry — previously a board-level banner sat hidden behind the open dialog. - After a successful import, the 60s issue-browse cache for the repo is invalidated, so reopening the modal no longer offers just-imported issues as selectable (they would now be skipped as duplicates).
Thirteenth codex review pass: - Always invalidate the repo's issue-browse cache after an import, including all-skipped imports (issues created by another tab/process), so the modal never offers already-imported issues as selectable within the 60s TTL. Rebuttal (P2, daemon thread — re-raised across passes 3/5/13): the auto-close runs on a non-daemon thread in sync/CLI contexts BY DESIGN. For a CLI process to RELIABLY close the issue it must wait for the close; abandoning it (daemon=True) is a silent feature failure (the pass-3 finding). The wait is bounded by the 10s client timeout and is zero in the common case (fast GitHub completes before the command exits). Documented as a Known Limitation rather than reverted.
…ects) (#565) Fourteenth codex review pass: - get_issue 404 repo-probe: a degraded probe response (429/5xx) now raises GitHubConnectError instead of IssueNotFoundError, so a GitHub outage during import isn't misreported as a stale issue number. - close_issue: a 3xx redirect (moved/renamed/transferred repo) is now treated as a failure — httpx doesn't follow redirects, so the PATCH wasn't applied and the issue is still open; we no longer report a silent success.
|
Warning Review limit reached
More reviews will be available in 13 minutes and 36 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
WalkthroughThis PR implements end-to-end GitHub issue import and auto-close for CodeFRAME tasks. It adds GitHub service functions to fetch and close issues, extends the task model to track GitHub linkage and auto-close preferences, introduces an import API endpoint with deduplication and rollback semantics, updates task API responses and logic, and provides complete frontend UI for importing issues and toggling auto-close behavior. ChangesGitHub Issue Import and Auto-Close Feature
Sequence Diagram(s)sequenceDiagram
participant User
participant TaskBoard
participant ImportModal
participant IntegrationsAPI
participant TasksService
participant GitHubAPI
User->>TaskBoard: Click "Import from GitHub"
TaskBoard->>ImportModal: Show modal
User->>ImportModal: Select issues, click Import
ImportModal->>IntegrationsAPI: POST /import with [issue_numbers]
IntegrationsAPI->>TasksService: Validate connection
IntegrationsAPI->>GitHubAPI: get_issue() for each number
alt Issues fetched
GitHubAPI-->>IntegrationsAPI: Issue details
IntegrationsAPI->>IntegrationsAPI: Deduplicate & skip PRs
IntegrationsAPI->>TasksService: create() for each unique
alt Create succeeds
TasksService-->>IntegrationsAPI: Created tasks
IntegrationsAPI->>IntegrationsAPI: Invalidate cache
IntegrationsAPI-->>ImportModal: {created, skipped, total}
ImportModal->>TaskBoard: Close + trigger mutate()
TaskBoard->>TaskBoard: Refresh board
else Create fails mid-batch
TasksService-->>IntegrationsAPI: Error
IntegrationsAPI->>TasksService: Rollback created
IntegrationsAPI-->>ImportModal: Import error
end
else Fetch fails
GitHubAPI-->>IntegrationsAPI: Error (502)
IntegrationsAPI-->>ImportModal: No tasks created
end
User->>TaskBoard: Mark task DONE with auto-close enabled
TaskBoard->>TasksService: update_status(DONE) + auto_close=true
TasksService->>GitHubAPI: close_issue() (background)
GitHubAPI-->>TasksService: 200 OK
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Code Review -- PR #609: GitHub Issue Import + Task Traceability (#565)This PR completes Phase 5.5 by wiring up the import execution that was stubbed in #564 and adding full task-issue traceability. The scope, structure, and test coverage are all strong. A few targeted notes below. Strengths
Issues1. Connection leak in If 2. Test fragility around credential resolution in
If 3.
Not asking for a refactor now -- the PR description documents this tradeoff and it is clearly deliberate -- but it is worth a 4. Minor: The function calls Nitpicks
SummaryThe implementation is correct, well-tested, and architecturally consistent with the existing patterns. The two items I would address before merge are the credential-resolution fragility in the dispatch tests (so the tests are clearly verifying what they claim) and the connection-leak edge case in |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
codeframe/core/tasks.py (1)
228-270: ⚡ Quick winMake late opt-in auto-close part of
update_auto_close().Right now this helper only flips the persisted flag. For an already-
DONEtask, callers have to remember to also invokeautoclose_if_done(), which makes the late-opt-in contract easy to miss across entry points.♻️ Suggested consolidation
def update_auto_close( workspace: Workspace, task_id: str, auto_close: bool, ) -> Task: @@ task = get(workspace, task_id) if not task: raise ValueError(f"Task not found: {task_id}") + was_auto_close = task.auto_close_github_issue now = _utc_now().isoformat() @@ task.auto_close_github_issue = auto_close task.updated_at = datetime.fromisoformat(now) + if auto_close and not was_auto_close: + autoclose_if_done(workspace, task) return task🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@codeframe/core/tasks.py` around lines 228 - 270, The update_auto_close function currently only flips the persisted auto_close flag; modify it so that after successfully updating the DB and updating the Task object (in update_auto_close), if auto_close is being turned on and the task is already in DONE state, immediately invoke the existing autoclose_if_done helper to perform the late opt-in auto-close flow; locate update_auto_close, get(), and autoclose_if_done() and add the call (or import) so the DB commit happens first, then set task.auto_close_github_issue = auto_close, task.updated_at = ..., and if auto_close and task.status == "DONE" (or Task.Status.DONE) call autoclose_if_done(workspace, task) (or the appropriate autoclose_if_done signature) to apply the late opt-in behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@codeframe/core/github_issues_service.py`:
- Around line 300-323: get_issue currently parses resp.json() even for 3xx
responses and only raises on >=400; adjust get_issue to treat any status_code >=
300 as an error (consistent with close_issue) before calling resp.json(), e.g.
call _raise_for_status or explicitly check resp.status_code >= 300 and raise an
appropriate integration/error (not NotAnIssueError) so redirects/3xx are not
parsed into empty issue payloads; keep the existing PR check ("pull_request" ->
NotAnIssueError) and field defaults but ensure they only run after confirming
resp is a successful 2xx response.
In `@codeframe/ui/routers/github_integrations_v2.py`:
- Around line 98-101: ImportRequest.issue_numbers currently allows zero/negative
values; update its type to enforce item-level >=1 validation so bad payloads
fail at the API boundary. Replace list[int] with a constrained integer type for
list items (e.g., use pydantic.conint(ge=1) or pydantic.types.PositiveInt) and
import the required symbol, keeping the Field min_length=1 for the list itself;
update any type hints/usages that reference ImportRequest.issue_numbers
accordingly.
In `@codeframe/ui/routers/tasks_v2.py`:
- Around line 309-344: The handler currently uses truthiness checks so a
provided empty string status ({"status": ""}) is treated as omitted; change
presence checks to use "is not None" when deciding to fetch current (i.e., if
body.status is not None or body.auto_close_github_issue is not None) and when
validating status use "if body.status is not None:"; inside that branch
explicitly reject empty strings (e.g., if body.status == "" raise the same
400/api_error as invalid status), then attempt TaskStatus(body.status.upper())
and use can_transition(current.status, new_status) as before; references:
body.status, body.auto_close_github_issue, tasks.get, current, TaskStatus,
can_transition.
In `@web-ui/src/components/tasks/TaskBoardView.tsx`:
- Around line 131-137: The catch block currently calls setImportError(...) even
if the import modal was closed while the request was in flight; guard that call
by checking the modal's open state (e.g., an isImportModalOpen prop/state or a
mounted/ref flag) before calling setImportError, and also clear any stale import
error when the modal is opened (add a useEffect or open handler that calls
setImportError('') when the import modal becomes open) so users don't see prior
errors on reopen; update the catch to only setImportError if the modal is still
open and/or component is mounted.
In `@web-ui/src/components/tasks/TaskDetailModal.tsx`:
- Around line 133-146: handleToggleAutoClose can issue overlapping PATCHes
causing out-of-order persistence; add a local in-flight guard (e.g.,
isUpdatingAutoClose state ref/boolean) and early-return when an update is
already in progress, clear any existing error before starting, perform the
optimistic setTask update, await tasksApi.updateGitHubSettings, and only on
catch revert to the previous value and setError; ensure you reset the in-flight
flag in finally. Apply the identical pattern to the other GitHub-setting toggle
handler in this component.
---
Nitpick comments:
In `@codeframe/core/tasks.py`:
- Around line 228-270: The update_auto_close function currently only flips the
persisted auto_close flag; modify it so that after successfully updating the DB
and updating the Task object (in update_auto_close), if auto_close is being
turned on and the task is already in DONE state, immediately invoke the existing
autoclose_if_done helper to perform the late opt-in auto-close flow; locate
update_auto_close, get(), and autoclose_if_done() and add the call (or import)
so the DB commit happens first, then set task.auto_close_github_issue =
auto_close, task.updated_at = ..., and if auto_close and task.status == "DONE"
(or Task.Status.DONE) call autoclose_if_done(workspace, task) (or the
appropriate autoclose_if_done signature) to apply the late opt-in behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ccb495d4-ec5f-4570-a4ab-c4c1259af34b
📒 Files selected for processing (19)
codeframe/core/github_issues_service.pycodeframe/core/tasks.pycodeframe/core/workspace.pycodeframe/ui/routers/github_integrations_v2.pycodeframe/ui/routers/tasks_v2.pytests/core/test_github_issues_service.pytests/core/test_task_github_traceability.pytests/ui/test_github_integrations_v2.pytests/ui/test_v2_routers_integration.pyweb-ui/src/__tests__/components/tasks/GitHubIssueBadge.test.tsxweb-ui/src/__tests__/components/tasks/GitHubIssueImportModal.test.tsxweb-ui/src/__tests__/components/tasks/TaskBoardView.import565.test.tsxweb-ui/src/__tests__/components/tasks/TaskDetailModal.test.tsxweb-ui/src/components/tasks/GitHubIssueBadge.tsxweb-ui/src/components/tasks/GitHubIssueImportModal.tsxweb-ui/src/components/tasks/TaskBoardView.tsxweb-ui/src/components/tasks/TaskDetailModal.tsxweb-ui/src/lib/api.tsweb-ui/src/types/index.ts
| _raise_for_status(resp.status_code, context="get issue") | ||
|
|
||
| raw = resp.json() | ||
| if not isinstance(raw, dict): | ||
| raw = {} | ||
| # The issues endpoint also returns pull requests (a PR is an issue with a | ||
| # ``pull_request`` member). Reject them so the import stays consistent | ||
| # with ``list_issues`` (which excludes PRs) and never links a PR as an | ||
| # issue. | ||
| if "pull_request" in raw: | ||
| raise NotAnIssueError(f"#{number} is a pull request, not an issue.") | ||
| labels_raw = raw.get("labels") or [] | ||
| labels = [ | ||
| (lbl.get("name") if isinstance(lbl, dict) else str(lbl)) | ||
| for lbl in labels_raw | ||
| ] | ||
| labels = [n for n in labels if n] | ||
| return { | ||
| "number": int(raw.get("number", number)), | ||
| "title": str(raw.get("title") or ""), | ||
| "body": str(raw.get("body") or ""), | ||
| "labels": labels, | ||
| "html_url": str(raw.get("html_url") or ""), | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="codeframe/core/github_issues_service.py"
# Show surrounding definitions for get_issue, close_issue, and _raise_for_status
rg -n "async def (get_issue|close_issue)\b|def _raise_for_status\b" "$file"
python3 - <<'PY'
import re, itertools, pathlib
path = pathlib.Path("codeframe/core/github_issues_service.py")
txt = path.read_text(encoding="utf-8")
def show_context(name, window=80):
# find def line
m = re.search(rf"(async\s+def\s+{name}\b|def\s+{name}\b)", txt)
if not m:
print("Missing:", name); return
start = txt.rfind("\n", 0, m.start())
# compute line numbers
lines = txt.splitlines()
# map index to line
idx = m.start()
line_no = txt[:idx].count("\n")+1
a=max(1,line_no-window)
b=min(len(lines), line_no+window)
for i in range(a,b+1):
print(f"{i}:{lines[i-1]}")
print("\n"+"="*60+"\n")
show_context("_raise_for_status", 120)
show_context("get_issue", 160)
show_context("close_issue", 160)
PY
# Also quickly check for any explicit redirect-follow settings or status handling in the client call sites.
rg -n "redirect|allow_redirect|3xx|status_code >|>= 300|>=300" "$file"Repository: frankbria/codeframe
Length of output: 33619
Handle redirect/3xx responses in get_issue() instead of parsing them as an issue payload.
close_issue() already treats resp.status_code >= 300 as a failure, but get_issue() only raises on >= 400 and then proceeds to resp.json() and defaults missing fields—so a 3xx response can produce empty/incorrect imported issue data instead of a clear integration error.
🛠️ Suggested guard
if resp.status_code == 404:
try:
repo_resp = await client.get(
f"{GITHUB_API_BASE}/repos/{owner}/{name}", headers=_headers(pat)
)
@@
if repo_resp.status_code >= 300:
raise GitHubConnectError(
f"GitHub repo check returned status {repo_resp.status_code}."
)
raise IssueNotFoundError(f"Issue #{number} was not found in '{repo}'.")
_raise_for_status(resp.status_code, context="get issue")
+ if resp.status_code >= 300:
+ raise GitHubConnectError(
+ f"GitHub get issue returned status {resp.status_code}; "
+ "issue details were not fetched."
+ )
raw = resp.json()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@codeframe/core/github_issues_service.py` around lines 300 - 323, get_issue
currently parses resp.json() even for 3xx responses and only raises on >=400;
adjust get_issue to treat any status_code >= 300 as an error (consistent with
close_issue) before calling resp.json(), e.g. call _raise_for_status or
explicitly check resp.status_code >= 300 and raise an appropriate
integration/error (not NotAnIssueError) so redirects/3xx are not parsed into
empty issue payloads; keep the existing PR check ("pull_request" ->
NotAnIssueError) and field defaults but ensure they only run after confirming
resp is a successful 2xx response.
| class ImportRequest(BaseModel): | ||
| issue_numbers: list[int] = Field( | ||
| ..., min_length=1, description="GitHub issue numbers to import as tasks" | ||
| ) |
There was a problem hiding this comment.
Reject non-positive issue numbers at the API boundary.
issue_numbers currently accepts 0 and negative values, so malformed payloads still go through the GitHub fetch path and fail later as lookup errors instead of a validation error. Add an item-level >= 1 constraint here so bad requests are rejected before any external call.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@codeframe/ui/routers/github_integrations_v2.py` around lines 98 - 101,
ImportRequest.issue_numbers currently allows zero/negative values; update its
type to enforce item-level >=1 validation so bad payloads fail at the API
boundary. Replace list[int] with a constrained integer type for list items
(e.g., use pydantic.conint(ge=1) or pydantic.types.PositiveInt) and import the
required symbol, keeping the Field min_length=1 for the list itself; update any
type hints/usages that reference ImportRequest.issue_numbers accordingly.
| if body.status or body.auto_close_github_issue is not None: | ||
| current = tasks.get(workspace, task_id) | ||
| if current is None: | ||
| raise HTTPException( | ||
| status_code=404, | ||
| detail=api_error( | ||
| "Task not found", | ||
| ErrorCodes.NOT_FOUND, | ||
| f"No task with id {task_id}", | ||
| ), | ||
| ) | ||
|
|
||
| new_status: Optional[TaskStatus] = None | ||
| if body.status: | ||
| try: | ||
| new_status = TaskStatus(body.status.upper()) | ||
| tasks.update_status(workspace, task_id, new_status) | ||
| except ValueError as e: | ||
| if "Invalid status" in str(e) or "not a valid" in str(e).lower(): | ||
| raise HTTPException( | ||
| status_code=400, | ||
| detail=api_error( | ||
| f"Invalid status: {body.status}", | ||
| ErrorCodes.VALIDATION_ERROR, | ||
| f"Valid values: {[s.value for s in TaskStatus]}", | ||
| ), | ||
| ) | ||
| # Status transition error | ||
| except ValueError: | ||
| raise HTTPException( | ||
| status_code=400, | ||
| detail=api_error("Invalid status transition", ErrorCodes.INVALID_STATE, str(e)), | ||
| detail=api_error( | ||
| f"Invalid status: {body.status}", | ||
| ErrorCodes.VALIDATION_ERROR, | ||
| f"Valid values: {[s.value for s in TaskStatus]}", | ||
| ), | ||
| ) | ||
| if not can_transition(current.status, new_status): | ||
| raise HTTPException( | ||
| status_code=400, | ||
| detail=api_error( | ||
| "Invalid status transition", | ||
| ErrorCodes.INVALID_STATE, | ||
| f"Cannot transition from {current.status.value} to " | ||
| f"{new_status.value}", | ||
| ), | ||
| ) | ||
|
|
There was a problem hiding this comment.
Reject empty status values instead of treating them as omitted.
These truthiness checks let {"status": ""} skip validation entirely, so the handler can return 200 while silently ignoring a malformed status update. Use is not None for the presence checks, then validate the provided value.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@codeframe/ui/routers/tasks_v2.py` around lines 309 - 344, The handler
currently uses truthiness checks so a provided empty string status ({"status":
""}) is treated as omitted; change presence checks to use "is not None" when
deciding to fetch current (i.e., if body.status is not None or
body.auto_close_github_issue is not None) and when validating status use "if
body.status is not None:"; inside that branch explicitly reject empty strings
(e.g., if body.status == "" raise the same 400/api_error as invalid status),
then attempt TaskStatus(body.status.upper()) and use
can_transition(current.status, new_status) as before; references: body.status,
body.auto_close_github_issue, tasks.get, current, TaskStatus, can_transition.
| } catch (err) { | ||
| const apiErr = err as ApiError; | ||
| // Keep the modal open and show the error inline there, so the user sees | ||
| // it (a board-level banner would sit behind the dialog) and keeps their | ||
| // selection for a retry. | ||
| setImportError(apiErr.detail || 'Failed to import issues from GitHub'); | ||
| } finally { |
There was a problem hiding this comment.
Clear stale import errors on reopen after close-during-import.
At Line 136, setImportError(...) still runs if the modal was closed while the import request was in flight. On the next open, users can see a stale prior error before retrying.
Suggested fix
- {githubConnected && (
- <Button
- variant="outline"
- onClick={() => setImportModalOpen(true)}
- >
+ {githubConnected && (
+ <Button
+ variant="outline"
+ onClick={() => {
+ setImportError(null);
+ setImportModalOpen(true);
+ }}
+ >
<GithubIcon className="mr-2 h-4 w-4" />
Import from GitHub
</Button>
)}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web-ui/src/components/tasks/TaskBoardView.tsx` around lines 131 - 137, The
catch block currently calls setImportError(...) even if the import modal was
closed while the request was in flight; guard that call by checking the modal's
open state (e.g., an isImportModalOpen prop/state or a mounted/ref flag) before
calling setImportError, and also clear any stale import error when the modal is
opened (add a useEffect or open handler that calls setImportError('') when the
import modal becomes open) so users don't see prior errors on reopen; update the
catch to only setImportError if the modal is still open and/or component is
mounted.
| // Toggle "close GitHub issue when DONE" for imported tasks (#565). Optimistic: | ||
| // flip locally, then persist; revert on failure. | ||
| const handleToggleAutoClose = async (checked: boolean) => { | ||
| if (!task) return; | ||
| const previous = task.auto_close_github_issue ?? false; | ||
| setTask({ ...task, auto_close_github_issue: checked }); | ||
| try { | ||
| await tasksApi.updateGitHubSettings(workspacePath, task.id, checked); | ||
| } catch (err) { | ||
| const apiErr = err as ApiError; | ||
| setError(apiErr.detail || 'Failed to update GitHub setting'); | ||
| setTask({ ...task, auto_close_github_issue: previous }); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Prevent overlapping auto-close writes to avoid persisted state drift.
The toggle can dispatch concurrent PATCH requests; if responses arrive out of order, the saved auto_close_github_issue can end up opposite to the final UI selection. Gate updates while one request is in flight, and clear stale errors before retry.
Suggested fix
@@
const [isUpdating, setIsUpdating] = useState(false);
+ const [isUpdatingGitHubSetting, setIsUpdatingGitHubSetting] = useState(false);
@@
const handleToggleAutoClose = async (checked: boolean) => {
- if (!task) return;
+ if (!task || isUpdatingGitHubSetting) return;
const previous = task.auto_close_github_issue ?? false;
- setTask({ ...task, auto_close_github_issue: checked });
+ setError(null);
+ setTask((prev) => (prev ? { ...prev, auto_close_github_issue: checked } : prev));
+ setIsUpdatingGitHubSetting(true);
try {
await tasksApi.updateGitHubSettings(workspacePath, task.id, checked);
} catch (err) {
const apiErr = err as ApiError;
setError(apiErr.detail || 'Failed to update GitHub setting');
- setTask({ ...task, auto_close_github_issue: previous });
+ setTask((prev) => (prev ? { ...prev, auto_close_github_issue: previous } : prev));
+ } finally {
+ setIsUpdatingGitHubSetting(false);
}
};
@@
<Checkbox
checked={task.auto_close_github_issue ?? false}
onCheckedChange={(c) => handleToggleAutoClose(c === true)}
+ disabled={isUpdatingGitHubSetting}
aria-label="Close GitHub issue when task is marked DONE"
/>Also applies to: 237-243
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web-ui/src/components/tasks/TaskDetailModal.tsx` around lines 133 - 146,
handleToggleAutoClose can issue overlapping PATCHes causing out-of-order
persistence; add a local in-flight guard (e.g., isUpdatingAutoClose state
ref/boolean) and early-return when an update is already in progress, clear any
existing error before starting, perform the optimistic setTask update, await
tasksApi.updateGitHubSettings, and only on catch revert to the previous value
and setError; ensure you reset the in-flight flag in finally. Apply the
identical pattern to the other GitHub-setting toggle handler in this component.
The @rate_limit_* decorators bind the limiter at router-import time and the limits library keeps counters in a process-shared in-memory store, so the fixed ip:testclient AI bucket (connect/import = 20/min) is shared across the whole test suite and exhausts in the full v2 run — 429ing /connect and surfacing as spurious 409s in the GitHub import tests (green in isolation, red in CI). Disable the global default limiter once in the root conftest, BEFORE any router import, so every route imports unwrapped. Verified with the exact CI invocation (pytest tests/ --ignore=tests/e2e -m v2): 2949 passed. Rate-limit-specific tests build their own explicit Limiter and are unaffected; the per-file workaround is removed.
…565) Address advisory review feedback: - Wrap the get_by_external_url DB connection in try/finally so it can't leak on a query error (matches the create/update_auto_close pattern). - Comment the GITHUB_TOKEN -> CredentialManager.get_credential resolution in the auto-close dispatch tests so the test intent is explicit. (Other advisory notes declined: in-place mutation in update_auto_close matches every sibling update_* in the module; a TODO comment on the context-aware dispatch is disallowed by project convention and the tradeoff is documented in the PR body.)
Code Review — PR #609: GitHub Issue Import + Task TraceabilityOverall this is a solid, well-structured implementation of Phase 5.5. The architecture is correct — auto-close lives in Bug / Correctness
if (
body.auto_close_github_issue
and original_auto_close is False
and new_status is None
):
tasks.autoclose_if_done(workspace, tasks.get(workspace, task_id))
Test Reliability
Tests in pat = CredentialManager().get_credential(CredentialProvider.GIT_GITHUB)
if not pat:
return # skipThis works only if Fragile Pattern (pre-existing, extended by this PR)
external_url=row[19] if len(row) > 19 else None,
auto_close_github_issue=bool(row[20]) if len(row) > 20 and row[20] is not None else False,This extends the existing Test Infrastructure Change (global effect)
Setting Minor / NitRollback test re-raises server exception through TestClient ( with pytest.raises(_sqlite3.OperationalError):
client.post(...)Relies on FastAPI Positives worth calling out
Verdict: Approve with the note on the |
Code Review - feat(integrations): GitHub Issue Import + Task Traceability (#565)OverviewThis PR completes Phase 5.5 cleanly: import endpoint, auto-close dispatch, dedup via unique index, and traceability fields are all wired up end-to-end. Architecture is sound, test coverage is excellent, and design decisions are well-documented. A few items worth discussing before merge: Medium - Address Before Merge1. Sequential issue fetching
2. In tasks.autoclose_if_done(workspace, tasks.get(workspace, task_id))
updated = tasks.get(workspace, task_id)
if updated is not None:
tasks.autoclose_if_done(workspace, updated)Low - Worth Noting3. Dead-code path in Within the 404 probe block in if repo_resp.status_code >= 400:
raise GitHubConnectError(...)
if repo_resp.status_code >= 300: # <- can never fire
raise GitHubConnectError(...)The second 4. Import rollback propagates unhandled exception (intentional but implicit) The Positives
SummaryItems 1-2 are worth addressing before merge; 3-4 are low-priority polish. The core implementation is solid and test coverage is thorough. No blocking issues found. |
Update CLAUDE.md Current Focus: #565 (import execution + task traceability + auto-close) is implemented and merged-ready; document the endpoint behavior, URL-keyed dedup + unique index, core-level auto-close targeting the source repo, and the single-PAT known limitation.
Code Review — PR #609: GitHub Issue Import + Task Traceability (#565)Overall assessment: This is a well-executed, production-ready feature. Architecture is clean, edge cases are covered, and test coverage is excellent. A few observations below. Architecture / Design — Strengths
Issues1. Stale docstring in test file
2. Late opt-in path re-reads the task redundantly In tasks.autoclose_if_done(workspace, tasks.get(workspace, task_id))This triggers a second DB read immediately after 3. Sequential GitHub API fetches in import endpoint Issues are fetched one at a time in a 4.
Correctness Confirmations (things that look subtle but are right)
Nits
SummaryTwo items to address before merge:
Everything else is correct or a minor efficiency/style note. Test coverage is comprehensive and the architecture decisions are well-reasoned. |
1 participant
Summary
Implements #565 (Phase 5.5): execute the GitHub issue import wired up in #563/#564, and add task↔issue traceability.
POST /api/v2/integrations/github/importturns selected issues into CodeFRAME tasks — title verbatim, body as description (with a best-effort**Labels:**footer), linked viagithub_issue_number+external_url. The#564import modal's "Import Selected" now calls it, shows in-progress state, an in-modal error on failure, and a board summary banner (N created · M skipped) on success.tasks.update_status) so all completion paths are covered.UNIQUE(workspace_id, external_url)index + URL-keyed checks prevent duplicate imports (atomic across concurrent requests; repo-aware so the same issue number in a different repo is still importable).Acceptance Criteria
npm testanduv run pytestpassTest Plan
2445 passed; frontend955 passed+npm run buildcleanruff, eslint on changed files)Known Limitations / Intentionally Deferred
GIT_GITHUBcredential (the [Phase 5.1] Settings page: API key management #555/[Phase 5.5] GitHub Issues import: repository connection (PAT auth) #563 model). If a workspace is reconnected to a different repo whose token lacks access to an older imported repo, that older task's auto-close will fail (logged, best-effort). Per-repo credential storage is a broader change than [Phase 5.5] GitHub Issues import: execute import and task traceability #565.PATCH /issues/{n}/closeendpoint dropped. The issue listed it, but it was unused and (lacking task context) would target the wrong repo after a reconnect. Auto-close via the core dispatch — which uses each task's source repo fromexternal_url— fully covers the requirement.Implementation Notes
Adapted from the issue's (stale, ~2mo old) Traycer plan: schema lives in
core/workspace.py(not the removedpersistence/schema_manager.py); issue ops added tocore/github_issues_service.py(not the legacy PR class); the existinggithub_integrations_v2.pyrouter was extended rather than adding a new one; frontend isweb-ui/(notlegacy/web-ui/). Dedup reuses the existinggithub_issue_numbercolumn and keys on the full issue URL.Closes #565
Summary by CodeRabbit
Release Notes
New Features
Improvements