Repository navigation
fix(tasks): single-line titles and single-token assignees on write (TASK-354) #2483
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
0a92164
fix(tasks): single-line titles and single-token assignees on write (T…
lilyshen0722 4c64984
fix(tasks): one rule for title and assignee on every write path (TASK…
lilyshen0722 b146189
fix(native): commonly_create_task schema asks for a handle (TASK-354)
lilyshen0722 4f4f298
test(tasks): mock the findOne select the PATCH revision path reads (T…
lilyshen0722 9f38aae
fix(tasks): reject non-string PATCH title; replay finds raw multiline…
lilyshen0722 7cd8270
fix(native): advertise agentName only for commonly_create_task assign…
lilyshen0722 bce8244
fix(tasks): try the exact raw title before the normalized one on repl…
lilyshen0722 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
176 changes: 176 additions & 0 deletions
176
backend/__tests__/unit/routes/tasksApi.titleAssigneeNormalization.test.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,176 @@ | ||
| /** | ||
| * Title and assignee are single-line on every write path (TASK-354). | ||
| * | ||
| * A newline in a title breaks each single-line surface that renders it, and an | ||
| * assignee with an inner space or control character names a different actor to | ||
| * claims and the wake, which match it verbatim. Case is left alone on purpose: | ||
| * the name is the identity, and lowering it would merge two real names. | ||
| */ | ||
| const request = require('supertest'); | ||
| const express = require('express'); | ||
|
|
||
| jest.mock('../../../middleware/auth', () => (req, res, next) => { | ||
| req.userId = 'u1'; | ||
| req.user = { id: 'u1', _id: 'u1' }; | ||
| next(); | ||
| }); | ||
| jest.mock('../../../middleware/agentRuntimeAuth', () => (req, res, next) => next()); | ||
|
|
||
| jest.mock('../../../models/Pod', () => ({ | ||
| findById: jest.fn(() => ({ | ||
| lean: jest.fn().mockResolvedValue({ type: 'chat', members: [{ toString: () => 'u1' }] }), | ||
| })), | ||
| })); | ||
|
|
||
| const mockCreate = jest.fn(); | ||
| const mockFindOneAndUpdate = jest.fn(); | ||
| jest.mock('../../../models/Task', () => ({ | ||
| find: jest.fn(() => ({ sort: () => ({ lean: async () => [] }) })), | ||
| findOne: jest.fn(() => ({ | ||
| sort: () => ({ lean: async () => null, select: () => ({ lean: async () => null }) }), | ||
| select: () => ({ lean: async () => ({ updatedAt: new Date(0), assignee: null, claimedBy: null }) }), | ||
| lean: async () => null, | ||
| })), | ||
| findOneAndUpdate: (...args) => mockFindOneAndUpdate(...args), | ||
| countDocuments: jest.fn().mockResolvedValue(0), | ||
| create: (...args) => mockCreate(...args), | ||
| })); | ||
|
|
||
| jest.mock('../../../models/User', () => ({ | ||
| findById: jest.fn(() => ({ | ||
| select: jest.fn(() => ({ lean: jest.fn().mockResolvedValue({ username: 'alice' }) })), | ||
| })), | ||
| })); | ||
|
|
||
| const tasksApiRoutes = require('../../../routes/tasksApi'); | ||
|
|
||
| const POD = '69b7ddff0ce64c9648365fc4'; | ||
| const TASK = '69b7ddff0ce64c9648365fc5'; | ||
|
|
||
| describe('task title and assignee normalization on write (TASK-354)', () => { | ||
| let app; | ||
|
|
||
| beforeEach(() => { | ||
| jest.clearAllMocks(); | ||
| mockCreate.mockImplementation(async (doc) => ({ | ||
| ...doc, | ||
| toObject() { return doc; }, | ||
| })); | ||
| mockFindOneAndUpdate.mockImplementation(async (_filter, update) => ({ | ||
| ...update.$set, | ||
| toObject() { return update.$set; }, | ||
| })); | ||
| app = express(); | ||
| app.use(express.json()); | ||
| app.use('/api/v1/tasks', tasksApiRoutes); | ||
| }); | ||
|
|
||
| describe('POST create', () => { | ||
| test('a newline in the title collapses to one line before it is stored', async () => { | ||
| const res = await request(app) | ||
| .post(`/api/v1/tasks/${POD}`) | ||
| .send({ title: ' Fix the\r\n board lease\nrow ' }); | ||
| expect(res.status).toBe(201); | ||
| expect(mockCreate.mock.calls[0][0].title).toBe('Fix the board lease row'); | ||
| }); | ||
|
|
||
| test('a title that is only a newline is refused as empty', async () => { | ||
| const res = await request(app).post(`/api/v1/tasks/${POD}`).send({ title: '\n' }); | ||
| expect(res.status).toBe(400); | ||
| expect(res.body.error).toBe('title is required'); | ||
| expect(mockCreate).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| test('the assignee is trimmed and its case is kept', async () => { | ||
| const res = await request(app) | ||
| .post(`/api/v1/tasks/${POD}`) | ||
| .send({ title: 'Ship it', assignee: ' Nova ' }); | ||
| expect(res.status).toBe(201); | ||
| expect(mockCreate.mock.calls[0][0].assignee).toBe('Nova'); | ||
| }); | ||
|
|
||
| test('an assignee with an inner space is refused before anything is stored', async () => { | ||
| const res = await request(app) | ||
| .post(`/api/v1/tasks/${POD}`) | ||
| .send({ title: 'Ship it', assignee: 'nova ops' }); | ||
| expect(res.status).toBe(400); | ||
| expect(res.body.error).toMatch(/assign by handle/); | ||
| expect(mockCreate).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| test('an assignee with a control character is refused', async () => { | ||
| const res = await request(app) | ||
| .post(`/api/v1/tasks/${POD}`) | ||
| .send({ title: 'Ship it', assignee: 'nova\u0007' }); | ||
| expect(res.status).toBe(400); | ||
| expect(mockCreate).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| test('a control character from the C1 range is refused too', async () => { | ||
| const res = await request(app) | ||
| .post(`/api/v1/tasks/${POD}`) | ||
| .send({ title: 'Ship it', assignee: 'nova\u0085' }); | ||
| expect(res.status).toBe(400); | ||
| expect(mockCreate).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| test('a replay looks up the exact raw multiline title before the normalized one', async () => { | ||
| const Task = require('../../../models/Task'); | ||
| await request(app) | ||
| .post(`/api/v1/tasks/${POD}`) | ||
| .send({ title: 'Fix\nboard', sourceRef: 'msg:1' }); | ||
| expect(Task.findOne.mock.calls[0][0].title).toBe('Fix\nboard'); | ||
| }); | ||
|
|
||
| test('a blank assignee stores as unassigned', async () => { | ||
| const res = await request(app) | ||
| .post(`/api/v1/tasks/${POD}`) | ||
| .send({ title: 'Ship it', assignee: ' ' }); | ||
| expect(res.status).toBe(201); | ||
| expect(mockCreate.mock.calls[0][0].assignee).toBeNull(); | ||
| }); | ||
| }); | ||
|
|
||
| describe('PATCH update', () => { | ||
| test('a newline in the title collapses to one line before the write', async () => { | ||
| const res = await request(app) | ||
| .patch(`/api/v1/tasks/${POD}/${TASK}`) | ||
| .send({ title: 'Renamed\nin two lines' }); | ||
| expect(res.status).toBe(200); | ||
| expect(mockFindOneAndUpdate.mock.calls[0][1].$set.title).toBe('Renamed in two lines'); | ||
| }); | ||
|
|
||
| test('a non-string title is refused before the write, so null never reaches Mongo', async () => { | ||
| const res = await request(app) | ||
| .patch(`/api/v1/tasks/${POD}/${TASK}`) | ||
| .send({ title: null }); | ||
| expect(res.status).toBe(400); | ||
| expect(mockFindOneAndUpdate).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| test('an assignee with an inner space is refused before the write', async () => { | ||
| const res = await request(app) | ||
| .patch(`/api/v1/tasks/${POD}/${TASK}`) | ||
| .send({ assignee: 'sprint impl' }); | ||
| expect(res.status).toBe(400); | ||
| expect(res.body.error).toMatch(/assign by handle/); | ||
| expect(mockFindOneAndUpdate).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| test('a padded assignee is trimmed on the write', async () => { | ||
| const res = await request(app) | ||
| .patch(`/api/v1/tasks/${POD}/${TASK}`) | ||
| .send({ assignee: ' Lily ' }); | ||
| expect(res.status).toBe(200); | ||
| expect(mockFindOneAndUpdate.mock.calls[0][1].$set.assignee).toBe('Lily'); | ||
| }); | ||
|
|
||
| test('a title that is only whitespace is refused rather than stored blank', async () => { | ||
| const res = await request(app) | ||
| .patch(`/api/v1/tasks/${POD}/${TASK}`) | ||
| .send({ title: ' \n ' }); | ||
| expect(res.status).toBe(400); | ||
| expect(mockFindOneAndUpdate).not.toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
| }); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| /** | ||
| * Task title and assignee rules, shared by every path that writes a task (the | ||
| * tasks API for POST and PATCH, and the native runtime's create tool). | ||
| * | ||
| * A title is one line: any run of whitespace, newlines included, collapses to a | ||
| * single space, so every single-line surface that shows it stays one line. | ||
| */ | ||
| export const toSingleLineTitle = (raw: string): string => raw.replace(/\s+/g, ' ').trim(); | ||
|
|
||
| /** C0, DEL and C1 control characters. */ | ||
| const isControlCharacter = (ch: string): boolean => { | ||
| const code = ch.charCodeAt(0); | ||
| return code < 0x20 || (code >= 0x7f && code <= 0x9f); | ||
| }; | ||
|
|
||
| /** | ||
| * An assignee is one handle (an agentName or instanceId). A board wake matches | ||
| * that handle verbatim, so an inner space or control character names someone | ||
| * else, and a display name stores a row no seat is woken for. Trimmed; case is | ||
| * kept. Blank after trimming means unassigned. | ||
| */ | ||
| export const parseAssignee = (raw: string): { value: string | null; error?: string } => { | ||
| const value = raw.trim(); | ||
| if (value === '') return { value: null }; | ||
| if (/\s/.test(value) || [...value].some(isControlCharacter)) { | ||
| return { | ||
| value: null, | ||
| error: 'assign by handle: one word with no spaces or control characters, e.g. nova', | ||
| }; | ||
| } | ||
| return { value }; | ||
| }; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.