feat(issue-lifecycle): auto-assign issue to current user during start#65
feat(issue-lifecycle): auto-assign issue to current user during start#65
Conversation
Sync from systeminit/swamp#1167. When `start` is called, the method reads the authenticated username from local auth, resolves it to a userId via the eligible-assignees endpoint, and PATCHes the issue's assignees additively. All assignment failures are best-effort warnings. Bumps model version to 2026.04.12.1. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Adversarial Review
Critical / High
None.
Medium
-
Optimistic success logging after silent PATCH failure —
issue_lifecycle.ts:516-529After calling
sc.updateAssignees(...), the code unconditionally posts a lifecycle entry ("Assigned to {username}") and logs an info message. ButupdateAssigneesdelegates topatchIssue, which swallows all errors (HTTP 500, network timeout, etc.) and returnsvoidregardless of outcome.Breaking scenario: The PATCH endpoint returns 500 (server error).
patchIssuelogs internally and returns normally. The lifecycle audit trail then records "Assigned to alice" and the local logger prints "Auto-assigned issue to alice" — but the issue's actual assignee list was never modified.Impact: Misleading audit trail. During incident investigation, a team member could see "Assigned to alice" in the lifecycle log and assume alice was notified/responsible, when the assignment never took effect.
Suggested fix: Have
patchIssue(or a new variant) return a boolean indicating success. Only post the lifecycle entry and info log when the PATCH actually succeeded:const patched = await sc.tryUpdateAssignees([...existingIds, resolvedUserId]); if (patched) { await sc.postLifecycleEntry({ step: "assigned", ... }); context.logger.info("Auto-assigned issue to {username}", ...); }
Low
-
No structural try/catch around assignment block —
issue_lifecycle.ts:491-531The comment states "All failures are warnings — assignment must never break the triage flow." Each individual async method (
loadAuthFile,resolveUserId,updateAssignees,postLifecycleEntry) has its own try/catch. However, the block as a whole is unwrapped. If a future change introduces a throwing code path in this block,start()would throw after writing thestateresource (phase: "triaging"), leaving the model in an inconsistent state from the caller's perspective.In the current code, I verified every operation is defensively wrapped, so this is theoretical. A top-level try/catch around lines 491-531 with a warning log would structurally guarantee the stated contract at near-zero cost.
-
Redundant auth file read —
issue_lifecycle.ts:493loadAuthFile()is called instart()to get the username. WhenSWAMP_API_KEYenv var is not set,createSwampClubClient()(line 436) already calledloadAuthFile()internally to get the API key. This results in two filesystem reads of the same file. Not a bug — just a minor inefficiency. Could be addressed by threading the username through the client or caching the auth file result.
Verdict
PASS — The code is well-structured with good defensive patterns. The auto-assignment is genuinely best-effort, error handling is consistent with existing patterns in the codebase, and test coverage is thorough (happy path, idempotency, no-username, 403 on assignees, PATCH failure). The optimistic lifecycle logging (Medium #1) is worth fixing but doesn't block — the assignment is advisory, and the lifecycle log is informational rather than decision-driving.
There was a problem hiding this comment.
Code Review
Blocking Issues
None.
Suggestions
-
Fetch stub vs.
Deno.serve:buildStartTestContextreplacesglobalThis.fetchwith a stub instead of usingDeno.serve({ port: 0 })as recommended by CLAUDE.md. The global mutation is safe here (sequential tests, restored infinally), and a stub IS an in-memory mock client which the rules also allow — but aDeno.serve-based approach would be more future-proof if tests ever run concurrently across files. -
Double
loadAuthFilecall:createSwampClubClientalready callsloadAuthFileinternally to extract the API key;start()then calls it again to get the username. Two file reads per invocation. Could threadusernameout ofcreateSwampClubClientor accept it as an optional param, but given the file is tiny and the second call is isolated to the auto-assign block, this is a low-priority concern. -
Lifecycle entry posted before confirming PATCH success: When
patchIssuesilently eats a 500 error,postLifecycleEntrystill records "Assigned to ``" — the history entry can be inaccurate. Since everything here is best-effort this is acceptable, but worth noting for future observability work.
The core logic is correct: all error paths (missing credentials, no username, userId not in eligible list, already assigned, PATCH failure) are handled gracefully as warnings without breaking the triage flow. The six new test cases cover the happy path, idempotency, and every failure mode. No any types, named exports only, env vars restored in finally blocks throughout.
Summary
startis called on an issue-lifecycle model, auto-assigns the issue to the current authenticated user via swamp-club API2026.04.12.1Test Plan
🤖 Generated with Claude Code