fix(server): don't implicitly reopen a blocked issue when the same PATCH wires blockers (#10269)
## Thinking Path
> - Paperclip is the open source app people use to manage AI agents for
work
> - Issues coordinate that work, and first-class blockers
(`blockedByIssueIds`) are how dependent work auto-resumes when its
prerequisites finish
> - A human commenting on a blocked issue implicitly reopens it to
`todo` — a deliberate heuristic so "please continue" comments revive
parked work
> - But that heuristic evaluates the issue's *pre-update* blocker set,
ignoring blockers being wired in by the very same PATCH
> - So the natural repair action for a bare-blocked issue — one PATCH
adding `blockedByIssueIds` plus an explanatory comment — silently flips
the issue to `todo`, contradicting the dependency edit it just made
> - This pull request suppresses the implicit reopen when the request
itself declares a non-empty blocker list
> - The benefit is that structured dependency edits always win over the
conversational-comment heuristic, so blocked issues keep their intended
waiting posture and auto-resume via `issue_blockers_resolved` as
designed
## Linked Issues or Issue Description
No existing issue describes this exact behavior; per the bug-report
template:
- **What happened:** On a `blocked` issue with an empty blocker set, a
board user sent one `PATCH /api/issues/:id` containing
`blockedByIssueIds: ["<unresolved-issue-id>"]` and a `comment`. The
response showed `status: "todo"` — the implicit comment-reopen fired
even though the same request wired an unresolved blocker. A follow-up
`PATCH { status: "blocked" }` was then needed to restore the waiting
posture (and because the blocker array replaces on every update, the two
fields had to be re-sent together).
- **Expected behavior:** A request that explicitly declares dependencies
is stating that the issue is waiting on other work. The implicit reopen
exists for plain conversational comments; it should not override a
structured dependency edit made in the same request.
- **Steps to reproduce:** (1) Create issue A with `status: "blocked"`
and no blockers; (2) as a board user, `PATCH /api/issues/A` with `{
"blockedByIssueIds": ["<id of an open issue>"], "comment": "wiring the
dependency" }`; (3) observe the response/issue status is `todo` instead
of remaining `blocked`.
- **Version/commit:** reproduced on `master` @ `d1b9448b5`.
- **Deployment mode:** `authenticated`, single-host (macOS launchd),
embedded Postgres.
Related (not fixed here): the family of "blocked with empty
`blockedByIssueIds` zombie" reports — Refs #9201, Refs #6523 — this bug
is one way an issue's status and blocker list end up contradicting each
other; and Refs #8062, which proposes a different auto-transition at the
status/blocker boundary.
## What Changed
- `shouldImplicitlyMoveCommentedIssueToTodo`
(server/src/routes/issues.ts) accepts an optional
`requestAddsExplicitBlockers` input and returns `false` when set,
alongside the existing suppression guards, with a comment documenting
the rationale.
- The `PATCH /api/issues/:id` call site passes
`requestAddsExplicitBlockers: Array.isArray(req.body.blockedByIssueIds)
&& req.body.blockedByIssueIds.length > 0`.
- Two route tests in `issue-comment-reopen-routes.test.ts`: a regression
test (comment + non-empty blocker list on a blocked issue must not flip
status) and a boundary test (comment + `blockedByIssueIds: []` still
implicitly reopens, preserving the existing clear-blockers behavior).
Deliberately unchanged: explicit `reopen`/`resume` flags still behave as
before, and the `POST /comments` route is untouched (its body cannot
carry `blockedByIssueIds`).
## Verification
- `cd server && pnpm vitest run
src/__tests__/issue-comment-reopen-routes.test.ts` → 74/74 pass.
- Reverting the `issues.ts` change makes the new regression test fail
with `expected 'todo' to be undefined` — it bites.
- `cd server && pnpm tsc --noEmit` → clean.
## Risks
- Low. The change is a single additional suppression guard on the
*implicit* reopen path, scoped to requests that carry a non-empty
`blockedByIssueIds` array; all other reopen behavior is untouched.
- Edge case considered: a request wiring only already-resolved blockers
plus a comment now stays `blocked` instead of implicitly reopening. This
is the conservative reading of caller intent (an explicit dependency
edit), and an explicit `status`/`reopen` in the same request still wins.
## Model Used
- Anthropic Claude — Fable 5 (`claude-fable-5`), extended thinking
enabled, agentic tool use via Claude Code (CLI). Production repro,
diagnosis, fix, and tests all model-authored under human direction.
## Checklist
- [x] I have included a thinking path that traces from project context
to this change
- [x] I have specified the model used (with version and capability
details)
- [x] I have checked ROADMAP.md and confirmed this PR does not duplicate
planned core work
- [x] I have searched GitHub for duplicate or related PRs and linked
them above
- [x] I have either (a) linked existing issues with `Fixes: #` / `Closes
#` / `Refs #` OR (b) described the issue in-PR following the relevant
issue template
- [x] I have not referenced internal/instance-local Paperclip issues or
links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip`
URLs)
- [x] My branch name describes the change (e.g. `docs/...`, `fix/...`)
and contains no internal Paperclip ticket id or instance-derived details
- [x] I have run tests locally and they pass
- [x] I have added or updated tests where applicable
- [x] I have updated relevant documentation to reflect my changes (none
applicable — behavior comment added inline)
- [x] I have considered and documented any risks above
- [ ] All Paperclip CI gates are green (pending first CI run on this PR)
- [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups
(pending first review pass)
- [x] I will address all Greptile and reviewer comments before
requesting merge
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
parent
6d2eab742f
commit
c6727e7b20
|
|
@ -13,6 +13,7 @@ const mockIssueService = vi.hoisted(() => ({
|
|||
findMentionedAgents: vi.fn(),
|
||||
listWakeableBlockedDependents: vi.fn(),
|
||||
getWakeableParentAfterChildCompletion: vi.fn(),
|
||||
getRelationSummaries: vi.fn(),
|
||||
}));
|
||||
|
||||
const mockAccessService = vi.hoisted(() => ({
|
||||
|
|
@ -1240,6 +1241,62 @@ describe.sequential("issue comment reopen routes", () => {
|
|||
));
|
||||
});
|
||||
|
||||
it("does not implicitly reopen a blocked issue via PATCH when the same request wires blockers", async () => {
|
||||
mockIssueService.getById.mockResolvedValue(makeIssue("blocked"));
|
||||
mockIssueService.getRelationSummaries.mockResolvedValue({ blockedBy: [], blocks: [] });
|
||||
mockIssueService.getDependencyReadiness.mockResolvedValue({
|
||||
issueId: "11111111-1111-4111-8111-111111111111",
|
||||
blockerIssueIds: [],
|
||||
unresolvedBlockerIssueIds: [],
|
||||
unresolvedBlockerCount: 0,
|
||||
allBlockersDone: true,
|
||||
isDependencyReady: true,
|
||||
});
|
||||
mockIssueService.update.mockImplementation(async (_id: string, patch: Record<string, unknown>) => ({
|
||||
...makeIssue("blocked"),
|
||||
...patch,
|
||||
}));
|
||||
|
||||
const res = await request(await installActor(createApp()))
|
||||
.patch("/api/issues/11111111-1111-4111-8111-111111111111")
|
||||
.send({
|
||||
blockedByIssueIds: ["33333333-3333-4333-8333-333333333333"],
|
||||
comment: "wired the dependency this issue is waiting on",
|
||||
});
|
||||
|
||||
expect(res.status).toBe(200);
|
||||
expect(mockIssueService.update).toHaveBeenCalled();
|
||||
const patch = mockIssueService.update.mock.calls[0][1] as Record<string, unknown>;
|
||||
expect(patch.status).toBeUndefined();
|
||||
expect(patch.blockedByIssueIds).toEqual(["33333333-3333-4333-8333-333333333333"]);
|
||||
});
|
||||
|
||||
it("still implicitly reopens a blocked issue via PATCH when the same request clears blockers", async () => {
|
||||
mockIssueService.getById.mockResolvedValue(makeIssue("blocked"));
|
||||
mockIssueService.getRelationSummaries.mockResolvedValue({ blockedBy: [], blocks: [] });
|
||||
mockIssueService.getDependencyReadiness.mockResolvedValue({
|
||||
issueId: "11111111-1111-4111-8111-111111111111",
|
||||
blockerIssueIds: [],
|
||||
unresolvedBlockerIssueIds: [],
|
||||
unresolvedBlockerCount: 0,
|
||||
allBlockersDone: true,
|
||||
isDependencyReady: true,
|
||||
});
|
||||
mockIssueService.update.mockImplementation(async (_id: string, patch: Record<string, unknown>) => ({
|
||||
...makeIssue("blocked"),
|
||||
...patch,
|
||||
}));
|
||||
|
||||
const res = await request(await installActor(createApp()))
|
||||
.patch("/api/issues/11111111-1111-4111-8111-111111111111")
|
||||
.send({ blockedByIssueIds: [], comment: "nothing left to wait on, please continue" });
|
||||
|
||||
expect(res.status).toBe(200);
|
||||
expect(mockIssueService.update).toHaveBeenCalled();
|
||||
const patch = mockIssueService.update.mock.calls[0][1] as Record<string, unknown>;
|
||||
expect(patch.status).toBe("todo");
|
||||
});
|
||||
|
||||
it("does not implicitly reopen closed issues via POST comments when no agent is assigned", async () => {
|
||||
mockIssueService.getById.mockResolvedValue({
|
||||
...makeIssue("done"),
|
||||
|
|
|
|||
|
|
@ -1836,7 +1836,14 @@ function shouldImplicitlyMoveCommentedIssueToTodo(input: {
|
|||
actorRunId: string | null | undefined;
|
||||
checkoutRunId: string | null | undefined;
|
||||
executionRunId: string | null | undefined;
|
||||
requestAddsExplicitBlockers?: boolean;
|
||||
}) {
|
||||
// A request that wires a non-empty blockedByIssueIds list is declaring that
|
||||
// the issue is waiting on other work. The implicit reopen exists for plain
|
||||
// conversational comments ("please continue"), not structured dependency
|
||||
// edits — flipping to todo here would contradict the caller's stated intent
|
||||
// in the same request.
|
||||
if (input.requestAddsExplicitBlockers) return false;
|
||||
// Local-CLI agents post comments under user auth, so the actor.type is "user"
|
||||
// even though the comment originates from the same heartbeat run that owns
|
||||
// the issue lock. Without this guard, an agent that closes its own issue and
|
||||
|
|
@ -8709,6 +8716,8 @@ export function issueRoutes(
|
|||
actorRunId: actor.runId,
|
||||
checkoutRunId: existing.checkoutRunId,
|
||||
executionRunId: existing.executionRunId,
|
||||
requestAddsExplicitBlockers:
|
||||
Array.isArray(req.body.blockedByIssueIds) && req.body.blockedByIssueIds.length > 0,
|
||||
})) ||
|
||||
shouldResumeInProgressScheduledRetry);
|
||||
const updateReferenceSummaryBefore = titleOrDescriptionChanged
|
||||
|
|
|
|||
Loading…
Reference in New Issue