Skip to content

feat: VER-496 add goal management controls #33

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.

Sign up for GitHub

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
mgunnin merged 1 commit into from
Jul 31, 2026
Merged

feat: VER-496 add goal management controls #33

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
178 changes: 178 additions & 0 deletions server/src/__tests__/goals.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,178 @@
import { beforeEach, describe, expect, it } from 'vitest';
import request from 'supertest';
import { randomUUID } from 'node:crypto';
import { createTestApp, createTestDb } from '../test-utils.js';

describe('Goals API', () => {
let app: ReturnType<typeof createTestApp>;
let companyId: string;
let ownerAgentId: string;

beforeEach(async () => {
const db = await createTestDb();
app = createTestApp(db);

const company = await request(app)
.post('/api/companies')
.send({ name: 'Goal Test Corp' })
.expect(201);
companyId = company.body.data.id;

const owner = await request(app)
.post(`/api/companies/${companyId}/agents`)
.send({ name: 'Goal Owner', role: 'ceo' })
.expect(201);
ownerAgentId = owner.body.data.id;
});

it('creates nested goals and persists operator updates', async () => {
const root = await request(app)
.post(`/api/companies/${companyId}/goals`)
.send({
title: 'Ship the operator workflow',
description: 'Make goal management durable.',
level: 'company',
status: 'active',
ownerAgentId,
progress: 20,
})
.expect(201);

const child = await request(app)
.post(`/api/companies/${companyId}/goals`)
.send({
title: 'Verify progress updates',
level: 'team',
status: 'draft',
parentId: root.body.data.id,
ownerAgentId,
progress: 0,
})
.expect(201);

const updated = await request(app)
.patch(`/api/companies/${companyId}/goals/${child.body.data.id}`)
.send({ title: 'Verify durable progress updates', status: 'active', progress: 65 })
.expect(200);

expect(updated.body.data).toEqual(expect.objectContaining({
parentId: root.body.data.id,
ownerAgentId,
status: 'active',
progress: 65,
}));

const list = await request(app)
.get(`/api/companies/${companyId}/goals`)
.expect(200);
expect(list.body.data).toEqual(expect.arrayContaining([
expect.objectContaining({ id: root.body.data.id, parentId: null }),
expect.objectContaining({ id: child.body.data.id, progress: 65 }),
]));
});

it('rejects parents outside the company', async () => {
await request(app)
.post(`/api/companies/${companyId}/goals`)
.send({ title: 'Invalid parent', parentId: randomUUID() })
.expect(400)
.expect(({ body }) => {
expect(body.message).toBe('Choose a parent goal from this company.');
});
});

it('rejects self-parent and descendant cycles', async () => {
const root = await request(app)
.post(`/api/companies/${companyId}/goals`)
.send({ title: 'Root goal' })
.expect(201);
const child = await request(app)
.post(`/api/companies/${companyId}/goals`)
.send({ title: 'Child goal', level: 'department', parentId: root.body.data.id })
.expect(201);

await request(app)
.patch(`/api/companies/${companyId}/goals/${root.body.data.id}`)
.send({ parentId: root.body.data.id })
.expect(400);

await request(app)
.patch(`/api/companies/${companyId}/goals/${root.body.data.id}`)
.send({ parentId: child.body.data.id })
.expect(400)
.expect(({ body }) => {
expect(body.message).toContain('descendants');
});
});

it('rejects owners outside the company', async () => {
const otherCompany = await request(app)
.post('/api/companies')
.send({ name: 'Other Goal Corp' })
.expect(201);
const otherOwner = await request(app)
.post(`/api/companies/${otherCompany.body.data.id}/agents`)
.send({ name: 'Other Owner', role: 'ceo' })
.expect(201);

await request(app)
.post(`/api/companies/${companyId}/goals`)
.send({ title: 'Wrong owner', ownerAgentId: otherOwner.body.data.id })
.expect(400)
.expect(({ body }) => {
expect(body.message).toBe('Choose an owner from this company.');
});
});

it('rejects blank titles on create and update', async () => {
await request(app)
.post(`/api/companies/${companyId}/goals`)
.send({ title: ' ' })
.expect(400);

const goal = await request(app)
.post(`/api/companies/${companyId}/goals`)
.send({ title: 'Valid goal' })
.expect(201);

await request(app)
.patch(`/api/companies/${companyId}/goals/${goal.body.data.id}`)
.send({ title: ' ' })
.expect(400);
});

it('enforces levels from parent to child while allowing skipped levels', async () => {
const root = await request(app)
.post(`/api/companies/${companyId}/goals`)
.send({ title: 'Company goal', level: 'company' })
.expect(201);

await request(app)
.post(`/api/companies/${companyId}/goals`)
.send({ title: 'Invalid peer', level: 'company', parentId: root.body.data.id })
.expect(400);

const team = await request(app)
.post(`/api/companies/${companyId}/goals`)
.send({ title: 'Skipped-level team goal', level: 'team', parentId: root.body.data.id })
.expect(201);

const individual = await request(app)
.post(`/api/companies/${companyId}/goals`)
.send({ title: 'Individual goal', level: 'individual', parentId: team.body.data.id })
.expect(201);

await request(app)
.post(`/api/companies/${companyId}/goals`)
.send({ title: 'Too deep', level: 'individual', parentId: individual.body.data.id })
.expect(400);

await request(app)
.patch(`/api/companies/${companyId}/goals/${team.body.data.id}`)
.send({ level: 'individual' })
.expect(400)
.expect(({ body }) => {
expect(body.message).toBe('A parent goal must use a level above each child.');
});
});
});
120 changes: 116 additions & 4 deletions server/src/routes/goals.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import type { DbInstance } from '../types.js';
import { routeParams } from '../utils/route-params.js';

const CreateGoalBody = z.object({
title: z.string().min(1).max(500),
title: z.string().trim().min(1).max(500),
description: z.string().max(5000).optional(),
level: z.enum(['company', 'department', 'team', 'individual']).default('company'),
status: z.enum(['draft', 'active', 'completed', 'cancelled']).default('draft'),
Expand All @@ -20,7 +20,7 @@ const CreateGoalBody = z.object({
});

const UpdateGoalBody = z.object({
title: z.string().min(1).max(500).optional(),
title: z.string().trim().min(1).max(500).optional(),
description: z.string().max(5000).nullable().optional(),
level: z.enum(['company', 'department', 'team', 'individual']).optional(),
status: z.enum(['draft', 'active', 'completed', 'cancelled']).optional(),
Expand All @@ -31,9 +31,103 @@ const UpdateGoalBody = z.object({
metrics: z.record(z.unknown()).optional(),
});

type GoalLevel = z.infer<typeof CreateGoalBody>['level'];

const GOAL_LEVEL_RANK: Record<GoalLevel, number> = {
company: 0,
department: 1,
team: 2,
individual: 3,
};

export function goalsRouter(db: DbInstance): Router {
const router = Router({ mergeParams: true });
const { goals } = db.schema;
const { agents, goals } = db.schema;

async function validateGoalReferences({
companyId,
goalId,
ownerAgentId,
parentId,
level,
}: {
companyId: string;
goalId?: string;
ownerAgentId?: string | null;
parentId?: string | null;
level?: GoalLevel;
}) {
if (parentId !== undefined || (goalId && level !== undefined)) {
const companyGoals = await db.drizzle
.select({ id: goals.id, parentId: goals.parentId, level: goals.level })
.from(goals)
.where(eq(goals.companyId, companyId));
const goalsById = new Map(companyGoals.map((goal) => [goal.id, goal]));

Comment on lines +60 to +66

@qodo-code-review qodo-code-review Bot Jul 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remediation recommended

2. Unneeded full goals scan 🐞 Bug ➹ Performance

POST /goals always runs validateGoalReferences()'s full-company SELECT+Map build even for root
goals, because CreateGoalBody defaults parentId to null and the guard only checks for undefined.
This makes every goal creation O(number of company goals) in DB work and memory allocation,
increasing latency/load as goal counts grow.
Agent Prompt 
## Issue description
`validateGoalReferences()` loads every goal for the company whenever `parentId !== undefined`, but `CreateGoalBody` defaults `parentId` to `null`. This means root goal creation (no parent) still triggers a full company goals query + `Map` allocation that is not used for any validation.

## Issue Context
- Root creates pass `parentId: null` into `validateGoalReferences()`.
- The guard `parentId !== undefined` treats `null` as “present”, so it performs the company-wide scan.

## Fix Focus Areas
- server/src/routes/goals.ts[10-20]
- server/src/routes/goals.ts[47-66]

## Recommended fix
Update the guard so it only loads company goals when a *non-null* parent is being validated, or when `goalId && level !== undefined` is needed for child-level checks. For example:

```ts
if ((parentId !== undefined && parentId !== null) || (goalId && level !== undefined)) {
  ...
}
```

Optionally, also consider making `CreateGoalBody.parentId` `optional()` (no default) and normalize to `null` only at insert time, so omitted parent values don’t look “set” to validation logic.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

if (parentId !== undefined && parentId !== null) {
const parent = goalsById.get(parentId);
if (!parent) {
throw new AppError(
400,
'GOAL_PARENT_INVALID',
'Choose a parent goal from this company.',
);
}

let ancestorId: string | null = parentId;
const visited = new Set<string>();
while (ancestorId) {
if (ancestorId === goalId || visited.has(ancestorId)) {
throw new AppError(
400,
'GOAL_PARENT_CYCLE',
'A goal cannot be its own parent or a child of one of its descendants.',
);
}
visited.add(ancestorId);
ancestorId = goalsById.get(ancestorId)?.parentId ?? null;
}

if (level !== undefined && GOAL_LEVEL_RANK[level] <= GOAL_LEVEL_RANK[parent.level]) {
throw new AppError(
400,
'GOAL_LEVEL_INVALID',
'A child goal must use a level below its parent.',
);
}
}

if (
goalId
&& level !== undefined
&& companyGoals.some(
(goal) => goal.parentId === goalId && GOAL_LEVEL_RANK[goal.level] <= GOAL_LEVEL_RANK[level],
)
) {
throw new AppError(
400,
'GOAL_LEVEL_INVALID',
'A parent goal must use a level above each child.',
);
}
}

if (ownerAgentId !== undefined && ownerAgentId !== null) {
const [owner] = await db.drizzle
.select({ id: agents.id })
.from(agents)
.where(and(eq(agents.id, ownerAgentId), eq(agents.companyId, companyId)))
.limit(1);

if (!owner) {
throw new AppError(
400,
'GOAL_OWNER_INVALID',
'Choose an owner from this company.',
);
}
}
}

// GET /api/companies/:companyId/goals
router.get('/', async (req, res) => {
Expand Down Expand Up @@ -78,6 +172,13 @@ export function goalsRouter(db: DbInstance): Router {
const companyId = routeParams(req).companyId;
const now = new Date();

await validateGoalReferences({
companyId,
ownerAgentId: body.ownerAgentId,
parentId: body.parentId,
level: body.level,
});

const [row] = await db.drizzle
.insert(goals)
.values({
Expand Down Expand Up @@ -140,13 +241,24 @@ export function goalsRouter(db: DbInstance): Router {
throw new AppError(404, 'GOAL_NOT_FOUND', `Goal ${id} not found`);
}

const relationChanged = body.parentId !== undefined || body.level !== undefined;
await validateGoalReferences({
companyId,
goalId: id,
ownerAgentId: body.ownerAgentId,
parentId: relationChanged
? body.parentId !== undefined ? body.parentId : existing.parentId
: undefined,
level: relationChanged ? body.level ?? existing.level : undefined,
});

const progressChanged =
body.progress !== undefined && body.progress !== existing.progress;

const [updated] = await db.drizzle
.update(goals)
.set({ ...body, updatedAt: new Date() })
.where(eq(goals.id, id))
.where(and(eq(goals.id, id), eq(goals.companyId, companyId)))
Comment on lines +244 to +261

@coderabbitai coderabbitai Bot Jul 31, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -eu

file="$(git ls-files | rg '(^|/)server/src/routes/goals\.ts$' | head -n1)"
printf '%s\n' "FILE=$file"
wc -l "$file"
printf '%s\n' '--- route structure ---'
ast-grep outline "$file" --lang typescript
printf '%s\n' '--- target and nearby code ---'
sed -n '180,330p' "$file"
printf '%s\n' '--- validation and tree references ---'
rg -n -C 4 'validateGoalReferences|/tree|nodeMap|parentId|for\(|transaction' "$file" server/src server/test test tests 2>/dev/null | head -n 500
printf '%s\n' '--- relevant tracked files ---'
git ls-files | rg -i '(goal|pglite|drizzle|migration|schema|test|package\.json)' | head -n 500

Repository: VerticalLabs-ai/eidolon

Length of output: 42814

🏁 Script executed:

#!/bin/bash
set -eu

printf '%s\n' '--- all goal route definitions and imports ---'
file="$(git ls-files | rg '(^|/)server/src/routes/goals\.ts$' | head -n1)"
sed -n '1,180p' "$file"
printf '%s\n' '--- exact validation implementation ---'
rg -n -l 'function validateGoalReferences|validateGoalReferences' server/src server 2>/dev/null | while IFS= read -r f; do
  echo "FILE=$f"
  rg -n -C 25 'function validateGoalReferences|validateGoalReferences' "$f"
done
printf '%s\n' '--- database setup and transaction support ---'
rg -n -C 5 'PGlite|pglite|drizzle\(|drizzle-orm|forUpdate|FOR UPDATE|for\(' server package.json '**/package.json' 2>/dev/null | head -n 500
printf '%s\n' '--- goal tree implementation ---'
rg -n -C 25 'nodeMap|goals/tree|tree' server/src server 2>/dev/null | head -n 500

Repository: VerticalLabs-ai/eidolon

Length of output: 50379

🏁 Script executed:

#!/bin/bash
set -eu
find . -path '*/server/src/routes/goals.ts' -print

Repository: VerticalLabs-ai/eidolon

Length of output: 190

🏁 Script executed:

rg -n -C 3 "validateGoalReferences|nodeMap|PGlite|pglite|transaction|for\('update'\)" .

Repository: VerticalLabs-ai/eidolon

Length of output: 47285

🌐 Web query:

PGlite PostgreSQL FOR UPDATE row locking transaction concurrent transactions support

💡 Result:

PGlite is a WebAssembly-based distribution of PostgreSQL that operates as a single-process, single-user instance [1]. Because of its architecture, it behaves differently regarding row locking and transaction concurrency compared to a standard, multi-process PostgreSQL server [1]. Row Locking (SELECT FOR UPDATE) PGlite supports the standard PostgreSQL SQL syntax, including row-level locking commands like SELECT FOR UPDATE [2][3]. However, because PGlite operates as a single-process system [1], these locks do not function as a mechanism for handling concurrency between multiple independent client connections in the same way they would on a standard Postgres server. Within the context of a single PGlite instance, there is only one user/connection [1], so there are no concurrent transactions from other users to block. The commands will execute, but they will not provide the cross-connection protection that row-level locks typically offer in a multi-user environment [1][2]. Transaction Concurrency PGlite does not support concurrent transactions in the traditional sense because it is fundamentally a single-user, single-process engine [1]. 1. Single-Process Architecture: PGlite runs in a single-threaded WASM environment [1]. It does not fork processes for new connections [1]. 2. Transaction Handling: While PGlite supports transactions via its.transaction API (which ensures atomic operations for the single connection) [3], it cannot execute multiple transactions simultaneously. 3. Multi-connection management: Tools like pglite-socket can facilitate interactions that appear to manage multiple clients or connection pools (e.g., from Node.js) by queueing and scheduling protocol messages [4]. These tools help maintain isolation and prevent corruption when multiple clients attempt to use the same PGlite instance, but they do so by serializing the work rather than enabling true parallel transaction processing [4]. If your application requires true concurrent transaction support, a standard PostgreSQL server deployment is required, as PGlite's architecture is specifically optimized for single-process, WASM-based environments [1].

Citations:

Serialize goal-reference validation and the update in one transaction.

Concurrent production requests can both pass the cycle check and persist A.parentId = B and B.parentId = A. GET /goals/tree then returns neither goal as a root. Move the existing read into the transaction, lock the company’s goals with FOR UPDATE, and run every validation query through the same transaction executor. PGlite supports these operations, but its single-connection harness cannot test cross-connection locking; cover this race against PostgreSQL.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@server/src/routes/goals.ts` around lines 244 - 261, Wrap the goal update flow
in a database transaction, moving the existing-goal read inside it and locking
the company’s goals with FOR UPDATE before validation. Update
validateGoalReferences and all related queries to use the transaction executor,
then perform the goals update within the same transaction so reference
validation and persistence are serialized; add PostgreSQL coverage for the
concurrent cycle race.

.returning();

if (progressChanged) {
Expand Down
Loading