3 Commits

Include renames

Author	SHA1	Message	Date
DottaandPaperclip	946735d03a	Persist task sorting in URLs and show newest routine tasks first ... Save task sort choices in URLs across task views. Open routine tasks by creation time, newest first, with stable sorting before pagination. Co-Authored-By: Paperclip <Paperclip-Paperclip@users.noreply.github.com>	2026-11-02 13:32:40 -06:00
3 people	2ebc236b08	fix(server): accept parentIssueId alias in GET /issues (#4032) ... ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - Agents coordinate through the server API. They find sub-tasks by filtering the company issues list by parent. > - `GET /api/companies/:companyId/issues` accepts `?parentId=`. Many callers send `?parentIssueId=` instead, which the handler never read. > - The mismatch is silent. The filter is dropped and the full company list comes back, so agents fetch everything and filter client-side. Issue #3846 reports this. > - `parentIssueId` is not an arbitrary spelling. It is the field name the wakeup payloads in this same route file already use, so callers expect it. > - This pull request accepts `parentIssueId` as an alias for `parentId` at the route boundary, on both the issues list and `issues/count`. > - The benefit is that parent filtering works for both spellings, and the list and its count cannot disagree. ## Linked Issues or Issue Description Fixes #3846 Related: #3870 proposes the same alias for the list route. ## What Changed - `server/src/routes/issues.ts`: `listFilters.parentId` in `GET /companies/:companyId/issues` now reads `req.query.parentId ?? req.query.parentIssueId`. - `server/src/routes/issues.ts`: `blockedCountFilters.parentId` in `GET /companies/:companyId/issues/count` reads the same alias, so the list and its count agree. - `server/src/__tests__/issues-parent-id-alias.test.ts`: new regression test for alias resolution, precedence, and absence. ## Verification - Run `pnpm run test:run -- server/src/__tests__/issues-parent-id-alias.test.ts`. - The test covers four query shapes: `?parentId=`, `?parentIssueId=`, both present (short form wins), and neither present (filter unset). - Existing callers are unaffected. The UI client `ui/src/api/issues.ts` only sets `parentId`. Nullish coalescing falls back only when the primary key is absent. - The service layer applies the filter with `if (filters?.parentId)` in `server/src/services/issues.ts`. This pull request does not change it. ## Risks - Low risk. The change only widens accepted query input. Both spellings resolve, and the short form still wins. - `?parentId=` with an empty value stays falsy and unfiltered, exactly as before. - This route has no validation middleware, and these list filters are not in the published OpenAPI surface. No contract needs an update. ## Model Used - Claude Opus 5 (`claude-opus-5`), extended thinking with tool use, run by the maintainer's triage agent. It rebased the original commit onto current `master`, extended the alias to `issues/count`, and wrote the regression test. @scokeepa authored the original one-line route change. ## 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 and contains no internal Paperclip ticket id or instance-derived details - [ ] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [ ] 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 --------- Co-authored-by: josangmun <cmeia.ai02@cmeia.co.kr> Co-authored-by: Andrew Aymeloglu <aaymeloglu@gmail.com>	2026-08-04 20:45:42 -05:00
LempkeyandDevin Foley	e1e2cef928	fix(issues): accept array-form ?status= filter and stop crashing on repeated keys (#4628) (#4890) ... ## Thinking Path > - Paperclip orchestrates AI agents for zero-human companies > - Boards, agents, and the public REST API all read issue lists via `GET /api/companies/:cid/issues`, with `?status=` as the most-common filter > - Express's default `qs` parser binds repeated keys to a `string[]` — the conventional URL form `?status=todo&status=in_progress` is therefore valid input > - The service layer treated `filters.status` as a string and called `.split(",")` unconditionally, returning HTTP 500 with `TypeError: filters.status.split is not a function`. The same buggy pattern lived at a second call site in the same file > - This PR adds a small `parseStatusFilter` helper that normalizes all four shapes the route can receive, routes both service-layer call sites through it, and widens the `IssueFilters.status` type so the contract stops lying about runtime reality > - The benefit is a passive 500 disappears for any client (curl, board, agent code) that builds `?status=` with array-style binding, and the type system now forces every future caller to handle both shapes correctly ## Linked Issues or Issue Description - Refs #4628 - Closes #4084 - Related earlier attempt: #1964 ## What Changed - **`server/src/services/issues.ts`** — Added exported helper `parseStatusFilter(input: string | readonly string[] | undefined): string[]` that normalizes single strings, CSV (`?status=todo,in_progress`), array (`?status=todo&status=in_progress`), and mixed array+CSV; trims and filters empties. Widened `IssueFilters.status` from `string` to `string | readonly string[]`. Replaced inline `.split` call sites in `list()`, blocked-count filtering, `count()`, and `countUnreadTouchedByUser()` with helper-driven branching. - **`server/src/routes/issues.ts`** — Replaced dishonest `req.query.status` casts with `string | string[] | undefined` at both issue-list and blocked-count entry points so the route contract matches Express `qs` runtime behavior. - **`server/src/__tests__/parse-status-filter.test.ts`** (new) — 10 unit cases: undefined, empty string, single, CSV, array, mixed array+CSV, whitespace trim, trailing/extra commas, no-mutation guarantee, hostile non-string entry guard. - **`server/src/__tests__/issues-list-query-parsing.test.ts`** (new) — 5 supertest cases against a minimal Express app whose handler mirrors the route cast/forwarding pattern: single, CSV, repeated-key array, mixed array+CSV, and no `?status` param. - **`server/src/__tests__/issues-service.test.ts`** — Added embedded-Postgres service coverage for array-form status filters through `list`, `count`, and `countUnreadTouchedByUser` on current master. **Why service-layer, not route-layer:** the bug is the service contract. Fixing only at the route would leave other service-layer call sites latent, keep `IssueFilters.status` inaccurate, and let future internal callers reintroduce the same crash. Widening the type is the forcing function that prevents recurrence. **Why `parseStatusFilter` is exported, not file-local:** the helper has direct unit coverage and keeps the normalization logic colocated with its only current call sites. ## Coordination with prior work - Supersedes **#4084** (thanks to @adlai88 for the original report-and-fix). This PR additionally fixes the extra current-master service call sites, widens `IssueFilters.status` so the type contract is honest, replaces the incorrect route casts, and ships direct regression coverage. - **#1964** bundles unrelated route/service changes; this PR keeps scope tight per CONTRIBUTING.md's one-PR-one-change guidance. ## Out-of-scope finding While verifying all query-string status parsing sites, I found a sibling bug in `server/src/services/execution-workspaces.ts:409` reachable from `routes/execution-workspaces.ts:48`, where repeated `?status=` keys can still hit the same `.split(",")` assumption. I left that out of this PR to keep the review surface small. ## Verification ```bash pnpm --filter @paperclipai/server exec vitest run \ src/__tests__/parse-status-filter.test.ts \ src/__tests__/issues-list-query-parsing.test.ts \ src/__tests__/issues-service.test.ts \ --testNamePattern='parseStatusFilter|issue list status query parsing|accepts array-form status filters in list and count|excludes plugin operation issues from unread inbox counts' ``` Result on the rebased head: `3` files passed, `17` tests passed, `72` skipped. GitHub CI on PR `#4890` is green on the rebased head `805731d3270783d0b80b33ee1dccdc6771febef6`, including `verify`, `Typecheck + Release Registry`, `Build`, `e2e`, general tests, serialized suites, Socket checks, Snyk, and `Greptile Review`. Local workspace typecheck commands still encounter unrelated current-master baseline errors under `packages/plugins/sdk` and `server/src/services/company-skills.ts`; no failures were produced from the `issues` files changed in this PR. ## Risks - **Type widening blast radius:** `IssueFilters.status` widens from `string` to `string | readonly string[]`. Any direct caller that still assumes `.split()` on the input now gets a useful typecheck failure instead of a latent runtime crash. - **Behavior change:** `?status=todo,in_progress&status=done` previously returned HTTP 500; it now returns HTTP 200 with the union of matching statuses. Single-string and CSV behavior remain unchanged. - **No migration. No breaking API changes. No new deps. No UI changes.** > For core feature work, check [`ROADMAP.md`](ROADMAP.md) first and discuss it in `#dev` before opening the PR. — Confirmed: this is a bug fix, not a feature. ROADMAP.md grep showed no overlap. ## Model Used - **Claude Opus 4.7** (Anthropic), `claude-opus-4-7`, 1M context, extended thinking. Used for problem scoping, implementation, and test authoring; the final rebasing, PR prep, and verification updates were handled in the maintainer workflow. ## 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 run tests locally and they pass - [x] I have added or updated tests where applicable - [x] If this change affects the UI, I have included before/after screenshots - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge ## Cross-references and status (maintainer) - Closes #4084 --------- Co-authored-by: Devin Foley <devin@paperclip.ing>	2026-06-11 21:52:43 -07:00