From c1b386e6bcbb4e95d0052eb739d5915f79e3eecf Mon Sep 17 00:00:00 2001 From: Oleksii Orlenko Date: Thu, 20 Aug 2026 15:23:37 +0200 Subject: [PATCH 1/6] fix(skills): make the artifact guard jj-aware and name the schema helpers v2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The guard shells out to git, so in a Jujutsu workspace with no git directory it failed before it could check anything — the review phases were unrunnable in the repo that ships them. Git stays the primary path and is unchanged. When git cannot answer, the guard finds the workspace root and accepts an artifact directory under its wip/ tree, which the repo ignores wholesale, and refuses anything outside it. The schema helpers enforce version 2 while being named V1. Rename them, and their six call sites, to match what they check. Claude-Session: https://claude.ai/code/session_01NnNjsNcPMtbJZhnZz5Zzbe Signed-off-by: Oleksii Orlenko --- skills-contrib/review-fetch-phase/SKILL.md | 2 +- .../scripts/extract-review-targets.mjs | 4 +- .../scripts/fetch-review-state.mjs | 8 +-- .../guard-review-artifacts-ignored.mjs | 59 +++++++++++++++++-- .../scripts/render-review-state.mjs | 4 +- .../scripts/review-artifacts.mjs | 8 +-- .../scripts/summarize-review-state.mjs | 4 +- .../scripts/validate-review-state.mjs | 4 +- skills-contrib/review-triage-phase/SKILL.md | 2 +- .../scripts/bootstrap-review-actions.mjs | 4 +- 10 files changed, 75 insertions(+), 24 deletions(-) diff --git a/skills-contrib/review-fetch-phase/SKILL.md b/skills-contrib/review-fetch-phase/SKILL.md index 6cf13f33314e..7715e42d7ade 100644 --- a/skills-contrib/review-fetch-phase/SKILL.md +++ b/skills-contrib/review-fetch-phase/SKILL.md @@ -31,7 +31,7 @@ If output directory is omitted, derive: - `/summary.txt` - `/review-targets.json` 2. Ensure `` exists. -3. Enforce artifact safety before generation (must be ignored by git): +3. Enforce artifact safety before generation (the artifacts must stay untracked). In a git checkout the guard asks git directly; in a Jujutsu workspace with no git directory it accepts a directory under the workspace root's ignored `wip/` tree: ```bash node ./scripts/guard-review-artifacts-ignored.mjs --dir diff --git a/skills-contrib/review-fetch-phase/scripts/extract-review-targets.mjs b/skills-contrib/review-fetch-phase/scripts/extract-review-targets.mjs index debbf7ca6cb5..68b6de2e4216 100644 --- a/skills-contrib/review-fetch-phase/scripts/extract-review-targets.mjs +++ b/skills-contrib/review-fetch-phase/scripts/extract-review-targets.mjs @@ -5,7 +5,7 @@ import { mkdir, readFile, writeFile } from 'node:fs/promises'; import { dirname, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; -import { assertReviewStateV1, formatCanonicalJson } from './review-artifacts.mjs'; +import { assertReviewStateV2, formatCanonicalJson } from './review-artifacts.mjs'; const EXIT_SUCCESS = 0; const EXIT_OPERATIONAL = 1; @@ -86,7 +86,7 @@ async function main() { const raw = await readFile(args.inPath, 'utf8'); const reviewState = JSON.parse(raw); - assertReviewStateV1(reviewState); + assertReviewStateV2(reviewState); const payload = buildTargetsPayload(reviewState, args.inPath); await mkdir(dirname(args.outPath), { recursive: true }); await writeFile(args.outPath, formatCanonicalJson(payload), 'utf8'); diff --git a/skills-contrib/review-fetch-phase/scripts/fetch-review-state.mjs b/skills-contrib/review-fetch-phase/scripts/fetch-review-state.mjs index b3429f24e38b..b54f213de601 100644 --- a/skills-contrib/review-fetch-phase/scripts/fetch-review-state.mjs +++ b/skills-contrib/review-fetch-phase/scripts/fetch-review-state.mjs @@ -7,9 +7,9 @@ import { dirname, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; import { renderReviewStateMarkdown as renderReviewStateMarkdownImpl } from './render-review-state.mjs'; import { - assertReviewStateV1, + assertReviewStateV2, formatCanonicalJson, - normalizeReviewStateV1, + normalizeReviewStateV2, } from './review-artifacts.mjs'; const EXIT_SUCCESS = 0; @@ -548,7 +548,7 @@ async function main() { } const fetchedAt = new Date().toISOString(); - const reviewState = normalizeReviewStateV1({ + const reviewState = normalizeReviewStateV2({ fetchedAt, sourceBranch, pr: payload.pr, @@ -556,7 +556,7 @@ async function main() { reviews: payload.reviews, issueComments: payload.issueComments, }); - assertReviewStateV1(reviewState); + assertReviewStateV2(reviewState); const jsonText = formatCanonicalJson(reviewState); const outJsonPath = deriveOutJsonPath(options.outPath, options.outJsonPath); diff --git a/skills-contrib/review-fetch-phase/scripts/guard-review-artifacts-ignored.mjs b/skills-contrib/review-fetch-phase/scripts/guard-review-artifacts-ignored.mjs index 08b27da1e427..4fb41199ab23 100644 --- a/skills-contrib/review-fetch-phase/scripts/guard-review-artifacts-ignored.mjs +++ b/skills-contrib/review-fetch-phase/scripts/guard-review-artifacts-ignored.mjs @@ -1,8 +1,8 @@ #!/usr/bin/env node import { spawnSync } from 'node:child_process'; -import { realpathSync } from 'node:fs'; -import { isAbsolute, join, relative, resolve, sep } from 'node:path'; +import { existsSync, realpathSync } from 'node:fs'; +import { isAbsolute, join, relative, resolve, sep, dirname } from 'node:path'; import { fileURLToPath } from 'node:url'; const EXIT_SUCCESS = 0; @@ -62,10 +62,54 @@ function isTracked(path) { return result.status === 0; } +/** + * The workspace root of a Jujutsu workspace, found by walking up to the `.jj` + * directory. Returns null outside one. + */ +function findJjWorkspaceRoot(startPath) { + let current = resolve(startPath); + for (;;) { + if (existsSync(join(current, '.jj'))) { + return current; + } + const parent = dirname(current); + if (parent === current) { + return null; + } + current = parent; + } +} + +/** + * In a Jujutsu workspace with no git directory, git cannot answer whether a + * path is ignored. The repo ignores its whole `wip/` tree, so an artifact dir + * under `/wip/` is covered by construction — that is what this + * checks, and it is the only case it accepts. + */ +function ensureUnderIgnoredWipTree(path) { + const absolutePath = resolve(path); + const workspaceRoot = findJjWorkspaceRoot(absolutePath); + if (workspaceRoot === null) { + throw new Error('error: not in a git repository or a jj workspace'); + } + const relativePath = relative(join(workspaceRoot, 'wip'), absolutePath); + if ( + relativePath === '' || + relativePath === '..' || + relativePath.startsWith(`..${sep}`) || + isAbsolute(relativePath) + ) { + throw new Error( + `error: without git, review artifacts must live under the ignored wip/ tree: ${join(workspaceRoot, 'wip')}`, + ); + } + return true; +} + function ensureInsideRepo(path) { const root = spawnSync('git', ['rev-parse', '--show-toplevel'], { encoding: 'utf8' }); if (root.status !== 0) { - throw new Error('error: not in a git repository'); + return ensureUnderIgnoredWipTree(path); } const repoRoot = root.stdout.trim(); const absolutePath = resolve(path); @@ -78,6 +122,7 @@ function ensureInsideRepo(path) { ) { throw new Error(`error: output dir must be inside repo: ${repoRoot}`); } + return false; } async function main() { @@ -87,7 +132,13 @@ async function main() { process.exit(EXIT_SUCCESS); } - ensureInsideRepo(args.outputDir); + const ignoredByWorkspaceLayout = ensureInsideRepo(args.outputDir); + if (ignoredByWorkspaceLayout) { + process.stdout.write( + `ok: review artifacts are under the ignored wip/ tree: ${args.outputDir}\n`, + ); + process.exit(EXIT_SUCCESS); + } const tracked = []; const notIgnored = []; diff --git a/skills-contrib/review-fetch-phase/scripts/render-review-state.mjs b/skills-contrib/review-fetch-phase/scripts/render-review-state.mjs index e15aa373d7ce..3ddd09d22ea8 100644 --- a/skills-contrib/review-fetch-phase/scripts/render-review-state.mjs +++ b/skills-contrib/review-fetch-phase/scripts/render-review-state.mjs @@ -5,7 +5,7 @@ import { mkdir, readFile, writeFile } from 'node:fs/promises'; import { dirname, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; -import { assertReviewStateV1 } from './review-artifacts.mjs'; +import { assertReviewStateV2 } from './review-artifacts.mjs'; const EXIT_SUCCESS = 0; const EXIT_OPERATIONAL = 1; @@ -125,7 +125,7 @@ function formatAuthorLogin(author) { } export function renderReviewStateMarkdown(payload, { sourcePath }) { - assertReviewStateV1(payload); + assertReviewStateV2(payload); const source = formatCodeSpan(sourcePath || 'review-state.json'); const lines = []; diff --git a/skills-contrib/review-fetch-phase/scripts/review-artifacts.mjs b/skills-contrib/review-fetch-phase/scripts/review-artifacts.mjs index 319f97dd48fe..67a5faaedc36 100644 --- a/skills-contrib/review-fetch-phase/scripts/review-artifacts.mjs +++ b/skills-contrib/review-fetch-phase/scripts/review-artifacts.mjs @@ -191,7 +191,7 @@ function normalizeIssueComment(comment) { }; } -function normalizeReviewStateV1(input) { +function normalizeReviewStateV2(input) { const normalizedThreads = []; const threadCandidates = Array.isArray(input?.reviewThreads) ? input.reviewThreads : []; for (const thread of threadCandidates) { @@ -417,7 +417,7 @@ function validateIssueCommentShape(entry, pointer) { } } -function assertReviewStateV1(reviewState) { +function assertReviewStateV2(reviewState) { if (typeof reviewState !== 'object' || reviewState === null) { throw new TypeError('review-state must be an object'); } @@ -508,9 +508,9 @@ function formatCanonicalJson(value) { } export { - assertReviewStateV1, + assertReviewStateV2, formatCanonicalJson, - normalizeReviewStateV1, + normalizeReviewStateV2, REVIEW_STATE_VERSION, stripReviewFrameworkMarkers, }; diff --git a/skills-contrib/review-fetch-phase/scripts/summarize-review-state.mjs b/skills-contrib/review-fetch-phase/scripts/summarize-review-state.mjs index 07e0673512ff..228827541f12 100644 --- a/skills-contrib/review-fetch-phase/scripts/summarize-review-state.mjs +++ b/skills-contrib/review-fetch-phase/scripts/summarize-review-state.mjs @@ -5,7 +5,7 @@ import { mkdir, readFile, writeFile } from 'node:fs/promises'; import { dirname, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; -import { assertReviewStateV1, formatCanonicalJson } from './review-artifacts.mjs'; +import { assertReviewStateV2, formatCanonicalJson } from './review-artifacts.mjs'; const EXIT_SUCCESS = 0; const EXIT_OPERATIONAL = 1; @@ -90,7 +90,7 @@ function parseCliArgs(argv) { } export function buildReviewStateSummary(payload) { - assertReviewStateV1(payload); + assertReviewStateV2(payload); return { version: 1, diff --git a/skills-contrib/review-fetch-phase/scripts/validate-review-state.mjs b/skills-contrib/review-fetch-phase/scripts/validate-review-state.mjs index e4679bb797de..2949a559c981 100644 --- a/skills-contrib/review-fetch-phase/scripts/validate-review-state.mjs +++ b/skills-contrib/review-fetch-phase/scripts/validate-review-state.mjs @@ -5,7 +5,7 @@ import { readFile } from 'node:fs/promises'; import { resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; -import { assertReviewStateV1, REVIEW_STATE_VERSION } from './review-artifacts.mjs'; +import { assertReviewStateV2, REVIEW_STATE_VERSION } from './review-artifacts.mjs'; const EXIT_SUCCESS = 0; const EXIT_OPERATIONAL = 1; @@ -60,7 +60,7 @@ async function main() { const raw = await readFile(args.inPath, 'utf8'); const parsed = JSON.parse(raw); - assertReviewStateV1(parsed); + assertReviewStateV2(parsed); process.stdout.write(`ok: ${args.inPath}\n`); } diff --git a/skills-contrib/review-triage-phase/SKILL.md b/skills-contrib/review-triage-phase/SKILL.md index 13665a249074..48bfafc372c2 100644 --- a/skills-contrib/review-triage-phase/SKILL.md +++ b/skills-contrib/review-triage-phase/SKILL.md @@ -46,7 +46,7 @@ Note: - `/review-state.json` - `/review-actions.json` - `/review-actions.md` -2. Enforce artifact safety before generation (must be ignored by git): +2. Enforce artifact safety before generation (the artifacts must stay untracked). In a git checkout the guard asks git directly; in a Jujutsu workspace with no git directory it accepts a directory under the workspace root's ignored `wip/` tree: ```bash node ../review-fetch-phase/scripts/guard-review-artifacts-ignored.mjs --dir diff --git a/skills-contrib/review-triage-phase/scripts/bootstrap-review-actions.mjs b/skills-contrib/review-triage-phase/scripts/bootstrap-review-actions.mjs index be630dad57cb..402dee8d04f9 100644 --- a/skills-contrib/review-triage-phase/scripts/bootstrap-review-actions.mjs +++ b/skills-contrib/review-triage-phase/scripts/bootstrap-review-actions.mjs @@ -5,7 +5,7 @@ import { mkdir, readFile, writeFile } from 'node:fs/promises'; import { dirname, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; -import { assertReviewStateV1 } from '../../review-fetch-phase/scripts/review-artifacts.mjs'; +import { assertReviewStateV2 } from '../../review-fetch-phase/scripts/review-artifacts.mjs'; import { assertReviewActionsV1 } from './review-artifacts.mjs'; const EXIT_SUCCESS = 0; @@ -124,7 +124,7 @@ async function main() { const raw = await readFile(args.inPath, 'utf8'); const reviewState = JSON.parse(raw); - assertReviewStateV1(reviewState); + assertReviewStateV2(reviewState); const reviewActions = buildReviewActions(reviewState, args.inPath); assertReviewActionsV1(reviewActions); From ed38d7d22b29c3430204f34a01df1726c40a3d2d Mon Sep 17 00:00:00 2001 From: Oleksii Orlenko Date: Thu, 20 Aug 2026 15:47:11 +0200 Subject: [PATCH 2/6] fix(skills): reject symlinked escapes from the ignored wip/ tree (TML-3223) The jj fallback compared lexical paths, so a symlink under wip/ pointing outside the workspace passed the prefix check while the artifacts landed somewhere .gitignore does not cover. Canonicalize the workspace root, the wip/ root, and the artifact path before comparing, resolving symlinks in the part of the path that exists. Reject a path that resolves outside the workspace with its own message. Cover the three states in a new test, wired into pnpm test:scripts. Claude-Session: https://claude.ai/code/session_01NnNjsNcPMtbJZhnZz5Zzbe Signed-off-by: Oleksii Orlenko --- package.json | 2 +- .../guard-review-artifacts-ignored.mjs | 54 +++++++++++++++---- .../guard-review-artifacts-ignored.test.mjs | 54 +++++++++++++++++++ 3 files changed, 100 insertions(+), 10 deletions(-) create mode 100644 skills-contrib/review-fetch-phase/scripts/guard-review-artifacts-ignored.test.mjs diff --git a/package.json b/package.json index 044e19e7f43b..a24427ba4e2a 100644 --- a/package.json +++ b/package.json @@ -46,7 +46,7 @@ "lint:docs": "node scripts/validate-package-readmes.mjs", "lint:manifests": "node scripts/validate-package-manifests.mjs && node scripts/validate-typescript-peer.mjs", "lint:workflows": "node scripts/lint-workflow-triggers.mjs", - "test:scripts": "node --test scripts/coverage-config.test.mjs scripts/coverage-report.test.mjs scripts/lint-workflow-triggers.test.mjs scripts/validate-skills.test.mjs scripts/determine-version-utils.test.ts scripts/check-upgrade-coverage.test.mjs scripts/check-release-notes.test.mjs scripts/set-version-utils.test.ts scripts/check-publish-deps.test.mjs scripts/check-conformance.test.mjs scripts/check-publish-deps-pn-pins.test.mjs scripts/check-publish-deps-declarations.test.mjs scripts/validate-package-manifests.test.mjs scripts/publish-packages-utils.test.mjs scripts/check-clean-tree.test.mjs scripts/lint-casts.test.mjs scripts/lint-throws.test.mjs scripts/list-error-codes.test.mjs scripts/lint-framework-vocabulary.test.mjs scripts/lint-single-import-root.test.mjs scripts/lint-legacy-name.test.mjs scripts/lint-consumer-internal-imports.test.mjs scripts/sync-agent-rules.test.mjs scripts/validate-typescript-peer.test.mjs scripts/run-logged.test.mjs scripts/migrate-migrations-layout.test.mjs skills-contrib/review-fetch-phase/scripts/render-review-state.test.mjs skills-contrib/review-triage-phase/scripts/render-review-actions.test.mjs", + "test:scripts": "node --test scripts/coverage-config.test.mjs scripts/coverage-report.test.mjs scripts/lint-workflow-triggers.test.mjs scripts/validate-skills.test.mjs scripts/determine-version-utils.test.ts scripts/check-upgrade-coverage.test.mjs scripts/check-release-notes.test.mjs scripts/set-version-utils.test.ts scripts/check-publish-deps.test.mjs scripts/check-conformance.test.mjs scripts/check-publish-deps-pn-pins.test.mjs scripts/check-publish-deps-declarations.test.mjs scripts/validate-package-manifests.test.mjs scripts/publish-packages-utils.test.mjs scripts/check-clean-tree.test.mjs scripts/lint-casts.test.mjs scripts/lint-throws.test.mjs scripts/list-error-codes.test.mjs scripts/lint-framework-vocabulary.test.mjs scripts/lint-single-import-root.test.mjs scripts/lint-legacy-name.test.mjs scripts/lint-consumer-internal-imports.test.mjs scripts/sync-agent-rules.test.mjs scripts/validate-typescript-peer.test.mjs scripts/run-logged.test.mjs scripts/migrate-migrations-layout.test.mjs skills-contrib/review-fetch-phase/scripts/guard-review-artifacts-ignored.test.mjs skills-contrib/review-fetch-phase/scripts/render-review-state.test.mjs skills-contrib/review-triage-phase/scripts/render-review-actions.test.mjs", "bump-version": "node scripts/bump-version.ts", "check:publish-deps": "node scripts/check-publish-deps.mjs", "check:conformance": "node scripts/check-conformance.mjs", diff --git a/skills-contrib/review-fetch-phase/scripts/guard-review-artifacts-ignored.mjs b/skills-contrib/review-fetch-phase/scripts/guard-review-artifacts-ignored.mjs index 4fb41199ab23..f64a0bda7729 100644 --- a/skills-contrib/review-fetch-phase/scripts/guard-review-artifacts-ignored.mjs +++ b/skills-contrib/review-fetch-phase/scripts/guard-review-artifacts-ignored.mjs @@ -2,7 +2,7 @@ import { spawnSync } from 'node:child_process'; import { existsSync, realpathSync } from 'node:fs'; -import { isAbsolute, join, relative, resolve, sep, dirname } from 'node:path'; +import { basename, isAbsolute, join, relative, resolve, sep, dirname } from 'node:path'; import { fileURLToPath } from 'node:url'; const EXIT_SUCCESS = 0; @@ -80,11 +80,45 @@ function findJjWorkspaceRoot(startPath) { } } +/** + * The canonical form of `path`: symlinks resolved in the part of it that + * exists, with the components that do not exist yet appended unchanged. + */ +function canonicalize(path) { + const missing = []; + let current = resolve(path); + for (;;) { + if (existsSync(current)) { + return join(realpathSync(current), ...missing.reverse()); + } + const parent = dirname(current); + if (parent === current) { + return resolve(path); + } + missing.push(basename(current)); + current = parent; + } +} + +function isInside(parentPath, childPath) { + const relativePath = relative(parentPath, childPath); + return ( + relativePath !== '' && + relativePath !== '..' && + !relativePath.startsWith(`..${sep}`) && + !isAbsolute(relativePath) + ); +} + /** * In a Jujutsu workspace with no git directory, git cannot answer whether a * path is ignored. The repo ignores its whole `wip/` tree, so an artifact dir * under `/wip/` is covered by construction — that is what this * checks, and it is the only case it accepts. + * + * Both sides are canonicalized first: a symlink under `wip/` points at a + * directory the ignore rule does not cover, so the comparison has to be made + * between real paths. */ function ensureUnderIgnoredWipTree(path) { const absolutePath = resolve(path); @@ -92,15 +126,17 @@ function ensureUnderIgnoredWipTree(path) { if (workspaceRoot === null) { throw new Error('error: not in a git repository or a jj workspace'); } - const relativePath = relative(join(workspaceRoot, 'wip'), absolutePath); - if ( - relativePath === '' || - relativePath === '..' || - relativePath.startsWith(`..${sep}`) || - isAbsolute(relativePath) - ) { + const wipRoot = join(workspaceRoot, 'wip'); + const canonicalWorkspaceRoot = canonicalize(workspaceRoot); + const canonicalPath = canonicalize(absolutePath); + if (!isInside(canonicalWorkspaceRoot, canonicalPath)) { + throw new Error( + `error: review artifacts must stay inside the workspace: ${absolutePath} resolves to ${canonicalPath}, outside ${canonicalWorkspaceRoot}`, + ); + } + if (!isInside(canonicalize(wipRoot), canonicalPath)) { throw new Error( - `error: without git, review artifacts must live under the ignored wip/ tree: ${join(workspaceRoot, 'wip')}`, + `error: without git, review artifacts must live under the ignored wip/ tree: ${wipRoot}`, ); } return true; diff --git a/skills-contrib/review-fetch-phase/scripts/guard-review-artifacts-ignored.test.mjs b/skills-contrib/review-fetch-phase/scripts/guard-review-artifacts-ignored.test.mjs new file mode 100644 index 000000000000..8e45aaca5ea3 --- /dev/null +++ b/skills-contrib/review-fetch-phase/scripts/guard-review-artifacts-ignored.test.mjs @@ -0,0 +1,54 @@ +import { spawnSync } from 'node:child_process'; +import { mkdirSync, mkdtempSync, realpathSync, rmSync, symlinkSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { dirname, join } from 'node:path'; +import { after, before, describe, it } from 'node:test'; +import assert from 'node:assert/strict'; +import { fileURLToPath } from 'node:url'; + +const guardPath = join(dirname(fileURLToPath(import.meta.url)), 'guard-review-artifacts-ignored.mjs'); + +let workspaceRoot; +let outsideRoot; + +function runGuard(dir) { + return spawnSync(process.execPath, [guardPath, '--dir', dir], { + cwd: workspaceRoot, + encoding: 'utf8', + }); +} + +describe('guard-review-artifacts-ignored in a jj workspace without git', () => { + before(() => { + const base = realpathSync(mkdtempSync(join(tmpdir(), 'guard-review-'))); + workspaceRoot = join(base, 'workspace'); + outsideRoot = join(base, 'outside'); + mkdirSync(join(workspaceRoot, '.jj'), { recursive: true }); + mkdirSync(join(workspaceRoot, 'wip', 'reviews', 'x'), { recursive: true }); + mkdirSync(join(workspaceRoot, 'docs'), { recursive: true }); + mkdirSync(outsideRoot, { recursive: true }); + symlinkSync(outsideRoot, join(workspaceRoot, 'wip', 'escape'), 'dir'); + }); + + after(() => { + rmSync(dirname(workspaceRoot), { recursive: true, force: true }); + }); + + it('accepts a directory under the ignored wip/ tree', () => { + const result = runGuard(join(workspaceRoot, 'wip', 'reviews', 'x')); + assert.equal(result.status, 0, result.stderr); + assert.match(result.stdout, /ok: review artifacts are under the ignored wip\/ tree/); + }); + + it('rejects a directory outside the wip/ tree', () => { + const result = runGuard(join(workspaceRoot, 'docs')); + assert.equal(result.status, 1); + assert.match(result.stderr, /must live under the ignored wip\/ tree/); + }); + + it('rejects a path that a symlink under wip/ points outside the workspace', () => { + const result = runGuard(join(workspaceRoot, 'wip', 'escape', 'reviews')); + assert.equal(result.status, 1); + assert.match(result.stderr, /outside/); + }); +}); From 808a89df2754bfebb493eb3381520d4a967cd8d0 Mon Sep 17 00:00:00 2001 From: Oleksii Orlenko Date: Fri, 21 Aug 2026 16:43:47 +0200 Subject: [PATCH 3/6] fix(skills): resolve the jj workspace root via jj itself (TML-3223) Ask `jj workspace root --ignore-working-copy` in the process working directory instead of walking up from the output path looking for a `.jj` directory. The walk could pick a different workspace, or accept a fake one marked by an empty `.jj` directory. Tests mock jj on PATH and cover the fake-workspace rejection. Signed-off-by: Oleksii Orlenko --- .../guard-review-artifacts-ignored.mjs | 25 +++++----- .../guard-review-artifacts-ignored.test.mjs | 46 +++++++++++++++++-- 2 files changed, 52 insertions(+), 19 deletions(-) diff --git a/skills-contrib/review-fetch-phase/scripts/guard-review-artifacts-ignored.mjs b/skills-contrib/review-fetch-phase/scripts/guard-review-artifacts-ignored.mjs index f64a0bda7729..dff7ed65e4ce 100644 --- a/skills-contrib/review-fetch-phase/scripts/guard-review-artifacts-ignored.mjs +++ b/skills-contrib/review-fetch-phase/scripts/guard-review-artifacts-ignored.mjs @@ -63,21 +63,18 @@ function isTracked(path) { } /** - * The workspace root of a Jujutsu workspace, found by walking up to the `.jj` - * directory. Returns null outside one. + * The workspace root jj reports for the process working directory, or null + * when jj is unavailable or the working directory is not in a jj workspace. */ -function findJjWorkspaceRoot(startPath) { - let current = resolve(startPath); - for (;;) { - if (existsSync(join(current, '.jj'))) { - return current; - } - const parent = dirname(current); - if (parent === current) { - return null; - } - current = parent; +function findJjWorkspaceRoot() { + const result = spawnSync('jj', ['workspace', 'root', '--ignore-working-copy'], { + encoding: 'utf8', + }); + if (result.status !== 0) { + return null; } + const root = result.stdout.trim(); + return root === '' ? null : root; } /** @@ -122,7 +119,7 @@ function isInside(parentPath, childPath) { */ function ensureUnderIgnoredWipTree(path) { const absolutePath = resolve(path); - const workspaceRoot = findJjWorkspaceRoot(absolutePath); + const workspaceRoot = findJjWorkspaceRoot(); if (workspaceRoot === null) { throw new Error('error: not in a git repository or a jj workspace'); } diff --git a/skills-contrib/review-fetch-phase/scripts/guard-review-artifacts-ignored.test.mjs b/skills-contrib/review-fetch-phase/scripts/guard-review-artifacts-ignored.test.mjs index 8e45aaca5ea3..263cd99c31eb 100644 --- a/skills-contrib/review-fetch-phase/scripts/guard-review-artifacts-ignored.test.mjs +++ b/skills-contrib/review-fetch-phase/scripts/guard-review-artifacts-ignored.test.mjs @@ -1,20 +1,39 @@ import { spawnSync } from 'node:child_process'; -import { mkdirSync, mkdtempSync, realpathSync, rmSync, symlinkSync } from 'node:fs'; +import { mkdirSync, mkdtempSync, realpathSync, rmSync, symlinkSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; -import { dirname, join } from 'node:path'; +import { delimiter, dirname, join } from 'node:path'; import { after, before, describe, it } from 'node:test'; import assert from 'node:assert/strict'; import { fileURLToPath } from 'node:url'; const guardPath = join(dirname(fileURLToPath(import.meta.url)), 'guard-review-artifacts-ignored.mjs'); +const FAKE_JJ_SCRIPT = `#!/bin/sh +case "$*" in + "workspace root --ignore-working-copy") ;; + *) echo "fake jj: unexpected args: $*" >&2; exit 2 ;; +esac +if [ -z "$FAKE_JJ_WORKSPACE_ROOT" ]; then + echo 'Error: There is no jj repo in "."' >&2 + exit 1 +fi +printf '%s\\n' "$FAKE_JJ_WORKSPACE_ROOT" +`; + let workspaceRoot; let outsideRoot; +let fakeWorkspaceRoot; +let fakeJjBinDir; -function runGuard(dir) { +function runGuard(dir, { jjRoot = workspaceRoot, cwd = workspaceRoot } = {}) { return spawnSync(process.execPath, [guardPath, '--dir', dir], { - cwd: workspaceRoot, + cwd, encoding: 'utf8', + env: { + ...process.env, + PATH: `${fakeJjBinDir}${delimiter}${process.env.PATH}`, + FAKE_JJ_WORKSPACE_ROOT: jjRoot ?? '', + }, }); } @@ -23,10 +42,15 @@ describe('guard-review-artifacts-ignored in a jj workspace without git', () => { const base = realpathSync(mkdtempSync(join(tmpdir(), 'guard-review-'))); workspaceRoot = join(base, 'workspace'); outsideRoot = join(base, 'outside'); - mkdirSync(join(workspaceRoot, '.jj'), { recursive: true }); + fakeWorkspaceRoot = join(base, 'fake-workspace'); + fakeJjBinDir = join(base, 'bin'); mkdirSync(join(workspaceRoot, 'wip', 'reviews', 'x'), { recursive: true }); mkdirSync(join(workspaceRoot, 'docs'), { recursive: true }); mkdirSync(outsideRoot, { recursive: true }); + mkdirSync(join(fakeWorkspaceRoot, '.jj'), { recursive: true }); + mkdirSync(join(fakeWorkspaceRoot, 'wip', 'reviews'), { recursive: true }); + mkdirSync(fakeJjBinDir, { recursive: true }); + writeFileSync(join(fakeJjBinDir, 'jj'), FAKE_JJ_SCRIPT, { mode: 0o755 }); symlinkSync(outsideRoot, join(workspaceRoot, 'wip', 'escape'), 'dir'); }); @@ -51,4 +75,16 @@ describe('guard-review-artifacts-ignored in a jj workspace without git', () => { assert.equal(result.status, 1); assert.match(result.stderr, /outside/); }); + + it('rejects a directory under another workspace marked only by a .jj directory', () => { + const result = runGuard(join(fakeWorkspaceRoot, 'wip', 'reviews')); + assert.equal(result.status, 1); + assert.match(result.stderr, /must stay inside the workspace/); + }); + + it('fails when jj reports no workspace for the working directory', () => { + const result = runGuard(join(workspaceRoot, 'wip', 'reviews', 'x'), { jjRoot: null }); + assert.equal(result.status, 1); + assert.match(result.stderr, /not in a git repository or a jj workspace/); + }); }); From 909ca1c609232e52804e3caa2812ca7d4c297938 Mon Sep 17 00:00:00 2001 From: Oleksii Orlenko Date: Fri, 21 Aug 2026 16:45:41 +0200 Subject: [PATCH 4/6] fix(skills): close symlink escapes in the wip/ artifact guard (TML-3223) Resolve the artifact path with realpathSync, so a missing path fails with ENOENT instead of being canonicalized component by component, and a dangling symlink cannot smuggle a path through. Keep the wip boundary as the literal wip entry under the real workspace root, so a symlinked wip cannot move the boundary to a non-ignored directory. Signed-off-by: Oleksii Orlenko --- .../guard-review-artifacts-ignored.mjs | 42 ++++++------------- .../guard-review-artifacts-ignored.test.mjs | 28 ++++++++++++- 2 files changed, 39 insertions(+), 31 deletions(-) diff --git a/skills-contrib/review-fetch-phase/scripts/guard-review-artifacts-ignored.mjs b/skills-contrib/review-fetch-phase/scripts/guard-review-artifacts-ignored.mjs index dff7ed65e4ce..2b5d18435e6a 100644 --- a/skills-contrib/review-fetch-phase/scripts/guard-review-artifacts-ignored.mjs +++ b/skills-contrib/review-fetch-phase/scripts/guard-review-artifacts-ignored.mjs @@ -1,8 +1,8 @@ #!/usr/bin/env node import { spawnSync } from 'node:child_process'; -import { existsSync, realpathSync } from 'node:fs'; -import { basename, isAbsolute, join, relative, resolve, sep, dirname } from 'node:path'; +import { realpathSync } from 'node:fs'; +import { isAbsolute, join, relative, resolve, sep } from 'node:path'; import { fileURLToPath } from 'node:url'; const EXIT_SUCCESS = 0; @@ -77,26 +77,6 @@ function findJjWorkspaceRoot() { return root === '' ? null : root; } -/** - * The canonical form of `path`: symlinks resolved in the part of it that - * exists, with the components that do not exist yet appended unchanged. - */ -function canonicalize(path) { - const missing = []; - let current = resolve(path); - for (;;) { - if (existsSync(current)) { - return join(realpathSync(current), ...missing.reverse()); - } - const parent = dirname(current); - if (parent === current) { - return resolve(path); - } - missing.push(basename(current)); - current = parent; - } -} - function isInside(parentPath, childPath) { const relativePath = relative(parentPath, childPath); return ( @@ -113,9 +93,11 @@ function isInside(parentPath, childPath) { * under `/wip/` is covered by construction — that is what this * checks, and it is the only case it accepts. * - * Both sides are canonicalized first: a symlink under `wip/` points at a - * directory the ignore rule does not cover, so the comparison has to be made - * between real paths. + * The artifact path is resolved to its real path, so it must exist (the + * workflows create it before this guard runs; a missing path fails with + * ENOENT) and cannot escape through a symlink. The `wip` boundary is the + * literal `wip` entry under the real workspace root, so a symlinked `wip` + * cannot move the boundary to a directory the ignore rule does not cover. */ function ensureUnderIgnoredWipTree(path) { const absolutePath = resolve(path); @@ -123,17 +105,17 @@ function ensureUnderIgnoredWipTree(path) { if (workspaceRoot === null) { throw new Error('error: not in a git repository or a jj workspace'); } - const wipRoot = join(workspaceRoot, 'wip'); - const canonicalWorkspaceRoot = canonicalize(workspaceRoot); - const canonicalPath = canonicalize(absolutePath); + const canonicalWorkspaceRoot = realpathSync(workspaceRoot); + const canonicalPath = realpathSync(absolutePath); if (!isInside(canonicalWorkspaceRoot, canonicalPath)) { throw new Error( `error: review artifacts must stay inside the workspace: ${absolutePath} resolves to ${canonicalPath}, outside ${canonicalWorkspaceRoot}`, ); } - if (!isInside(canonicalize(wipRoot), canonicalPath)) { + const wipBoundary = join(canonicalWorkspaceRoot, 'wip'); + if (!isInside(wipBoundary, canonicalPath)) { throw new Error( - `error: without git, review artifacts must live under the ignored wip/ tree: ${wipRoot}`, + `error: without git, review artifacts must live under the ignored wip/ tree: ${wipBoundary}`, ); } return true; diff --git a/skills-contrib/review-fetch-phase/scripts/guard-review-artifacts-ignored.test.mjs b/skills-contrib/review-fetch-phase/scripts/guard-review-artifacts-ignored.test.mjs index 263cd99c31eb..a6b1d28439ef 100644 --- a/skills-contrib/review-fetch-phase/scripts/guard-review-artifacts-ignored.test.mjs +++ b/skills-contrib/review-fetch-phase/scripts/guard-review-artifacts-ignored.test.mjs @@ -23,6 +23,7 @@ printf '%s\\n' "$FAKE_JJ_WORKSPACE_ROOT" let workspaceRoot; let outsideRoot; let fakeWorkspaceRoot; +let wipLinkWorkspaceRoot; let fakeJjBinDir; function runGuard(dir, { jjRoot = workspaceRoot, cwd = workspaceRoot } = {}) { @@ -44,14 +45,18 @@ describe('guard-review-artifacts-ignored in a jj workspace without git', () => { outsideRoot = join(base, 'outside'); fakeWorkspaceRoot = join(base, 'fake-workspace'); fakeJjBinDir = join(base, 'bin'); + wipLinkWorkspaceRoot = join(base, 'wip-link-workspace'); mkdirSync(join(workspaceRoot, 'wip', 'reviews', 'x'), { recursive: true }); mkdirSync(join(workspaceRoot, 'docs'), { recursive: true }); - mkdirSync(outsideRoot, { recursive: true }); + mkdirSync(join(outsideRoot, 'reviews'), { recursive: true }); mkdirSync(join(fakeWorkspaceRoot, '.jj'), { recursive: true }); mkdirSync(join(fakeWorkspaceRoot, 'wip', 'reviews'), { recursive: true }); + mkdirSync(join(wipLinkWorkspaceRoot, 'docs', 'reviews'), { recursive: true }); mkdirSync(fakeJjBinDir, { recursive: true }); writeFileSync(join(fakeJjBinDir, 'jj'), FAKE_JJ_SCRIPT, { mode: 0o755 }); symlinkSync(outsideRoot, join(workspaceRoot, 'wip', 'escape'), 'dir'); + symlinkSync(join(base, 'nonexistent'), join(workspaceRoot, 'wip', 'dangling'), 'dir'); + symlinkSync(join(wipLinkWorkspaceRoot, 'docs'), join(wipLinkWorkspaceRoot, 'wip'), 'dir'); }); after(() => { @@ -76,6 +81,27 @@ describe('guard-review-artifacts-ignored in a jj workspace without git', () => { assert.match(result.stderr, /outside/); }); + it('rejects a path through a dangling symlink under wip/', () => { + const result = runGuard(join(workspaceRoot, 'wip', 'dangling', 'reviews')); + assert.equal(result.status, 1); + assert.match(result.stderr, /ENOENT/); + }); + + it('rejects an artifact dir when wip itself is a symlink to a non-ignored directory', () => { + const result = runGuard(join(wipLinkWorkspaceRoot, 'wip', 'reviews'), { + jjRoot: wipLinkWorkspaceRoot, + cwd: wipLinkWorkspaceRoot, + }); + assert.equal(result.status, 1); + assert.match(result.stderr, /must live under the ignored wip\/ tree/); + }); + + it('fails with ENOENT for a missing output path', () => { + const result = runGuard(join(workspaceRoot, 'wip', 'reviews', 'missing')); + assert.equal(result.status, 1); + assert.match(result.stderr, /ENOENT/); + }); + it('rejects a directory under another workspace marked only by a .jj directory', () => { const result = runGuard(join(fakeWorkspaceRoot, 'wip', 'reviews')); assert.equal(result.status, 1); From 7552e14626f088f22ae599c7caed03fe0fb8fbf3 Mon Sep 17 00:00:00 2001 From: Oleksii Orlenko Date: Fri, 21 Aug 2026 16:46:49 +0200 Subject: [PATCH 5/6] test(skills): manage the guard test fixture with mkdtempDisposableSync (TML-3223) Keep the disposable returned for the fixture root and remove it in after(), instead of reconstructing the root with dirname() and rmSync(). Signed-off-by: Oleksii Orlenko --- .../scripts/guard-review-artifacts-ignored.test.mjs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/skills-contrib/review-fetch-phase/scripts/guard-review-artifacts-ignored.test.mjs b/skills-contrib/review-fetch-phase/scripts/guard-review-artifacts-ignored.test.mjs index a6b1d28439ef..4454a5aa1955 100644 --- a/skills-contrib/review-fetch-phase/scripts/guard-review-artifacts-ignored.test.mjs +++ b/skills-contrib/review-fetch-phase/scripts/guard-review-artifacts-ignored.test.mjs @@ -1,5 +1,5 @@ import { spawnSync } from 'node:child_process'; -import { mkdirSync, mkdtempSync, realpathSync, rmSync, symlinkSync, writeFileSync } from 'node:fs'; +import { mkdirSync, mkdtempDisposableSync, realpathSync, symlinkSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { delimiter, dirname, join } from 'node:path'; import { after, before, describe, it } from 'node:test'; @@ -20,6 +20,7 @@ fi printf '%s\\n' "$FAKE_JJ_WORKSPACE_ROOT" `; +let tempRoot; let workspaceRoot; let outsideRoot; let fakeWorkspaceRoot; @@ -40,7 +41,8 @@ function runGuard(dir, { jjRoot = workspaceRoot, cwd = workspaceRoot } = {}) { describe('guard-review-artifacts-ignored in a jj workspace without git', () => { before(() => { - const base = realpathSync(mkdtempSync(join(tmpdir(), 'guard-review-'))); + tempRoot = mkdtempDisposableSync(join(tmpdir(), 'guard-review-')); + const base = realpathSync(tempRoot.path); workspaceRoot = join(base, 'workspace'); outsideRoot = join(base, 'outside'); fakeWorkspaceRoot = join(base, 'fake-workspace'); @@ -60,7 +62,7 @@ describe('guard-review-artifacts-ignored in a jj workspace without git', () => { }); after(() => { - rmSync(dirname(workspaceRoot), { recursive: true, force: true }); + tempRoot.remove(); }); it('accepts a directory under the ignored wip/ tree', () => { From 45627e184f0cd2cdc86b8da40c5145cdcdc11707 Mon Sep 17 00:00:00 2001 From: Oleksii Orlenko Date: Fri, 21 Aug 2026 17:04:13 +0200 Subject: [PATCH 6/6] refactor(skills): reuse isInside in ensureInsideRepo (TML-3223) Replace the inline relative-path condition with !isInside(repoRoot, absolutePath); the condition was the exact negation of the helper. Signed-off-by: Oleksii Orlenko --- .../scripts/guard-review-artifacts-ignored.mjs | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/skills-contrib/review-fetch-phase/scripts/guard-review-artifacts-ignored.mjs b/skills-contrib/review-fetch-phase/scripts/guard-review-artifacts-ignored.mjs index 2b5d18435e6a..76a25ac34a6c 100644 --- a/skills-contrib/review-fetch-phase/scripts/guard-review-artifacts-ignored.mjs +++ b/skills-contrib/review-fetch-phase/scripts/guard-review-artifacts-ignored.mjs @@ -128,13 +128,7 @@ function ensureInsideRepo(path) { } const repoRoot = root.stdout.trim(); const absolutePath = resolve(path); - const relativePath = relative(repoRoot, absolutePath); - if ( - relativePath === '' || - relativePath === '..' || - relativePath.startsWith(`..${sep}`) || - isAbsolute(relativePath) - ) { + if (!isInside(repoRoot, absolutePath)) { throw new Error(`error: output dir must be inside repo: ${repoRoot}`); } return false;