sync: update from private repo (6fcb0d0)
This commit is contained in:
oss-sync
@@ -1,7 +1,7 @@
|
||||
import { describe, expect, it, beforeEach, afterEach } from 'vitest';
|
||||
import express from 'express';
|
||||
import request from 'supertest';
|
||||
import { mkdtempSync, rmSync } from 'fs';
|
||||
import { existsSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'fs';
|
||||
import { join } from 'path';
|
||||
import { tmpdir } from 'os';
|
||||
import { Repository, localTaskRepoName } from '../db/repository.js';
|
||||
@@ -911,3 +911,120 @@ describe('POST /api/local/tasks/:id/continue', () => {
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Regression: /continue must accept pieces that exist ONLY in the task
|
||||
// owner's per-user dir. The old pieceExists(name) callback never checked
|
||||
// that dir and always returned false → 400 piece_not_found for user-custom
|
||||
// pieces even though the worker would have loaded them successfully.
|
||||
// ---------------------------------------------------------------------------
|
||||
describe('POST /api/local/tasks/:id/continue — user-custom piece resolution', () => {
|
||||
let tempDir = '';
|
||||
let repo: Repository;
|
||||
let app: express.Application;
|
||||
let aliceUser: Express.User;
|
||||
|
||||
beforeEach(() => {
|
||||
tempDir = mkdtempSync(join(tmpdir(), 'lt-ucpiece-'));
|
||||
repo = new Repository(join(tempDir, 'db.sqlite'));
|
||||
const real = repo.createUser({ email: '[email protected]', name: 'uc', role: 'user', status: 'active' });
|
||||
aliceUser = {
|
||||
...real,
|
||||
orgIds: [],
|
||||
defaultVisibility: 'private',
|
||||
defaultVisibilityOrgId: null,
|
||||
};
|
||||
app = express();
|
||||
app.use(express.json());
|
||||
app.use((req, _res, next) => {
|
||||
(req as unknown as { user: Express.User }).user = aliceUser;
|
||||
next();
|
||||
});
|
||||
|
||||
// Simulate the server.ts pieceExists implementation:
|
||||
// per-user dir → (global-custom skipped here) → builtin dir.
|
||||
const builtinDir = join(tempDir, 'pieces');
|
||||
const userFolderRoot = join(tempDir, 'users');
|
||||
mkdirSync(builtinDir, { recursive: true });
|
||||
// 'manual-writer' only exists in the builtin dir (used for the starter job).
|
||||
writeFileSync(
|
||||
join(builtinDir, 'manual-writer.yaml'),
|
||||
'name: manual-writer\nmovements: []\n',
|
||||
);
|
||||
// 'my-custom-piece' exists ONLY in Alice's per-user dir.
|
||||
const userPiecesPath = join(userFolderRoot, aliceUser.id, 'pieces');
|
||||
mkdirSync(userPiecesPath, { recursive: true });
|
||||
writeFileSync(
|
||||
join(userPiecesPath, 'my-custom-piece.yaml'),
|
||||
'name: my-custom-piece\nmovements: []\n',
|
||||
);
|
||||
|
||||
mountLocalTasksApi(app, {
|
||||
repo,
|
||||
worktreeDir: join(tempDir, 'workspaces'),
|
||||
pieceExists: (name: string, ownerId?: string) => {
|
||||
// Check owner's per-user dir first (mirrors worker resolution).
|
||||
if (ownerId) {
|
||||
if (existsSync(join(userFolderRoot, ownerId, 'pieces', `${name}.yaml`))) return true;
|
||||
}
|
||||
// Fall back to builtin dir.
|
||||
return existsSync(join(builtinDir, `${name}.yaml`));
|
||||
},
|
||||
});
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
repo.close();
|
||||
rmSync(tempDir, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
it('accepts a piece that exists only in the owner per-user dir (regression: was 400)', async () => {
|
||||
// Set up a task owned by alice with a terminal starter job.
|
||||
const task = await repo.createLocalTask({
|
||||
title: 't',
|
||||
body: 'b',
|
||||
pieceName: 'manual-writer',
|
||||
ownerId: aliceUser.id,
|
||||
});
|
||||
const prev = await repo.createJob({
|
||||
repo: localTaskRepoName(task.id),
|
||||
issueNumber: task.id,
|
||||
instruction: 'go',
|
||||
pieceName: 'manual-writer',
|
||||
ownerId: aliceUser.id,
|
||||
});
|
||||
await repo.updateJob(prev.id, { status: 'succeeded' });
|
||||
|
||||
// 'my-custom-piece' is ONLY in alice's per-user dir — should succeed.
|
||||
const res = await request(app)
|
||||
.post(`/api/local/tasks/${task.id}/continue`)
|
||||
.send({ piece: 'my-custom-piece', instruction: 'continue with user-custom piece' });
|
||||
expect(res.status).toBe(201);
|
||||
expect(res.body.jobId).toBeTruthy();
|
||||
const newJob = await repo.getJob(res.body.jobId);
|
||||
expect(newJob?.pieceName).toBe('my-custom-piece');
|
||||
});
|
||||
|
||||
it('rejects a piece that exists in neither builtin nor user-custom dir', async () => {
|
||||
const task = await repo.createLocalTask({
|
||||
title: 't',
|
||||
body: 'b',
|
||||
pieceName: 'manual-writer',
|
||||
ownerId: aliceUser.id,
|
||||
});
|
||||
const prev = await repo.createJob({
|
||||
repo: localTaskRepoName(task.id),
|
||||
issueNumber: task.id,
|
||||
instruction: 'go',
|
||||
pieceName: 'manual-writer',
|
||||
ownerId: aliceUser.id,
|
||||
});
|
||||
await repo.updateJob(prev.id, { status: 'succeeded' });
|
||||
|
||||
const res = await request(app)
|
||||
.post(`/api/local/tasks/${task.id}/continue`)
|
||||
.send({ piece: 'nonexistent-piece', instruction: 'go' });
|
||||
expect(res.status).toBe(400);
|
||||
expect(res.body.error).toBe('piece_not_found');
|
||||
});
|
||||
});
|
||||
|
||||
@@ -18,8 +18,12 @@ export interface LocalTasksApiOptions {
|
||||
* Server-side validator for piece names accepted by the
|
||||
* /continue endpoint. Returns true if the piece is loadable.
|
||||
* When unset, /continue rejects all requests with 500 (misconfiguration).
|
||||
*
|
||||
* `ownerId` is the task owner's user id; when provided the implementation
|
||||
* MUST also check the owner's per-user piece dir so that user-custom pieces
|
||||
* are accepted (matching the resolution order the worker uses at run time).
|
||||
*/
|
||||
pieceExists?: (name: string) => boolean;
|
||||
pieceExists?: (name: string, ownerId?: string) => boolean;
|
||||
/**
|
||||
* Optional. When set, accepting browserSessionProfileId on task create
|
||||
* verifies the profile belongs to the requesting user. Without it, the
|
||||
@@ -506,7 +510,7 @@ export function mountLocalTasksApi(app: Application, opts: LocalTasksApiOptions)
|
||||
res.status(500).json({ error: 'piece_validation_unavailable' });
|
||||
return;
|
||||
}
|
||||
if (!opts.pieceExists(piece)) {
|
||||
if (!opts.pieceExists(piece, task?.ownerId ?? undefined)) {
|
||||
res.status(400).json({ error: 'piece_not_found', piece });
|
||||
return;
|
||||
}
|
||||
|
||||
+556
-13
@@ -1,7 +1,7 @@
|
||||
import { describe, it, expect, beforeEach } from 'vitest';
|
||||
import express from 'express';
|
||||
import request from 'supertest';
|
||||
import { mkdtempSync, writeFileSync, mkdirSync, existsSync } from 'fs';
|
||||
import { mkdtempSync, writeFileSync, mkdirSync, existsSync, readFileSync } from 'fs';
|
||||
import { join } from 'path';
|
||||
import { tmpdir } from 'os';
|
||||
import { mountPiecesApi } from './pieces-api.js';
|
||||
@@ -295,13 +295,16 @@ describe('Pieces API (no auth — legacy behavior)', () => {
|
||||
expect(res.status).toBe(201);
|
||||
});
|
||||
|
||||
it('DELETE /api/pieces/:name deletes piece', async () => {
|
||||
it('DELETE /api/pieces/:name — legacy no-auth pieces land in piecesDir (builtin source) and are non-deletable', async () => {
|
||||
// In legacy no-auth mode with no customPiecesDir, POST writes to piecesDir.
|
||||
// Those files are resolved as source='builtin' and are non-deletable.
|
||||
await request(app).post('/api/pieces').send({
|
||||
name: 'deleteme', description: 'x', max_movements: 1, initial_movement: 'a',
|
||||
movements: [{ name: 'a', edit: false, persona: 'x', instruction: 'x', allowed_tools: [], rules: [] }],
|
||||
movements: [{ name: 'a', edit: false, persona: 'x', instruction: 'x', allowed_tools: [], default_next: 'COMPLETE', rules: [] }],
|
||||
});
|
||||
const res = await request(app).delete('/api/pieces/deleteme');
|
||||
expect(res.status).toBe(200);
|
||||
expect(res.status).toBe(403);
|
||||
expect(res.body.error).toMatch(/cannot delete a built-in/i);
|
||||
});
|
||||
|
||||
it('DELETE /api/pieces/general is forbidden', async () => {
|
||||
@@ -371,16 +374,21 @@ describe('Pieces API (auth-aware: per-user custom + write authz)', () => {
|
||||
expect(byName['bob-tool']).toBeUndefined();
|
||||
});
|
||||
|
||||
it("GET /api/pieces — user-custom shadows built-in with the same name", async () => {
|
||||
it("GET /api/pieces — user-custom and built-in both appear when same name (no hiding)", async () => {
|
||||
mkdirSync(join(userPiecesRootDir, 'alice', 'pieces'), { recursive: true });
|
||||
writeFileSync(join(userPiecesRootDir, 'alice', 'pieces', 'general.yaml'), makeMinimalPieceYaml('general', 'alice override'));
|
||||
|
||||
const res = await request(makeAuthApp(piecesDir, userPiecesRootDir, { id: 'alice', role: 'user' }))
|
||||
.get('/api/pieces');
|
||||
expect(res.status).toBe(200);
|
||||
const general = res.body.pieces.find((p: any) => p.name === 'general');
|
||||
expect(general.source).toBe('user-custom');
|
||||
expect(general.description).toBe('alice override');
|
||||
const generals = res.body.pieces.filter((p: any) => p.name === 'general');
|
||||
// Both builtin and user-custom should appear
|
||||
expect(generals).toHaveLength(2);
|
||||
const customGeneral = generals.find((p: any) => p.source === 'user-custom');
|
||||
const builtinGeneral = generals.find((p: any) => p.source === 'builtin');
|
||||
expect(customGeneral).toBeDefined();
|
||||
expect(customGeneral.description).toBe('alice override');
|
||||
expect(builtinGeneral).toBeDefined();
|
||||
});
|
||||
|
||||
it('POST /api/pieces creates a user-custom piece for non-admin caller', async () => {
|
||||
@@ -399,7 +407,9 @@ describe('Pieces API (auth-aware: per-user custom + write authz)', () => {
|
||||
expect(existsSync(join(piecesDir, 'alice-custom.yaml'))).toBe(false);
|
||||
});
|
||||
|
||||
it('POST /api/pieces by admin writes to piecesDir (legacy behavior)', async () => {
|
||||
it('POST /api/pieces by admin writes to user-custom dir (not piecesDir)', async () => {
|
||||
// POST always targets user-custom dir regardless of admin role.
|
||||
// Admins edit built-ins via PUT on existing ones.
|
||||
const res = await request(makeAuthApp(piecesDir, userPiecesRootDir, { id: 'admin1', role: 'admin' }))
|
||||
.post('/api/pieces')
|
||||
.send({
|
||||
@@ -410,7 +420,9 @@ describe('Pieces API (auth-aware: per-user custom + write authz)', () => {
|
||||
movements: [{ name: 'only', edit: false, persona: 'p', instruction: 'i', allowed_tools: ['Read'], default_next: 'COMPLETE', rules: [] }],
|
||||
});
|
||||
expect(res.status).toBe(201);
|
||||
expect(existsSync(join(piecesDir, 'admin-piece.yaml'))).toBe(true);
|
||||
// Piece lands in admin's user-custom dir, NOT in piecesDir.
|
||||
expect(existsSync(join(userPiecesRootDir, 'admin1', 'pieces', 'admin-piece.yaml'))).toBe(true);
|
||||
expect(existsSync(join(piecesDir, 'admin-piece.yaml'))).toBe(false);
|
||||
});
|
||||
|
||||
it('PUT /api/pieces/:name on built-in by non-admin returns 403', async () => {
|
||||
@@ -479,10 +491,11 @@ describe('Pieces API (auth-aware: per-user custom + write authz)', () => {
|
||||
expect(existsSync(join(userPiecesRootDir, 'alice', 'pieces', 'goner.yaml'))).toBe(false);
|
||||
});
|
||||
|
||||
it('admin can edit and delete built-in pieces', async () => {
|
||||
it('admin can edit built-in pieces (PUT 200) but NOT delete them (DELETE 403)', async () => {
|
||||
writeFileSync(join(piecesDir, 'admin-target.yaml'), makeMinimalPieceYaml('admin-target', 'before'));
|
||||
const adminApp = makeAuthApp(piecesDir, userPiecesRootDir, { id: 'admin1', role: 'admin' });
|
||||
|
||||
// Admin CAN edit (PUT) a built-in
|
||||
const putRes = await request(adminApp).put('/api/pieces/admin-target').send({
|
||||
name: 'admin-target',
|
||||
description: 'after',
|
||||
@@ -492,8 +505,538 @@ describe('Pieces API (auth-aware: per-user custom + write authz)', () => {
|
||||
});
|
||||
expect(putRes.status).toBe(200);
|
||||
|
||||
// Admin CANNOT delete a built-in — 403 for everyone
|
||||
const delRes = await request(adminApp).delete('/api/pieces/admin-target');
|
||||
expect(delRes.status).toBe(200);
|
||||
expect(existsSync(join(piecesDir, 'admin-target.yaml'))).toBe(false);
|
||||
expect(delRes.status).toBe(403);
|
||||
expect(delRes.body.ok).toBe(false);
|
||||
expect(delRes.body.error).toMatch(/cannot delete a built-in/i);
|
||||
// File must still exist
|
||||
expect(existsSync(join(piecesDir, 'admin-target.yaml'))).toBe(true);
|
||||
});
|
||||
|
||||
// --- Task 2A: built-in must not be hidden by same-named custom ---
|
||||
it("GET /api/pieces — built-in is NOT hidden when user has a same-named custom", async () => {
|
||||
mkdirSync(join(userPiecesRootDir, 'alice', 'pieces'), { recursive: true });
|
||||
writeFileSync(join(userPiecesRootDir, 'alice', 'pieces', 'chat.yaml'), makeMinimalPieceYaml('chat', 'alice chat override'));
|
||||
|
||||
const res = await request(makeAuthApp(piecesDir, userPiecesRootDir, { id: 'alice', role: 'user' }))
|
||||
.get('/api/pieces');
|
||||
expect(res.status).toBe(200);
|
||||
// Both the user-custom AND the builtin should appear
|
||||
const chatPieces = res.body.pieces.filter((p: any) => p.name === 'chat');
|
||||
expect(chatPieces).toHaveLength(2);
|
||||
const sources = chatPieces.map((p: any) => p.source).sort();
|
||||
expect(sources).toEqual(['builtin', 'user-custom']);
|
||||
// Custom has the custom description, builtin has the original
|
||||
const customChat = chatPieces.find((p: any) => p.source === 'user-custom');
|
||||
const builtinChat = chatPieces.find((p: any) => p.source === 'builtin');
|
||||
expect(customChat.description).toBe('alice chat override');
|
||||
expect(builtinChat.description).toBe('built-in chat');
|
||||
});
|
||||
|
||||
// --- Task 2B: CreatePiece rejects name collision with built-in ---
|
||||
it('POST /api/pieces rejects custom creation with a built-in name', async () => {
|
||||
// 'general' and 'chat' are in piecesDir (built-in)
|
||||
const res = await request(makeAuthApp(piecesDir, userPiecesRootDir, { id: 'alice', role: 'user' }))
|
||||
.post('/api/pieces')
|
||||
.send({
|
||||
name: 'chat',
|
||||
description: 'my chat',
|
||||
max_movements: 1,
|
||||
initial_movement: 'only',
|
||||
movements: [{ name: 'only', edit: false, persona: 'p', instruction: 'i', allowed_tools: ['Read'], default_next: 'COMPLETE', rules: [] }],
|
||||
});
|
||||
expect(res.status).toBe(409);
|
||||
expect(res.body.error).toMatch(/built-in/i);
|
||||
});
|
||||
|
||||
it('POST /api/pieces with a fresh custom name still works for non-admin', async () => {
|
||||
const res = await request(makeAuthApp(piecesDir, userPiecesRootDir, { id: 'alice', role: 'user' }))
|
||||
.post('/api/pieces')
|
||||
.send({
|
||||
name: 'fresh-custom',
|
||||
description: 'fresh',
|
||||
max_movements: 1,
|
||||
initial_movement: 'only',
|
||||
movements: [{ name: 'only', edit: false, persona: 'p', instruction: 'i', allowed_tools: ['Read'], default_next: 'COMPLETE', rules: [] }],
|
||||
});
|
||||
expect(res.status).toBe(201);
|
||||
});
|
||||
|
||||
// --- Task 2C: regression — non-admin DELETE of built-in → 403 ---
|
||||
it('non-admin DELETE of built-in returns 403', async () => {
|
||||
writeFileSync(join(piecesDir, 'deletable-builtin.yaml'), makeMinimalPieceYaml('deletable-builtin', 'del'));
|
||||
const res = await request(makeAuthApp(piecesDir, userPiecesRootDir, { id: 'alice', role: 'user' }))
|
||||
.delete('/api/pieces/deletable-builtin');
|
||||
expect(res.status).toBe(403);
|
||||
expect(existsSync(join(piecesDir, 'deletable-builtin.yaml'))).toBe(true);
|
||||
});
|
||||
|
||||
// --- Fix 2: GET /api/pieces/:name?source=builtin fetches the specific source ---
|
||||
it('GET /api/pieces/:name?source=builtin returns builtin even when user-custom exists with same name', async () => {
|
||||
// Alice has a user-custom 'chat' that overrides by default priority
|
||||
mkdirSync(join(userPiecesRootDir, 'alice', 'pieces'), { recursive: true });
|
||||
writeFileSync(join(userPiecesRootDir, 'alice', 'pieces', 'chat.yaml'), makeMinimalPieceYaml('chat', 'alice custom chat'));
|
||||
|
||||
const res = await request(makeAuthApp(piecesDir, userPiecesRootDir, { id: 'alice', role: 'user' }))
|
||||
.get('/api/pieces/chat?source=builtin');
|
||||
expect(res.status).toBe(200);
|
||||
expect(res.body.source).toBe('builtin');
|
||||
expect(res.body.piece.description).toBe('built-in chat');
|
||||
});
|
||||
|
||||
it('GET /api/pieces/:name without ?source uses priority resolution (user-custom first)', async () => {
|
||||
mkdirSync(join(userPiecesRootDir, 'alice', 'pieces'), { recursive: true });
|
||||
writeFileSync(join(userPiecesRootDir, 'alice', 'pieces', 'chat.yaml'), makeMinimalPieceYaml('chat', 'alice custom chat'));
|
||||
|
||||
const res = await request(makeAuthApp(piecesDir, userPiecesRootDir, { id: 'alice', role: 'user' }))
|
||||
.get('/api/pieces/chat');
|
||||
expect(res.status).toBe(200);
|
||||
expect(res.body.source).toBe('user-custom');
|
||||
expect(res.body.piece.description).toBe('alice custom chat');
|
||||
});
|
||||
|
||||
// --- Fix 3: DELETE of a user-custom named 'chat' by its owner must succeed ---
|
||||
it("DELETE /api/pieces/chat on owner's user-custom named 'chat' returns 200 (guard only blocks builtin)", async () => {
|
||||
mkdirSync(join(userPiecesRootDir, 'alice', 'pieces'), { recursive: true });
|
||||
writeFileSync(join(userPiecesRootDir, 'alice', 'pieces', 'chat.yaml'), makeMinimalPieceYaml('chat', 'alice custom chat'));
|
||||
|
||||
const res = await request(makeAuthApp(piecesDir, userPiecesRootDir, { id: 'alice', role: 'user' }))
|
||||
.delete('/api/pieces/chat');
|
||||
expect(res.status).toBe(200);
|
||||
// User-custom file removed; built-in chat still present
|
||||
expect(existsSync(join(userPiecesRootDir, 'alice', 'pieces', 'chat.yaml'))).toBe(false);
|
||||
expect(existsSync(join(piecesDir, 'chat.yaml'))).toBe(true);
|
||||
});
|
||||
|
||||
it('DELETE /api/pieces/chat on builtin by non-admin returns 403', async () => {
|
||||
// No user-custom for alice, so findPieceForCaller resolves to builtin
|
||||
const res = await request(makeAuthApp(piecesDir, userPiecesRootDir, { id: 'alice', role: 'user' }))
|
||||
.delete('/api/pieces/chat');
|
||||
expect(res.status).toBe(403);
|
||||
expect(existsSync(join(piecesDir, 'chat.yaml'))).toBe(true);
|
||||
});
|
||||
|
||||
// --- Fix source param for PUT/DELETE (P1-b) ---
|
||||
|
||||
it('PUT /api/pieces/chat?source=builtin by admin updates the builtin, not a same-named custom', async () => {
|
||||
// Alice (admin) has a user-custom chat AND there is a builtin chat.
|
||||
mkdirSync(join(userPiecesRootDir, 'admin1', 'pieces'), { recursive: true });
|
||||
writeFileSync(join(userPiecesRootDir, 'admin1', 'pieces', 'chat.yaml'), makeMinimalPieceYaml('chat', 'admin custom chat'));
|
||||
const adminApp = makeAuthApp(piecesDir, userPiecesRootDir, { id: 'admin1', role: 'admin' });
|
||||
|
||||
const res = await request(adminApp).put('/api/pieces/chat?source=builtin').send({
|
||||
name: 'chat',
|
||||
description: 'builtin updated',
|
||||
max_movements: 1,
|
||||
initial_movement: 'only',
|
||||
movements: [{ name: 'only', edit: false, persona: 'p', instruction: 'i', allowed_tools: ['Read'], default_next: 'COMPLETE', rules: [] }],
|
||||
});
|
||||
expect(res.status).toBe(200);
|
||||
// Builtin was updated
|
||||
const builtinContent = readFileSync(join(piecesDir, 'chat.yaml'), 'utf-8');
|
||||
expect(builtinContent).toContain('builtin updated');
|
||||
// Custom is untouched
|
||||
const customContent = readFileSync(join(userPiecesRootDir, 'admin1', 'pieces', 'chat.yaml'), 'utf-8');
|
||||
expect(customContent).toContain('admin custom chat');
|
||||
});
|
||||
|
||||
it('DELETE /api/pieces/chat?source=builtin by admin deletes the builtin, not same-named custom', async () => {
|
||||
// admin1 has a user-custom chat AND builtin chat
|
||||
mkdirSync(join(userPiecesRootDir, 'admin1', 'pieces'), { recursive: true });
|
||||
writeFileSync(join(userPiecesRootDir, 'admin1', 'pieces', 'chat.yaml'), makeMinimalPieceYaml('chat', 'admin custom'));
|
||||
const adminApp = makeAuthApp(piecesDir, userPiecesRootDir, { id: 'admin1', role: 'admin' });
|
||||
|
||||
const res = await request(adminApp).delete('/api/pieces/chat?source=builtin');
|
||||
// Built-in pieces are non-deletable for everyone (including admins)
|
||||
expect(res.status).toBe(403);
|
||||
// Both still intact
|
||||
expect(existsSync(join(piecesDir, 'chat.yaml'))).toBe(true);
|
||||
expect(existsSync(join(userPiecesRootDir, 'admin1', 'pieces', 'chat.yaml'))).toBe(true);
|
||||
});
|
||||
|
||||
it('DELETE /api/pieces/extra?source=user-custom targets user-custom, not builtin', async () => {
|
||||
writeFileSync(join(piecesDir, 'extra.yaml'), makeMinimalPieceYaml('extra', 'builtin extra'));
|
||||
mkdirSync(join(userPiecesRootDir, 'alice', 'pieces'), { recursive: true });
|
||||
writeFileSync(join(userPiecesRootDir, 'alice', 'pieces', 'extra.yaml'), makeMinimalPieceYaml('extra', 'alice extra'));
|
||||
|
||||
const res = await request(makeAuthApp(piecesDir, userPiecesRootDir, { id: 'alice', role: 'user' }))
|
||||
.delete('/api/pieces/extra?source=user-custom');
|
||||
expect(res.status).toBe(200);
|
||||
// User-custom removed
|
||||
expect(existsSync(join(userPiecesRootDir, 'alice', 'pieces', 'extra.yaml'))).toBe(false);
|
||||
// Builtin untouched
|
||||
expect(existsSync(join(piecesDir, 'extra.yaml'))).toBe(true);
|
||||
});
|
||||
|
||||
// P2 fix: GET /api/pieces/:name (no ?source) includes source in response body
|
||||
// so the UI can derive the correct read-only state regardless of URL params.
|
||||
it('GET /api/pieces/general (no ?source) returns source: builtin in the body', async () => {
|
||||
const res = await request(makeAuthApp(piecesDir, userPiecesRootDir, { id: 'alice', role: 'user' }))
|
||||
.get('/api/pieces/general');
|
||||
expect(res.status).toBe(200);
|
||||
expect(res.body.piece.name).toBe('general');
|
||||
expect(res.body.source).toBe('builtin');
|
||||
expect(res.body.custom).toBe(false);
|
||||
});
|
||||
|
||||
it('POST /api/pieces creates in user-custom dir for admin (not piecesDir)', async () => {
|
||||
// Regression for P2: POST always targets user-custom, admin is no exception.
|
||||
const res = await request(makeAuthApp(piecesDir, userPiecesRootDir, { id: 'admin1', role: 'admin' }))
|
||||
.post('/api/pieces')
|
||||
.send({
|
||||
name: 'admin-new-custom',
|
||||
description: 'admin custom piece',
|
||||
max_movements: 1,
|
||||
initial_movement: 'only',
|
||||
movements: [{ name: 'only', edit: false, persona: 'p', instruction: 'i', allowed_tools: ['Read'], default_next: 'COMPLETE', rules: [] }],
|
||||
});
|
||||
expect(res.status).toBe(201);
|
||||
expect(existsSync(join(userPiecesRootDir, 'admin1', 'pieces', 'admin-new-custom.yaml'))).toBe(true);
|
||||
expect(existsSync(join(piecesDir, 'admin-new-custom.yaml'))).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Finding 2 regression: authenticated POST with userPiecesRootDir UNSET → 503
|
||||
// Never falls through to shared/builtin dirs for an authenticated user.
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
describe('Pieces API (Finding 2: POST auth fallback hole)', () => {
|
||||
let piecesDir: string;
|
||||
|
||||
function makeAppWithAuthButNoUserPiecesRoot(user: UserShape | null): express.Application {
|
||||
const app = express();
|
||||
app.use(express.json());
|
||||
app.use((req, _res, next) => {
|
||||
if (user) (req as any).user = user;
|
||||
next();
|
||||
});
|
||||
// Intentionally: no userPiecesRootDir configured, no customPiecesDir.
|
||||
mountPiecesApi(app, { piecesDir });
|
||||
return app;
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
const tempDir = mkdtempSync(join(tmpdir(), 'pieces-api-f2-'));
|
||||
piecesDir = join(tempDir, 'pieces');
|
||||
mkdirSync(piecesDir);
|
||||
writeFileSync(join(piecesDir, 'general.yaml'), makeGeneralPieceYaml());
|
||||
});
|
||||
|
||||
it('authenticated non-admin POST with userPiecesRootDir unset returns 503 (does not write shared/builtin)', async () => {
|
||||
const app = makeAppWithAuthButNoUserPiecesRoot({ id: 'alice', role: 'user' });
|
||||
const res = await request(app)
|
||||
.post('/api/pieces')
|
||||
.send({
|
||||
name: 'should-not-exist',
|
||||
description: 'fallback hole test',
|
||||
max_movements: 1,
|
||||
initial_movement: 'only',
|
||||
movements: [{ name: 'only', edit: false, persona: 'p', instruction: 'i', allowed_tools: ['Read'], default_next: 'COMPLETE', rules: [] }],
|
||||
});
|
||||
expect(res.status).toBe(503);
|
||||
expect(res.body.ok).toBe(false);
|
||||
expect(res.body.error).toMatch(/not configured/i);
|
||||
// Must NOT have written anything to the builtin dir.
|
||||
expect(existsSync(join(piecesDir, 'should-not-exist.yaml'))).toBe(false);
|
||||
});
|
||||
|
||||
it('authenticated admin POST with userPiecesRootDir unset also returns 503', async () => {
|
||||
const app = makeAppWithAuthButNoUserPiecesRoot({ id: 'admin1', role: 'admin' });
|
||||
const res = await request(app)
|
||||
.post('/api/pieces')
|
||||
.send({
|
||||
name: 'admin-fallback-hole',
|
||||
description: 'admin hole test',
|
||||
max_movements: 1,
|
||||
initial_movement: 'only',
|
||||
movements: [{ name: 'only', edit: false, persona: 'p', instruction: 'i', allowed_tools: ['Read'], default_next: 'COMPLETE', rules: [] }],
|
||||
});
|
||||
expect(res.status).toBe(503);
|
||||
expect(res.body.ok).toBe(false);
|
||||
// Must NOT have written anything to the builtin dir.
|
||||
expect(existsSync(join(piecesDir, 'admin-fallback-hole.yaml'))).toBe(false);
|
||||
});
|
||||
|
||||
it('unauthenticated POST with no userPiecesRootDir falls back to piecesDir (legacy no-auth mode)', async () => {
|
||||
// No user: genuine no-auth legacy mode — should still work (existing behavior).
|
||||
const app = makeAppWithAuthButNoUserPiecesRoot(null);
|
||||
const res = await request(app)
|
||||
.post('/api/pieces')
|
||||
.send({
|
||||
name: 'legacy-fallback',
|
||||
description: 'legacy',
|
||||
max_movements: 1,
|
||||
initial_movement: 'only',
|
||||
movements: [{ name: 'only', edit: false, persona: 'p', instruction: 'i', allowed_tools: ['Read'], default_next: 'COMPLETE', rules: [] }],
|
||||
});
|
||||
expect(res.status).toBe(201);
|
||||
expect(existsSync(join(piecesDir, 'legacy-fallback.yaml'))).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Fix 1: invalid ?source param → 400 (no fallback to priority resolution)
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
describe('Pieces API (Fix 1: invalid ?source → 400, no destructive fallback)', () => {
|
||||
let piecesDir: string;
|
||||
let userPiecesRootDir: string;
|
||||
|
||||
beforeEach(() => {
|
||||
const tempDir = mkdtempSync(join(tmpdir(), 'pieces-api-fix1-'));
|
||||
piecesDir = join(tempDir, 'pieces');
|
||||
userPiecesRootDir = join(tempDir, 'users');
|
||||
mkdirSync(piecesDir);
|
||||
mkdirSync(userPiecesRootDir);
|
||||
// A builtin and a user-custom with the same name
|
||||
writeFileSync(join(piecesDir, 'chat.yaml'), makeMinimalPieceYaml('chat', 'builtin chat'));
|
||||
// Alice has a user-custom 'chat'
|
||||
mkdirSync(join(userPiecesRootDir, 'alice', 'pieces'), { recursive: true });
|
||||
writeFileSync(join(userPiecesRootDir, 'alice', 'pieces', 'chat.yaml'), makeMinimalPieceYaml('chat', 'alice custom chat'));
|
||||
});
|
||||
|
||||
function makeApp(user: UserShape | null) {
|
||||
const app = express();
|
||||
app.use(express.json());
|
||||
app.use((req, _res, next) => {
|
||||
if (user) (req as any).user = user;
|
||||
next();
|
||||
});
|
||||
mountPiecesApi(app, { piecesDir, userPiecesRootDir });
|
||||
return app;
|
||||
}
|
||||
|
||||
it('GET /api/pieces/:name?source=builtinn (typo) → 400 (not priority-fallback)', async () => {
|
||||
const res = await request(makeApp({ id: 'alice', role: 'user' }))
|
||||
.get('/api/pieces/chat?source=builtinn');
|
||||
expect(res.status).toBe(400);
|
||||
expect(res.body.ok).toBe(false);
|
||||
expect(res.body.error).toMatch(/invalid source/i);
|
||||
});
|
||||
|
||||
it('DELETE /api/pieces/chat?source=builtinn (typo) → 400, does NOT delete user-custom', async () => {
|
||||
const res = await request(makeApp({ id: 'alice', role: 'user' }))
|
||||
.delete('/api/pieces/chat?source=builtinn');
|
||||
expect(res.status).toBe(400);
|
||||
expect(res.body.ok).toBe(false);
|
||||
expect(res.body.error).toMatch(/invalid source/i);
|
||||
// User-custom must be untouched
|
||||
expect(existsSync(join(userPiecesRootDir, 'alice', 'pieces', 'chat.yaml'))).toBe(true);
|
||||
});
|
||||
|
||||
it('PUT /api/pieces/chat?source=user_custom (underscore typo) → 400, does NOT mutate user-custom', async () => {
|
||||
const res = await request(makeApp({ id: 'alice', role: 'user' }))
|
||||
.put('/api/pieces/chat?source=user_custom')
|
||||
.send({
|
||||
name: 'chat',
|
||||
description: 'hijacked',
|
||||
max_movements: 1,
|
||||
initial_movement: 'only',
|
||||
movements: [{ name: 'only', edit: false, persona: 'p', instruction: 'i', allowed_tools: ['Read'], default_next: 'COMPLETE', rules: [] }],
|
||||
});
|
||||
expect(res.status).toBe(400);
|
||||
expect(res.body.ok).toBe(false);
|
||||
expect(res.body.error).toMatch(/invalid source/i);
|
||||
// User-custom description must be unchanged
|
||||
const content = readFileSync(join(userPiecesRootDir, 'alice', 'pieces', 'chat.yaml'), 'utf-8');
|
||||
expect(content).toContain('alice custom chat');
|
||||
expect(content).not.toContain('hijacked');
|
||||
});
|
||||
|
||||
it('valid ?source=builtin still works (not rejected)', async () => {
|
||||
const res = await request(makeApp({ id: 'alice', role: 'user' }))
|
||||
.get('/api/pieces/chat?source=builtin');
|
||||
expect(res.status).toBe(200);
|
||||
expect(res.body.source).toBe('builtin');
|
||||
});
|
||||
|
||||
it('absent ?source still does priority resolution (not rejected)', async () => {
|
||||
const res = await request(makeApp({ id: 'alice', role: 'user' }))
|
||||
.get('/api/pieces/chat');
|
||||
expect(res.status).toBe(200);
|
||||
// priority: user-custom wins
|
||||
expect(res.body.source).toBe('user-custom');
|
||||
});
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Fix 2: POST /api/pieces returns actual source in response body
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
describe('Pieces API (Fix 2: POST returns actual source)', () => {
|
||||
let piecesDir: string;
|
||||
let userPiecesRootDir: string;
|
||||
|
||||
function minimalPieceBody(name: string) {
|
||||
return {
|
||||
name,
|
||||
description: 'test',
|
||||
max_movements: 1,
|
||||
initial_movement: 'only',
|
||||
movements: [{ name: 'only', edit: false, persona: 'p', instruction: 'i', allowed_tools: ['Read'], default_next: 'COMPLETE', rules: [] }],
|
||||
};
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
const tempDir = mkdtempSync(join(tmpdir(), 'pieces-api-fix2-'));
|
||||
piecesDir = join(tempDir, 'pieces');
|
||||
userPiecesRootDir = join(tempDir, 'users');
|
||||
mkdirSync(piecesDir);
|
||||
mkdirSync(userPiecesRootDir);
|
||||
});
|
||||
|
||||
it('authenticated POST returns source: user-custom', async () => {
|
||||
const app = express();
|
||||
app.use(express.json());
|
||||
app.use((req, _res, next) => { (req as any).user = { id: 'alice', role: 'user' }; next(); });
|
||||
mountPiecesApi(app, { piecesDir, userPiecesRootDir });
|
||||
const res = await request(app).post('/api/pieces').send(minimalPieceBody('my-piece'));
|
||||
expect(res.status).toBe(201);
|
||||
expect(res.body.ok).toBe(true);
|
||||
expect(res.body.source).toBe('user-custom');
|
||||
});
|
||||
|
||||
it('legacy (no-auth) POST with customPiecesDir returns source: global-custom', async () => {
|
||||
const tempDir = mkdtempSync(join(tmpdir(), 'pieces-api-fix2-gc-'));
|
||||
const gcDir = join(tempDir, 'custom');
|
||||
mkdirSync(join(tempDir, 'pieces'));
|
||||
mkdirSync(gcDir);
|
||||
const app = express();
|
||||
app.use(express.json());
|
||||
// No user set — legacy no-auth mode
|
||||
mountPiecesApi(app, { piecesDir: join(tempDir, 'pieces'), customPiecesDir: gcDir });
|
||||
const res = await request(app).post('/api/pieces').send(minimalPieceBody('gc-piece'));
|
||||
expect(res.status).toBe(201);
|
||||
expect(res.body.ok).toBe(true);
|
||||
expect(res.body.source).toBe('global-custom');
|
||||
});
|
||||
|
||||
it('legacy (no-auth) POST without customPiecesDir returns source: builtin', async () => {
|
||||
const app = express();
|
||||
app.use(express.json());
|
||||
// No user, no customPiecesDir
|
||||
mountPiecesApi(app, { piecesDir });
|
||||
const res = await request(app).post('/api/pieces').send(minimalPieceBody('legacy-piece'));
|
||||
expect(res.status).toBe(201);
|
||||
expect(res.body.ok).toBe(true);
|
||||
expect(res.body.source).toBe('builtin');
|
||||
});
|
||||
|
||||
it('no-auth POST with userPiecesRootDir returns source: user-custom (regression fix)', async () => {
|
||||
const app = express();
|
||||
app.use(express.json());
|
||||
// No user, but userPiecesRootDir is configured (the fixed path)
|
||||
mountPiecesApi(app, { piecesDir, userPiecesRootDir });
|
||||
const res = await request(app).post('/api/pieces').send(minimalPieceBody('noauth-custom'));
|
||||
expect(res.status).toBe(201);
|
||||
expect(res.body.ok).toBe(true);
|
||||
expect(res.body.source).toBe('user-custom');
|
||||
// Piece is in data/users/local/pieces, NOT in piecesDir
|
||||
expect(existsSync(join(userPiecesRootDir, 'local', 'pieces', 'noauth-custom.yaml'))).toBe(true);
|
||||
expect(existsSync(join(piecesDir, 'noauth-custom.yaml'))).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Regression fix: no-auth + userPiecesRootDir → 'local' user-custom namespace
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
describe('Pieces API (regression: no-auth uses local user-custom, not piecesDir)', () => {
|
||||
let piecesDir: string;
|
||||
let userPiecesRootDir: string;
|
||||
|
||||
function minimalPieceBody(name: string) {
|
||||
return {
|
||||
name,
|
||||
description: 'test',
|
||||
max_movements: 1,
|
||||
initial_movement: 'only',
|
||||
movements: [{ name: 'only', edit: false, persona: 'p', instruction: 'i', allowed_tools: ['Read'], default_next: 'COMPLETE', rules: [] }],
|
||||
};
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
const tempDir = mkdtempSync(join(tmpdir(), 'pieces-noauth-local-'));
|
||||
piecesDir = join(tempDir, 'pieces');
|
||||
userPiecesRootDir = join(tempDir, 'users');
|
||||
mkdirSync(piecesDir);
|
||||
mkdirSync(userPiecesRootDir);
|
||||
writeFileSync(join(piecesDir, 'general.yaml'), makeGeneralPieceYaml());
|
||||
});
|
||||
|
||||
function makeNoAuthApp() {
|
||||
const app = express();
|
||||
app.use(express.json());
|
||||
// No user middleware — simulates no-auth mode
|
||||
mountPiecesApi(app, { piecesDir, userPiecesRootDir });
|
||||
return app;
|
||||
}
|
||||
|
||||
it('no-auth POST creates under local user-custom dir, NOT in piecesDir', async () => {
|
||||
const app = makeNoAuthApp();
|
||||
const res = await request(app).post('/api/pieces').send(minimalPieceBody('my-noauth-piece'));
|
||||
expect(res.status).toBe(201);
|
||||
expect(res.body.source).toBe('user-custom');
|
||||
// Must be in the 'local' user-custom dir
|
||||
expect(existsSync(join(userPiecesRootDir, 'local', 'pieces', 'my-noauth-piece.yaml'))).toBe(true);
|
||||
// Must NOT be in bundled piecesDir
|
||||
expect(existsSync(join(piecesDir, 'my-noauth-piece.yaml'))).toBe(false);
|
||||
});
|
||||
|
||||
it('no-auth created piece (with userPiecesRootDir) is DELETABLE — DELETE returns 200', async () => {
|
||||
const app = makeNoAuthApp();
|
||||
// Create it first
|
||||
await request(app).post('/api/pieces').send(minimalPieceBody('deletable-noauth'));
|
||||
expect(existsSync(join(userPiecesRootDir, 'local', 'pieces', 'deletable-noauth.yaml'))).toBe(true);
|
||||
|
||||
// Now delete — must succeed (not 403)
|
||||
const delRes = await request(app).delete('/api/pieces/deletable-noauth');
|
||||
expect(delRes.status).toBe(200);
|
||||
expect(delRes.body.ok).toBe(true);
|
||||
expect(existsSync(join(userPiecesRootDir, 'local', 'pieces', 'deletable-noauth.yaml'))).toBe(false);
|
||||
});
|
||||
|
||||
it('bundled built-in piece is still non-deletable (403) for no-auth callers', async () => {
|
||||
const app = makeNoAuthApp();
|
||||
const res = await request(app).delete('/api/pieces/general');
|
||||
expect(res.status).toBe(403);
|
||||
expect(res.body.error).toMatch(/cannot delete a built-in/i);
|
||||
expect(existsSync(join(piecesDir, 'general.yaml'))).toBe(true);
|
||||
});
|
||||
|
||||
it('no-auth LIST includes the local user-custom piece (source=user-custom)', async () => {
|
||||
// Pre-create a piece in the 'local' user-custom dir
|
||||
mkdirSync(join(userPiecesRootDir, 'local', 'pieces'), { recursive: true });
|
||||
writeFileSync(
|
||||
join(userPiecesRootDir, 'local', 'pieces', 'local-custom.yaml'),
|
||||
makeMinimalPieceYaml('local-custom', 'no-auth local piece'),
|
||||
);
|
||||
|
||||
const app = makeNoAuthApp();
|
||||
const res = await request(app).get('/api/pieces');
|
||||
expect(res.status).toBe(200);
|
||||
const byName = Object.fromEntries(res.body.pieces.map((p: any) => [p.name, p]));
|
||||
expect(byName['local-custom']).toBeDefined();
|
||||
expect(byName['local-custom'].source).toBe('user-custom');
|
||||
expect(byName['local-custom'].ownerId).toBe('local');
|
||||
// Built-in also visible
|
||||
expect(byName['general']).toBeDefined();
|
||||
expect(byName['general'].source).toBe('builtin');
|
||||
});
|
||||
|
||||
it('no-auth GET resolves user-custom piece from local dir by priority', async () => {
|
||||
mkdirSync(join(userPiecesRootDir, 'local', 'pieces'), { recursive: true });
|
||||
writeFileSync(
|
||||
join(userPiecesRootDir, 'local', 'pieces', 'my-piece.yaml'),
|
||||
makeMinimalPieceYaml('my-piece', 'local custom'),
|
||||
);
|
||||
|
||||
const app = makeNoAuthApp();
|
||||
const res = await request(app).get('/api/pieces/my-piece');
|
||||
expect(res.status).toBe(200);
|
||||
expect(res.body.source).toBe('user-custom');
|
||||
expect(res.body.ownerId).toBe('local');
|
||||
});
|
||||
});
|
||||
|
||||
+183
-47
@@ -100,6 +100,19 @@ function validateName(name: string): boolean {
|
||||
return VALID_PIECE_NAME.test(name);
|
||||
}
|
||||
|
||||
const VALID_SOURCES = new Set<string>(['builtin', 'user-custom', 'global-custom']);
|
||||
|
||||
/**
|
||||
* Returns true when `source` is absent (caller wants priority resolution).
|
||||
* Returns false when `source` is a known valid value.
|
||||
* Returns a 400 error string when `source` is present but unrecognised.
|
||||
*/
|
||||
function parseSourceParam(source: string | undefined): { valid: true; value: PieceSource | undefined } | { valid: false; error: string } {
|
||||
if (source === undefined) return { valid: true, value: undefined };
|
||||
if (VALID_SOURCES.has(source)) return { valid: true, value: source as PieceSource };
|
||||
return { valid: false, error: 'Invalid source' };
|
||||
}
|
||||
|
||||
export function findPieceFile(name: string, piecesDir: string, customPiecesDir?: string): { path: string; custom: boolean } | null {
|
||||
if (customPiecesDir) {
|
||||
const customPath = join(customPiecesDir, `${name}.yaml`);
|
||||
@@ -122,6 +135,9 @@ export interface PiecesApiOptions {
|
||||
userPiecesRootDir?: string;
|
||||
}
|
||||
|
||||
/** Canonical owner id for no-auth / legacy mode. Mirrors worker-bootstrap and piece-catalog default. */
|
||||
const LOCAL_OWNER = 'local';
|
||||
|
||||
type AuthedUser = { id: string; role?: string };
|
||||
|
||||
function getUser(req: Request): AuthedUser | undefined {
|
||||
@@ -137,6 +153,7 @@ function isAdminOrLegacy(user: AuthedUser | undefined): boolean {
|
||||
/**
|
||||
* Lookup priority for a given caller:
|
||||
* 1. Caller's own user-custom dir (overrides everything below).
|
||||
* No-auth callers use the 'local' owner id.
|
||||
* 2. Global custom dir (admin-managed, all users see).
|
||||
* 3. Built-in dir.
|
||||
*/
|
||||
@@ -145,9 +162,10 @@ function findPieceForCaller(
|
||||
user: AuthedUser | undefined,
|
||||
name: string,
|
||||
): { path: string; source: PieceSource; ownerId?: string } | null {
|
||||
if (opts.userPiecesRootDir && user) {
|
||||
const ucPath = join(userPiecesDir(opts.userPiecesRootDir, user.id), `${name}.yaml`);
|
||||
if (existsSync(ucPath)) return { path: ucPath, source: 'user-custom', ownerId: user.id };
|
||||
if (opts.userPiecesRootDir) {
|
||||
const ownerId = user?.id ?? LOCAL_OWNER;
|
||||
const ucPath = join(userPiecesDir(opts.userPiecesRootDir, ownerId), `${name}.yaml`);
|
||||
if (existsSync(ucPath)) return { path: ucPath, source: 'user-custom', ownerId };
|
||||
}
|
||||
if (opts.customPiecesDir) {
|
||||
const gcPath = join(opts.customPiecesDir, `${name}.yaml`);
|
||||
@@ -178,30 +196,31 @@ export function mountPiecesApi(
|
||||
app.get('/api/pieces', (req: Request, res: Response) => {
|
||||
try {
|
||||
const user = getUser(req);
|
||||
const seen = new Set<string>();
|
||||
const pieces: PieceSummary[] = [];
|
||||
|
||||
// Order matters: user-custom overrides global-custom, which overrides built-in.
|
||||
const sources: Array<{ dir: string; source: PieceSource; ownerId?: string }> = [];
|
||||
if (opts.userPiecesRootDir && user) {
|
||||
const ucDir = userPiecesDir(opts.userPiecesRootDir, user.id);
|
||||
if (existsSync(ucDir)) sources.push({ dir: ucDir, source: 'user-custom', ownerId: user.id });
|
||||
// Build custom sources (user-custom first, then global-custom).
|
||||
// Within custom umbrella: dedup by name so user-custom wins over global-custom.
|
||||
// Built-ins are ALWAYS emitted separately — they are never hidden by a same-named custom.
|
||||
const customSources: Array<{ dir: string; source: PieceSource; ownerId?: string }> = [];
|
||||
if (opts.userPiecesRootDir) {
|
||||
// No-auth callers use the 'local' owner id, mirroring worker and piece-catalog defaults.
|
||||
const ownerId = user?.id ?? LOCAL_OWNER;
|
||||
const ucDir = userPiecesDir(opts.userPiecesRootDir, ownerId);
|
||||
if (existsSync(ucDir)) customSources.push({ dir: ucDir, source: 'user-custom', ownerId });
|
||||
}
|
||||
if (opts.customPiecesDir && existsSync(opts.customPiecesDir)) {
|
||||
sources.push({ dir: opts.customPiecesDir, source: 'global-custom' });
|
||||
}
|
||||
if (existsSync(opts.piecesDir)) {
|
||||
sources.push({ dir: opts.piecesDir, source: 'builtin' });
|
||||
customSources.push({ dir: opts.customPiecesDir, source: 'global-custom' });
|
||||
}
|
||||
|
||||
for (const { dir, source, ownerId } of sources) {
|
||||
// Emit custom pieces (dedup within custom umbrella only).
|
||||
const seenCustom = new Set<string>();
|
||||
for (const { dir, source, ownerId } of customSources) {
|
||||
for (const f of listPieceFiles(dir)) {
|
||||
try {
|
||||
const p = loadPieceFile(f);
|
||||
const name = p.name ?? f.replace(/.*\//, '').replace('.yaml', '');
|
||||
if (seen.has(name)) continue;
|
||||
seen.add(name);
|
||||
// Drift is meaningful only for global-custom that shadows a built-in.
|
||||
if (seenCustom.has(name)) continue;
|
||||
seenCustom.add(name);
|
||||
let drift: DriftStatus | undefined;
|
||||
if (source === 'global-custom' && existsSync(opts.piecesDir)) {
|
||||
const builtinPath = join(opts.piecesDir, `${name}.yaml`);
|
||||
@@ -212,7 +231,7 @@ export function mountPiecesApi(
|
||||
description: p.description,
|
||||
triggers: p.triggers,
|
||||
requiredMcp: Array.isArray(p.required_mcp) ? p.required_mcp.filter((v: unknown): v is string => typeof v === 'string') : undefined,
|
||||
custom: source !== 'builtin',
|
||||
custom: true,
|
||||
source,
|
||||
ownerId,
|
||||
drift,
|
||||
@@ -222,6 +241,27 @@ export function mountPiecesApi(
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Always emit ALL built-ins (never hidden by custom pieces of the same name).
|
||||
if (existsSync(opts.piecesDir)) {
|
||||
for (const f of listPieceFiles(opts.piecesDir)) {
|
||||
try {
|
||||
const p = loadPieceFile(f);
|
||||
const name = p.name ?? f.replace(/.*\//, '').replace('.yaml', '');
|
||||
pieces.push({
|
||||
name,
|
||||
description: p.description,
|
||||
triggers: p.triggers,
|
||||
requiredMcp: Array.isArray(p.required_mcp) ? p.required_mcp.filter((v: unknown): v is string => typeof v === 'string') : undefined,
|
||||
custom: false,
|
||||
source: 'builtin',
|
||||
});
|
||||
} catch {
|
||||
// skip malformed piece files
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
res.json({ pieces });
|
||||
} catch (e) {
|
||||
res.status(500).json({ error: `Failed to list pieces: ${e}` });
|
||||
@@ -232,7 +272,31 @@ export function mountPiecesApi(
|
||||
if (!validateName(req.params.name)) { res.status(400).json({ error: 'Invalid piece name' }); return; }
|
||||
try {
|
||||
const user = getUser(req);
|
||||
const found = findPieceForCaller(opts, user, req.params.name);
|
||||
const sourceParsed = parseSourceParam(req.query.source as string | undefined);
|
||||
if (!sourceParsed.valid) { res.status(400).json({ ok: false, error: sourceParsed.error }); return; }
|
||||
const requestedSource = sourceParsed.value;
|
||||
|
||||
let found: { path: string; source: PieceSource; ownerId?: string } | null = null;
|
||||
|
||||
if (requestedSource === 'builtin') {
|
||||
const biPath = join(opts.piecesDir, `${req.params.name}.yaml`);
|
||||
if (existsSync(biPath)) found = { path: biPath, source: 'builtin' };
|
||||
} else if (requestedSource === 'user-custom') {
|
||||
if (opts.userPiecesRootDir) {
|
||||
const ownerId = user?.id ?? LOCAL_OWNER;
|
||||
const ucPath = join(userPiecesDir(opts.userPiecesRootDir, ownerId), `${req.params.name}.yaml`);
|
||||
if (existsSync(ucPath)) found = { path: ucPath, source: 'user-custom', ownerId };
|
||||
}
|
||||
} else if (requestedSource === 'global-custom') {
|
||||
if (opts.customPiecesDir) {
|
||||
const gcPath = join(opts.customPiecesDir, `${req.params.name}.yaml`);
|
||||
if (existsSync(gcPath)) found = { path: gcPath, source: 'global-custom' };
|
||||
}
|
||||
} else {
|
||||
// No source param: priority resolution (user-custom > global-custom > builtin).
|
||||
found = findPieceForCaller(opts, user, req.params.name);
|
||||
}
|
||||
|
||||
if (!found) { res.status(404).json({ error: 'Piece not found' }); return; }
|
||||
const piece = loadPieceFile(found.path);
|
||||
res.json({
|
||||
@@ -253,7 +317,29 @@ export function mountPiecesApi(
|
||||
if (!validateName(req.params.name)) { res.status(400).json({ error: 'Invalid piece name' }); return; }
|
||||
try {
|
||||
const user = getUser(req);
|
||||
const found = findPieceForCaller(opts, user, req.params.name);
|
||||
const sourceParsed = parseSourceParam(req.query.source as string | undefined);
|
||||
if (!sourceParsed.valid) { res.status(400).json({ ok: false, error: sourceParsed.error }); return; }
|
||||
const requestedSource = sourceParsed.value;
|
||||
|
||||
let found: { path: string; source: PieceSource; ownerId?: string } | null = null;
|
||||
if (requestedSource === 'builtin') {
|
||||
const biPath = join(opts.piecesDir, `${req.params.name}.yaml`);
|
||||
if (existsSync(biPath)) found = { path: biPath, source: 'builtin' };
|
||||
} else if (requestedSource === 'user-custom') {
|
||||
if (opts.userPiecesRootDir) {
|
||||
const ownerId = user?.id ?? LOCAL_OWNER;
|
||||
const ucPath = join(userPiecesDir(opts.userPiecesRootDir, ownerId), `${req.params.name}.yaml`);
|
||||
if (existsSync(ucPath)) found = { path: ucPath, source: 'user-custom', ownerId };
|
||||
}
|
||||
} else if (requestedSource === 'global-custom') {
|
||||
if (opts.customPiecesDir) {
|
||||
const gcPath = join(opts.customPiecesDir, `${req.params.name}.yaml`);
|
||||
if (existsSync(gcPath)) found = { path: gcPath, source: 'global-custom' };
|
||||
}
|
||||
} else {
|
||||
found = findPieceForCaller(opts, user, req.params.name);
|
||||
}
|
||||
|
||||
if (!found) { res.status(404).json({ error: 'Piece not found' }); return; }
|
||||
|
||||
// Authz: built-in / global-custom → admin (or legacy no-auth); user-custom → owner (or admin).
|
||||
@@ -262,7 +348,7 @@ export function mountPiecesApi(
|
||||
res.status(403).json({ ok: false, error: 'Only admins can modify built-in or global-custom pieces' });
|
||||
return;
|
||||
}
|
||||
} else if (found.ownerId !== user?.id && !isAdminOrLegacy(user)) {
|
||||
} else if (found.ownerId !== (user?.id ?? LOCAL_OWNER) && !isAdminOrLegacy(user)) {
|
||||
// Different user's user-custom — and not admin. Should be unreachable since
|
||||
// findPieceForCaller scopes user-custom to the caller, but guard anyway.
|
||||
res.status(403).json({ ok: false, error: "Cannot modify another user's custom piece" });
|
||||
@@ -293,33 +379,55 @@ export function mountPiecesApi(
|
||||
if (error) { res.status(400).json({ ok: false, error }); return; }
|
||||
|
||||
const user = getUser(req);
|
||||
const adminOrLegacy = isAdminOrLegacy(user);
|
||||
|
||||
// Determine destination dir:
|
||||
// - admin / legacy → preserve existing behavior (write to piecesDir).
|
||||
// - non-admin user → write to their user-custom dir.
|
||||
// POST always creates in user-custom dir. "+" is always Create Custom.
|
||||
// Admins edit built-ins via PUT on existing ones, not by POST-creating new built-ins.
|
||||
// No-auth / legacy with userPiecesRootDir: use the 'local' owner id so pieces are
|
||||
// user-custom (deletable) and never pollute piecesDir.
|
||||
// No-auth / legacy WITHOUT userPiecesRootDir: fall back to global customPiecesDir or piecesDir.
|
||||
let destDir: string;
|
||||
if (adminOrLegacy) {
|
||||
destDir = opts.piecesDir;
|
||||
} else {
|
||||
if (!opts.userPiecesRootDir) {
|
||||
res.status(503).json({ ok: false, error: 'User pieces directory not configured on this server' });
|
||||
return;
|
||||
}
|
||||
destDir = userPiecesDir(opts.userPiecesRootDir, user!.id);
|
||||
let createdSource: PieceSource;
|
||||
if (opts.userPiecesRootDir) {
|
||||
// Both authenticated and no-auth go to user-custom dir when userPiecesRootDir is set.
|
||||
// Authenticated users: 503 guard below is only needed when root is NOT set.
|
||||
const ownerId = user?.id ?? LOCAL_OWNER;
|
||||
destDir = userPiecesDir(opts.userPiecesRootDir, ownerId);
|
||||
mkdirSync(destDir, { recursive: true });
|
||||
createdSource = 'user-custom';
|
||||
} else if (user) {
|
||||
// Authenticated caller but userPiecesRootDir is not configured — cannot safely write.
|
||||
res.status(503).json({ ok: false, error: 'User piece storage not configured' });
|
||||
return;
|
||||
} else if (opts.customPiecesDir) {
|
||||
// Legacy (no-auth) with global custom dir configured.
|
||||
destDir = opts.customPiecesDir;
|
||||
mkdirSync(destDir, { recursive: true });
|
||||
createdSource = 'global-custom';
|
||||
} else {
|
||||
// Pure legacy single-user: fall back to piecesDir (existing behavior).
|
||||
destDir = opts.piecesDir;
|
||||
createdSource = 'builtin';
|
||||
}
|
||||
|
||||
// Reject if any visible-to-caller piece with this name already exists
|
||||
// (built-in, global-custom, or caller's user-custom).
|
||||
// Always reject if the name collides with a built-in — Custom and Default are
|
||||
// separate namespaces. (This also covers the admin case, since admins should
|
||||
// use PUT to update an existing built-in, not POST to create a duplicate.)
|
||||
const builtinPath = join(opts.piecesDir, `${req.body.name}.yaml`);
|
||||
if (existsSync(builtinPath) && destDir !== opts.piecesDir) {
|
||||
res.status(409).json({ ok: false, error: `"${req.body.name}" は組み込み (built-in) Piece と同名です。Custom Piece には別名を付けてください。` });
|
||||
return;
|
||||
}
|
||||
|
||||
// Reject if the caller's visible piece with this name already exists
|
||||
// (global-custom or caller's own user-custom).
|
||||
if (findPieceForCaller(opts, user, req.body.name)) {
|
||||
res.status(409).json({ ok: false, error: 'Piece already exists' }); return;
|
||||
}
|
||||
|
||||
const filePath = join(destDir, `${req.body.name}.yaml`);
|
||||
writeFileSync(filePath, stringify(req.body, { lineWidth: 120 }), 'utf-8');
|
||||
logger.info(`[pieces-api] created piece=${req.body.name} dest=${destDir} actor=${user?.id ?? 'legacy'}`);
|
||||
res.status(201).json({ ok: true });
|
||||
logger.info(`[pieces-api] created piece=${req.body.name} dest=${destDir} source=${createdSource} actor=${user?.id ?? 'legacy'}`);
|
||||
res.status(201).json({ ok: true, source: createdSource });
|
||||
} catch (e) {
|
||||
res.status(500).json({ error: `Failed to create piece: ${e}` });
|
||||
}
|
||||
@@ -327,23 +435,51 @@ export function mountPiecesApi(
|
||||
|
||||
app.delete('/api/pieces/:name', (req: Request, res: Response) => {
|
||||
if (!validateName(req.params.name)) { res.status(400).json({ error: 'Invalid piece name' }); return; }
|
||||
if (req.params.name === 'general' || req.params.name === 'chat') {
|
||||
res.status(403).json({ ok: false, error: 'Cannot delete general piece' }); return;
|
||||
}
|
||||
try {
|
||||
const user = getUser(req);
|
||||
const found = findPieceForCaller(opts, user, req.params.name);
|
||||
const sourceParsed = parseSourceParam(req.query.source as string | undefined);
|
||||
if (!sourceParsed.valid) { res.status(400).json({ ok: false, error: sourceParsed.error }); return; }
|
||||
const requestedSource = sourceParsed.value;
|
||||
|
||||
let found: { path: string; source: PieceSource; ownerId?: string } | null = null;
|
||||
if (requestedSource === 'builtin') {
|
||||
const biPath = join(opts.piecesDir, `${req.params.name}.yaml`);
|
||||
if (existsSync(biPath)) found = { path: biPath, source: 'builtin' };
|
||||
} else if (requestedSource === 'user-custom') {
|
||||
if (opts.userPiecesRootDir) {
|
||||
const ownerId = user?.id ?? LOCAL_OWNER;
|
||||
const ucPath = join(userPiecesDir(opts.userPiecesRootDir, ownerId), `${req.params.name}.yaml`);
|
||||
if (existsSync(ucPath)) found = { path: ucPath, source: 'user-custom', ownerId };
|
||||
}
|
||||
} else if (requestedSource === 'global-custom') {
|
||||
if (opts.customPiecesDir) {
|
||||
const gcPath = join(opts.customPiecesDir, `${req.params.name}.yaml`);
|
||||
if (existsSync(gcPath)) found = { path: gcPath, source: 'global-custom' };
|
||||
}
|
||||
} else {
|
||||
found = findPieceForCaller(opts, user, req.params.name);
|
||||
}
|
||||
|
||||
if (!found) { res.status(404).json({ error: 'Piece not found' }); return; }
|
||||
|
||||
// Authz mirrors PUT: built-in / global-custom → admin; user-custom → owner.
|
||||
if (found.source !== 'user-custom') {
|
||||
// Built-in (Default) pieces are non-deletable for everyone — including admins.
|
||||
// This covers general/chat and all other built-ins. Admins may still EDIT
|
||||
// built-ins via PUT; only deletion is prohibited.
|
||||
if (found.source === 'builtin') {
|
||||
res.status(403).json({ ok: false, error: 'Cannot delete a built-in (Default) piece' }); return;
|
||||
}
|
||||
|
||||
// Authz for non-builtin sources: global-custom → admin; user-custom → owner.
|
||||
if (found.source === 'global-custom') {
|
||||
if (!isAdminOrLegacy(user)) {
|
||||
res.status(403).json({ ok: false, error: 'Only admins can delete built-in or global-custom pieces' });
|
||||
res.status(403).json({ ok: false, error: 'Only admins can delete global-custom pieces' });
|
||||
return;
|
||||
}
|
||||
} else if (found.source === 'user-custom') {
|
||||
if (found.ownerId !== (user?.id ?? LOCAL_OWNER) && !isAdminOrLegacy(user)) {
|
||||
res.status(403).json({ ok: false, error: "Cannot delete another user's custom piece" });
|
||||
return;
|
||||
}
|
||||
} else if (found.ownerId !== user?.id && !isAdminOrLegacy(user)) {
|
||||
res.status(403).json({ ok: false, error: "Cannot delete another user's custom piece" });
|
||||
return;
|
||||
}
|
||||
|
||||
unlinkSync(found.path);
|
||||
|
||||
+11
-1
@@ -43,6 +43,7 @@ import { createWorkerMetrics, type WorkerMetrics } from '../metrics/worker-metri
|
||||
import { createMetricsHandler } from '../metrics/http-handler.js';
|
||||
import { buildDirectProbe, buildProxyProbe } from '../engine/backend-probes.js';
|
||||
import { startTrashCleanup } from '../user-folder/trash-cleanup.js';
|
||||
import { userPiecesDir } from '../user-folder/paths.js';
|
||||
import { startReflectionRetentionSweep } from '../engine/reflection/retention.js';
|
||||
import type { AuthConfig } from '../config.js';
|
||||
import { isKeyConfigured } from '../mcp/crypto.js';
|
||||
@@ -777,7 +778,16 @@ export function createCoreServer(opts: CoreServerOptions): {
|
||||
generateTitle: opts.generateTitle,
|
||||
selectPiece: opts.selectPiece,
|
||||
pieceExists: opts.piecesDir
|
||||
? (name: string) => findPieceFile(name, opts.piecesDir!, opts.customPiecesDir) !== null
|
||||
? (name: string, ownerId?: string) => {
|
||||
// Mirror the worker's per-user → global-custom → builtin resolution order.
|
||||
// 1. Owner's per-user dir (matches what worker uses at job run time).
|
||||
// No-auth tasks (ownerId null) fall back to 'local', mirroring the worker.
|
||||
const ufl = loadConfig().userFolderRoot ?? './data/users';
|
||||
const ownerForPieces = ownerId ?? 'local';
|
||||
if (existsSync(join(userPiecesDir(ufl, ownerForPieces), `${name}.yaml`))) return true;
|
||||
// 2. Global-custom + builtin via existing helper.
|
||||
return findPieceFile(name, opts.piecesDir!, opts.customPiecesDir) !== null;
|
||||
}
|
||||
: undefined,
|
||||
sessRepo,
|
||||
getMaxUploadMb: opts.configManager
|
||||
|
||||
@@ -396,9 +396,9 @@ movements:
|
||||
expect(() => loadPiece('multi', 'pieces', tempDir)).not.toThrow();
|
||||
});
|
||||
|
||||
it('all 13 bundled pieces load without validation errors', () => {
|
||||
it('all 12 bundled pieces load without validation errors', () => {
|
||||
const piecesDir = join(process.cwd(), 'pieces');
|
||||
const names = ['brainstorming', 'chat', 'data-process', 'game-tweet-generator', 'general',
|
||||
const names = ['brainstorming', 'chat', 'data-process', 'general',
|
||||
'office-process', 'piece-builder', 'research', 'slide', 'sns-research',
|
||||
'ssh-console', 'ssh-ops', 'x-ai-digest'];
|
||||
for (const name of names) {
|
||||
@@ -1111,3 +1111,50 @@ describe('allowed_ssh_connections validation (Phase 4)', () => {
|
||||
expect(() => validatePieceDef(piece)).toThrow(/Piece "ssh-test" has invalid allowed_ssh_connections/);
|
||||
});
|
||||
});
|
||||
|
||||
// --- Task 1: loadPiece multi-dir support ---
|
||||
describe('loadPiece multi-dir (string | string[])', () => {
|
||||
it('resolves from a list of custom dirs (per-user wins over builtin name miss)', () => {
|
||||
const dirA = mkdtempSync(join(tmpdir(), 'pa-')); // empty
|
||||
const dirB = mkdtempSync(join(tmpdir(), 'pb-'));
|
||||
writeFileSync(
|
||||
join(dirB, 'mycustom.yaml'),
|
||||
`name: mycustom\ndescription: d\nmax_movements: 1\ninitial_movement: go\nmovements:\n - name: go\n edit: false\n persona: w\n instruction: x\n allowed_tools: []\n rules: []\n default_next: COMPLETE\n`,
|
||||
);
|
||||
// array form: searches dirA then dirB then builtin
|
||||
const p = loadPiece('mycustom', 'pieces', [dirA, dirB]);
|
||||
expect(p.name).toBe('mycustom');
|
||||
// builtin still resolvable when not in any custom dir
|
||||
expect(() => loadPiece('chat', 'pieces', [dirA, dirB])).not.toThrow();
|
||||
rmSync(dirA, { recursive: true });
|
||||
rmSync(dirB, { recursive: true });
|
||||
});
|
||||
|
||||
it('first dir wins when same name appears in two custom dirs', () => {
|
||||
const dirA = mkdtempSync(join(tmpdir(), 'pa-'));
|
||||
const dirB = mkdtempSync(join(tmpdir(), 'pb-'));
|
||||
writeFileSync(
|
||||
join(dirA, 'dup.yaml'),
|
||||
`name: dup\ndescription: from-a\nmax_movements: 1\ninitial_movement: go\nmovements:\n - name: go\n edit: false\n persona: w\n instruction: x\n allowed_tools: []\n rules: []\n default_next: COMPLETE\n`,
|
||||
);
|
||||
writeFileSync(
|
||||
join(dirB, 'dup.yaml'),
|
||||
`name: dup\ndescription: from-b\nmax_movements: 1\ninitial_movement: go\nmovements:\n - name: go\n edit: false\n persona: w\n instruction: x\n allowed_tools: []\n rules: []\n default_next: COMPLETE\n`,
|
||||
);
|
||||
const p = loadPiece('dup', 'pieces', [dirA, dirB]);
|
||||
expect(p.description).toBe('from-a');
|
||||
rmSync(dirA, { recursive: true });
|
||||
rmSync(dirB, { recursive: true });
|
||||
});
|
||||
|
||||
it('string form still works (backward compat)', () => {
|
||||
const dir = mkdtempSync(join(tmpdir(), 'pc-'));
|
||||
writeFileSync(
|
||||
join(dir, 'strcompat.yaml'),
|
||||
`name: strcompat\ndescription: str\nmax_movements: 1\ninitial_movement: go\nmovements:\n - name: go\n edit: false\n persona: w\n instruction: x\n allowed_tools: []\n rules: []\n default_next: COMPLETE\n`,
|
||||
);
|
||||
const p = loadPiece('strcompat', 'pieces', dir);
|
||||
expect(p.name).toBe('strcompat');
|
||||
rmSync(dir, { recursive: true });
|
||||
});
|
||||
});
|
||||
|
||||
+19
-22
@@ -209,29 +209,22 @@ export function validateAllowedSshConnections(piece: PieceDef): string[] {
|
||||
}
|
||||
|
||||
// pieces/ ディレクトリから piece 定義を読み込む（customPiecesDir を優先探索）
|
||||
export function loadPiece(pieceName: string, piecesDir: string = 'pieces', customPiecesDir?: string): PieceDef {
|
||||
// customPiecesDir は string | string[] を受け付ける（配列の場合は先頭から順に探索）
|
||||
export function loadPiece(pieceName: string, piecesDir: string = 'pieces', customPiecesDir?: string | string[]): PieceDef {
|
||||
let raw: string;
|
||||
let source: string;
|
||||
if (customPiecesDir) {
|
||||
const customPath = join(customPiecesDir, `${pieceName}.yaml`);
|
||||
if (existsSync(customPath)) {
|
||||
logger.debug(`[piece-runner] loadPiece piece=${pieceName} source=custom path=${customPath}`);
|
||||
raw = readFileSync(customPath, 'utf-8');
|
||||
source = 'custom';
|
||||
} else {
|
||||
const filePath = join(piecesDir, `${pieceName}.yaml`);
|
||||
if (!existsSync(filePath)) {
|
||||
logger.warn(`[piece-runner] loadPiece piece=${pieceName} not found dirs=[${customPiecesDir}, ${piecesDir}]`);
|
||||
throw new Error(`Piece not found: ${pieceName}`);
|
||||
}
|
||||
logger.debug(`[piece-runner] loadPiece piece=${pieceName} source=builtin path=${filePath}`);
|
||||
raw = readFileSync(filePath, 'utf-8');
|
||||
source = 'builtin';
|
||||
}
|
||||
const customDirs = customPiecesDir == null ? [] : (Array.isArray(customPiecesDir) ? customPiecesDir : [customPiecesDir]);
|
||||
const found = customDirs
|
||||
.map((d) => join(d, `${pieceName}.yaml`))
|
||||
.find((p) => existsSync(p));
|
||||
if (found) {
|
||||
logger.debug(`[piece-runner] loadPiece piece=${pieceName} source=custom path=${found}`);
|
||||
raw = readFileSync(found, 'utf-8');
|
||||
source = 'custom';
|
||||
} else {
|
||||
const filePath = join(piecesDir, `${pieceName}.yaml`);
|
||||
if (!existsSync(filePath)) {
|
||||
logger.warn(`[piece-runner] loadPiece piece=${pieceName} not found dirs=[${piecesDir}]`);
|
||||
logger.warn(`[piece-runner] loadPiece piece=${pieceName} not found dirs=[${[...customDirs, piecesDir].join(', ')}]`);
|
||||
throw new Error(`Piece not found: ${pieceName}`);
|
||||
}
|
||||
logger.debug(`[piece-runner] loadPiece piece=${pieceName} source=builtin path=${filePath}`);
|
||||
@@ -252,11 +245,12 @@ export function loadPiece(pieceName: string, piecesDir: string = 'pieces', custo
|
||||
/**
|
||||
* pieces/ ディレクトリ内の全 Piece から triggers を読み込む（customPiecesDir があれば優先、同名は custom が勝つ）
|
||||
*/
|
||||
export function loadAllPieceTriggers(piecesDir: string = 'pieces', customPiecesDir?: string): Array<{ name: string; keywords: string[] }> {
|
||||
export function loadAllPieceTriggers(piecesDir: string = 'pieces', customPiecesDir?: string | string[]): Array<{ name: string; keywords: string[] }> {
|
||||
const triggers: Array<{ name: string; keywords: string[] }> = [];
|
||||
const seen = new Set<string>();
|
||||
|
||||
const dirs = customPiecesDir ? [customPiecesDir, piecesDir] : [piecesDir];
|
||||
const customDirsArr = Array.isArray(customPiecesDir) ? customPiecesDir : (customPiecesDir ? [customPiecesDir] : []);
|
||||
const dirs = [...customDirsArr, piecesDir];
|
||||
logger.info(`[piece-runner] loadAllPieceTriggers scanning dirs=[${dirs.join(', ')}]`);
|
||||
for (const dir of dirs) {
|
||||
if (!existsSync(dir)) {
|
||||
@@ -299,7 +293,7 @@ export async function runPiece(
|
||||
abortController?: AbortController;
|
||||
safetyConfig?: { maxIterations?: number; maxRevisits?: number; bashUnrestricted?: boolean; bashSandbox?: 'auto' | 'always' | 'off' };
|
||||
searchFilter?: SearchFilterConfig;
|
||||
customPiecesDir?: string;
|
||||
customPiecesDir?: string | string[];
|
||||
contextManager?: ContextManager;
|
||||
vlmEnabled?: boolean;
|
||||
/** Phase 5: parent's job id, used to populate MemoryHandoff.parentJobId
|
||||
@@ -657,7 +651,7 @@ function prepareMovementContext(
|
||||
options?: {
|
||||
spawnSubTask?: (params: { title: string; instruction: string; piece?: string }) => Promise<{ jobId: string; subtaskIndex: number; workspacePath: string }>;
|
||||
searchFilter?: SearchFilterConfig;
|
||||
customPiecesDir?: string;
|
||||
customPiecesDir?: string | string[];
|
||||
vlmEnabled?: boolean;
|
||||
/** Traceability T-1: per-run event logger threaded into ToolContext. */
|
||||
eventLogger?: EventLogger;
|
||||
@@ -724,6 +718,9 @@ function prepareMovementContext(
|
||||
toolsConfig,
|
||||
eventLogger: options?.eventLogger,
|
||||
searchFilter: options?.searchFilter,
|
||||
// Pass the full array so in-agent reads (ListPieces/GetPiece) can see ALL
|
||||
// custom dirs (per-user + global-custom). CreatePiece writes to dirs[0]
|
||||
// (the per-user dir) — handled inside pieces.ts.
|
||||
customPiecesDir: options?.customPiecesDir,
|
||||
spawnSubTask: options?.spawnSubTask,
|
||||
missionBrief: options?.missionBrief,
|
||||
|
||||
@@ -0,0 +1,142 @@
|
||||
import { describe, expect, it } from 'vitest';
|
||||
import ExcelJS from 'exceljs';
|
||||
import { mkdtempSync } from 'fs';
|
||||
import { tmpdir } from 'os';
|
||||
import { join } from 'path';
|
||||
import { resolveThemePalette, applyTint, extractSheetStyles } from './excel-styles.js';
|
||||
|
||||
async function roundTrip(build: (ws: ExcelJS.Worksheet) => void): Promise<ExcelJS.Worksheet> {
|
||||
const wb = new ExcelJS.Workbook(); const ws = wb.addWorksheet('S'); build(ws);
|
||||
const p = join(mkdtempSync(join(tmpdir(), 'xls-')), 't.xlsx');
|
||||
await wb.xlsx.writeFile(p);
|
||||
const wb2 = new ExcelJS.Workbook(); await wb2.xlsx.readFile(p);
|
||||
return wb2.getWorksheet('S')!;
|
||||
}
|
||||
|
||||
describe('resolveThemePalette', () => {
|
||||
it('parses srgbClr + sysClr and applies the lt/dk swap', () => {
|
||||
const xml = `<a:theme><a:themeElements><a:clrScheme name="x">
|
||||
<a:dk1><a:sysClr val="windowText" lastClr="000000"/></a:dk1>
|
||||
<a:lt1><a:sysClr val="window" lastClr="FFFFFF"/></a:lt1>
|
||||
<a:dk2><a:srgbClr val="44546A"/></a:dk2>
|
||||
<a:lt2><a:srgbClr val="E7E6E6"/></a:lt2>
|
||||
<a:accent1><a:srgbClr val="4472C4"/></a:accent1>
|
||||
<a:accent2><a:srgbClr val="ED7D31"/></a:accent2>
|
||||
<a:accent3><a:srgbClr val="A5A5A5"/></a:accent3>
|
||||
<a:accent4><a:srgbClr val="FFC000"/></a:accent4>
|
||||
<a:accent5><a:srgbClr val="5B9BD5"/></a:accent5>
|
||||
<a:accent6><a:srgbClr val="70AD47"/></a:accent6>
|
||||
<a:hlink><a:srgbClr val="0563C1"/></a:hlink>
|
||||
<a:folHlink><a:srgbClr val="954F72"/></a:folHlink>
|
||||
</a:clrScheme></a:themeElements></a:theme>`;
|
||||
const pal = resolveThemePalette(xml);
|
||||
expect(pal[0]).toBe('#FFFFFF'); // theme idx 0 = Light1 = lt1
|
||||
expect(pal[1]).toBe('#000000'); // theme idx 1 = Dark1 = dk1
|
||||
expect(pal[4]).toBe('#4472C4'); // accent1
|
||||
});
|
||||
it('returns [] on missing scheme', () => { expect(resolveThemePalette(undefined)).toEqual([]); });
|
||||
});
|
||||
|
||||
describe('applyTint', () => {
|
||||
it('lightens with positive tint and darkens with negative', () => {
|
||||
const lighter = applyTint('#4472C4', 0.4);
|
||||
const darker = applyTint('#4472C4', -0.4);
|
||||
expect(lighter).not.toBe('#4472C4');
|
||||
expect(darker).not.toBe('#4472C4');
|
||||
// positive tint raises luminance
|
||||
const lum = (h: string) => parseInt(h.slice(1,3),16)+parseInt(h.slice(3,5),16)+parseInt(h.slice(5,7),16);
|
||||
expect(lum(lighter)).toBeGreaterThan(lum('#4472C4'));
|
||||
expect(lum(darker)).toBeLessThan(lum('#4472C4'));
|
||||
});
|
||||
});
|
||||
|
||||
describe('extractSheetStyles', () => {
|
||||
it('dedups identical fills into one legend entry with a merged range', async () => {
|
||||
const ws = await roundTrip((ws) => {
|
||||
for (const a of ['A1','B1','C1','D1']) ws.getCell(a).fill = { type:'pattern', pattern:'solid', fgColor:{ argb:'FFFFF2CC' } };
|
||||
});
|
||||
const r = extractSheetStyles(ws, [], 250);
|
||||
expect(r.legend.length).toBe(1);
|
||||
expect(r.legend[0].sig).toContain('#FFF2CC');
|
||||
expect(r.assignments[0].ranges).toEqual(['A1:D1']); // horizontal run merged
|
||||
});
|
||||
it('captures a decorated but EMPTY cell', async () => {
|
||||
const ws = await roundTrip((ws) => { ws.getCell('B5').fill = { type:'pattern', pattern:'solid', fgColor:{ argb:'FFFF0000' } }; });
|
||||
const r = extractSheetStyles(ws, [], 250);
|
||||
expect(r.assignments.some(a => a.ranges.includes('B5'))).toBe(true);
|
||||
});
|
||||
it('reports font bold + color', async () => {
|
||||
const ws = await roundTrip((ws) => { ws.getCell('A1').value='x'; ws.getCell('A1').font={ bold:true, color:{ argb:'FF9C0006' } }; });
|
||||
const r = extractSheetStyles(ws, [], 250);
|
||||
expect(r.legend[0].sig).toMatch(/font:.*bold/);
|
||||
expect(r.legend[0].sig).toContain('#9C0006');
|
||||
});
|
||||
it('merges vertically-adjacent identical column-runs into a rectangle', async () => {
|
||||
const ws = await roundTrip((ws) => {
|
||||
for (let row=2; row<=20; row++) ws.getCell(`B${row}`).fill={ type:'pattern', pattern:'solid', fgColor:{ argb:'FF00FF00' } };
|
||||
});
|
||||
const r = extractSheetStyles(ws, [], 250);
|
||||
expect(r.assignments[0].ranges).toEqual(['B2:B20']);
|
||||
});
|
||||
it('respects max_style_ranges cap', async () => {
|
||||
const ws = await roundTrip((ws) => {
|
||||
// 5 disjoint single cells with distinct fills (no merge possible)
|
||||
const colors=['FFAA0000','FF00AA00','FF0000AA','FFAAAA00','FF00AAAA'];
|
||||
colors.forEach((c,i)=>{ ws.getCell(`A${i*2+1}`).fill={type:'pattern',pattern:'solid',fgColor:{argb:c}}; });
|
||||
});
|
||||
const r = extractSheetStyles(ws, [], 2);
|
||||
expect(r.truncated).toBe(true);
|
||||
});
|
||||
|
||||
// Fix 4 (M-1): merges
|
||||
it('includes merged cell range in result merges', async () => {
|
||||
const ws = await roundTrip((ws) => {
|
||||
ws.getCell('A1').value = 'merged';
|
||||
ws.mergeCells('A1:C2');
|
||||
});
|
||||
const r = extractSheetStyles(ws, [], 250);
|
||||
expect(r.merges).toContain('A1:C2');
|
||||
});
|
||||
|
||||
// Fix 4 (M-2): conditional formatting — round-trip keeps conditionalFormattings
|
||||
it('detects conditional formatting', async () => {
|
||||
const ws = await roundTrip((ws) => {
|
||||
ws.addConditionalFormatting({
|
||||
ref: 'A1:A10',
|
||||
rules: [{ type: 'expression', formulae: ['TRUE'], style: { fill: { type: 'pattern', pattern: 'solid', bgColor: { argb: 'FFFF0000' } } } }],
|
||||
});
|
||||
});
|
||||
// ExcelJS preserves conditionalFormattings after round-trip (empirically verified)
|
||||
const r = extractSheetStyles(ws, [], 250);
|
||||
expect(r.conditionalFormatting).toBe(true);
|
||||
});
|
||||
|
||||
// Fix 4 (M-3): border + numFmt + alignment
|
||||
it('captures border, numFmt, and alignment in cell signature', async () => {
|
||||
const ws = await roundTrip((ws) => {
|
||||
const cell = ws.getCell('B2');
|
||||
cell.value = 0.42;
|
||||
cell.border = { bottom: { style: 'thin' } };
|
||||
cell.numFmt = '0.00%';
|
||||
cell.alignment = { horizontal: 'center' };
|
||||
});
|
||||
const r = extractSheetStyles(ws, [], 250);
|
||||
expect(r.legend.length).toBeGreaterThan(0);
|
||||
const sig = r.legend[0]!.sig;
|
||||
expect(sig).toContain('border:');
|
||||
expect(sig).toContain('numFmt:"0.00%"');
|
||||
expect(sig).toContain('align:');
|
||||
});
|
||||
|
||||
// Fix 4 (I-1 range honoring): rangeBounds filters cells
|
||||
it('honors rangeBounds — includes A1 but excludes Z99', async () => {
|
||||
const ws = await roundTrip((ws) => {
|
||||
ws.getCell('A1').fill = { type: 'pattern', pattern: 'solid', fgColor: { argb: 'FF0000FF' } };
|
||||
ws.getCell('Z99').fill = { type: 'pattern', pattern: 'solid', fgColor: { argb: 'FF00FF00' } };
|
||||
});
|
||||
const r = extractSheetStyles(ws, [], 250, { minRow: 1, maxRow: 5, minCol: 1, maxCol: 5 });
|
||||
const allRanges = r.assignments.flatMap(a => a.ranges);
|
||||
expect(allRanges.some(rng => rng === 'A1' || rng.startsWith('A1:'))).toBe(true);
|
||||
expect(allRanges.every(rng => !rng.includes('Z99') && !rng.startsWith('Z'))).toBe(true);
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,515 @@
|
||||
/**
|
||||
* excel-styles.ts — Pure helper for ExcelJS style extraction.
|
||||
* No dependency on office.ts internals; only imports exceljs types + stdlib.
|
||||
*/
|
||||
|
||||
import type ExcelJS from 'exceljs';
|
||||
|
||||
// --- Public API ---
|
||||
|
||||
export interface SheetStyleResult {
|
||||
legend: Array<{ id: string; sig: string }>; // s1 = "fill:#FFF2CC;font:bold,#9C0006"
|
||||
assignments: Array<{ id: string; ranges: string[] }>; // s1: ['A1:D1','A10:D10']
|
||||
merges: string[]; // ['A1:D1']
|
||||
conditionalFormatting: boolean;
|
||||
truncated: boolean; // true if range count hit the cap
|
||||
}
|
||||
|
||||
/**
|
||||
* Parse <a:clrScheme> from a theme1 XML string into a theme-index→'#RRGGBB' map.
|
||||
* Returns [] if themeXml is missing/unparseable (caller then emits raw theme refs).
|
||||
*/
|
||||
export function resolveThemePalette(themeXml: string | undefined): string[] {
|
||||
if (!themeXml) return [];
|
||||
|
||||
// Extract the clrScheme block
|
||||
const schemeMatch = themeXml.match(/<a:clrScheme[^>]*>([\s\S]*?)<\/a:clrScheme>/);
|
||||
if (!schemeMatch) return [];
|
||||
const schemeXml = schemeMatch[1]!;
|
||||
|
||||
// The 12 entries in XML order: dk1, lt1, dk2, lt2, accent1-6, hlink, folHlink
|
||||
const tagNames = ['dk1', 'lt1', 'dk2', 'lt2', 'accent1', 'accent2', 'accent3', 'accent4', 'accent5', 'accent6', 'hlink', 'folHlink'];
|
||||
|
||||
const scheme: string[] = [];
|
||||
for (const tag of tagNames) {
|
||||
const tagMatch = schemeXml.match(new RegExp(`<a:${tag}>[\\s\\S]*?</a:${tag}>`));
|
||||
if (!tagMatch) {
|
||||
// Missing entry
|
||||
return [];
|
||||
}
|
||||
const block = tagMatch[0]!;
|
||||
|
||||
// Try srgbClr first
|
||||
const srgbMatch = block.match(/<a:srgbClr[^>]*val="([0-9A-Fa-f]{6})"/);
|
||||
if (srgbMatch) {
|
||||
scheme.push('#' + srgbMatch[1]!.toUpperCase());
|
||||
continue;
|
||||
}
|
||||
|
||||
// Try sysClr with lastClr
|
||||
const sysMatch = block.match(/<a:sysClr[^>]*lastClr="([0-9A-Fa-f]{6})"/);
|
||||
if (sysMatch) {
|
||||
scheme.push('#' + sysMatch[1]!.toUpperCase());
|
||||
continue;
|
||||
}
|
||||
|
||||
// Unrecognized
|
||||
return [];
|
||||
}
|
||||
|
||||
if (scheme.length < 12) return [];
|
||||
|
||||
// Apply the well-known lt/dk swap:
|
||||
// themeIndex 0=Light1=lt1=scheme[1], 1=Dark1=dk1=scheme[0],
|
||||
// 2=Light2=lt2=scheme[3], 3=Dark2=dk2=scheme[2],
|
||||
// 4-9=accent1-6=scheme[4..9], 10=hlink=scheme[10], 11=folHlink=scheme[11]
|
||||
return [
|
||||
scheme[1]!, // 0: Light1 (lt1)
|
||||
scheme[0]!, // 1: Dark1 (dk1)
|
||||
scheme[3]!, // 2: Light2 (lt2)
|
||||
scheme[2]!, // 3: Dark2 (dk2)
|
||||
scheme[4]!, // 4: accent1
|
||||
scheme[5]!, // 5: accent2
|
||||
scheme[6]!, // 6: accent3
|
||||
scheme[7]!, // 7: accent4
|
||||
scheme[8]!, // 8: accent5
|
||||
scheme[9]!, // 9: accent6
|
||||
scheme[10]!, // 10: hlink
|
||||
scheme[11]!, // 11: folHlink
|
||||
];
|
||||
}
|
||||
|
||||
/**
|
||||
* OOXML tint applied to a '#RRGGBB' hex. tint in [-1,1]. Best-effort (HSL lum).
|
||||
* if (tint < 0) L = L * (1 + tint); else L = L * (1 - tint) + tint
|
||||
*/
|
||||
export function applyTint(hexRgb: string, tint: number): string {
|
||||
if (tint === 0) return hexRgb;
|
||||
|
||||
// Parse hex to [0,1]
|
||||
const r = parseInt(hexRgb.slice(1, 3), 16) / 255;
|
||||
const g = parseInt(hexRgb.slice(3, 5), 16) / 255;
|
||||
const b = parseInt(hexRgb.slice(5, 7), 16) / 255;
|
||||
|
||||
// RGB to HSL
|
||||
const max = Math.max(r, g, b);
|
||||
const min = Math.min(r, g, b);
|
||||
let h = 0;
|
||||
let s = 0;
|
||||
let l = (max + min) / 2;
|
||||
|
||||
if (max !== min) {
|
||||
const d = max - min;
|
||||
s = l > 0.5 ? d / (2 - max - min) : d / (max + min);
|
||||
switch (max) {
|
||||
case r: h = ((g - b) / d + (g < b ? 6 : 0)) / 6; break;
|
||||
case g: h = ((b - r) / d + 2) / 6; break;
|
||||
case b: h = ((r - g) / d + 4) / 6; break;
|
||||
}
|
||||
}
|
||||
|
||||
// Apply tint to luminance
|
||||
if (tint < 0) {
|
||||
l = l * (1 + tint);
|
||||
} else {
|
||||
l = l * (1 - tint) + tint;
|
||||
}
|
||||
l = Math.max(0, Math.min(1, l));
|
||||
|
||||
// HSL back to RGB
|
||||
function hue2rgb(p: number, q: number, t: number): number {
|
||||
if (t < 0) t += 1;
|
||||
if (t > 1) t -= 1;
|
||||
if (t < 1/6) return p + (q - p) * 6 * t;
|
||||
if (t < 1/2) return q;
|
||||
if (t < 2/3) return p + (q - p) * (2/3 - t) * 6;
|
||||
return p;
|
||||
}
|
||||
|
||||
let rOut: number, gOut: number, bOut: number;
|
||||
if (s === 0) {
|
||||
rOut = gOut = bOut = l;
|
||||
} else {
|
||||
const q = l < 0.5 ? l * (1 + s) : l + s - l * s;
|
||||
const p = 2 * l - q;
|
||||
rOut = hue2rgb(p, q, h + 1/3);
|
||||
gOut = hue2rgb(p, q, h);
|
||||
bOut = hue2rgb(p, q, h - 1/3);
|
||||
}
|
||||
|
||||
const toHex = (v: number) => Math.round(v * 255).toString(16).padStart(2, '0').toUpperCase();
|
||||
return '#' + toHex(rOut) + toHex(gOut) + toHex(bOut);
|
||||
}
|
||||
|
||||
// --- Internal helpers ---
|
||||
|
||||
// Standard 56-color indexed palette (BIFF8/OOXML standard colors)
|
||||
// Index 64 = system fg, 65 = system bg → omit (return undefined)
|
||||
const INDEXED_COLORS: (string | undefined)[] = [
|
||||
'#000000', '#FFFFFF', '#FF0000', '#00FF00', '#0000FF', '#FFFF00', '#FF00FF', '#00FFFF',
|
||||
'#000000', '#FFFFFF', '#FF0000', '#00FF00', '#0000FF', '#FFFF00', '#FF00FF', '#00FFFF',
|
||||
'#800000', '#008000', '#000080', '#808000', '#800080', '#008080', '#C0C0C0', '#808080',
|
||||
'#9999FF', '#993366', '#FFFFCC', '#CCFFFF', '#660066', '#FF8080', '#0066CC', '#CCCCFF',
|
||||
'#000080', '#FF00FF', '#FFFF00', '#00FFFF', '#800080', '#800000', '#008080', '#0000FF',
|
||||
'#00CCFF', '#CCFFFF', '#CCFFCC', '#FFFF99', '#99CCFF', '#FF99CC', '#CC99FF', '#FFCC99',
|
||||
'#3366FF', '#33CCCC', '#99CC00', '#FFCC00', '#FF9900', '#FF6600', '#666699', '#969696',
|
||||
'#003366', '#339966', '#003300', '#333300', '#993300', '#993366', '#333399', '#333333',
|
||||
undefined, // 64 = system fg
|
||||
undefined, // 65 = system bg
|
||||
];
|
||||
|
||||
type ExcelJSColor = { argb?: string; theme?: number; tint?: number; indexed?: number };
|
||||
|
||||
function renderColor(color: ExcelJSColor | undefined, palette: string[]): string | null {
|
||||
if (!color) return null;
|
||||
|
||||
if (color.argb) {
|
||||
// ARGB: 'FFRRGGBB' → '#RRGGBB'
|
||||
const argb = color.argb;
|
||||
if (argb.length === 8) {
|
||||
return '#' + argb.slice(2).toUpperCase();
|
||||
}
|
||||
return null;
|
||||
}
|
||||
|
||||
if (color.theme !== undefined) {
|
||||
const tint = color.tint ?? 0;
|
||||
if (palette.length > 0 && palette[color.theme]) {
|
||||
const resolved = applyTint(palette[color.theme]!, tint);
|
||||
if (tint !== 0) {
|
||||
return `theme(${color.theme},tint=${tint.toFixed(4)},resolved=${resolved})`;
|
||||
}
|
||||
return `theme(${color.theme},resolved=${resolved})`;
|
||||
}
|
||||
// Palette empty or missing this index — raw only
|
||||
if (tint !== 0) {
|
||||
return `theme(${color.theme},tint=${tint.toFixed(4)})`;
|
||||
}
|
||||
return `theme(${color.theme})`;
|
||||
}
|
||||
|
||||
if (color.indexed !== undefined) {
|
||||
const hex = INDEXED_COLORS[color.indexed];
|
||||
if (!hex) return null; // system fg/bg
|
||||
return `indexed(${color.indexed},${hex})`;
|
||||
}
|
||||
|
||||
return null;
|
||||
}
|
||||
|
||||
/**
|
||||
* Returns true if a rendered color token represents the default color for the given theme index.
|
||||
* Handles both `theme(N)` (no palette) and `theme(N,resolved=...)` (palette resolved) forms.
|
||||
* Used to suppress the default font color (theme 1 = Dark1 ≈ black) and default border color
|
||||
* (theme 0 = Light1 ≈ white/auto) from style signatures.
|
||||
*/
|
||||
function isDefaultColorToken(token: string, defaultThemeIndex: number): boolean {
|
||||
// Exact bare form: "theme(N)"
|
||||
if (token === `theme(${defaultThemeIndex})`) return true;
|
||||
// Resolved form: "theme(N,resolved=#RRGGBB)" — tint variants are NOT default
|
||||
if (token.startsWith(`theme(${defaultThemeIndex},resolved=`) && !token.includes(',tint=')) return true;
|
||||
return false;
|
||||
}
|
||||
|
||||
/**
|
||||
* Build a canonical style signature string from non-default cell parts.
|
||||
* Returns null if the cell has no non-default styling.
|
||||
*/
|
||||
function cellStyleSignature(
|
||||
cell: ExcelJS.Cell,
|
||||
palette: string[],
|
||||
): string | null {
|
||||
const parts: string[] = [];
|
||||
|
||||
// Fill: only pattern=solid (or other patterns) with fgColor
|
||||
const fill = cell.fill as ExcelJS.Fill | undefined;
|
||||
if (fill && (fill.type === 'pattern' || fill.type === 'gradient')) {
|
||||
if (fill.type === 'pattern' && fill.pattern && fill.pattern !== 'none') {
|
||||
const pf = fill as ExcelJS.FillPattern;
|
||||
if (pf.fgColor) {
|
||||
const colorStr = renderColor(pf.fgColor as ExcelJSColor, palette);
|
||||
if (colorStr) {
|
||||
parts.push(`fill:${colorStr}`);
|
||||
}
|
||||
}
|
||||
} else if (fill.type === 'gradient') {
|
||||
parts.push('fill:gradient');
|
||||
}
|
||||
}
|
||||
|
||||
// Font: only explicit flags + explicit color
|
||||
const font = cell.font as ExcelJS.Font | undefined;
|
||||
if (font) {
|
||||
const fontParts: string[] = [];
|
||||
if (font.bold === true) fontParts.push('bold');
|
||||
if (font.italic === true) fontParts.push('italic');
|
||||
if (font.underline) fontParts.push('underline');
|
||||
|
||||
// Color: only include if explicitly set (not the default theme 1 / auto-color)
|
||||
const fontColor = renderColor(font.color as ExcelJSColor | undefined, palette);
|
||||
if (fontColor && !isDefaultColorToken(fontColor, 1)) {
|
||||
fontParts.push(fontColor);
|
||||
}
|
||||
|
||||
if (fontParts.length > 0) {
|
||||
parts.push(`font:${fontParts.join(',')}`);
|
||||
}
|
||||
}
|
||||
|
||||
// Border: for each side with a style
|
||||
const border = cell.border as ExcelJS.Borders | undefined;
|
||||
if (border) {
|
||||
const sides: string[] = [];
|
||||
for (const side of ['top', 'bottom', 'left', 'right', 'diagonal'] as const) {
|
||||
const edge = border[side] as ExcelJS.Border | undefined;
|
||||
if (edge?.style) {
|
||||
const colorStr = renderColor(edge.color as ExcelJSColor | undefined, palette);
|
||||
if (colorStr && !isDefaultColorToken(colorStr, 0)) {
|
||||
sides.push(`${side}(${edge.style},${colorStr})`);
|
||||
} else {
|
||||
sides.push(`${side}(${edge.style})`);
|
||||
}
|
||||
}
|
||||
}
|
||||
if (sides.length > 0) {
|
||||
parts.push(`border:${sides.join(',')}`);
|
||||
}
|
||||
}
|
||||
|
||||
// numFmt
|
||||
const numFmt = cell.numFmt as string | undefined;
|
||||
if (numFmt && numFmt !== 'General' && numFmt !== '') {
|
||||
parts.push(`numFmt:"${numFmt}"`);
|
||||
}
|
||||
|
||||
// Alignment
|
||||
const alignment = cell.alignment as ExcelJS.Alignment | undefined;
|
||||
if (alignment) {
|
||||
const alignParts: string[] = [];
|
||||
if (alignment.horizontal) alignParts.push(alignment.horizontal);
|
||||
if (alignment.vertical) alignParts.push(alignment.vertical);
|
||||
if (alignment.wrapText) alignParts.push('wrap');
|
||||
if (alignParts.length > 0) {
|
||||
parts.push(`align:${alignParts.join(',')}`);
|
||||
}
|
||||
}
|
||||
|
||||
if (parts.length === 0) return null;
|
||||
return parts.join(';');
|
||||
}
|
||||
|
||||
/** Parse an Excel range like "A1:C3" into 1-based row/col bounds. Returns null on parse failure. */
|
||||
function parseRange(rangeStr: string): { minRow: number; maxRow: number; minCol: number; maxCol: number } | null {
|
||||
const match = rangeStr.match(/^([A-Z]+)(\d+):([A-Z]+)(\d+)$/i);
|
||||
if (!match) return null;
|
||||
function letterToCol(letters: string): number {
|
||||
let col = 0;
|
||||
for (const ch of letters.toUpperCase()) {
|
||||
col = col * 26 + (ch.charCodeAt(0) - 64);
|
||||
}
|
||||
return col;
|
||||
}
|
||||
return {
|
||||
minRow: parseInt(match[2]!, 10),
|
||||
maxRow: parseInt(match[4]!, 10),
|
||||
minCol: letterToCol(match[1]!),
|
||||
maxCol: letterToCol(match[3]!),
|
||||
};
|
||||
}
|
||||
|
||||
/** Convert 1-based column index to Excel column letter(s). */
|
||||
function colIndexToLetter(colIdx: number): string {
|
||||
let s = '';
|
||||
let n = colIdx;
|
||||
while (n > 0) {
|
||||
const rem = (n - 1) % 26;
|
||||
s = String.fromCharCode(65 + rem) + s;
|
||||
n = Math.floor((n - 1) / 26);
|
||||
}
|
||||
return s;
|
||||
}
|
||||
|
||||
/**
|
||||
* Scan a worksheet (includeEmpty:true so decorated-but-empty cells count) and
|
||||
* produce the dedup legend + range map. `palette` from resolveThemePalette.
|
||||
*
|
||||
* @param rangeBounds - When provided, only cells within [minRow..maxRow] x [minCol..maxCol]
|
||||
* are scanned, and merges are filtered to those intersecting this region. This must match
|
||||
* the same range filter applied to the values table so styles and values cover the same cells.
|
||||
*/
|
||||
export function extractSheetStyles(
|
||||
ws: ExcelJS.Worksheet,
|
||||
palette: string[],
|
||||
maxRanges: number,
|
||||
rangeBounds?: { minRow: number; maxRow: number; minCol: number; maxCol: number },
|
||||
): SheetStyleResult {
|
||||
// Step 1: Scan all cells, build sig→id map and per-sig cell list
|
||||
const sigToId = new Map<string, string>();
|
||||
const cellsBySig = new Map<string, Array<[number, number]>>();
|
||||
let counter = 0;
|
||||
|
||||
ws.eachRow({ includeEmpty: true }, (row, r) => {
|
||||
if (rangeBounds && (r < rangeBounds.minRow || r > rangeBounds.maxRow)) return;
|
||||
row.eachCell({ includeEmpty: true }, (cell, c) => {
|
||||
if (rangeBounds && (c < rangeBounds.minCol || c > rangeBounds.maxCol)) return;
|
||||
const sig = cellStyleSignature(cell, palette);
|
||||
if (!sig) return;
|
||||
|
||||
if (!sigToId.has(sig)) {
|
||||
sigToId.set(sig, `s${++counter}`);
|
||||
cellsBySig.set(sig, []);
|
||||
}
|
||||
cellsBySig.get(sig)!.push([r, c]);
|
||||
});
|
||||
});
|
||||
|
||||
// Step 2: Build legend in id order
|
||||
const idOrder: Array<{ id: string; sig: string }> = [];
|
||||
for (const [sig, id] of sigToId) {
|
||||
idOrder.push({ id, sig });
|
||||
}
|
||||
idOrder.sort((a, b) => {
|
||||
const na = parseInt(a.id.slice(1), 10);
|
||||
const nb = parseInt(b.id.slice(1), 10);
|
||||
return na - nb;
|
||||
});
|
||||
|
||||
// Step 3: For each sig, group cells into rectangles
|
||||
const assignments: Array<{ id: string; ranges: string[] }> = [];
|
||||
let totalRanges = 0;
|
||||
let truncated = false;
|
||||
|
||||
for (const { id, sig } of idOrder) {
|
||||
if (truncated) break;
|
||||
|
||||
const cells = cellsBySig.get(sig)!;
|
||||
|
||||
// Bucket by row
|
||||
const byRow = new Map<number, number[]>();
|
||||
for (const [r, c] of cells) {
|
||||
if (!byRow.has(r)) byRow.set(r, []);
|
||||
byRow.get(r)!.push(c);
|
||||
}
|
||||
|
||||
// For each row, sort and split into contiguous horizontal runs
|
||||
// A run is (row, colStart, colEnd)
|
||||
const allRuns: Array<{ r: number; c1: number; c2: number }> = [];
|
||||
for (const [r, cols] of byRow) {
|
||||
cols.sort((a, b) => a - b);
|
||||
let start = cols[0]!;
|
||||
let prev = cols[0]!;
|
||||
for (let i = 1; i < cols.length; i++) {
|
||||
if (cols[i]! !== prev + 1) {
|
||||
allRuns.push({ r, c1: start, c2: prev });
|
||||
start = cols[i]!;
|
||||
}
|
||||
prev = cols[i]!;
|
||||
}
|
||||
allRuns.push({ r, c1: start, c2: prev });
|
||||
}
|
||||
|
||||
// Sort runs by row, then c1
|
||||
allRuns.sort((a, b) => a.r !== b.r ? a.r - b.r : a.c1 - b.c1);
|
||||
|
||||
// Merge vertically: greedy - open rectangles extended when next row has same c1,c2.
|
||||
// O(K) implementation: openRects is keyed by (c1,c2); after finishing each row r,
|
||||
// any rect not extended this row (r2 !== r) is flushed to closedRects immediately.
|
||||
type Rect = { r1: number; r2: number; c1: number; c2: number };
|
||||
// Key: `${c1},${c2}` → open rect
|
||||
const openMap = new Map<string, Rect>();
|
||||
const closedRects: Rect[] = [];
|
||||
|
||||
// Group runs by row so we can flush after each row
|
||||
const runsByRow = new Map<number, Array<{ c1: number; c2: number }>>();
|
||||
for (const run of allRuns) {
|
||||
if (!runsByRow.has(run.r)) runsByRow.set(run.r, []);
|
||||
runsByRow.get(run.r)!.push({ c1: run.c1, c2: run.c2 });
|
||||
}
|
||||
|
||||
// Process rows in ascending order
|
||||
const sortedRows = [...runsByRow.keys()].sort((a, b) => a - b);
|
||||
for (const r of sortedRows) {
|
||||
const runs = runsByRow.get(r)!;
|
||||
const extendedKeys = new Set<string>();
|
||||
|
||||
for (const { c1, c2 } of runs) {
|
||||
const key = `${c1},${c2}`;
|
||||
const existing = openMap.get(key);
|
||||
if (existing && existing.r2 === r - 1) {
|
||||
// Extend the open rect into this row
|
||||
existing.r2 = r;
|
||||
extendedKeys.add(key);
|
||||
} else {
|
||||
// If there was a stale open rect with this key, close it first
|
||||
if (existing) {
|
||||
closedRects.push(existing);
|
||||
}
|
||||
// Open a new rect
|
||||
openMap.set(key, { r1: r, r2: r, c1, c2 });
|
||||
extendedKeys.add(key);
|
||||
}
|
||||
}
|
||||
|
||||
// Flush any open rects that were NOT extended this row (r2 !== r)
|
||||
for (const [key, rect] of openMap) {
|
||||
if (rect.r2 !== r) {
|
||||
closedRects.push(rect);
|
||||
openMap.delete(key);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Close all remaining open rects
|
||||
for (const rect of openMap.values()) {
|
||||
closedRects.push(rect);
|
||||
}
|
||||
|
||||
// Emit ranges
|
||||
const ranges: string[] = [];
|
||||
for (const rect of closedRects) {
|
||||
if (totalRanges >= maxRanges) {
|
||||
truncated = true;
|
||||
break;
|
||||
}
|
||||
let rangeStr: string;
|
||||
if (rect.r1 === rect.r2 && rect.c1 === rect.c2) {
|
||||
rangeStr = `${colIndexToLetter(rect.c1)}${rect.r1}`;
|
||||
} else {
|
||||
rangeStr = `${colIndexToLetter(rect.c1)}${rect.r1}:${colIndexToLetter(rect.c2)}${rect.r2}`;
|
||||
}
|
||||
ranges.push(rangeStr);
|
||||
totalRanges++;
|
||||
}
|
||||
|
||||
if (ranges.length > 0) {
|
||||
assignments.push({ id, ranges });
|
||||
}
|
||||
if (truncated) break;
|
||||
}
|
||||
|
||||
// Step 4: Merges from ws.model.merges (filtered to rangeBounds when provided)
|
||||
const allMerges: string[] = (ws.model.merges as string[] | undefined) ?? [];
|
||||
const merges: string[] = rangeBounds
|
||||
? allMerges.filter((m) => {
|
||||
// Parse merge range "A1:C3" and check for intersection with rangeBounds
|
||||
const mp = parseRange(m);
|
||||
if (!mp) return true; // unparseable: keep it
|
||||
// Two rectangles intersect if NOT (one is completely to the side/above/below the other)
|
||||
return !(mp.maxRow < rangeBounds.minRow || mp.minRow > rangeBounds.maxRow ||
|
||||
mp.maxCol < rangeBounds.minCol || mp.minCol > rangeBounds.maxCol);
|
||||
})
|
||||
: allMerges;
|
||||
|
||||
// Step 5: Conditional formatting
|
||||
const conditionalFormatting = Boolean(
|
||||
((ws as unknown) as { conditionalFormattings?: unknown[] }).conditionalFormattings?.length
|
||||
);
|
||||
|
||||
return {
|
||||
legend: idOrder,
|
||||
assignments,
|
||||
merges,
|
||||
conditionalFormatting,
|
||||
truncated,
|
||||
};
|
||||
}
|
||||
@@ -358,4 +358,27 @@ describe('Read* tools — format mismatch rejection (issue #246)', () => {
|
||||
expect(result?.isError).toBe(false);
|
||||
expect(result?.output).toContain('Sheet1');
|
||||
});
|
||||
|
||||
it('ReadExcel includes a Styles section only when include_styles=true', async () => {
|
||||
workspacePath = makeWorkspace();
|
||||
fs.mkdirSync(path.join(workspacePath, 'input'), { recursive: true });
|
||||
|
||||
const ExcelJS = (await import('exceljs')).default;
|
||||
const wb = new ExcelJS.Workbook();
|
||||
const ws = wb.addWorksheet('Sheet1');
|
||||
ws.getCell('A1').value = 'Header';
|
||||
ws.getCell('A1').fill = { type: 'pattern', pattern: 'solid', fgColor: { argb: 'FFFFF2CC' } };
|
||||
ws.getCell('A1').font = { bold: true };
|
||||
await wb.xlsx.writeFile(path.join(workspacePath, 'input', 'styled.xlsx'));
|
||||
|
||||
// Without include_styles: no Styles section (backward compat)
|
||||
const plain = await executeTool('ReadExcel', { path: 'input/styled.xlsx' }, makeContext(workspacePath));
|
||||
expect(plain!.output).not.toContain('### Styles');
|
||||
|
||||
// With include_styles: Styles section present with fill color and font bold
|
||||
const styled = await executeTool('ReadExcel', { path: 'input/styled.xlsx', include_styles: true }, makeContext(workspacePath));
|
||||
expect(styled!.output).toContain('### Styles');
|
||||
expect(styled!.output).toContain('#FFF2CC');
|
||||
expect(styled!.output).toMatch(/bold/);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -9,6 +9,7 @@ import { PDFParse } from 'pdf-parse';
|
||||
import { ToolDef } from '../../llm/openai-compat.js';
|
||||
import type { ToolContext, ToolResult } from './core.js';
|
||||
import { resolveAndGuard, resolveOutputPathWithin, truncateToBudget, getToolOutputBudgetTokens } from './core.js';
|
||||
import { resolveThemePalette, extractSheetStyles } from './excel-styles.js';
|
||||
import { logger } from '../../logger.js';
|
||||
import { callVisionModel, resolveImagePath } from './image.js';
|
||||
import type {
|
||||
@@ -204,7 +205,7 @@ const READ_EXCEL_DEF: ToolDef = {
|
||||
type: 'function',
|
||||
function: {
|
||||
name: 'ReadExcel',
|
||||
description: 'Excel (.xlsx) を読み取りテキストで返す。詳細は ReadToolDoc({ name: "ReadExcel" })。',
|
||||
description: 'Excel (.xlsx) を読み取りテキストで返す。include_styles:true でセル装飾も取得可。詳細は ReadToolDoc({ name: "ReadExcel" })。',
|
||||
parameters: {
|
||||
type: 'object',
|
||||
properties: {
|
||||
@@ -212,6 +213,8 @@ const READ_EXCEL_DEF: ToolDef = {
|
||||
sheet: { type: 'string', description: 'シート名（省略時は全シート）' },
|
||||
range: { type: 'string', description: 'セル範囲（例: A1:D10、省略時はシート全体）' },
|
||||
max_cells: { type: 'number', description: '最大セル数（デフォルト: 1000）' },
|
||||
include_styles: { type: 'boolean', description: 'true でセル装飾(背景色/フォント/罫線/書式/結合)を ### Styles として追記。デフォルト false' },
|
||||
max_style_ranges: { type: 'number', description: 'include_styles 時に出力する style range の上限（デフォルト 250）' },
|
||||
},
|
||||
required: ['path'],
|
||||
},
|
||||
@@ -538,6 +541,8 @@ async function executeReadExcel(
|
||||
const sheetFilter = typeof input['sheet'] === 'string' ? input['sheet'] : undefined;
|
||||
const rangeFilter = typeof input['range'] === 'string' ? input['range'] : undefined;
|
||||
const maxCells = typeof input['max_cells'] === 'number' ? input['max_cells'] : 1000;
|
||||
const includeStyles = input['include_styles'] === true;
|
||||
const maxStyleRanges = typeof input['max_style_ranges'] === 'number' ? input['max_style_ranges'] : 250;
|
||||
|
||||
let resolved: string;
|
||||
try {
|
||||
@@ -583,6 +588,9 @@ async function executeReadExcel(
|
||||
return { output: `Failed to read Excel file: ${(e as Error).message}`, isError: true };
|
||||
}
|
||||
|
||||
const themeXml = includeStyles ? ((wb.model as unknown as { themes?: { theme1?: string } }).themes?.theme1) : undefined;
|
||||
const palette = includeStyles ? resolveThemePalette(themeXml) : [];
|
||||
|
||||
const filename = path.basename(resolved);
|
||||
const sheetBlocks: ExcelSheetBlock[] = [];
|
||||
let totalCells = 0;
|
||||
@@ -697,6 +705,26 @@ async function executeReadExcel(
|
||||
parts.push('');
|
||||
}
|
||||
|
||||
if (includeStyles) {
|
||||
let rangeBudget = maxStyleRanges;
|
||||
for (const ws of wb.worksheets) {
|
||||
if (sheetFilter && ws.name !== sheetFilter) continue;
|
||||
const styleBounds = rangeFilter ? parseRange(rangeFilter) : null;
|
||||
const styles = extractSheetStyles(ws, palette, rangeBudget, styleBounds ?? undefined);
|
||||
rangeBudget -= styles.assignments.reduce((n, a) => n + a.ranges.length, 0);
|
||||
if (styles.legend.length === 0 && styles.merges.length === 0 && !styles.conditionalFormatting) continue;
|
||||
parts.push(`### Styles — ${ws.name}`);
|
||||
for (const { id, sig } of styles.legend) parts.push(`${id} = ${sig}`);
|
||||
parts.push('');
|
||||
for (const { id, ranges } of styles.assignments) parts.push(`${id}: ${ranges.join(',')}`);
|
||||
if (styles.merges.length) parts.push(`merges: ${styles.merges.join(',')}`);
|
||||
if (styles.conditionalFormatting) parts.push('conditionalFormatting: present (effective styles not evaluated)');
|
||||
if (styles.truncated || rangeBudget <= 0) parts.push(`[styles truncated: max_style_ranges=${maxStyleRanges} reached]`);
|
||||
parts.push('');
|
||||
if (rangeBudget <= 0) break;
|
||||
}
|
||||
}
|
||||
|
||||
if (doc.warnings.length > 0) {
|
||||
parts.push('### Warnings');
|
||||
for (const w of doc.warnings) {
|
||||
|
||||
@@ -52,8 +52,11 @@ export async function executeTool(
|
||||
}
|
||||
|
||||
const builtinPath = join('pieces', `${piece}.yaml`);
|
||||
const customPath = ctx.customPiecesDir ? join(ctx.customPiecesDir, `${piece}.yaml`) : null;
|
||||
if (!existsSync(builtinPath) && !(customPath && existsSync(customPath))) {
|
||||
const customDirs = ctx.customPiecesDir
|
||||
? (Array.isArray(ctx.customPiecesDir) ? ctx.customPiecesDir : [ctx.customPiecesDir])
|
||||
: [];
|
||||
const customExists = customDirs.some(d => existsSync(join(d, `${piece}.yaml`)));
|
||||
if (!existsSync(builtinPath) && !customExists) {
|
||||
return {
|
||||
output: `指定されたピース "${piece}" が見つかりません。利用可能なピースを確認してください。`,
|
||||
isError: true,
|
||||
|
||||
@@ -1,5 +1,5 @@
|
||||
import { describe, it, expect, beforeEach, afterEach } from 'vitest';
|
||||
import { mkdtempSync, rmSync, existsSync, readFileSync, writeFileSync, cpSync } from 'fs';
|
||||
import { mkdtempSync, mkdirSync, rmSync, existsSync, readFileSync, writeFileSync, cpSync } from 'fs';
|
||||
import { join } from 'path';
|
||||
import { tmpdir } from 'os';
|
||||
import { executeTool } from './pieces.js';
|
||||
@@ -87,6 +87,111 @@ movements:
|
||||
});
|
||||
});
|
||||
|
||||
describe('customPiecesDir as array — P1-a regression', () => {
|
||||
/**
|
||||
* When worker passes customPiecesDir=[userDir, globalDir] to ToolContext,
|
||||
* ListPieces and GetPiece must see pieces from BOTH dirs.
|
||||
* CreatePiece must write to dirs[0] (per-user dir, not global).
|
||||
*/
|
||||
let userDir: string;
|
||||
let globalDir: string;
|
||||
|
||||
beforeEach(() => {
|
||||
userDir = mkdtempSync(join(tmpdir(), 'pieces-user-'));
|
||||
globalDir = mkdtempSync(join(tmpdir(), 'pieces-global-'));
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
rmSync(userDir, { recursive: true, force: true });
|
||||
rmSync(globalDir, { recursive: true, force: true });
|
||||
});
|
||||
|
||||
const userPiece = `name: user-only-piece
|
||||
description: user piece
|
||||
max_movements: 1
|
||||
initial_movement: do
|
||||
movements:
|
||||
- name: do
|
||||
edit: false
|
||||
persona: p
|
||||
instruction: i
|
||||
allowed_tools: []
|
||||
rules:
|
||||
- condition: done
|
||||
next: do
|
||||
`;
|
||||
|
||||
const globalPiece = `name: global-only-piece
|
||||
description: global piece
|
||||
max_movements: 1
|
||||
initial_movement: do
|
||||
movements:
|
||||
- name: do
|
||||
edit: false
|
||||
persona: p
|
||||
instruction: i
|
||||
allowed_tools: []
|
||||
rules:
|
||||
- condition: done
|
||||
next: do
|
||||
`;
|
||||
|
||||
function arrayCtx(): ToolContext {
|
||||
return { workspacePath: '/tmp/dummy', editAllowed: false, customPiecesDir: [userDir, globalDir] };
|
||||
}
|
||||
|
||||
it('ListPieces returns pieces from BOTH per-user and global-custom dirs', async () => {
|
||||
writeFileSync(join(userDir, 'user-only-piece.yaml'), userPiece, 'utf-8');
|
||||
writeFileSync(join(globalDir, 'global-only-piece.yaml'), globalPiece, 'utf-8');
|
||||
|
||||
const result = await executeTool('ListPieces', {}, arrayCtx());
|
||||
expect(result?.isError).toBe(false);
|
||||
expect(result?.output).toContain('user-only-piece');
|
||||
expect(result?.output).toContain('global-only-piece');
|
||||
});
|
||||
|
||||
it('GetPiece finds a piece in the second custom dir (global-custom)', async () => {
|
||||
writeFileSync(join(globalDir, 'global-only-piece.yaml'), globalPiece, 'utf-8');
|
||||
|
||||
const result = await executeTool('GetPiece', { name: 'global-only-piece' }, arrayCtx());
|
||||
expect(result?.isError).toBe(false);
|
||||
expect(result?.output).toContain('global piece');
|
||||
});
|
||||
|
||||
it('GetPiece prefers dirs[0] (per-user) over dirs[1] (global) for same name', async () => {
|
||||
const userVersion = userPiece.replace('description: user piece', 'description: per-user version');
|
||||
writeFileSync(join(userDir, 'global-only-piece.yaml'), userVersion, 'utf-8');
|
||||
writeFileSync(join(globalDir, 'global-only-piece.yaml'), globalPiece, 'utf-8');
|
||||
|
||||
const result = await executeTool('GetPiece', { name: 'global-only-piece' }, arrayCtx());
|
||||
expect(result?.isError).toBe(false);
|
||||
expect(result?.output).toContain('per-user version');
|
||||
});
|
||||
|
||||
it('CreatePiece writes to dirs[0] (per-user dir), not global dir', async () => {
|
||||
const newPiece = `name: brand-new-piece
|
||||
description: new
|
||||
max_movements: 5
|
||||
initial_movement: do
|
||||
movements:
|
||||
- name: do
|
||||
edit: false
|
||||
persona: p
|
||||
instruction: i
|
||||
allowed_tools: []
|
||||
rules:
|
||||
- condition: done
|
||||
next: do
|
||||
`;
|
||||
const result = await executeTool('CreatePiece', { yaml_content: newPiece }, arrayCtx());
|
||||
expect(result?.isError).toBe(false);
|
||||
// Must land in userDir (dirs[0])
|
||||
expect(existsSync(join(userDir, 'brand-new-piece.yaml'))).toBe(true);
|
||||
// Must NOT land in globalDir (dirs[1])
|
||||
expect(existsSync(join(globalDir, 'brand-new-piece.yaml'))).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
describe('UpdatePiece', () => {
|
||||
// Uses the real bundled `chat.yaml` to exercise the built-in guard. The
|
||||
// file's pre-update content is captured up-front so we can detect any
|
||||
|
||||
+24
-12
@@ -7,10 +7,20 @@ import type { ToolContext, ToolResult } from './core.js';
|
||||
const BUILTIN_PIECES_DIR = resolve(process.cwd(), 'pieces');
|
||||
const VALID_NAME = /^[a-z0-9-]+$/;
|
||||
|
||||
function findPiecePath(name: string, customDir: string | undefined): string | null {
|
||||
if (customDir) {
|
||||
const customPath = join(customDir, `${name}.yaml`);
|
||||
if (existsSync(customPath)) return customPath;
|
||||
/** Normalise `string | string[] | undefined` → `string[]` (may be empty). */
|
||||
function toCustomDirs(customDir: string | string[] | undefined): string[] {
|
||||
if (!customDir) return [];
|
||||
return Array.isArray(customDir) ? customDir : [customDir];
|
||||
}
|
||||
|
||||
/**
|
||||
* Search all custom dirs in order, then fall back to built-in.
|
||||
* Returns the first path that exists, or null.
|
||||
*/
|
||||
function findPiecePath(name: string, customDir: string | string[] | undefined): string | null {
|
||||
for (const d of toCustomDirs(customDir)) {
|
||||
const p = join(d, `${name}.yaml`);
|
||||
if (existsSync(p)) return p;
|
||||
}
|
||||
const builtinPath = join(BUILTIN_PIECES_DIR, `${name}.yaml`);
|
||||
if (existsSync(builtinPath)) return builtinPath;
|
||||
@@ -19,17 +29,16 @@ function findPiecePath(name: string, customDir: string | undefined): string | nu
|
||||
|
||||
/**
|
||||
* A piece is "built-in" when it lives only under the bundled BUILTIN_PIECES_DIR
|
||||
* (no override in customDir). Built-ins are git-tracked and shipped with the
|
||||
* (no override in any custom dir). Built-ins are git-tracked and shipped with the
|
||||
* app — letting the LLM rewrite them in place corrupts the install (a real
|
||||
* incident: the agent silently replaced game-tweet-generator with a version
|
||||
* missing max_movements, making every subsequent run abort instantly). The
|
||||
* LLM should use CreatePiece with a new name to derive a customized variant
|
||||
* instead.
|
||||
*/
|
||||
function isBuiltinOnly(name: string, customDir: string | undefined): boolean {
|
||||
if (customDir) {
|
||||
const customPath = join(customDir, `${name}.yaml`);
|
||||
if (existsSync(customPath)) return false;
|
||||
function isBuiltinOnly(name: string, customDir: string | string[] | undefined): boolean {
|
||||
for (const d of toCustomDirs(customDir)) {
|
||||
if (existsSync(join(d, `${name}.yaml`))) return false;
|
||||
}
|
||||
const builtinPath = join(BUILTIN_PIECES_DIR, `${name}.yaml`);
|
||||
return existsSync(builtinPath);
|
||||
@@ -156,7 +165,9 @@ function executeListPieces(ctx: ToolContext): ToolResult {
|
||||
const seen = new Set<string>();
|
||||
const pieces: Array<{ name: string; description: string; keywords: string[]; custom: boolean }> = [];
|
||||
const dirs: Array<{ dir: string; custom: boolean }> = [];
|
||||
if (ctx.customPiecesDir && existsSync(ctx.customPiecesDir)) dirs.push({ dir: ctx.customPiecesDir, custom: true });
|
||||
for (const d of toCustomDirs(ctx.customPiecesDir)) {
|
||||
if (existsSync(d)) dirs.push({ dir: d, custom: true });
|
||||
}
|
||||
dirs.push({ dir: BUILTIN_PIECES_DIR, custom: false });
|
||||
|
||||
for (const { dir, custom } of dirs) {
|
||||
@@ -231,8 +242,9 @@ function executeCreatePiece(input: Record<string, unknown>, ctx: ToolContext): T
|
||||
return { output: `Piece "${piece.name}" already exists. Use UpdatePiece to modify it.`, isError: true };
|
||||
}
|
||||
|
||||
// カスタムディレクトリがあればそこに、なければ builtin に書き込み
|
||||
const targetDir = ctx.customPiecesDir ?? BUILTIN_PIECES_DIR;
|
||||
// Write to the FIRST custom dir (per-user dir), or fall back to builtin if no custom dirs.
|
||||
const customDirs = toCustomDirs(ctx.customPiecesDir);
|
||||
const targetDir = customDirs[0] ?? BUILTIN_PIECES_DIR;
|
||||
mkdirSync(targetDir, { recursive: true });
|
||||
const filePath = join(targetDir, `${piece.name}.yaml`);
|
||||
try {
|
||||
|
||||
@@ -3,6 +3,8 @@ import { existsSync, mkdirSync, readFileSync, writeFileSync, mkdtempSync } from
|
||||
import { join } from 'path';
|
||||
import { tmpdir } from 'os';
|
||||
import { buildRetryHandoffSummary, Worker } from './worker.js';
|
||||
import { userPiecesDir } from './user-folder/paths.js';
|
||||
import { loadPiece } from './engine/piece-runner.js';
|
||||
import type { AppConfig } from './config.js';
|
||||
import type { Job } from './db/repository.js';
|
||||
|
||||
@@ -358,3 +360,53 @@ describe('Worker', () => {
|
||||
void worker; // worker インスタンスを参照して lint 警告を回避
|
||||
});
|
||||
});
|
||||
|
||||
// ---------------------------------------------------------------------------
|
||||
// Regression: null-ownerId job resolves pieces from data/users/local/pieces
|
||||
// ---------------------------------------------------------------------------
|
||||
|
||||
describe('Worker piece resolution for null-ownerId job', () => {
|
||||
it('a no-auth job (ownerId null) can load a piece from data/users/local/pieces', () => {
|
||||
const tempDir = mkdtempSync(join(tmpdir(), 'worker-piece-res-'));
|
||||
const piecesDir = join(tempDir, 'pieces');
|
||||
const userFolderRoot = join(tempDir, 'users');
|
||||
mkdirSync(piecesDir);
|
||||
|
||||
// Write a piece under local user-custom dir (where no-auth POST now writes)
|
||||
const localPiecesDir = userPiecesDir(userFolderRoot, 'local');
|
||||
mkdirSync(localPiecesDir, { recursive: true });
|
||||
writeFileSync(join(localPiecesDir, 'local-piece.yaml'), [
|
||||
'name: local-piece',
|
||||
'description: local custom piece',
|
||||
'max_movements: 1',
|
||||
'initial_movement: only',
|
||||
'movements:',
|
||||
' - name: only',
|
||||
' edit: false',
|
||||
' persona: p',
|
||||
' instruction: i',
|
||||
' allowed_tools: [Read]',
|
||||
' default_next: COMPLETE',
|
||||
' rules: []',
|
||||
].join('\n'));
|
||||
|
||||
// Simulate what worker.ts does: ownerForPieces = job.ownerId ?? 'local'
|
||||
const ownerForPieces = (null as string | null) ?? 'local';
|
||||
const customPieceDirs = [userPiecesDir(userFolderRoot, ownerForPieces)].filter(Boolean);
|
||||
|
||||
// loadPiece should find the piece in the local user-custom dir
|
||||
const piece = loadPiece('local-piece', piecesDir, customPieceDirs);
|
||||
expect(piece.name).toBe('local-piece');
|
||||
expect(piece.description).toBe('local custom piece');
|
||||
});
|
||||
|
||||
it('a null-ownerId job resolves the local user-custom dir path consistently', () => {
|
||||
const userFolderRoot = '/data/users';
|
||||
// The fixed worker uses: job.ownerId ?? 'local'
|
||||
const ownerForNullJob = (null as string | null) ?? 'local';
|
||||
const resolvedDir = userPiecesDir(userFolderRoot, ownerForNullJob);
|
||||
expect(resolvedDir).toContain('local');
|
||||
// Same dir that no-auth POST writes to
|
||||
expect(resolvedDir).toBe(userPiecesDir(userFolderRoot, 'local'));
|
||||
});
|
||||
});
|
||||
|
||||
+13
-4
@@ -1,4 +1,5 @@
|
||||
import { Repository, Job, localTaskRepoName, type JobRole } from './db/repository.js';
|
||||
import { userPiecesDir } from './user-folder/paths.js';
|
||||
import { BrowserSessionRepo } from './db/browser-session-repo.js';
|
||||
import { assertProfileOwner } from './engine/browser-session-auth.js';
|
||||
import { initMasterKey, decryptUserDek, decryptStateBlob } from './crypto/sessions.js';
|
||||
@@ -922,9 +923,17 @@ export class Worker {
|
||||
// watchdog 誤検知防止: runPiece 実行中に updated_at を定期更新
|
||||
let heartbeatTimer: ReturnType<typeof setInterval> | undefined;
|
||||
try {
|
||||
// Piece 読み込み
|
||||
logger.info(`[worker:${this.workerId}] job ${jobId} loadPiece piece=${job.pieceName} customPiecesDir=${this.config.customPiecesDir ?? 'none'} piecesDir=pieces`);
|
||||
const piece = loadPiece(job.pieceName, 'pieces', this.config.customPiecesDir);
|
||||
// Piece 読み込み: per-user カスタムディレクトリ → global カスタムディレクトリ → builtin の順に探索
|
||||
// No-auth jobs (ownerId null) resolve pieces from data/users/local/pieces, matching
|
||||
// where no-auth POST now writes (LOCAL_OWNER='local' in pieces-api.ts).
|
||||
const userFolderRoot = this.config.userFolderRoot ?? './data/users';
|
||||
const ownerForPieces = job.ownerId ?? 'local';
|
||||
const customPieceDirs = [
|
||||
userPiecesDir(userFolderRoot, ownerForPieces),
|
||||
this.config.customPiecesDir,
|
||||
].filter((d): d is string => !!d);
|
||||
logger.info(`[worker:${this.workerId}] job ${jobId} loadPiece piece=${job.pieceName} customDirs=[${customPieceDirs.join(', ') || 'none'}] piecesDir=pieces`);
|
||||
const piece = loadPiece(job.pieceName, 'pieces', customPieceDirs);
|
||||
if (
|
||||
piece.model &&
|
||||
this.availableModels.size > 0 &&
|
||||
@@ -1283,7 +1292,7 @@ export class Worker {
|
||||
abortController: jobAbortController,
|
||||
safetyConfig: this.config.safety,
|
||||
searchFilter: this.config.searchFilter,
|
||||
customPiecesDir: this.config.customPiecesDir,
|
||||
customPiecesDir: customPieceDirs,
|
||||
contextManager,
|
||||
vlmEnabled: workerDef.vlm === true,
|
||||
jobId, // Phase 5: subtask handoff parent identity
|
||||
|
||||
Reference in New Issue
Block a user