diff --git a/docs/tools/office.md b/docs/tools/office.md index a72f0c9..2b810a6 100644 --- a/docs/tools/office.md +++ b/docs/tools/office.md @@ -65,12 +65,53 @@ ReadPdf({ path: "input/spec.pdf", query: "第\\d+条", query_mode: "regex" }) ### ReadExcel ```js -ReadExcel({ file_path: "input/data.xlsx" }) +ReadExcel({ path: "input/data.xlsx" }) // → 全シートのセル内容をテキスト形式で返す ``` 巨大な Excel は token を食うので、シートを絞る場合は SplitExcelSheets を使う。 +#### include_styles — セル装飾の取得(オプション) + +```js +ReadExcel({ path: "input/report.xlsx", include_styles: true }) +// → 値テーブルの後に ### Styles セクションが追記される +``` + +デフォルトは `false`(後方互換)。`include_styles: true` を指定すると、値テーブルの直後に各シートの `### Styles` セクションが出力される。 + +**出力形式 — dedup legend + range map:** + +``` +### Styles — Sheet1 +s1 = fill:#FFF2CC;font:bold +s2 = border:top(thin),bottom(double,#808080);numFmt:"0.00%" + +s1: A1:D1,A5:D5 +s2: B2 +merges: B2:D4 +``` + +- **legend**: スタイル ID (`s1`, `s2`, ...) とそのシグネチャを列挙。シグネチャは `fill` / `font` / `border` / `numFmt` / `align` のうち非デフォルト部分を `;` 区切りで結合した文字列。 +- **range map**: 同一スタイルを持つセルを矩形にまとめて `A1:D3` 形式で列挙。単一セルは `A1`。 +- **merges**: 結合セル範囲の一覧(`ws.model.merges` から取得)。 +- **conditionalFormatting**: 条件付き書式が存在する場合 `present (effective styles not evaluated)` と表示(実際の表示色は評価しない)。 +- **装飾のみ・値なしセルも捕捉**: スタイルスキャンは `includeEmpty:true` で実行するため、値がなく書式だけ設定されたセルも対象に含まれる。 +- **`sheet`/`range` フィルタを尊重**: `include_styles` は値テーブルと同じ `sheet`/`range` フィルタでスキャン範囲を限定する。指定した範囲外のセルのスタイルと、範囲に交差しない結合セルは結果に含まれない。 +- **デフォルト色の省略**: デフォルト色 (theme index 1 = 通常黒) に明示設定したフォント色はデフォルト扱いで省略される。同様に border の theme index 0(自動色)も省略される。 + +**テーマカラーの解決(ベストエフォート):** + +テーマカラーはファイルの `theme1.xml` から読み取り、OOXML の lt/dk スワップを適用して `#RRGGBB` に解決する。解決できた場合は `theme(N,resolved=#RRGGBB)` の形式で表示し、`resolved=` は近似値であることに注意。テーマ XML が解析できない場合は `theme(N,tint=T)` のように生の参照のまま出力する。 + +**max_style_ranges — range 数の上限:** + +`max_style_ranges`(デフォルト 250)で全シートを通じた range 出力数を制限する。上限に達した場合は `[styles truncated: max_style_ranges=N reached]` を表示し、以降のスタイルは省略される。値テーブルが先に出力されるため、スタイルセクションがトークン上限で切り捨てられても値は失われない。 + +**編集には python + openpyxl を推奨:** + +ReadExcel はスタイルを読むだけで、書き込みは行わない。既存スタイルを保持したまま値を編集する場合は `Bash` ツールで python + openpyxl を使うこと(openpyxl は触れていないセルのスタイルを保持する)。 + ### ReadDocx ```js diff --git a/pieces/game-tweet-generator.yaml b/pieces/game-tweet-generator.yaml deleted file mode 100644 index ca692a1..0000000 --- a/pieces/game-tweet-generator.yaml +++ /dev/null @@ -1,86 +0,0 @@ -name: game-tweet-generator -description: | - 指定したゲームアカウントの X (Twitter) 投稿を調査し、その情報を元に特定の SNS アカウント(例: NFTGamerJP)風のツイート文章を生成する。 - 選ぶべき場合: 「ゲームの最新情報を調べて、こんな風にツイートして」と指示されたとき - 選ぶべきでない場合: 一般的な SNS 調査のみ、またはツイート生成以外の目的 -triggers: - keywords: - - ゲームツイート作成 - - ゲーム調査ツイート - - アカウント風ツイート - - ゲームアップデート調査 - - ツイート文案作成 -max_movements: 1 -initial_movement: generate -movements: - - name: generate - edit: true - persona: researcher_and_writer - instruction: | - AVAX に関連するゲームアカウントの X (Twitter) 投稿を調査し、その情報を元にターゲットアカウント(例: NFTGamerJP)風のツイート文章を生成する。 - - ## 関係するアカウント - - - Off The Grid - - DeFi Kingdoms - - The Grotto - - Fort Block Games - - Beam - - ## ワークフロー - - 1. **ゲームアカウントの特定** - - Task instruction から対象となるゲームアカウントを抽出する - - 複数アカウント指定がある場合は主要なアカウントを優先 - - 2. **最新投稿の調査** - - XUserPosts で対象アカウントの最新投稿を取得し、直近 1 週間以内を重点的に確認 - - アップデート情報・イベント告知・コミュニティ反応などを選定 - - 3. **詳細調査** - - 重要な投稿は XPostDetail でスレッド文脈・リプライを確認 - - ゲームの最新動向が不足する場合は WebSearch で補完 - - 4. **ターゲットアカウントのスタイル分析** - - 指定されたターゲットアカウントの過去投稿を XUserPosts で取得し、以下を分析: - - 使用する絵文字の種類と配置 - - 文中の装飾(改行・区切り線等)とハッシュタグのパターン - - 情報提供とエンターテインメントのバランス - - 投稿の長さと構成 - - 5. **ツイート文章の生成** - - 調査したゲーム情報を元に、ターゲットアカウントのスタイルに合わせた文章を作成 - - 重要なアップデート・イベント情報の要約、適切な絵文字、関連ハッシュタグを含める - - 短め(簡潔版)と詳細版など複数バリエーションを提示する - - ## 原則 - - - 【必須】モデルの内部知識だけで情報を書かないこと。必ず実際のツイートデータを収集する - - 調査が一部失敗しても、取得できた情報で最善の提案を行う - - ターゲットアカウントのスタイルを参考にしつつ、情報に基づいた独自の文章を作ること(単なるコピーは不可) - - ## 完了方法 - この piece は単一 movement のため、終了は必ず `complete` ツールで行う。`transition` は使わない。 - - - **ツイート文章を生成できた場合**: `complete({status: "success", result: "生成したツイート文章(複数バリエーション含む)と、根拠となった調査結果のサマリ"})` - - `result` がそのままユーザーに表示される最終出力。短いメモではなく完成形を入れる - - **調査対象や目的が曖昧で確認が必要**: `complete({status: "needs_user_input", missing_info: "確認したい内容", why_no_default: "デフォルトで進められない理由"})` - - **技術的失敗で打ち切り**: `complete({status: "aborted", abort_reason: "失敗の理由"})` - - allowed_tools: - - XSearch - - XUserPosts - - XPostDetail - - XFetchCardMedia - - WebSearch - - WebFetch - - Read - - Write - - Edit - - Glob - - Grep - - Bash - - 'mcp__*' - # default_next is the engine-internal fallback. Not exposed to the LLM. - default_next: COMPLETE - rules: [] diff --git a/src/bridge/local-tasks-api.test.ts b/src/bridge/local-tasks-api.test.ts index 52e7783..1186afa 100644 --- a/src/bridge/local-tasks-api.test.ts +++ b/src/bridge/local-tasks-api.test.ts @@ -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: 'uc@x.com', 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'); + }); +}); diff --git a/src/bridge/local-tasks-api.ts b/src/bridge/local-tasks-api.ts index 20a4d4c..b72ee2b 100644 --- a/src/bridge/local-tasks-api.ts +++ b/src/bridge/local-tasks-api.ts @@ -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; } diff --git a/src/bridge/pieces-api.test.ts b/src/bridge/pieces-api.test.ts index 525d8db..b866873 100644 --- a/src/bridge/pieces-api.test.ts +++ b/src/bridge/pieces-api.test.ts @@ -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'); }); }); diff --git a/src/bridge/pieces-api.ts b/src/bridge/pieces-api.ts index ce8edbe..9f8d055 100644 --- a/src/bridge/pieces-api.ts +++ b/src/bridge/pieces-api.ts @@ -100,6 +100,19 @@ function validateName(name: string): boolean { return VALID_PIECE_NAME.test(name); } +const VALID_SOURCES = new Set(['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(); 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(); + 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); diff --git a/src/bridge/server.ts b/src/bridge/server.ts index 5a45399..61fbfd8 100644 --- a/src/bridge/server.ts +++ b/src/bridge/server.ts @@ -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 diff --git a/src/engine/piece-runner.test.ts b/src/engine/piece-runner.test.ts index 334e427..78a0a3c 100644 --- a/src/engine/piece-runner.test.ts +++ b/src/engine/piece-runner.test.ts @@ -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 }); + }); +}); diff --git a/src/engine/piece-runner.ts b/src/engine/piece-runner.ts index fde8ee3..ce92ab3 100644 --- a/src/engine/piece-runner.ts +++ b/src/engine/piece-runner.ts @@ -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(); - 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, diff --git a/src/engine/tools/excel-styles.test.ts b/src/engine/tools/excel-styles.test.ts new file mode 100644 index 0000000..bcc7578 --- /dev/null +++ b/src/engine/tools/excel-styles.test.ts @@ -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 { + 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 = ` + + + + + + + + + + + + + `; + 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); + }); +}); diff --git a/src/engine/tools/excel-styles.ts b/src/engine/tools/excel-styles.ts new file mode 100644 index 0000000..91f179c --- /dev/null +++ b/src/engine/tools/excel-styles.ts @@ -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 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(/]*>([\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(`[\\s\\S]*?`)); + if (!tagMatch) { + // Missing entry + return []; + } + const block = tagMatch[0]!; + + // Try srgbClr first + const srgbMatch = block.match(/]*val="([0-9A-Fa-f]{6})"/); + if (srgbMatch) { + scheme.push('#' + srgbMatch[1]!.toUpperCase()); + continue; + } + + // Try sysClr with lastClr + const sysMatch = block.match(/]*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(); + const cellsBySig = new Map>(); + 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(); + 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(); + const closedRects: Rect[] = []; + + // Group runs by row so we can flush after each row + const runsByRow = new Map>(); + 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(); + + 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, + }; +} diff --git a/src/engine/tools/office.test.ts b/src/engine/tools/office.test.ts index 2364211..37b24cf 100644 --- a/src/engine/tools/office.test.ts +++ b/src/engine/tools/office.test.ts @@ -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/); + }); }); diff --git a/src/engine/tools/office.ts b/src/engine/tools/office.ts index 140372e..4ff64a5 100644 --- a/src/engine/tools/office.ts +++ b/src/engine/tools/office.ts @@ -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) { diff --git a/src/engine/tools/orchestration.ts b/src/engine/tools/orchestration.ts index 9d3b3a5..7602d3b 100644 --- a/src/engine/tools/orchestration.ts +++ b/src/engine/tools/orchestration.ts @@ -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, diff --git a/src/engine/tools/pieces.test.ts b/src/engine/tools/pieces.test.ts index 3e4aeef..c97af86 100644 --- a/src/engine/tools/pieces.test.ts +++ b/src/engine/tools/pieces.test.ts @@ -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 diff --git a/src/engine/tools/pieces.ts b/src/engine/tools/pieces.ts index 4933135..1870c34 100644 --- a/src/engine/tools/pieces.ts +++ b/src/engine/tools/pieces.ts @@ -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(); 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, 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 { diff --git a/src/worker.test.ts b/src/worker.test.ts index 456da81..e0545d7 100644 --- a/src/worker.test.ts +++ b/src/worker.test.ts @@ -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')); + }); +}); diff --git a/src/worker.ts b/src/worker.ts index 817e0f0..20c003e 100644 --- a/src/worker.ts +++ b/src/worker.ts @@ -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 | 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 diff --git a/ui/src/App.tsx b/ui/src/App.tsx index f50b237..8b33201 100644 --- a/ui/src/App.tsx +++ b/ui/src/App.tsx @@ -399,7 +399,7 @@ function AppInner({ isAdmin, authEnabled, user }: { isAdmin: boolean; authEnable {page === 'settings' &&
} - {page === 'pieces' && isAdmin &&
} + {page === 'pieces' &&
} {page === 'schedules' &&
} {page === 'users' && isAdmin && authEnabled &&
} {page === 'captcha' &&
} diff --git a/ui/src/api.ts b/ui/src/api.ts index 72a7328..362b016 100644 --- a/ui/src/api.ts +++ b/ui/src/api.ts @@ -358,8 +358,10 @@ export async function reloadConfig(): Promise { // --- Pieces --- export interface DriftStatus { drifted: boolean; forkedFromCommit: string | null; latestCommit: string | null } -export interface PieceSummary { name: string; description: string; triggers?: { keywords: string[] }; custom?: boolean; drift?: DriftStatus; requiredMcp?: string[] } +export interface PieceSummary { name: string; description: string; triggers?: { keywords: string[] }; custom?: boolean; source?: 'builtin' | 'user-custom' | 'global-custom'; ownerId?: string; drift?: DriftStatus; requiredMcp?: string[] } export interface PieceDef { name: string; description: string; max_movements: number; initial_movement: string; triggers?: { keywords: string[] }; movements: any[]; requiredMcp?: string[] } +/** Full response from GET /api/pieces/:name — includes the server-resolved source. */ +export interface PieceFetchResult { piece: PieceDef; source: 'builtin' | 'user-custom' | 'global-custom'; ownerId?: string } export async function fetchPieces(): Promise { const res = await fetch(`${BASE}/pieces`); @@ -368,15 +370,17 @@ export async function fetchPieces(): Promise { return data.pieces; } -export async function fetchPiece(name: string): Promise { - const res = await fetch(`${BASE}/pieces/${name}`); +export async function fetchPiece(name: string, source?: 'builtin' | 'user-custom' | 'global-custom'): Promise { + const url = source ? `${BASE}/pieces/${name}?source=${source}` : `${BASE}/pieces/${name}`; + const res = await fetch(url); const data = await res.json(); if (!res.ok) throw new Error(data?.error ?? 'Failed to fetch piece'); - return data.piece; + return { piece: data.piece, source: data.source, ownerId: data.ownerId }; } -export async function updatePiece(name: string, piece: PieceDef): Promise { - const res = await fetch(`${BASE}/pieces/${name}`, { +export async function updatePiece(name: string, piece: PieceDef, source?: 'builtin' | 'user-custom' | 'global-custom'): Promise { + const url = source ? `${BASE}/pieces/${name}?source=${source}` : `${BASE}/pieces/${name}`; + const res = await fetch(url, { method: 'PUT', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify(piece), @@ -384,17 +388,22 @@ export async function updatePiece(name: string, piece: PieceDef): Promise if (!res.ok) { const d = await res.json(); throw new Error(d?.error ?? 'Failed to update piece'); } } -export async function createPiece(piece: PieceDef): Promise { +export interface PieceCreateResult { source: 'builtin' | 'user-custom' | 'global-custom' } + +export async function createPiece(piece: PieceDef): Promise { const res = await fetch(`${BASE}/pieces`, { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify(piece), }); if (!res.ok) { const d = await res.json(); throw new Error(d?.error ?? 'Failed to create piece'); } + const d = await res.json(); + return { source: d.source ?? 'user-custom' }; } -export async function deletePiece(name: string): Promise { - const res = await fetch(`${BASE}/pieces/${name}`, { method: 'DELETE' }); +export async function deletePiece(name: string, source?: 'builtin' | 'user-custom' | 'global-custom'): Promise { + const url = source ? `${BASE}/pieces/${name}?source=${source}` : `${BASE}/pieces/${name}`; + const res = await fetch(url, { method: 'DELETE' }); if (!res.ok) { const d = await res.json(); throw new Error(d?.error ?? 'Failed to delete piece'); } } diff --git a/ui/src/components/create/CreateTaskDialog.tsx b/ui/src/components/create/CreateTaskDialog.tsx index 6dcde59..debf7f6 100644 --- a/ui/src/components/create/CreateTaskDialog.tsx +++ b/ui/src/components/create/CreateTaskDialog.tsx @@ -5,6 +5,7 @@ import { CreateLocalTaskInput, fetchMyOrgs, Visibility, listBrowserSessionProfil import { AttachmentDropzone } from './AttachmentDropzone'; import { ScheduleFields } from './ScheduleFields'; import { usePieceList } from '../../hooks/usePieces'; +import { resolvePieceOptions } from '../../lib/splitPieces'; import { useAuthState } from '../../App'; interface CreateTaskDialogProps { @@ -80,7 +81,8 @@ export function CreateTaskDialog({ onClose, onSubmit, initialPiece, initialBody, scheduledAt: '', }); - const selectedPiece = (pieces ?? []).find(p => p.name === form.piece); + const resolvedPieces = resolvePieceOptions(pieces ?? []); + const selectedPiece = resolvedPieces.find(p => p.name === form.piece); const missingMcp = selectedPiece?.requiredMcp ? selectedPiece.requiredMcp.filter( (id) => !(connections ?? []).find((c) => c.serverId === id && c.connected), @@ -241,7 +243,7 @@ export function CreateTaskDialog({ onClose, onSubmit, initialPiece, initialBody, className="w-full px-2.5 py-1.5 border border-slate-200 rounded-lg text-xs outline-none focus:border-accent" > - {(pieces ?? []).map(p => ( + {resolvedPieces.map(p => ( ))} diff --git a/ui/src/components/detail/ContinueWithPieceDialog.tsx b/ui/src/components/detail/ContinueWithPieceDialog.tsx index 5726312..4487e39 100644 --- a/ui/src/components/detail/ContinueWithPieceDialog.tsx +++ b/ui/src/components/detail/ContinueWithPieceDialog.tsx @@ -2,6 +2,7 @@ import { useState } from 'react'; import { useMutation, useQuery, useQueryClient } from '@tanstack/react-query'; import { continueTaskWithPiece, fetchLocalTaskComments } from '../../api'; import { usePieceList } from '../../hooks/usePieces'; +import { resolvePieceOptions } from '../../lib/splitPieces'; import { MarkdownText } from '../../lib/markdown-text'; interface PrevJobInfo { @@ -123,7 +124,7 @@ export function ContinueWithPieceDialog({ disabled={piecesQuery.isLoading} className="px-3 py-2 rounded-md border border-hairline text-[13px] focus:outline-none focus:ring-2 focus:ring-accent/30 focus:border-accent" > - {(piecesQuery.data ?? []).map(p => ( + {resolvePieceOptions(piecesQuery.data ?? []).map(p => (