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/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..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 @@ -62,22 +62,76 @@ function isTracked(path) { return result.status === 0; } +/** + * 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() { + 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; +} + +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. + * + * 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); + const workspaceRoot = findJjWorkspaceRoot(); + if (workspaceRoot === null) { + throw new Error('error: not in a git repository or a jj workspace'); + } + 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}`, + ); + } + const wipBoundary = join(canonicalWorkspaceRoot, 'wip'); + if (!isInside(wipBoundary, canonicalPath)) { + throw new Error( + `error: without git, review artifacts must live under the ignored wip/ tree: ${wipBoundary}`, + ); + } + 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); - 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; } async function main() { @@ -87,7 +141,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/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..4454a5aa1955 --- /dev/null +++ b/skills-contrib/review-fetch-phase/scripts/guard-review-artifacts-ignored.test.mjs @@ -0,0 +1,118 @@ +import { spawnSync } from 'node:child_process'; +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'; +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 tempRoot; +let workspaceRoot; +let outsideRoot; +let fakeWorkspaceRoot; +let wipLinkWorkspaceRoot; +let fakeJjBinDir; + +function runGuard(dir, { jjRoot = workspaceRoot, cwd = workspaceRoot } = {}) { + return spawnSync(process.execPath, [guardPath, '--dir', dir], { + cwd, + encoding: 'utf8', + env: { + ...process.env, + PATH: `${fakeJjBinDir}${delimiter}${process.env.PATH}`, + FAKE_JJ_WORKSPACE_ROOT: jjRoot ?? '', + }, + }); +} + +describe('guard-review-artifacts-ignored in a jj workspace without git', () => { + before(() => { + tempRoot = mkdtempDisposableSync(join(tmpdir(), 'guard-review-')); + const base = realpathSync(tempRoot.path); + workspaceRoot = join(base, 'workspace'); + 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(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(() => { + tempRoot.remove(); + }); + + 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/); + }); + + 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); + 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/); + }); +}); 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);