mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-11-11 05:11:49 +01:00
fix(issues): name the existing order in dependency-cycle denials (#20342)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Tasks can wait for other tasks through blocking dependencies, and the server rejects a dependency that closes a cycle > - The rejection said only "Blocking relations cannot contain cycles" > - In chat, an agent asked to "make Website wait for Design too" got this 422 while Design already waited for Website > - The agent had no sign that the existing order must stay, so it guessed a swap: it removed Design's dependency on Website and added the reverse > - This pull request makes the 422 name the existing order, say that nothing changed, and say to ask the requester which order they want > - The benefit is that agents keep the existing prerequisite and ask the person, instead of silently reversing it ## Linked Issues or Issue Description **What happened** A chat agent was asked to make Website also wait for Design. Design already waited for Website. The server correctly returned 422 "Blocking relations cannot contain cycles". The agent then removed the existing Design-waits-for-Website dependency and added Website-waits-for-Design. The person lost the existing prerequisite without being asked. **Expected behavior** The rejection keeps both saved dependency sets as they were. The agent explains the conflict with the real task names and asks the person which order they want before it changes any dependency. **Steps to reproduce** 1. Create tasks Website and Design. Make Design blocked by Website. 2. PATCH Website with `blockedByIssueIds: [Design]`. 3. See that the 422 says only "Blocking relations cannot contain cycles", with no task names and no next step. ## What Changed - `server/src/services/issues.ts`: the cycle check now records the path it finds. The 422 message names the existing chain by task identifier (for example "COM-2 waits for COM-1, so COM-1 cannot also wait for COM-2"). It never includes titles, because the chain can pass through tasks the caller cannot read. Identifiers are the same fields that locked relation stubs already show. It says no dependencies changed and the existing ones are kept. It says not to remove or reverse an existing dependency, and to ask the requester which order they want. - The 422 `details` now carry `code: "blocking_cycle"`, the updated issue, the requested blocker, and the existing chain as ids and identifiers. - `server/src/__tests__/issues-service.test.ts`: a regression test checks the message and details for a direct cycle, the chain text for a two-step cycle through a task with a private title, that no title appears in the error, and that the existing dependency is unchanged. ## Verification - `cd server && pnpm exec vitest run src/__tests__/issues-service.test.ts` passes (152 tests). The new test fails on the old message. - Chat eval `task-coordination.reject-dependency-cycle` ran end to end with the first commit (run 42678089404, which named tasks by identifier and title). The agent kept Design waiting for Website and asked the person which order to keep with a question card. The saved dependency sets did not change. The second commit removes titles from the message; the rest of the text is the same. ## Risks - Low risk. The status code (422) and the first words of the message do not change. Clients that match the full old message exactly would no longer match. No code in this repository does that. - The check reads the identifiers of the issues in the chain only when it rejects a request, so the success path is unchanged. - The error can show the identifier of a task that the caller cannot read. Relation responses already show this identifier in locked stubs, so this shows no new data. ## Model Used - Anthropic Claude Opus 5.5 (`claude-opus-5-5`), tool use and code execution in Claude Code. ## 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 - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
DottaandClaude Opus 5.5 authored and GitHub committed
1 parent
251c6d5c69
commit
0f71ab796e
2 files changed
+104
-6
No files matched your search
@@ -4514,6 +4514,55 @@ const dependencyTestDatabaseUrl = resolveExternalTestDatabaseUrl("PAPERCLIP_ISSU
|
||||
).rejects.toMatchObject({ status: 422 });
|
||||
});
|
||||
|
||||
it("names the existing order in a blocking-cycle denial and keeps it", async () => {
|
||||
const companyId = randomUUID();
|
||||
await seedIssueCompany(db, companyId);
|
||||
|
||||
const website = randomUUID();
|
||||
const design = randomUUID();
|
||||
const copy = randomUUID();
|
||||
await db.insert(issues).values([
|
||||
{ id: website, companyId, identifier: "COM-1", title: "Website", status: "backlog", priority: "medium" },
|
||||
{ id: design, companyId, identifier: "COM-2", title: "Design", status: "backlog", priority: "medium" },
|
||||
// The intermediate issue's title must not appear: the caller may not be able to read it.
|
||||
{ id: copy, companyId, identifier: "COM-3", title: "Private launch copy", status: "backlog", priority: "medium" },
|
||||
]);
|
||||
await svc.update(design, { blockedByIssueIds: [website] });
|
||||
|
||||
const direct = await svc
|
||||
.update(website, { blockedByIssueIds: [design] })
|
||||
.catch((error: unknown) => error);
|
||||
expect(direct).toMatchObject({
|
||||
status: 422,
|
||||
message:
|
||||
"Blocking relations cannot contain cycles: COM-2 waits for COM-1, so COM-1 cannot also wait for COM-2. " +
|
||||
"No dependencies were changed and the existing ones are kept. " +
|
||||
"Do not remove or reverse an existing dependency to make this fit; ask whoever requested the change which order they want and change dependencies only after they choose.",
|
||||
details: {
|
||||
code: "blocking_cycle",
|
||||
issueId: website,
|
||||
requestedBlockerIssueId: design,
|
||||
existingChain: [
|
||||
{ id: website, identifier: "COM-1" },
|
||||
{ id: design, identifier: "COM-2" },
|
||||
],
|
||||
},
|
||||
});
|
||||
|
||||
await svc.update(copy, { blockedByIssueIds: [design] });
|
||||
const transitive = await svc
|
||||
.update(website, { blockedByIssueIds: [copy] })
|
||||
.catch((error: unknown) => error);
|
||||
expect((transitive as Error).message).toContain(
|
||||
"COM-2 waits for COM-1, and COM-3 waits for COM-2, so COM-1 cannot also wait for COM-3.",
|
||||
);
|
||||
expect(JSON.stringify({ message: (transitive as Error).message, details: (transitive as { details?: unknown }).details }))
|
||||
.not.toMatch(/Website|Design|Private launch copy/);
|
||||
|
||||
expect((await svc.getRelationSummaries(design)).blockedBy.map((relation) => relation.id)).toEqual([website]);
|
||||
expect((await svc.getRelationSummaries(website)).blockedBy).toEqual([]);
|
||||
});
|
||||
|
||||
it("only returns dependents once every blocker is done", async () => {
|
||||
const companyId = randomUUID();
|
||||
const assigneeAgentId = randomUUID();
|
||||
|
||||
@@ -7643,20 +7643,69 @@ export function issueService(db: Db) {
|
||||
}
|
||||
|
||||
for (const blockerIssueId of blockerIssueIds) {
|
||||
const queue = [...(adjacency.get(issueId) ?? [])];
|
||||
// Breadth-first search from the issue being updated along existing
|
||||
// "blocks" edges, remembering how each issue was reached so the
|
||||
// denial can name the existing chain that the new edge would close.
|
||||
const previous = new Map<string, string>();
|
||||
const queue = [issueId];
|
||||
const visited = new Set<string>([issueId]);
|
||||
while (queue.length > 0) {
|
||||
const current = queue.shift()!;
|
||||
if (current === blockerIssueId) {
|
||||
throw unprocessable("Blocking relations cannot contain cycles");
|
||||
for (const next of adjacency.get(current) ?? []) {
|
||||
if (visited.has(next)) continue;
|
||||
visited.add(next);
|
||||
previous.set(next, current);
|
||||
if (next === blockerIssueId) {
|
||||
const chain = [next];
|
||||
while (chain[0] !== issueId) chain.unshift(previous.get(chain[0])!);
|
||||
throw await blockingCycleError(companyId, chain, dbOrTx);
|
||||
}
|
||||
queue.push(next);
|
||||
}
|
||||
if (visited.has(current)) continue;
|
||||
visited.add(current);
|
||||
queue.push(...(adjacency.get(current) ?? []));
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Builds the 422 for a dependency that would close a blocking cycle.
|
||||
* `chain` runs from the issue being updated to the requested blocker along
|
||||
* existing "blocks" edges. The message names the existing order, says that
|
||||
* nothing changed, and says the existing order must not be dropped or
|
||||
* reversed without the requester's choice: a bare "cycle" denial left
|
||||
* agents to guess, and they swapped the existing dependency.
|
||||
*
|
||||
* The chain can pass through issues the caller cannot read, and this
|
||||
* service has no request to decide read access. It therefore names issues
|
||||
* by identifier only, the same fields a locked relation stub exposes, and
|
||||
* never by title.
|
||||
*/
|
||||
async function blockingCycleError(companyId: string, chain: string[], dbOrTx: DbReader) {
|
||||
const rows = await dbOrTx
|
||||
.select({ id: issues.id, identifier: issues.identifier })
|
||||
.from(issues)
|
||||
.where(and(eq(issues.companyId, companyId), inArray(issues.id, chain)));
|
||||
const identifierById = new Map(rows.map((row) => [row.id, row.identifier]));
|
||||
const label = (id: string) => identifierById.get(id) ?? id;
|
||||
const issueLabel = label(chain[0]!);
|
||||
const blockerLabel = label(chain[chain.length - 1]!);
|
||||
// Each issue in the chain already waits for the one before it.
|
||||
const existingOrder = chain
|
||||
.slice(1)
|
||||
.map((id, index) => `${label(id)} waits for ${label(chain[index]!)}`)
|
||||
.join(", and ");
|
||||
return unprocessable(
|
||||
`Blocking relations cannot contain cycles: ${existingOrder}, so ${issueLabel} cannot also wait for ${blockerLabel}. ` +
|
||||
"No dependencies were changed and the existing ones are kept. " +
|
||||
"Do not remove or reverse an existing dependency to make this fit; ask whoever requested the change which order they want and change dependencies only after they choose.",
|
||||
{
|
||||
code: "blocking_cycle",
|
||||
issueId: chain[0],
|
||||
requestedBlockerIssueId: chain[chain.length - 1],
|
||||
existingChain: chain.map((id) => ({ id, identifier: identifierById.get(id) ?? null })),
|
||||
},
|
||||
);
|
||||
}
|
||||
|
||||
async function syncBlockedByIssueIds(
|
||||
issueId: string,
|
||||
companyId: string,
|
||||
|
||||
Reference in new issue
Block a user