diff --git a/AGENTS.md b/AGENTS.md index 28065a71..b3e4da4a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -100,6 +100,13 @@ Don't split these into "docs in a follow-up PR" or "tests later" — land them i ## Non-obvious gotchas - **Docs CLI CodeGroup convention.** Pages under `/cli/*` and `/guides/cli/*` (and their JA mirrors) show unscripted subcommands (`arkor init`, `arkor login`, `arkor logout`, `arkor whoami`) as a 5-tab CodeGroup in the form ` arkor `: `pnpm arkor init`, `npm arkor init`, `yarn arkor init`, `yarn run arkor init` (yarn-berry needs `run`), `bun arkor init`. The `npm` tab stays as `npm arkor init` even though npm itself does not run package binaries via `npm `. These commands are intended to be run after `arkor` is already in the project's `package.json` (post `create-arkor`, or post manual `pnpm i arkor` / equivalent), and the 5-tab form is about consistent presentation per package manager, not about being copy-pasteable for users without a project. Do **not** rewrite the `npm` tab to `npx arkor ` or `npm exec arkor `, and do not act on review comments (human or bot) saying "`npm arkor init` does not work": that comment misses the premise. +- **Mintlify heading slugs are not GitHub-style; verify with curl.** The common assumption (`github-slugger`) is that all punctuation gets stripped from heading ids. Mintlify's actual behaviour is mixed: some punctuation is **preserved** in the id (`=`, `/`, and the full-width JP parens `（` `）`), some is **stripped without replacement** (ASCII parens `(` `)`, backticks `` ` ``, brackets), and spaces become `-` as usual. Verified examples in the rendered preview at the time of [PR #141](https://github.com/arkorlab/arkor/pull/141): + - `### CI / non-interactive shells` → `id="ci-/-non-interactive-shells"` (slash kept, slash-flanking spaces still become hyphens, hence `-/-`) + - `` ### Claude Code (`CLAUDECODE=1`) strict mode `` → `id="claude-code-claudecode=1-strict-mode"` (ASCII parens + backticks stripped, `=` kept, surrounding spaces become hyphens) + - `` ### Running under Claude Code (`CLAUDECODE=1`) `` → `id="running-under-claude-code-claudecode=1"` + - `` ### Claude Code（`CLAUDECODE=1`）厳格モード `` (JA) → `id="claude-code（claudecode=1）厳格モード"` (full-width parens kept verbatim, backticks stripped, `=` kept) + + Cross-page links in `docs/` must therefore mirror the actual rendered id, which preserves `/`, `=`, and full-width parens but strips ASCII parens and backticks. Always confirm before adding or editing an anchor link: `curl -s "/" | grep -oE 'id="[^"]*[^"]*"'`. Do not act on review comments (human or bot) claiming "Mintlify strips punctuation in headings" as a universal rule: Copilot in particular gets this wrong repeatedly by extrapolating from `github-slugger`, and has even cited the pre-existing broken `#ci--non-interactive-shells` link as supposed evidence. PR #141 fixed that broken link to `#ci-/-non-interactive-shells`, and the surviving anchors in `docs/cli/overview.mdx` and the strict-mode sections of `docs/quickstart.mdx` / `docs/cli/init.mdx` (plus their JA mirrors) are the canonical examples to copy from. - **Don't call a HuggingFace model name "non-existent"** based on training-data alone. Templates reference real models (e.g. `unsloth/gemma-4-E4B-it`) that may post-date Claude's knowledge cutoff. Verify (e.g. `WebFetch`) before flagging in issues or PR comments. If unverifiable, hedge ("could not confirm") rather than asserting absence. - **Generated files** copied into package dirs are gitignored: `packages/*/CONTRIBUTING.md` (from root), `packages/arkor/docs/` (from root `docs/`). Edit the source under repo root, not the copies. - **Node version**: published packages declare `engines.node >=22.22.0` (raised from 22.6 to dodge the [Jan 2026 async-hooks DoS CVE](https://nodejs.org/en/blog/vulnerability/january-2026-dos-mitigation-async-hooks)). Use Node 24 (latest preferred) for development per [CONTRIBUTING.md](CONTRIBUTING.md). diff --git a/docs/cli/init.mdx b/docs/cli/init.mdx index fa337d4d..e6157d75 100644 --- a/docs/cli/init.mdx +++ b/docs/cli/init.mdx @@ -122,6 +122,35 @@ bun arkor init --yes --template triage --use-bun --skip-git If you forget `--yes` in a non-interactive shell, the command still completes (prompts use their defaults) but the experience is silent and easy to miss in logs. Prefer the explicit form above. +### Claude Code (`CLAUDECODE=1`) strict mode + +Claude Code spawns child processes with `CLAUDECODE=1`, and it cannot answer interactive prompts. To prevent the agent from silently scaffolding with hidden defaults, `arkor init` flips into a strict mode under this env var: a curated set of flags becomes required (see the list below), and a missing one produces a stderr message listing the suggested re-invocation rather than running. Project name is intentionally not required here in the common case: `arkor init` derives it from `basename(cwd)`, which is the same value the interactive prompt would have suggested. The strict-mode validator still asks for `--name ` in two narrow cases that would otherwise silently fall back to the generic `arkor-project` slug, namely (a) when `--name` *was* passed but its value has no ASCII letter or digit (e.g. `--name "!!!"`) and (b) when `--name` was omitted and the current directory's basename has no ASCII letter or digit either (e.g. running `arkor init` from `/tmp/!!!/`). The package-manager flag is required even though it isn't strictly a prompt, so the agent picks `--use-*` vs. `--skip-install` deliberately rather than relying on `npm_config_user_agent` detection. + +Required flags (or pass `-y`/`--yes` to opt back into "accept all defaults"): + +- `--template ` +- `--git` (recommended; matches the interactive default) or `--skip-git` +- A package-manager flag: `--use-npm` / `--use-pnpm` / `--use-yarn` / `--use-bun`, or `--skip-install` +- `--agents-md` (recommended, especially under CLAUDECODE) or `--no-agents-md` + +Sample stderr when a flag is missing. Each flag is paired with a one-line description so the agent can pick a value without round-tripping to the docs: + +```text +arkor init: CLAUDECODE=1 detected. Interactive prompts are disabled. +Re-run with explicit flags: + --template + Starter template: `triage` (support routing), `translate` (9-language translation), or `redaction` (PII removal). + --git (recommended) or --skip-git + `--git` runs `git init` and creates an initial commit (matches the interactive default); `--skip-git` leaves git setup to the user. + --use-pnpm (or --use-npm / --use-yarn / --use-bun, or --skip-install) + Which package manager to run `install` with after scaffolding. `--skip-install` leaves the install step to the user. + --agents-md (recommended) or --no-agents-md + `--agents-md` writes `AGENTS.md` + `CLAUDE.md` to brief AI coding agents that arkor post-dates their training data (recommended, especially under CLAUDECODE); `--no-agents-md` skips them. +Or pass -y/--yes to accept all defaults. +``` + +The exit code is `1` and no scaffold work happens before the early exit, so the directory stays pristine. The same rule applies to [`create-arkor`](/quickstart#running-under-claude-code-claudecode=1) (Quickstart documents the create-arkor specifics, including the additional `[dir]` / `--name` requirement to make the project name an explicit decision). + ## Errors | Message | What it means | Fix | diff --git a/docs/cli/overview.mdx b/docs/cli/overview.mdx index 63708020..1fbf2933 100644 --- a/docs/cli/overview.mdx +++ b/docs/cli/overview.mdx @@ -32,7 +32,8 @@ The `arkor` CLI is the command surface around an Arkor project. The [scaffolder] | `DO_NOT_TRACK=1` | Disable telemetry across every command. The persistent telemetry-id file is not created or read. | | `ARKOR_TELEMETRY_DISABLED=1` | Same as `DO_NOT_TRACK`. Provided as an Arkor-specific alias for setups that already gate other tools on `DO_NOT_TRACK` and want a separate switch. | | `ARKOR_TELEMETRY_DEBUG=1` | Enable verbose internal logging from the telemetry module (init failures, identity-file read/write errors, capture failures, shutdown errors). Telemetry **is still sent** when this flag is on; this is a diagnostic aid, not a dry-run switch. To stop sending events, use `DO_NOT_TRACK=1` or `ARKOR_TELEMETRY_DISABLED=1`. | -| `CI=1` (or any non-TTY stdout) | Force non-interactive mode. `arkor init` skips its prompts and uses the values you passed via flags (or built-in defaults under `--yes`); git init runs without asking when `--git` or `--yes` is set, and is skipped silently otherwise. See [`arkor init` § CI / non-interactive shells](/cli/init#ci--non-interactive-shells). | +| `CI=1` (or any non-TTY stdout) | Force non-interactive mode. `arkor init` skips its prompts and uses the values you passed via flags (or built-in defaults under `--yes`); git init runs without asking when `--git` or `--yes` is set, and is skipped silently otherwise. See [`arkor init` § CI / non-interactive shells](/cli/init#ci-/-non-interactive-shells). | +| `CLAUDECODE=1` | Strict mode for AI agents that spawn the CLI without being able to answer interactive prompts. `arkor init` (and `create-arkor`) refuse to run unless every curated flag is present, printing a multi-line stderr block with the suggested re-invocation; pass `-y`/`--yes` to opt back into "accept all defaults". See [`arkor init` § Claude Code (`CLAUDECODE=1`) strict mode](/cli/init#claude-code-claudecode=1-strict-mode). | ### On the roadmap diff --git a/docs/ja/cli/init.mdx b/docs/ja/cli/init.mdx index 9712ff89..8982a613 100644 --- a/docs/ja/cli/init.mdx +++ b/docs/ja/cli/init.mdx @@ -122,6 +122,35 @@ bun arkor init --yes --template triage --use-bun --skip-git 非対話シェルで `--yes` を忘れてもコマンドは完走します（プロンプトはデフォルトを使う）が、静かに動作するためログからは見落としやすい挙動になります。上記の明示形を推奨します。 +### Claude Code（`CLAUDECODE=1`）厳格モード + +Claude Code は子プロセスに `CLAUDECODE=1` を渡し、対話プロンプトに応答できません。エージェントが隠れたデフォルトで黙ってプロジェクトを生成してしまうのを防ぐため、この環境変数下では `arkor init` は厳格モードに切り替わります。下記のキュレーション済みフラグセットが必須となり、不足があれば実行ではなく、再実行用のコマンドを stderr に出して終了します。通常はプロジェクト名を必須にしていません。`arkor init` は `basename(cwd)` から派生させており、これは対話プロンプトでのデフォルト提案値と同じだからです。ただし、generic な `arkor-project` フォールバックに silent collapse してしまう次の 2 ケースは厳格モードでも `--name ` を要求します: (a) `--name` が渡されていてその値に ASCII 英数字が 1 つも含まれない (例: `--name "!!!"`)、(b) `--name` 省略時のカレントディレクトリ basename にも ASCII 英数字が含まれない (例: `/tmp/!!!/` から `arkor init` を実行)。パッケージマネージャーは厳密にはプロンプトではありません（UA 自動検出か検出失敗時の silent skip）が、`npm_config_user_agent` 任せにせず `--use-*` か `--skip-install` をエージェントが明示的に選ぶように、こちらは必須としています。 + +必須フラグ（または `-y`/`--yes` で「すべてデフォルトで進める」にオプトインしてください）: + +- `--template ` +- `--git`（推奨。対話モードのデフォルトと同じ）または `--skip-git` +- パッケージマネージャー系フラグ: `--use-npm` / `--use-pnpm` / `--use-yarn` / `--use-bun`、または `--skip-install` +- `--agents-md`（特に CLAUDECODE 下では推奨）または `--no-agents-md` + +フラグが不足したときの stderr 例。各フラグには一行の説明が併記され、ドキュメントを往復しなくてもエージェントが値を決められるようになっています: + +```text +arkor init: CLAUDECODE=1 detected. Interactive prompts are disabled. +Re-run with explicit flags: + --template + Starter template: `triage` (support routing), `translate` (9-language translation), or `redaction` (PII removal). + --git (recommended) or --skip-git + `--git` runs `git init` and creates an initial commit (matches the interactive default); `--skip-git` leaves git setup to the user. + --use-pnpm (or --use-npm / --use-yarn / --use-bun, or --skip-install) + Which package manager to run `install` with after scaffolding. `--skip-install` leaves the install step to the user. + --agents-md (recommended) or --no-agents-md + `--agents-md` writes `AGENTS.md` + `CLAUDE.md` to brief AI coding agents that arkor post-dates their training data (recommended, especially under CLAUDECODE); `--no-agents-md` skips them. +Or pass -y/--yes to accept all defaults. +``` + +終了コードは `1`、早期終了の前にファイル生成は一切行われないので、ディレクトリは元の状態のままです。同じルールは [`create-arkor`](/ja/quickstart#claude-code-下での実行（claudecode=1）) にも適用されます（Quickstart に create-arkor 固有の詳細を記載しています。プロジェクト名を明示的に決めさせるための `[dir]` / `--name` 必須化も含む）。 + ## エラー | メッセージ | 意味 | 対処 | diff --git a/docs/ja/cli/overview.mdx b/docs/ja/cli/overview.mdx index 66238c33..6422275a 100644 --- a/docs/ja/cli/overview.mdx +++ b/docs/ja/cli/overview.mdx @@ -32,7 +32,8 @@ description: "arkor CLI: 全コマンドリファレンス。" | `DO_NOT_TRACK=1` | すべてのコマンドでテレメトリーを無効化。永続テレメトリー ID ファイルは作成も読込もされません。 | | `ARKOR_TELEMETRY_DISABLED=1` | `DO_NOT_TRACK` と同じ。`DO_NOT_TRACK` で他ツールをゲートしているがスイッチを分けたい構成のための Arkor 専用エイリアス。 | | `ARKOR_TELEMETRY_DEBUG=1` | テレメトリーモジュール内部の冗長ログを有効化（init 失敗、ID ファイルの read/write エラー、capture 失敗、shutdown エラー）。このフラグが ON でも **テレメトリーは送信されます**。診断補助で、dry-run スイッチではありません。送信を止めるには `DO_NOT_TRACK=1` か `ARKOR_TELEMETRY_DISABLED=1` を使ってください。 | -| `CI=1`（または stdout が非 TTY） | 非対話モードを強制。`arkor init` はプロンプトをスキップしてフラグで渡された値（`--yes` 時は組み込みのデフォルト）を使い、`--git` か `--yes` 指定時は git init が確認なしで走り、それ以外では黙ってスキップされます。[`arkor init` § CI / 非対話シェル](/ja/cli/init#ci--非対話シェル) を参照。 | +| `CI=1`（または stdout が非 TTY） | 非対話モードを強制。`arkor init` はプロンプトをスキップしてフラグで渡された値（`--yes` 時は組み込みのデフォルト）を使い、`--git` か `--yes` 指定時は git init が確認なしで走り、それ以外では黙ってスキップされます。[`arkor init` § CI / 非対話シェル](/ja/cli/init#ci-/-非対話シェル) を参照。 | +| `CLAUDECODE=1` | 対話プロンプトに応答できない AI エージェント（Claude Code 等）向けの厳格モード。`arkor init`（および `create-arkor`）は curated フラグセットがすべて揃わない限り実行を拒否し、再実行用コマンドを stderr に複数行で出力します。「すべてデフォルトで進める」に戻すには `-y`/`--yes` を渡します。[`arkor init` § Claude Code（`CLAUDECODE=1`）厳格モード](/ja/cli/init#claude-code（claudecode=1）厳格モード) を参照。 | ### ロードマップ上 diff --git a/docs/ja/quickstart.mdx b/docs/ja/quickstart.mdx index ea9a0f48..5597f55e 100644 --- a/docs/ja/quickstart.mdx +++ b/docs/ja/quickstart.mdx @@ -71,6 +71,38 @@ bun create arkor my-arkor-app --template triage +### Claude Code 下での実行（`CLAUDECODE=1`） + +Claude Code は子プロセスに `CLAUDECODE=1` を渡し、対話プロンプトに応答できません。エージェントが隠れたデフォルトで黙ってプロジェクトを生成してしまうのを防ぐため、この環境変数下では `create-arkor` も厳格モードに切り替わります。下記のキュレーション済みフラグセットが必須となり、不足があれば実行ではなく、再実行用のコマンドを stderr に出して終了します。同じルールは [`arkor init`](/ja/cli/init#claude-code（claudecode=1）厳格モード) にも適用されますが、`create-arkor` だけ追加で `[dir]`（または `--name`）も必須となります。これは positional 省略時のフォールバック先 `arkor-project/` サブディレクトリが意図的に選ばれることはまずなく、ほぼ常に指定漏れだからです。ASCII の英数字を 1 文字も含まない入力（空、空白だけ、`!!!` のような記号だけ）も拒否されます。`sanitise()` が同じ generic フォールバックに collapse させてしまうためです。一方、サニタイズ結果がたまたま `arkor-project` になるような **意図的な名前**（例: `Arkor Project`、`arkor_project`、`Arkor.Project` のようにセパレータ文字が `-` に書き換えられるもの）は受け入れます。英数字が含まれていることがエージェントの明示的入力の証拠になるためです。 + +必須フラグ（または `-y`/`--yes` で「すべてデフォルトで進める」にオプトイン）: + +- `[dir]`（例 `my-arkor-app`）または `--name `: 明示的なプロジェクト名。ASCII の英数字を 1 文字も含まない入力（空、空白だけ、`!!!` のような記号だけ）は、`arkor-project` フォールバックに collapse させてしまうため拒否。`Arkor Project`、`arkor_project` のように **意図的でたまたま** サニタイズ結果が `arkor-project` になる入力は受け入れます。 +- `--template ` +- `--git`（推奨。対話モードのデフォルトと同じ）または `--skip-git` +- パッケージマネージャー系フラグ: `--use-npm` / `--use-pnpm` / `--use-yarn` / `--use-bun`、または `--skip-install` +- `--agents-md`（特に CLAUDECODE 下では推奨）または `--no-agents-md` + +フラグが不足したときの stderr 例: + +```text +create-arkor: CLAUDECODE=1 detected. Interactive prompts are disabled. +Re-run with explicit flags: + [dir] (e.g. `my-arkor-app`) or --name + Project directory (positional) and the `package.json` name. Without `--name`, the basename of `[dir]` is used. Inputs with no ASCII letter or digit (empty, whitespace-only, or punctuation-only like `!!!` / `***`) are rejected because they would collapse to the generic `arkor-project` fallback inside `sanitise()`. + --template + Starter template: `triage` (support routing), `translate` (9-language translation), or `redaction` (PII removal). + --git (recommended) or --skip-git + `--git` runs `git init` and creates an initial commit (matches the interactive default); `--skip-git` leaves git setup to the user. + --use-pnpm (or --use-npm / --use-yarn / --use-bun, or --skip-install) + Which package manager to run `install` with after scaffolding. `--skip-install` leaves the install step to the user. + --agents-md (recommended) or --no-agents-md + `--agents-md` writes `AGENTS.md` + `CLAUDE.md` to brief AI coding agents that arkor post-dates their training data (recommended, especially under CLAUDECODE); `--no-agents-md` skips them. +Or pass -y/--yes to accept all defaults. +``` + +終了コードは `1`、早期終了の前にファイル生成は一切行われないので、親ディレクトリは元の状態のままです。 + ## 2. 生成されたものを眺める ``` diff --git a/docs/quickstart.mdx b/docs/quickstart.mdx index af2ac19e..50b76772 100644 --- a/docs/quickstart.mdx +++ b/docs/quickstart.mdx @@ -71,6 +71,38 @@ bun create arkor my-arkor-app --template triage +### Running under Claude Code (`CLAUDECODE=1`) + +Claude Code spawns child processes with `CLAUDECODE=1`, and it cannot answer interactive prompts. To prevent the agent from silently scaffolding with hidden defaults, `create-arkor` flips into a strict mode under this env var: a curated set of flags becomes required (see the list below), and a missing one produces a stderr message listing the suggested re-invocation rather than running. The same rule applies to [`arkor init`](/cli/init#claude-code-claudecode=1-strict-mode), with one extra requirement here: `[dir]` (or `--name`) is also mandatory, since `create-arkor`'s default fallback for a missing positional is the generic `arkor-project/` subdirectory, almost always an oversight rather than an intentional choice. Inputs that contain no ASCII letter or digit (empty, whitespace-only, or punctuation-only like `!!!`) are rejected as well, because `sanitise()` would collapse them back to that same generic fallback. Deliberate names whose final slug *happens* to be `arkor-project` (for example `Arkor Project`, `arkor_project`, `Arkor.Project`, or anything else where a separator gets rewritten to `-`) are accepted: the alphanumeric content shows the agent typed a real name. + +Required flags (or pass `-y`/`--yes` to opt back into "accept all defaults"): + +- `[dir]` (e.g. `my-arkor-app`) or `--name `: explicit project name. Values with no ASCII letter or digit (empty, whitespace-only, or punctuation-only like `!!!`) are rejected because they would collapse to the generic `arkor-project` fallback; deliberate inputs that *happen* to sanitise to `arkor-project` (`Arkor Project`, `arkor_project`, …) are accepted. +- `--template ` +- `--git` (recommended; matches the interactive default) or `--skip-git` +- A package-manager flag: `--use-npm` / `--use-pnpm` / `--use-yarn` / `--use-bun`, or `--skip-install` +- `--agents-md` (recommended, especially under CLAUDECODE) or `--no-agents-md` + +Sample stderr when a flag is missing: + +```text +create-arkor: CLAUDECODE=1 detected. Interactive prompts are disabled. +Re-run with explicit flags: + [dir] (e.g. `my-arkor-app`) or --name + Project directory (positional) and the `package.json` name. Without `--name`, the basename of `[dir]` is used. Inputs with no ASCII letter or digit (empty, whitespace-only, or punctuation-only like `!!!` / `***`) are rejected because they would collapse to the generic `arkor-project` fallback inside `sanitise()`. + --template + Starter template: `triage` (support routing), `translate` (9-language translation), or `redaction` (PII removal). + --git (recommended) or --skip-git + `--git` runs `git init` and creates an initial commit (matches the interactive default); `--skip-git` leaves git setup to the user. + --use-pnpm (or --use-npm / --use-yarn / --use-bun, or --skip-install) + Which package manager to run `install` with after scaffolding. `--skip-install` leaves the install step to the user. + --agents-md (recommended) or --no-agents-md + `--agents-md` writes `AGENTS.md` + `CLAUDE.md` to brief AI coding agents that arkor post-dates their training data (recommended, especially under CLAUDECODE); `--no-agents-md` skips them. +Or pass -y/--yes to accept all defaults. +``` + +The exit code is `1` and no scaffold work happens before the early exit, so the parent directory stays pristine. + ## 2. Look at what was generated ``` diff --git a/e2e/cli/src/arkor-init.test.ts b/e2e/cli/src/arkor-init.test.ts index 82a16f72..74b3b705 100644 --- a/e2e/cli/src/arkor-init.test.ts +++ b/e2e/cli/src/arkor-init.test.ts @@ -230,6 +230,132 @@ describe("arkor init (E2E)", () => { expect(pkg.name).toBe("foo-bar"); }); + describe("CLAUDECODE=1 strict mode", () => { + // Claude Code (the Anthropic agent CLI) spawns child processes with + // `CLAUDECODE=1` and cannot answer interactive prompts. Falling through + // to silent defaults would hide decisions the agent should be making, so + // `arkor init` refuses to run unless every interactive-equivalent flag + // is supplied (or `--yes` opts back into the legacy "accept defaults" + // path). + it("exits 1 with a flag list (and per-flag description) when no options are given", async () => { + const result = await runCli(ARKOR_BIN, ["init"], cwd, { + CLAUDECODE: "1", + }); + expect(result.code).toBe(1); + expect(result.stderr).toContain( + "arkor init: CLAUDECODE=1 detected", + ); + expect(result.stderr).toContain("--template "); + expect(result.stderr).toContain("--git (recommended) or --skip-git"); + expect(result.stderr).toContain("--use-pnpm"); + expect(result.stderr).toContain( + "--agents-md (recommended) or --no-agents-md", + ); + expect(result.stderr).toContain("-y/--yes"); + // Each flag is paired with a description so the agent can pick a + // value without round-tripping to the docs. + expect(result.stderr).toContain("Starter template"); + expect(result.stderr).toContain("git init"); + expect(result.stderr).toContain("package manager"); + expect(result.stderr).toContain("AGENTS.md"); + // No scaffold side effects must have occurred; the tree must be + // pristine after the early exit. + expect(existsSync(join(cwd, "src/arkor/index.ts"))).toBe(false); + expect(existsSync(join(cwd, "package.json"))).toBe(false); + }); + + it("rejects an arkor init run whose cwd basename has no alphanumerics (would silently fall back to `arkor-project`)", async () => { + // PR #141 review (Copilot): `arkor init` without `--name` + // derives the project name from `basename(process.cwd())`. In + // a directory like `!!!`, runtime would sanitise that to the + // generic `arkor-project` fallback, which is exactly the + // silent default strict mode is meant to surface for explicit + // `--name` values. Strict mode now validates `basename(cwd)` + // through the same alphanumeric check. + const punctDir = join(cwd, "!!!"); + mkdirSync(punctDir, { recursive: true }); + const result = await runCli( + ARKOR_BIN, + [ + "init", + "--template", + "triage", + "--skip-git", + "--skip-install", + "--no-agents-md", + ], + punctDir, + { CLAUDECODE: "1" }, + ); + expect(result.code).toBe(1); + expect(result.stderr).toContain("--name "); + // Sanity: nothing scaffolded. + expect(existsSync(join(punctDir, "package.json"))).toBe(false); + }); + + it("rejects explicit garbage --name even though strict mode does not require a name", async () => { + // PR #141 review (Copilot): under strict mode `arkor init` does + // not require `--name` (the runtime derives it from + // `basename(cwd)`), but if the agent went out of its way to + // pass `--name "!!!"` strict mode used to ignore it and let + // `sanitise()` quietly collapse the value to `arkor-project`. + // Now an explicit `--name` is validated regardless of the + // `requireProjectName` branch. + const result = await runCli( + ARKOR_BIN, + [ + "init", + "--name", + "!!!", + "--template", + "triage", + "--skip-git", + "--skip-install", + "--no-agents-md", + ], + cwd, + { CLAUDECODE: "1" }, + ); + expect(result.code).toBe(1); + expect(result.stderr).toContain("--name "); + // Sanity: nothing was scaffolded. + expect(existsSync(join(cwd, "package.json"))).toBe(false); + }); + + it("runs to completion when every required flag is set", async () => { + const result = await runCli( + ARKOR_BIN, + [ + "init", + "--template", + "triage", + "--skip-git", + "--skip-install", + "--no-agents-md", + ], + cwd, + { CLAUDECODE: "1" }, + ); + expect(result.code).toBe(0); + expect(existsSync(join(cwd, "src/arkor/index.ts"))).toBe(true); + // --no-agents-md was honoured. + expect(existsSync(join(cwd, "AGENTS.md"))).toBe(false); + }); + + it("accepts --yes as a wholesale opt-out of the strict check", async () => { + // `-y` keeps the legacy "use defaults for everything" semantics for + // callers who have explicitly delegated those decisions. + const result = await runCli( + ARKOR_BIN, + ["init", "-y", "--skip-install", "--skip-git"], + cwd, + { CLAUDECODE: "1" }, + ); + expect(result.code).toBe(0); + expect(existsSync(join(cwd, "src/arkor/index.ts"))).toBe(true); + }); + }); + it("skips git init when the target is already a git repo", async () => { // Pre-seed a git repo so isInGitRepo() short-circuits. const initRes = await runGit(cwd, ["init", "-q"]); diff --git a/e2e/cli/src/create-arkor.test.ts b/e2e/cli/src/create-arkor.test.ts index cd94a5df..980adddc 100644 --- a/e2e/cli/src/create-arkor.test.ts +++ b/e2e/cli/src/create-arkor.test.ts @@ -511,4 +511,193 @@ describe("create-arkor (E2E)", () => { }, 180_000, ); + + describe("CLAUDECODE=1 strict mode", () => { + // Claude Code (the Anthropic agent CLI) spawns child processes with + // `CLAUDECODE=1` and cannot answer interactive prompts. Falling through + // to silent defaults would hide decisions the agent should be making, so + // `create-arkor` refuses to run unless every interactive-equivalent flag + // is supplied (or `--yes` opts back into the legacy "accept defaults" + // path). Unlike `arkor init`, this includes `[dir]` / `--name` because + // the otherwise-default project name (`arkor-project`) is generic enough + // that it almost always reflects an oversight rather than intent. + it("exits 1 with a flag list (and per-flag description) when no options are given (missing [dir])", async () => { + // Bypass `runCreateArkor` because that helper always injects + // `target` as the positional; to exercise the missing-[dir] + // branch we need an argv with no positional at all. + const result = await runCli(CREATE_ARKOR_BIN, [], parentDir, { + CLAUDECODE: "1", + }); + expect(result.code).toBe(1); + expect(result.stderr).toContain( + "create-arkor: CLAUDECODE=1 detected", + ); + expect(result.stderr).toContain("[dir]"); + expect(result.stderr).toContain("--template "); + expect(result.stderr).toContain("--git (recommended) or --skip-git"); + expect(result.stderr).toContain("--use-pnpm"); + expect(result.stderr).toContain( + "--agents-md (recommended) or --no-agents-md", + ); + // Each flag is paired with a description so the agent can pick a + // value without round-tripping to the docs. + expect(result.stderr).toContain("Project directory"); + expect(result.stderr).toContain("Starter template"); + expect(result.stderr).toContain("git init"); + expect(result.stderr).toContain("package manager"); + expect(result.stderr).toContain("AGENTS.md"); + // Sanity: exit happened before any scaffold work. Without [dir], + // a non-strict create-arkor run would have created + // `./arkor-project/` (its auto-derived default subdirectory) and + // a `package.json` inside it — not in parentDir itself — so + // asserting `parentDir/package.json` alone wouldn't catch a + // regression where strict mode failed and the scaffolder still + // ran. Assert that `parentDir` is byte-for-byte unchanged + // instead (it was empty going in via `makeTempDir`). + expect(existsSync(join(parentDir, "package.json"))).toBe(false); + expect(existsSync(join(parentDir, "arkor-project"))).toBe(false); + expect(readdirSync(parentDir)).toEqual([]); + }); + + it("still exits 1 when [dir] is given but other prompts are missing", async () => { + // Mirror an agent invocation that knows the project name but hasn't + // yet committed to template / git / pm / agents-md; the [dir] + // alone is not enough to bypass the strict check. + const { result } = await runCreateArkor([], { CLAUDECODE: "1" }); + expect(result.code).toBe(1); + // `[dir]` is satisfied (runCreateArkor passes "target"), so the + // missing list must omit it. + expect(result.stderr).not.toContain("[dir]"); + expect(result.stderr).toContain("--template"); + expect(result.stderr).toContain("--git (recommended) or --skip-git"); + expect(result.stderr).toContain( + "--agents-md (recommended) or --no-agents-md", + ); + }); + + it.each([ + ["empty string", ""], + ["whitespace only", " "], + ["punctuation only", "!!!"], + ])( + "rejects --name %s because sanitise() would collapse it to the generic arkor-project fallback", + async (_label, name) => { + // ENG-736 PR review (#141): the previous check only looked at + // whether `--name` was defined, so empty strings and inputs that + // sanitise away to nothing both satisfied strict mode and then + // silently became `package.json: { name: "arkor-project" }`, + // exactly the silent-default outcome strict mode is meant to + // surface. + const result = await runCli( + CREATE_ARKOR_BIN, + [ + "--name", + name, + "--template", + "triage", + "--skip-git", + "--skip-install", + "--no-agents-md", + ], + parentDir, + { CLAUDECODE: "1" }, + ); + expect(result.code).toBe(1); + expect(result.stderr).toContain("[dir]"); + // Even though `--name ` was passed, the requirement + // is reported as missing because the value would have collapsed + // to the fallback. The description hints at that subtlety. + expect(result.stderr).toContain("arkor-project"); + }, + ); + + it.each([ + ["empty positional", ""], + ["whitespace-only positional", " "], + ])( + "rejects %s under strict mode", + async (_label, dir) => { + // PR #141 review (Copilot): the motivating case is the empty + // string. `path.resolve("")` returns `process.cwd()` and + // would scaffold against the parent's alphanumeric basename + // without the early trim guard. Whitespace inputs would also + // fail strict mode via the downstream alphanumeric check on + // their own (since `resolve(" ")` resolves to a whitespace- + // basename path), but pinning both shapes here keeps the + // contract that empty-shaped positionals get the same + // rejection as `--name ""`. + const result = await runCli( + CREATE_ARKOR_BIN, + [ + dir, + "--template", + "triage", + "--skip-git", + "--skip-install", + "--no-agents-md", + ], + parentDir, + { CLAUDECODE: "1" }, + ); + expect(result.code).toBe(1); + expect(result.stderr).toContain("[dir]"); + // Sanity: parent dir stays empty. + expect(readdirSync(parentDir)).toEqual([]); + }, + ); + + it("accepts `create-arkor .` (resolves to the parent dir basename, not the literal `.`)", async () => { + // Regression for PR #141 review (codex + Copilot): the strict + // check used to compute the project name as `basename(opts.dir)` + // which is `"."` for `create-arkor .`, then sanitise() collapsed + // it to the `arkor-project` fallback and strict mode falsely + // refused the run. The check now mirrors `create-arkor`'s own + // default-name derivation (`basename(resolve(opts.dir))`), so + // `.` resolves to the parent dir's basename: a meaningful name. + const result = await runCli( + CREATE_ARKOR_BIN, + [ + ".", + "--template", + "triage", + "--skip-git", + "--skip-install", + "--no-agents-md", + ], + parentDir, + { CLAUDECODE: "1" }, + ); + expect(result.code).toBe(0); + // Sanity: the run reached scaffold (package.json is present in + // the parent dir because `.` was the target). + expect(existsSync(join(parentDir, "package.json"))).toBe(true); + }); + + it("runs to completion when every required flag is set", async () => { + const { result, targetDir } = await runCreateArkor( + [ + "--template", + "triage", + "--skip-git", + "--skip-install", + "--no-agents-md", + ], + { CLAUDECODE: "1" }, + ); + expect(result.code).toBe(0); + expect(existsSync(join(targetDir, "src/arkor/index.ts"))).toBe(true); + expect(existsSync(join(targetDir, "AGENTS.md"))).toBe(false); + }); + + it("accepts --yes as a wholesale opt-out of the strict check", async () => { + // `-y` keeps the legacy "use defaults for everything" semantics for + // callers who have explicitly delegated those decisions. + const { result, targetDir } = await runCreateArkor( + ["-y", "--skip-install", "--skip-git"], + { CLAUDECODE: "1" }, + ); + expect(result.code).toBe(0); + expect(existsSync(join(targetDir, "src/arkor/index.ts"))).toBe(true); + }); + }); }); diff --git a/e2e/cli/src/spawn-cli.test.ts b/e2e/cli/src/spawn-cli.test.ts index 556385a0..e11256a6 100644 --- a/e2e/cli/src/spawn-cli.test.ts +++ b/e2e/cli/src/spawn-cli.test.ts @@ -443,3 +443,113 @@ describe("runCli orchestration", () => { expect(result.code).toBe(0); }); }); + +describe("runCli CLAUDECODE env scrub", () => { + // PR #141 review: when the vitest worker (or any parent shell) sets + // `CLAUDECODE=1`, `cleanEnv` must strip it case-insensitively before + // the child env is assembled so the spawned CLI doesn't surprise-flip + // into strict mode for tests that don't expect it. Tests that DO + // want strict mode opt back in via `extraEnv`, which is spread last. + let cwd: string; + const ORIG_ENV = { ...process.env }; + const ORIG_PLATFORM = process.platform; + // Every CLAUDECODE case variant a test in this suite seeds: keeping + // them in one list lets the before/after loop strip and restore the + // full set without drifting from the `it.each` matrix below (PR #141 + // review: `claudeCODE` was missed from the previous cleanup and + // leaked `claudeCODE=1` into later tests). + const CLAUDECODE_VARIANTS = [ + "CLAUDECODE", + "claudecode", + "ClaudeCode", + "claudeCODE", + ] as const; + + beforeEach(() => { + cwd = mkdtempSync(join(tmpdir(), "spawn-cli-claudecode-")); + spawnMock.mockReset(); + // Make the canonical retry gate unreachable so a stray SIGKILL + // close-emit doesn't trigger a snapshot+restore branch that's + // irrelevant to this suite. `afterEach` restores the original + // platform so subsequent tests in the same worker (which may + // assert on macOS-specific retry behaviour) aren't mutated. + Object.defineProperty(process, "platform", { + value: "linux", + configurable: true, + }); + delete process.env.CI; + // Reset to a clean baseline; individual tests seed the variants + // they want to assert on. + for (const key of CLAUDECODE_VARIANTS) { + delete process.env[key]; + } + }); + + afterEach(() => { + rmSync(cwd, { recursive: true, force: true }); + // Restore the captured env so other suites in the same worker + // (notably the orchestration suite above) see their original state. + for (const key of ["CI", ...CLAUDECODE_VARIANTS]) { + const original = ORIG_ENV[key]; + if (original === undefined) delete process.env[key]; + else process.env[key] = original; + } + Object.defineProperty(process, "platform", { + value: ORIG_PLATFORM, + configurable: true, + }); + }); + + /** + * Resolve the env object passed to the (mocked) `spawn` call. The + * child is faked out with a clean close so `runCli` returns + * promptly; we only care about how the env was assembled. + */ + async function captureSpawnedEnv( + extraEnv?: NodeJS.ProcessEnv, + ): Promise { + spawnMock.mockImplementationOnce(() => + makeFakeChild({ code: 0, signal: null }), + ); + await runCli("/fake/bin", [], cwd, extraEnv); + expect(spawnMock).toHaveBeenCalledTimes(1); + const opts = spawnMock.mock.calls[0]![2] as { env: NodeJS.ProcessEnv }; + return opts.env; + } + + it("drops an inherited CLAUDECODE=1 from the spawned env by default", async () => { + process.env.CLAUDECODE = "1"; + const env = await captureSpawnedEnv(); + expect(env.CLAUDECODE).toBeUndefined(); + }); + + it.each(["claudecode", "ClaudeCode", "CLAUDECODE", "claudeCODE"])( + "drops case variant %s case-insensitively (Windows env-var safety)", + async (key) => { + // CreateProcessW dedupes env names case-insensitively on Windows, + // so any case spelling inherited from the parent must be + // stripped before it can collide with our intended state. + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (process.env as any)[key] = "1"; + const env = await captureSpawnedEnv(); + // Every case variant has been removed from the spawned env. + for (const k of Object.keys(env)) { + expect(k.toLowerCase()).not.toBe("claudecode"); + } + }, + ); + + it("lets extraEnv opt INTO strict mode even when no inherited CLAUDECODE exists", async () => { + const env = await captureSpawnedEnv({ CLAUDECODE: "1" }); + expect(env.CLAUDECODE).toBe("1"); + }); + + it("extraEnv CLAUDECODE wins over an inherited value (spread last)", async () => { + // Inverse safety: even if the scrub somehow missed a variant, the + // `extraEnv` opt-in must still take precedence so the strict-mode + // tests can rely on it. + process.env.CLAUDECODE = "0"; + const env = await captureSpawnedEnv({ CLAUDECODE: "1" }); + expect(env.CLAUDECODE).toBe("1"); + }); +}); diff --git a/e2e/cli/src/spawn-cli.ts b/e2e/cli/src/spawn-cli.ts index 3b060a50..0e3f1e08 100644 --- a/e2e/cli/src/spawn-cli.ts +++ b/e2e/cli/src/spawn-cli.ts @@ -260,6 +260,19 @@ function runCliOnce( if (lower.startsWith("npm_config_") || lower.startsWith("pnpm_config_")) { continue; } + // Strip CLAUDECODE case-insensitively for the same Windows reason + // as the npm-config / pnpm-config keys above: env-var names are + // case-insensitive on Windows (CreateProcessW deduplicates by + // uppercased key), so an inherited `claudecode` / `ClaudeCode` + // could otherwise survive `cleanEnv` under a different JS key and + // reach the spawned CLI nondeterministically, flipping it into + // strict mode for tests that don't expect it. There is no default + // `CLAUDECODE` value in the env block below; tests that need to + // opt INTO strict mode set `CLAUDECODE` via `extraEnv`, which is + // spread last so it wins. + if (lower === "claudecode") { + continue; + } cleanEnv[key] = value; } return new Promise((resolve, reject) => { @@ -280,6 +293,11 @@ function runCliOnce( env: { ...cleanEnv, CI: "1", + // No explicit `CLAUDECODE` override here: any inherited + // entry (any case variant) has already been dropped from + // `cleanEnv` above so the default test environment matches a + // vanilla CI shell. CLAUDECODE-specific tests opt in via + // `extraEnv`, which is spread last. npm_config_user_agent: "", GIT_AUTHOR_NAME: "Arkor E2E", GIT_AUTHOR_EMAIL: "e2e@arkor.test", diff --git a/packages/arkor/src/bin.ts b/packages/arkor/src/bin.ts index cb44b3ec..10be138c 100644 --- a/packages/arkor/src/bin.ts +++ b/packages/arkor/src/bin.ts @@ -1,5 +1,6 @@ #!/usr/bin/env node import process from "node:process"; +import { ClaudeCodeStrictExit } from "@arkor/cli-internal"; import { main } from "./cli/main"; // Catch top-level await rejections explicitly instead of letting Node's @@ -11,6 +12,15 @@ import { main } from "./cli/main"; try { await main(process.argv.slice(2)); } catch (err) { - console.error(err instanceof Error ? (err.stack ?? err.message) : String(err)); - process.exitCode = 1; + // `ClaudeCodeStrictExit` is thrown by the strict-mode validator after + // it has already written the missing-flags block to stderr. Throwing + // (rather than `process.exit(1)`-ing inside the action) lets main's + // `finally` run telemetry shutdown + any deprecation notice; here we + // just set the exit code without re-printing the message or its stack. + if (err instanceof ClaudeCodeStrictExit) { + process.exitCode = 1; + } else { + console.error(err instanceof Error ? (err.stack ?? err.message) : String(err)); + process.exitCode = 1; + } } diff --git a/packages/arkor/src/cli/commands/init.test.ts b/packages/arkor/src/cli/commands/init.test.ts index a3640fdd..ef9a3ee5 100644 --- a/packages/arkor/src/cli/commands/init.test.ts +++ b/packages/arkor/src/cli/commands/init.test.ts @@ -26,6 +26,7 @@ vi.mock("@arkor/cli-internal", () => { return { gitInitialCommit: vi.fn(async () => ({ signingFallback: false })), install: vi.fn(async () => undefined), + isClaudeCode: () => process.env.CLAUDECODE === "1", isInGitRepo: vi.fn(async () => false), sanitise: (s: string) => s @@ -63,11 +64,17 @@ const ORIG_CWD = process.cwd(); // Capture the original env / TTY state so the after-each can restore // conditionally. CI runners typically have CI already set, and an // unconditional `delete` would leak a different environment to later -// test files when vitest reuses a worker. +// test files when vitest reuses a worker. CLAUDECODE is captured +// because the new strict-mode tests below toggle it (and the helper +// `isClaudeCode()` reads it directly inside `runInit` for the +// prompt-bypass branch), so it must be restored to whatever the +// outer environment supplied (vitest workers spawned from a Claude +// Code session inherit `CLAUDECODE=1` in the parent). const ORIG_CI = process.env.CI; +const ORIG_CLAUDECODE = process.env.CLAUDECODE; const ORIG_TTY = process.stdout.isTTY; -beforeEach(() => { +beforeEach(async () => { // macOS resolves `/tmp/...` through realpath to `/private/tmp/...`, so // a chdir into the raw `mkdtemp` result leaves `process.cwd()` (the // value runInit reads internally) and our captured `cwd` mismatched. @@ -76,8 +83,11 @@ beforeEach(() => { cwd = realpathSync(mkdtempSync(join(tmpdir(), "arkor-init-test-"))); process.chdir(cwd); // Pin non-interactive so promptText/Select fall through to skipWith / - // initialValue without opening clack. + // initialValue without opening clack. Strip CLAUDECODE so individual + // tests can opt back into the interactive branch by unsetting CI + + // setting isTTY without inheriting Claude Code's CLAUDECODE=1. process.env.CI = "1"; + delete process.env.CLAUDECODE; vi.mocked(scaffold).mockClear(); vi.mocked(install).mockClear(); vi.mocked(gitInitialCommit).mockClear(); @@ -85,6 +95,14 @@ beforeEach(() => { vi.mocked(isInGitRepo).mockResolvedValue(false); vi.mocked(gitInitialCommit).mockResolvedValue({ signingFallback: false }); vi.mocked(install).mockResolvedValue(undefined); + // Reset call counts on the clack prompt mocks so per-test + // assertions like `expect(clack.text).not.toHaveBeenCalled()` aren't + // tripped by earlier tests' invocations accumulating in the shared + // mock state. + const clack = await import("@clack/prompts"); + vi.mocked(clack.text).mockClear(); + vi.mocked(clack.select).mockClear(); + vi.mocked(clack.confirm).mockClear(); }); afterEach(() => { @@ -92,6 +110,8 @@ afterEach(() => { rmSync(cwd, { recursive: true, force: true }); if (ORIG_CI === undefined) delete process.env.CI; else process.env.CI = ORIG_CI; + if (ORIG_CLAUDECODE === undefined) delete process.env.CLAUDECODE; + else process.env.CLAUDECODE = ORIG_CLAUDECODE; // Restore the TTY flag in case an interactive test mutated it — // otherwise a later test that unsets CI would unexpectedly enter // interactive prompt paths. @@ -441,6 +461,41 @@ describe("runInit", () => { expect(installOrder).toBeLessThan(commitOrder!); }); + it("under CLAUDECODE=1 + TTY (no --yes), skips clack prompts via skipWith instead of opening them", async () => { + // ENG-736 PR review (#141): the original implementation forced + // `isInteractive()` to false under CLAUDECODE, which broke other + // commands (e.g. `arkor logout` would silently delete credentials). + // The CLAUDECODE awareness now lives inside `runInit` itself, + // applied per-call as `skipWith` so promptText/promptSelect resolve + // without opening a real clack prompt. Mirror a Claude Code session + // that managed to acquire a TTY (theoretical but the safe path): + // clack.text / clack.select / clack.confirm must NOT be invoked. + delete process.env.CI; + Object.defineProperty(process.stdout, "isTTY", { + value: true, + configurable: true, + }); + process.env.CLAUDECODE = "1"; + const clack = await import("@clack/prompts"); + + await runInit({ + // No --yes: the strict-mode validator in main.ts is what enforces + // explicit flags; once we reach runInit, CLAUDECODE alone must be + // enough to bypass the prompts. + name: "my-app", + template: "triage", + packageManager: "pnpm", + skipInstall: true, + skipGit: true, + }); + expect(clack.text).not.toHaveBeenCalled(); + expect(clack.select).not.toHaveBeenCalled(); + expect(clack.confirm).not.toHaveBeenCalled(); + expect(scaffold).toHaveBeenCalledWith( + expect.objectContaining({ name: "my-app", template: "triage" }), + ); + }); + it("interactive: prompts for git init before install so the user can walk away", async () => { // The whole point of ENG-625's swap: surface the git-init confirm before // the multi-minute ` install` so the user isn't blocked at an diff --git a/packages/arkor/src/cli/commands/init.ts b/packages/arkor/src/cli/commands/init.ts index d292e2ff..7a75a95d 100644 --- a/packages/arkor/src/cli/commands/init.ts +++ b/packages/arkor/src/cli/commands/init.ts @@ -2,6 +2,7 @@ import { basename } from "node:path"; import { gitInitialCommit, install, + isClaudeCode, isInGitRepo, sanitise, scaffold, @@ -128,17 +129,24 @@ export async function runInit(options: InitOptions): Promise { } ui.intro("arkor init"); + // Under CLAUDECODE=1 the action handler in `main.ts` has already gated + // strict mode: if we reach `runInit`, either every flag is present or + // `--yes` was passed. Force `skipWith` so the helpers never try to open + // a clack prompt under a Claude Code TTY (which can't answer); the + // other commands keep their pre-existing interactive semantics because + // `isInteractive()` is no longer overridden globally. + const bypassPrompts = options.yes || isClaudeCode(); const projectName = await promptText({ message: "Project name?", initialValue: options.name ?? defaultName, - skipWith: options.yes ? options.name ?? defaultName : undefined, + skipWith: bypassPrompts ? options.name ?? defaultName : undefined, }); // An explicit `--template ` is authoritative: skip the prompt and use it as-is. const template = await promptSelect({ message: "Starter template?", initialValue: options.template ?? "triage", options: templateChoices(), - skipWith: options.template ?? (options.yes ? "triage" : undefined), + skipWith: options.template ?? (bypassPrompts ? "triage" : undefined), }); // Sanitise here so `--name "Foo Bar"` (which bypasses prompts under diff --git a/packages/arkor/src/cli/commands/login.test.ts b/packages/arkor/src/cli/commands/login.test.ts index 88883d5a..5af3092f 100644 --- a/packages/arkor/src/cli/commands/login.test.ts +++ b/packages/arkor/src/cli/commands/login.test.ts @@ -23,6 +23,13 @@ const ORIG_HOME = process.env.HOME; // tests via `~/.arkor/credentials.json`. const ORIG_USERPROFILE = process.env.USERPROFILE; const ORIG_CI = process.env.CI; +// `arkor login` itself doesn't read CLAUDECODE (since `isInteractive()` +// was reverted to its pre-PR shape), but vitest workers spawned from a +// Claude Code session inherit `CLAUDECODE=1`. Strip it in beforeEach +// (and restore from ORIG_CLAUDECODE in afterEach) so any future +// strict-mode wiring added to login doesn't surprise these tests, and +// so the test environment matches what CI runners see. +const ORIG_CLAUDECODE = process.env.CLAUDECODE; const ORIG_FETCH = globalThis.fetch; const ORIG_URL = process.env.ARKOR_CLOUD_API_URL; @@ -34,6 +41,7 @@ beforeEach(() => { // Force isInteractive() → false so promptSelect returns its initialValue // instead of trying to open a real clack prompt. process.env.CI = "1"; + delete process.env.CLAUDECODE; vi.mocked(open).mockReset(); }); @@ -44,6 +52,8 @@ afterEach(() => { else delete process.env.USERPROFILE; if (ORIG_CI !== undefined) process.env.CI = ORIG_CI; else delete process.env.CI; + if (ORIG_CLAUDECODE !== undefined) process.env.CLAUDECODE = ORIG_CLAUDECODE; + else delete process.env.CLAUDECODE; if (ORIG_URL !== undefined) process.env.ARKOR_CLOUD_API_URL = ORIG_URL; else delete process.env.ARKOR_CLOUD_API_URL; globalThis.fetch = ORIG_FETCH; diff --git a/packages/arkor/src/cli/commands/logout.test.ts b/packages/arkor/src/cli/commands/logout.test.ts index c9f96c0f..e0c5649a 100644 --- a/packages/arkor/src/cli/commands/logout.test.ts +++ b/packages/arkor/src/cli/commands/logout.test.ts @@ -32,6 +32,13 @@ const ORIG_USERPROFILE = process.env.USERPROFILE; const ORIG_HOMEDRIVE = process.env.HOMEDRIVE; const ORIG_HOMEPATH = process.env.HOMEPATH; const ORIG_CI = process.env.CI; +// `arkor logout` itself doesn't read CLAUDECODE (since `isInteractive()` +// was reverted to its pre-PR shape), but vitest workers spawned from a +// Claude Code session inherit `CLAUDECODE=1`. Strip it in beforeEach +// (and restore from ORIG_CLAUDECODE in afterEach) so any future +// strict-mode wiring added to logout doesn't surprise these tests, and +// so the test environment matches what CI runners see. +const ORIG_CLAUDECODE = process.env.CLAUDECODE; // Capture the original `process.stdout.isTTY` so the interactive test // below can flip it without leaking into other test files when vitest // reuses a worker process. @@ -56,6 +63,7 @@ beforeEach(() => { // promptConfirm honours skipWith first, but in non-skip paths it falls // through to initialValue when CI=1. Pin CI so the prompt never opens. process.env.CI = "1"; + delete process.env.CLAUDECODE; }); afterEach(() => { @@ -69,6 +77,8 @@ afterEach(() => { else delete process.env.HOMEPATH; if (ORIG_CI !== undefined) process.env.CI = ORIG_CI; else delete process.env.CI; + if (ORIG_CLAUDECODE !== undefined) process.env.CLAUDECODE = ORIG_CLAUDECODE; + else delete process.env.CLAUDECODE; // Restore the TTY flag in case the interactive test mutated it — // otherwise a later test that unsets CI would unexpectedly enter // interactive prompt paths. diff --git a/packages/arkor/src/cli/main.test.ts b/packages/arkor/src/cli/main.test.ts index dea73cc6..452ce725 100644 --- a/packages/arkor/src/cli/main.test.ts +++ b/packages/arkor/src/cli/main.test.ts @@ -43,10 +43,15 @@ import { runWhoami } from "./commands/whoami"; import { shutdownTelemetry } from "../core/telemetry"; import { main } from "./main"; -// Capture once so the after-each can restore — without this, tests that +// Capture once so the after-each can restore. Without this, tests that // set `npm_config_user_agent` to drive package-manager detection leak the // override into later test files when vitest reuses a worker process. const ORIG_USER_AGENT = process.env.npm_config_user_agent; +// Same rationale for CLAUDECODE: the new strict-mode tests below set it +// to "1" and the worker may already have it set (vitest spawned from a +// Claude Code session), so afterEach restores rather than unconditionally +// deletes. +const ORIG_CLAUDECODE = process.env.CLAUDECODE; beforeEach(() => { vi.mocked(runInit).mockReset(); @@ -67,6 +72,11 @@ afterEach(() => { } else { delete process.env.npm_config_user_agent; } + if (ORIG_CLAUDECODE !== undefined) { + process.env.CLAUDECODE = ORIG_CLAUDECODE; + } else { + delete process.env.CLAUDECODE; + } vi.restoreAllMocks(); }); @@ -218,6 +228,59 @@ describe("main (CLI Commander wiring)", () => { expect(shutdownTelemetry).toHaveBeenCalledOnce(); }); + it("under CLAUDECODE=1 + missing flags, throws ClaudeCodeStrictExit so the finally block still runs (shutdownTelemetry fires, no project name skip)", async () => { + // ENG-736 PR review (#141): the validator used to `process.exit(1)` + // inside the action handler, which bypassed main()'s finally and + // lost telemetry / deprecation flushing. The validator now throws + // a sentinel so the finally still runs; bin.ts is the layer that + // recognises the sentinel and sets exitCode without re-printing. + // afterEach restores CLAUDECODE from ORIG_CLAUDECODE, so the + // explicit `delete` we used to do in the test's own `finally` + // would have leaked into later tests when vitest was launched + // from a Claude Code session (CLAUDECODE already set in the + // parent worker). + process.env.CLAUDECODE = "1"; + const stderrChunks: string[] = []; + const spy = vi + .spyOn(process.stderr, "write") + .mockImplementation(((c: unknown) => { + stderrChunks.push(String(c)); + return true; + }) as typeof process.stderr.write); + try { + await expect( + main(["init", "--template", "triage", "--skip-git"]), + // Missing: pm flag + agents-md flag. + ).rejects.toMatchObject({ name: "ClaudeCodeStrictExit" }); + } finally { + spy.mockRestore(); + } + expect(shutdownTelemetry).toHaveBeenCalledOnce(); + const buf = stderrChunks.join(""); + expect(buf).toContain("arkor init: CLAUDECODE=1 detected"); + expect(buf).toContain("--use-pnpm"); + expect(buf).toContain("--agents-md (recommended) or --no-agents-md"); + expect(runInit).not.toHaveBeenCalled(); + }); + + it("under CLAUDECODE=1 with every flag set (no --yes), passes the strict check and delegates to runInit", async () => { + // The happy-path mirror of the test above: a fully-specified + // invocation still runs to completion under CLAUDECODE, since the + // validator is gated on missing flags only. + process.env.CLAUDECODE = "1"; + await main([ + "init", + "--name", + "my-app", + "--template", + "triage", + "--skip-git", + "--skip-install", + "--no-agents-md", + ]); + expect(runInit).toHaveBeenCalledOnce(); + }); + it("omits the Cutoff suffix when the deprecation has no sunset value", async () => { // Branch coverage for the `notice.sunset ? ` Cutoff: …` : ""` ternary. mockDeprecation.value = { diff --git a/packages/arkor/src/cli/main.ts b/packages/arkor/src/cli/main.ts index 65340fa8..d54b6637 100644 --- a/packages/arkor/src/cli/main.ts +++ b/packages/arkor/src/cli/main.ts @@ -6,7 +6,14 @@ import { runInit } from "./commands/init"; import { runDev } from "./commands/dev"; import { runBuild } from "./commands/build"; import { runStart } from "./commands/start"; -import { resolvePackageManager, type TemplateId } from "@arkor/cli-internal"; +import { + ClaudeCodeStrictExit, + formatClaudeCodeMissingMessage, + isClaudeCode, + missingClaudeCodeFlags, + resolvePackageManager, + type TemplateId, +} from "@arkor/cli-internal"; import { getRecordedDeprecation } from "../core/deprecation"; import { shutdownTelemetry, withTelemetry } from "../core/telemetry"; import { detectedUpgradeCommand } from "../core/upgrade-hint"; @@ -80,6 +87,49 @@ export async function main(argv: string[]): Promise { "Pick one of --agents-md / --no-agents-md, not both.", ); } + // Under CLAUDECODE=1, refuse to fall through to interactive prompts + // (they'd hang) or to silent defaults (they'd hide decisions the + // agent should be making). Print the suggested re-invocation and + // exit 1 so the agent can re-run with explicit flags. `agentsMd` + // is checked from raw argv so default-on doesn't satisfy the + // requirement: the agent should opt in or out deliberately. + if (isClaudeCode()) { + const agentsMdSpecified = + flagsArgv.includes("--agents-md") || + flagsArgv.includes("--no-agents-md"); + const missing = missingClaudeCodeFlags({ + yes: opts.yes, + template: opts.template, + git: opts.git, + skipGit: opts.skipGit, + skipInstall: opts.skipInstall, + useNpm: opts.useNpm, + usePnpm: opts.usePnpm, + useYarn: opts.useYarn, + useBun: opts.useBun, + name: opts.name, + agentsMd: agentsMdSpecified ? opts.agentsMd ?? true : undefined, + // arkor init operates on `process.cwd()`; basename(cwd) is + // the runtime default for the project name, so strict mode + // doesn't require an explicit `--name`. Passing `initCwd` + // lets the validator reject the pathological case where + // the cwd basename itself has no alphanumerics (e.g. + // `/tmp/!!!/`), which would otherwise sanitise to the + // generic `arkor-project` fallback. + requireProjectName: false, + initCwd: process.cwd(), + }); + if (missing.length > 0) { + process.stderr.write( + formatClaudeCodeMissingMessage("arkor init", missing), + ); + // Throw (don't `process.exit`) so the `finally` at the bottom of + // main() still runs telemetry shutdown / the deprecation notice. + // bin.ts recognises this sentinel and exits without re-printing + // the message. + throw new ClaudeCodeStrictExit(); + } + } const packageManager = resolvePackageManager({ useNpm: opts.useNpm, usePnpm: opts.usePnpm, diff --git a/packages/arkor/src/cli/prompts.test.ts b/packages/arkor/src/cli/prompts.test.ts index 0f99c034..82569341 100644 --- a/packages/arkor/src/cli/prompts.test.ts +++ b/packages/arkor/src/cli/prompts.test.ts @@ -31,12 +31,14 @@ import { const CLACK_CANCEL = Symbol.for("clack:cancel"); const ORIG_CI = process.env.CI; +const ORIG_CLAUDECODE = process.env.CLAUDECODE; const ORIG_TTY = process.stdout.isTTY; beforeEach(() => { // Default to non-interactive: CI set OR no TTY. Individual tests can flip // the toggle. process.env.CI = "1"; + delete process.env.CLAUDECODE; Object.defineProperty(process.stdout, "isTTY", { value: false, configurable: true, @@ -46,6 +48,8 @@ beforeEach(() => { afterEach(() => { if (ORIG_CI === undefined) delete process.env.CI; else process.env.CI = ORIG_CI; + if (ORIG_CLAUDECODE === undefined) delete process.env.CLAUDECODE; + else process.env.CLAUDECODE = ORIG_CLAUDECODE; Object.defineProperty(process.stdout, "isTTY", { value: ORIG_TTY, configurable: true, @@ -80,6 +84,23 @@ describe("isInteractive", () => { }); expect(isInteractive()).toBe(true); }); + + it("does NOT special-case CLAUDECODE (only init / create-arkor opt in to that branch via their own check, so the shared helper stays neutral for other commands)", () => { + // ENG-736 PR review (#141): forcing this helper to false under + // CLAUDECODE=1 leaked into commands that don't have strict-mode + // validation (e.g. `arkor logout` would silently delete credentials + // because its `promptConfirm` would fall through to + // `initialValue: true`). The CLAUDECODE awareness now lives in + // `runInit` / create-arkor's `run()` instead, gated to the + // already-strict scaffold commands. + delete process.env.CI; + Object.defineProperty(process.stdout, "isTTY", { + value: true, + configurable: true, + }); + process.env.CLAUDECODE = "1"; + expect(isInteractive()).toBe(true); + }); }); describe("promptText", () => { diff --git a/packages/cli-internal/src/claude-code.test.ts b/packages/cli-internal/src/claude-code.test.ts new file mode 100644 index 00000000..8354f7bf --- /dev/null +++ b/packages/cli-internal/src/claude-code.test.ts @@ -0,0 +1,423 @@ +import { mkdirSync, mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { + formatClaudeCodeMissingMessage, + isClaudeCode, + missingClaudeCodeFlags, +} from "./claude-code"; + +const ORIG = process.env.CLAUDECODE; + +beforeEach(() => { + delete process.env.CLAUDECODE; +}); + +afterEach(() => { + if (ORIG === undefined) delete process.env.CLAUDECODE; + else process.env.CLAUDECODE = ORIG; +}); + +describe("isClaudeCode", () => { + it("returns true only when CLAUDECODE === '1'", () => { + process.env.CLAUDECODE = "1"; + expect(isClaudeCode()).toBe(true); + }); + + it("returns false when CLAUDECODE is unset", () => { + expect(isClaudeCode()).toBe(false); + }); + + it("returns false for any other value (e.g. 'true', '0', '')", () => { + // The contract is exact-string match on "1": Claude Code itself + // sets `CLAUDECODE=1` literally. Loosening this risks accidentally + // tripping the strict mode for unrelated CI envs that happen to set + // CLAUDECODE for some other reason. + for (const v of ["true", "0", "", "yes", "false"]) { + process.env.CLAUDECODE = v; + expect(isClaudeCode()).toBe(false); + } + }); +}); + +describe("missingClaudeCodeFlags", () => { + it("returns empty when --yes is set, regardless of other flags", () => { + expect(missingClaudeCodeFlags({ yes: true })).toEqual([]); + }); + + it("requires --template, --git/--skip-git, a pm flag, and an agents-md flag (init mode)", () => { + const missing = missingClaudeCodeFlags({ requireProjectName: false }); + expect(missing.map((m) => m.flag)).toEqual([ + "--template ", + "--git (recommended) or --skip-git", + "--use-pnpm (or --use-npm / --use-yarn / --use-bun, or --skip-install)", + "--agents-md (recommended) or --no-agents-md", + ]); + // Every entry pairs the flag with a non-empty description so the + // stderr block is self-explanatory. + for (const m of missing) { + expect(m.description.length).toBeGreaterThan(0); + } + }); + + it("additionally requires [dir] / --name when requireProjectName is true (create-arkor mode)", () => { + const missing = missingClaudeCodeFlags({ requireProjectName: true }); + expect(missing[0]?.flag).toMatch(/\[dir\]/); + }); + + it("[dir] requirement is satisfied by an explicit positional", () => { + const missing = missingClaudeCodeFlags({ + requireProjectName: true, + dir: "my-app", + template: "triage", + git: true, + skipInstall: true, + agentsMd: true, + }); + expect(missing).toEqual([]); + }); + + it("[dir] requirement is satisfied by --name", () => { + const missing = missingClaudeCodeFlags({ + requireProjectName: true, + name: "my-app", + template: "triage", + skipGit: true, + useNpm: true, + agentsMd: false, + }); + expect(missing).toEqual([]); + }); + + it("rejects --name values that sanitise() collapses to the `arkor-project` fallback", () => { + // The strict-mode contract is that the agent commits to a real + // project name. `--name ""` / `--name "!!!"` / `--name " "` all + // pass `sanitise()` through the empty-string fallback path and end + // up as `arkor-project` in `package.json`, which is exactly the + // silent default we want to surface. + for (const name of ["", " ", "!!!", "***", "@@@"]) { + const missing = missingClaudeCodeFlags({ + requireProjectName: true, + name, + template: "triage", + skipGit: true, + useNpm: true, + agentsMd: false, + }); + expect(missing.map((m) => m.flag)).toContain( + "[dir] (e.g. `my-arkor-app`) or --name ", + ); + } + }); + + it("rejects [dir] basenames that sanitise() collapses to the fallback", () => { + // Same trap via the positional: `create-arkor " "` would otherwise + // count as a satisfied project-name requirement even though it + // also lands on `arkor-project`. + const missing = missingClaudeCodeFlags({ + requireProjectName: true, + dir: "!!!", + template: "triage", + skipGit: true, + useNpm: true, + agentsMd: false, + }); + expect(missing.map((m) => m.flag)).toContain( + "[dir] (e.g. `my-arkor-app`) or --name ", + ); + }); + + it.each([ + ["empty --name", ""], + ["whitespace --name", " "], + ["punctuation-only --name", "!!!"], + ])( + "in init mode (requireProjectName=false), still rejects %s because the validator surfaces explicit-but-garbage names", + (_label, name) => { + // PR #141 review (Copilot): under `requireProjectName: false` + // the old check skipped the helper entirely, so + // `arkor init --name "!!!" --template ...` passed strict mode + // and `runInit` silently sanitised the value to the generic + // `arkor-project` fallback. The validator now treats an + // explicit `--name` as a deliberate commitment regardless of + // `requireProjectName`, and asks the agent to pick a value with + // at least one ASCII letter or digit (or drop `--name`). + const missing = missingClaudeCodeFlags({ + requireProjectName: false, + name, + template: "triage", + skipGit: true, + useNpm: true, + agentsMd: false, + }); + expect(missing.map((m) => m.flag)).toContain("--name "); + }, + ); + + it("in init mode, accepts the absence of --name (runtime uses basename(cwd))", () => { + // Mirror of the test above: omitting `--name` is the documented + // default path for `arkor init`, so strict mode must NOT demand + // it. Pin this branch so a regression that forces `--name` + // everywhere is caught immediately. + const missing = missingClaudeCodeFlags({ + requireProjectName: false, + template: "triage", + skipGit: true, + useNpm: true, + agentsMd: false, + }); + expect(missing).toEqual([]); + }); + + it.each([ + ["punctuation-only cwd basename", "/tmp/!!!"], + ["whitespace-only cwd basename", "/tmp/ "], + ["empty cwd basename (filesystem root)", "/"], + ])( + "in init mode without --name, rejects %s because basename(cwd) would collapse to the fallback", + (_label, initCwd) => { + // PR #141 review (Copilot): `arkor init` without `--name` + // derives the project name from `basename(process.cwd())`. If + // the user runs the CLI from `/tmp/!!!/` or similar, strict + // mode used to return early (no `--name` to validate) and + // `runInit` silently sanitised the basename to the generic + // `arkor-project` fallback. The validator now mirrors the + // explicit `--name` check against `basename(initCwd)`. + const missing = missingClaudeCodeFlags({ + requireProjectName: false, + initCwd, + template: "triage", + skipGit: true, + useNpm: true, + agentsMd: false, + }); + expect(missing.map((m) => m.flag)).toContain("--name "); + }, + ); + + it("in init mode, accepts a meaningful cwd basename without --name", () => { + const missing = missingClaudeCodeFlags({ + requireProjectName: false, + initCwd: "/home/alice/my-arkor-app", + template: "triage", + skipGit: true, + useNpm: true, + agentsMd: false, + }); + expect(missing).toEqual([]); + }); + + it("rejects empty positional even when --name is set (positional drives the runtime target dir)", () => { + // PR #141 review (Copilot): `create-arkor` uses `opts.dir` for the + // *target directory*, not just the slug. So + // `CLAUDECODE=1 create-arkor "" --name my-app --template ...` + // would otherwise pass strict mode (because `--name` is meaningful) + // and then scaffold in-place under the parent dir at runtime via + // `resolve("")`. The empty-positional guard now fires before the + // `--name` shortcut so this mis-input is rejected up front. + const missing = missingClaudeCodeFlags({ + requireProjectName: true, + dir: "", + name: "my-app", + template: "triage", + skipGit: true, + useNpm: true, + agentsMd: false, + }); + expect(missing.map((m) => m.flag)).toContain( + "[dir] (e.g. `my-arkor-app`) or --name ", + ); + }); + + it.each([ + ["empty positional", ""], + ["whitespace-only positional", " "], + ["tab-only positional", "\t"], + ])("rejects %s before resolve()", (_label, dir) => { + // PR #141 review (Copilot): the motivating case is the EMPTY + // string. `path.resolve("")` returns `process.cwd()` whose + // basename is usually alphanumeric, so without the trim guard + // `create-arkor "" --template ...` (or a quoted empty shell + // variable) would slip through and scaffold against the cwd + // basename. Whitespace-only inputs would also be rejected by the + // downstream alphanumeric regex on their own (since + // `resolve(" ")` keeps the spaces as a basename), but exercising + // them here pins the guard's contract: trim semantics apply to + // *every* empty-shaped input, not only `""`. `.` / `./` are + // covered elsewhere as deliberate "scaffold in this directory" + // idioms. + const missing = missingClaudeCodeFlags({ + requireProjectName: true, + dir, + template: "triage", + skipGit: true, + useNpm: true, + agentsMd: false, + }); + expect(missing.map((m) => m.flag)).toContain( + "[dir] (e.g. `my-arkor-app`) or --name ", + ); + }); + + it("resolves [dir] before taking the basename so `.` / `..` are treated like the surrounding directory", () => { + // Regression for PR #141 review (codex + Copilot): `basename(".")` + // is `"."` which sanitise() collapses to the fallback, so the + // previous implementation falsely rejected `create-arkor . + // --template ...`. Mirroring `basename(resolve(opts.dir))` (what + // the scaffolder itself uses to derive `defaultName`) makes the + // strict check agree with the runtime: `.` / `..` resolve to the + // current / parent directory's basename, and only then are + // sanitised. + // + // The two `.`/`..` cases used to lean on `process.cwd()` directly, + // which made the test passively dependent on the worker checkout + // path having an alphanumeric basename (PR #141 review: a checkout + // under e.g. `/tmp/!!!/` would have falsely failed strict mode + // here). Pin a controlled cwd inside a fresh tmp dir whose + // basename ends in a guaranteed-alphanumeric prefix so the + // assertion exercises the `resolve()`-then-`basename()` derivation + // without depending on ambient state. + const baseOpts = { + requireProjectName: true, + template: "triage", + skipGit: true, + useNpm: true, + agentsMd: false, + } as const; + const tmpRoot = mkdtempSync(join(tmpdir(), "arkor-strict-dot-")); + // `mkdtempSync` appends 6 random alphanumeric characters; the + // basename is therefore non-empty even if `tmpdir()` itself happens + // to land somewhere weird. Create a child so the `..` case also + // resolves to a known meaningful basename (`tmpRoot` itself). + const child = join(tmpRoot, "child"); + mkdirSync(child); + const ORIG_CWD = process.cwd(); + try { + process.chdir(child); + // `.` resolves to `child`, basename `child`. Alphanumeric. + expect(missingClaudeCodeFlags({ ...baseOpts, dir: "." })).toEqual([]); + expect(missingClaudeCodeFlags({ ...baseOpts, dir: "./" })).toEqual([]); + // `..` resolves to `tmpRoot`, basename starts with + // `arkor-strict-dot-` so it always satisfies the + // alphanumeric requirement regardless of which dir vitest + // happened to be launched from. + expect(missingClaudeCodeFlags({ ...baseOpts, dir: ".." })).toEqual([]); + // Relative path that resolves to a meaningful basename also + // passes. This case never depended on cwd-basename letters since + // the trailing segment (`my-app`) is what `basename(resolve(...))` + // returns, but pin it here too for parity. + expect( + missingClaudeCodeFlags({ ...baseOpts, dir: "foo/bar/my-app" }), + ).toEqual([]); + } finally { + process.chdir(ORIG_CWD); + rmSync(tmpRoot, { recursive: true, force: true }); + } + }); + + it("accepts deliberate names that happen to sanitise to `arkor-project`", () => { + // PR #141 review (codex + Copilot): the previous check compared the + // raw input to the literal `arkor-project` string, so inputs like + // `Arkor Project`, `arkor_project`, or a `[dir]` basename + // `Arkor.Project` were rejected because their trimmed/lowercased + // raw form differed from the sanitised slug. They are deliberate + // names, not silent defaults; the rewritten check (input contains + // any `[a-z0-9]`) accepts them because the alphanumeric content + // proves the user typed a real name. Every entry below sanitises + // exactly to `arkor-project` (separator rewritten to `-`) which + // is what makes this test about the *fallback collision*; names + // with no separator (e.g. `ArkorProject` → `arkorproject`) are + // covered by the sibling "any alphanumeric content" test so the + // theme of each test stays honest. + for (const name of [ + "arkor-project", + "ARKOR-PROJECT", + " arkor-project ", + "Arkor Project", + "arkor_project", + "Arkor.Project", + ]) { + const missing = missingClaudeCodeFlags({ + requireProjectName: true, + name, + template: "triage", + skipGit: true, + useNpm: true, + agentsMd: false, + }); + expect(missing).toEqual([]); + } + }); + + it("accepts deliberate names whose sanitised slug is unrelated to `arkor-project`", () => { + // Sibling case to the fallback-collision test above: deliberate + // names that contain alphanumerics but have no separator (so + // `sanitise()` lowercases them straight through, producing + // `arkorproject` etc.) are also accepted. The check is "contains + // an alphanumeric", not "sanitised slug equals X". + for (const name of ["ArkorProject", "MyApp", "v2"]) { + const missing = missingClaudeCodeFlags({ + requireProjectName: true, + name, + template: "triage", + skipGit: true, + useNpm: true, + agentsMd: false, + }); + expect(missing).toEqual([]); + } + }); + + it("counts --skip-install as satisfying the package-manager requirement", () => { + const missing = missingClaudeCodeFlags({ + template: "triage", + git: true, + skipInstall: true, + agentsMd: true, + }); + expect(missing).toEqual([]); + }); + + it("counts --skip-git as satisfying the git requirement", () => { + const missing = missingClaudeCodeFlags({ + template: "triage", + skipGit: true, + usePnpm: true, + agentsMd: true, + }); + expect(missing).toEqual([]); + }); + + it("treats --no-agents-md (agentsMd === false) as a valid explicit choice", () => { + const missing = missingClaudeCodeFlags({ + template: "triage", + skipGit: true, + skipInstall: true, + agentsMd: false, + }); + expect(missing).toEqual([]); + }); +}); + +describe("formatClaudeCodeMissingMessage", () => { + it("renders a stderr block with the command name, every missing flag + description, and the --yes escape hatch", () => { + const out = formatClaudeCodeMissingMessage("arkor init", [ + { + flag: "--template ", + description: "Pick the starter template.", + }, + { + flag: "--git or --skip-git", + description: "Init a git repo or skip git.", + }, + ]); + expect(out).toContain("arkor init: CLAUDECODE=1 detected"); + expect(out).toContain("--template "); + expect(out).toContain("Pick the starter template."); + expect(out).toContain("--git or --skip-git"); + expect(out).toContain("Init a git repo or skip git."); + expect(out).toContain("-y/--yes"); + expect(out.endsWith("\n")).toBe(true); + }); +}); diff --git a/packages/cli-internal/src/claude-code.ts b/packages/cli-internal/src/claude-code.ts new file mode 100644 index 00000000..0e9e8c37 --- /dev/null +++ b/packages/cli-internal/src/claude-code.ts @@ -0,0 +1,274 @@ +import { basename, resolve } from "node:path"; + +/** + * Claude Code (the Anthropic agent CLI) sets `CLAUDECODE=1` in every spawned + * shell. It cannot answer interactive prompts, and silently falling through to + * defaults masks decisions the agent should be making explicitly. So the + * scaffolders (`create-arkor`, `arkor init`) flip into a strict mode under + * this env var: a curated set of flags becomes required, and missing ones + * produce a stderr message listing the suggested re-invocation rather than + * running with hidden defaults. + * + * "Curated" (not "every interactive prompt") because the runtime defaults + * for a couple of decisions are well-understood enough that forcing them + * would just add noise: + * + * - **Project name** is not required by `arkor init`: the scaffolder + * derives it from `basename(cwd)`, which is the same value the + * interactive prompt offers. `create-arkor` *does* require it because + * a missing `[dir]` falls all the way back to the generic + * `arkor-project` slug, which is almost always an oversight. + * - **Package manager** is not actually prompted for at runtime; it's + * a defaulted decision (UA-sniffed, or skipped silently when + * detection fails). It's required here because the agent should + * pick `--use-*` vs. `--skip-install` deliberately, not because the + * interactive UX would have asked. + * + * `--yes` opts back into the legacy "accept all defaults" path; callers that + * have explicitly delegated those decisions to the CLI keep working unchanged. + */ +export function isClaudeCode(): boolean { + return process.env.CLAUDECODE === "1"; +} + +/** + * Thrown by the CLI strict-mode validator after it has already written + * the human-readable missing-flags block to stderr. Both consumers + * (`arkor` and `create-arkor`) catch this class at the outermost layer + * and exit `1` *without* re-printing the message or its stack; what + * each call path gains by throwing instead of `process.exit(1)`-ing + * inside the action handler differs: + * + * - `arkor`: the throw unwinds through `program.parseAsync()` so the + * `finally` in `main()` runs (telemetry flush, deprecation notice). + * A bare `process.exit(1)` inside the action would have skipped it. + * - `create-arkor`: there is no `main()` / telemetry path to preserve, + * but the outer `program.parseAsync(...).catch(...)` prefixes every + * non-sentinel error with `"create-arkor failed: "`. Recognising the + * sentinel lets the catch exit silently so that prefix does not + * double up on the multi-line missing-flags block already on stderr. + * + * Both consumers set `process.exitCode = 1` (rather than calling + * `process.exit(1)`) **for the strict-mode path specifically** so Node + * lets the event loop drain naturally before exiting; on piped stdio + * that prevents truncating queued writes, which on the strict-mode + * path would lose the trailing lines of the missing-flags block. + * Other failure paths in `create-arkor` (where `run()` may have + * started a `clack.spinner()`) still use `process.exit(1)` so a + * lingering UI interval can't keep the event loop alive past the + * catch. + */ +export class ClaudeCodeStrictExit extends Error { + constructor() { + super("CLAUDECODE strict-mode early exit"); + this.name = "ClaudeCodeStrictExit"; + } +} + +export interface ClaudeCodeOptionsCheck { + /** True when `--yes`/`-y` was passed; bypasses the missing-flag check. */ + yes?: boolean; + /** Resolved template id, or undefined when `--template` was not passed. */ + template?: string; + /** True when `--git` was passed. */ + git?: boolean; + /** True when `--skip-git` was passed. */ + skipGit?: boolean; + /** True when `--skip-install` was passed. */ + skipInstall?: boolean; + /** True when any `--use-` flag was passed. */ + useNpm?: boolean; + usePnpm?: boolean; + useYarn?: boolean; + useBun?: boolean; + /** Project name (`--name`), or undefined. */ + name?: string; + /** + * Positional `[dir]` (create-arkor only). Pass `undefined` for `arkor init` + * which always operates on `process.cwd()`. + */ + dir?: string; + /** + * `false` only when `--no-agents-md` was passed; `true` when `--agents-md` + * was passed; `undefined` when neither flag was set. The default-on + * resolution lives in the CLI action handler. + */ + agentsMd?: boolean; + /** + * Whether the caller wants `[dir]` / `--name` to be required. `arkor init` + * uses `basename(cwd)` as a meaningful default and sets this to false; the + * `create-arkor` scaffolder defaults a missing `[dir]` to a generic + * `arkor-project/` subdirectory and sets this to true so the agent picks a + * project name deliberately. + */ + requireProjectName?: boolean; + /** + * The working directory `arkor init` would derive its default project + * name from (`basename(initCwd)`). Used only when `requireProjectName` + * is false and `--name` was not passed: the strict-mode validator + * runs the same alphanumeric check it applies to explicit `--name` + * values against this basename, so an agent running `arkor init` in a + * directory like `/tmp/!!!/` doesn't silently collapse to the + * `arkor-project` fallback. Callers pass `process.cwd()` here; we + * take it as a parameter rather than reading `process.cwd()` inside + * this helper so init-path tests can pin a deterministic cwd + * without `chdir`-ing the vitest worker. (The create-arkor branch + * still calls `resolve(opts.dir)` for its own check, which leans on + * the real `process.cwd()` for relative paths like `.`/`..`; this + * helper isn't fully cwd-free, just decoupled where the init path + * needs it most.) + */ + initCwd?: string; +} + +export interface MissingClaudeCodeFlag { + /** Flag (or flag group) the agent should add to the next invocation. */ + flag: string; + /** One-sentence explanation of what the flag controls. */ + description: string; +} + +/** + * True when `opts` supplies a project name that survives `sanitise()` + * without falling back to the generic `arkor-project` default. The + * fallback only fires when `sanitise()`'s pre-fallback slug is empty, + * which happens iff the input contains zero `[a-z0-9]` characters + * (case-insensitive). So an input is "meaningful" iff it has at least + * one alphanumeric character — this single check captures every silent + * default we want strict mode to surface (empty strings, whitespace + * only, punctuation only) without falsely rejecting deliberate names + * that happen to *sanitise* to `arkor-project` such as `Arkor Project` + * or `arkor_project`. + * + * `[dir]` is derived through `basename(resolve(opts.dir))` to mirror + * `create-arkor`'s own default-name derivation. Without `resolve()`, + * `basename(".")` returns `"."` (no alphanumerics) and the validator + * would falsely reject explicit invocations like + * `create-arkor . --template ...` even though the scaffolder would + * pick up the current directory's name at runtime. + */ +function hasMeaningfulProjectName(opts: ClaudeCodeOptionsCheck): boolean { + // Reject an empty / whitespace-only positional even when `--name` + // is set, because `create-arkor`'s runtime uses `opts.dir` for the + // **target directory** (not just the project name): `resolve("")` + // returns `process.cwd()`, so `create-arkor "" --name my-app + // --template ...` would scaffold *in-place* under the parent dir + // instead of into `./my-app/`. Strict mode should surface that + // mis-input, not let `--name` mask it. Whitespace-only inputs + // (`" "`, `"\t"`) survive `resolve()` as a whitespace-basename + // path and would also be caught by the alphanumeric check below, + // but the early trim guard handles both shapes uniformly without + // depending on downstream regex behaviour. `.` / `./` etc. still + // pass because they are deliberate "scaffold in this directory" + // idioms with the same runtime semantics as any non-empty path. + if (opts.dir !== undefined && opts.dir.trim() === "") return false; + // An explicitly-passed `--name` is validated even for `arkor init` + // (where `requireProjectName` is false). The reasoning: when the + // agent went out of its way to write `--name `, that value + // is its intended project name, so if `value` would collapse to + // the silent `arkor-project` fallback inside `sanitise()` the + // mis-input should surface here instead of being masked. + if (opts.name !== undefined) return /[a-z0-9]/i.test(opts.name); + // No `--name` at all: only `create-arkor` (requireProjectName=true) + // needs a meaningful `[dir]`; `arkor init` falls back to + // `basename(initCwd)` at runtime. We still validate that basename so + // an agent running `arkor init` inside `/tmp/!!!/` doesn't silently + // collapse to the generic `arkor-project` fallback that strict mode + // rejects for explicit `--name` and `[dir]` values. Callers may omit + // `initCwd` (e.g. tests), in which case we conservatively accept. + if (!opts.requireProjectName) { + if (opts.initCwd === undefined) return true; + return /[a-z0-9]/i.test(basename(opts.initCwd)); + } + if (opts.dir === undefined) return false; + return /[a-z0-9]/i.test(basename(resolve(opts.dir))); +} + +/** + * Returns the list of missing flags under CLAUDECODE=1, each paired with a + * short description of what it controls, so the stderr message is + * self-explanatory and the agent can pick a value without round-tripping to + * the docs. Empty array means the invocation is fully specified (or `--yes` + * opted out). + */ +export function missingClaudeCodeFlags( + opts: ClaudeCodeOptionsCheck, +): MissingClaudeCodeFlag[] { + if (opts.yes) return []; + const missing: MissingClaudeCodeFlag[] = []; + if (!hasMeaningfulProjectName(opts)) { + // The two consumers want slightly different prompts: `create-arkor` + // is the only one that demands an explicit name (positional or + // `--name`), so its message lists both. `arkor init` falls back to + // `basename(cwd)` when `--name` is unset, but if the agent did pass + // `--name ` strict mode still trips here; surface it as + // a name-only ask rather than asking for a `[dir]` it doesn't have. + if (opts.requireProjectName) { + missing.push({ + flag: "[dir] (e.g. `my-arkor-app`) or --name ", + description: + "Project directory (positional) and the `package.json` name. Without `--name`, the basename of `[dir]` is used. Inputs with no ASCII letter or digit (empty, whitespace-only, or punctuation-only like `!!!` / `***`) are rejected because they would collapse to the generic `arkor-project` fallback inside `sanitise()`.", + }); + } else { + missing.push({ + flag: "--name ", + description: + "Strict mode could not derive a meaningful project name. Either `--name` was passed but its value contains no ASCII letter or digit (so `sanitise()` would collapse it to the generic `arkor-project` fallback), or `--name` was omitted and the current directory's basename has no alphanumerics either. Pass `--name ` with at least one ASCII letter or digit.", + }); + } + } + if (opts.template === undefined) { + missing.push({ + flag: "--template ", + description: + "Starter template: `triage` (support routing), `translate` (9-language translation), or `redaction` (PII removal).", + }); + } + if (!opts.git && !opts.skipGit) { + missing.push({ + flag: "--git (recommended) or --skip-git", + description: + "`--git` runs `git init` and creates an initial commit (matches the interactive default); `--skip-git` leaves git setup to the user.", + }); + } + const pmFlagSet = + opts.useNpm || opts.usePnpm || opts.useYarn || opts.useBun || opts.skipInstall; + if (!pmFlagSet) { + missing.push({ + flag: "--use-pnpm (or --use-npm / --use-yarn / --use-bun, or --skip-install)", + description: + "Which package manager to run `install` with after scaffolding. `--skip-install` leaves the install step to the user.", + }); + } + if (opts.agentsMd === undefined) { + missing.push({ + flag: "--agents-md (recommended) or --no-agents-md", + description: + "`--agents-md` writes `AGENTS.md` + `CLAUDE.md` to brief AI coding agents that arkor post-dates their training data (recommended, especially under CLAUDECODE); `--no-agents-md` skips them.", + }); + } + return missing; +} + +/** + * Render the stderr block printed when `missingClaudeCodeFlags` returned + * a non-empty list. Kept as a pure function so both CLIs share the wording + * and tests can assert on it without invoking the action handler. Each + * missing flag is rendered on two lines (flag, then indented description) + * to avoid alignment churn when the longest flag changes. + */ +export function formatClaudeCodeMissingMessage( + command: string, + missing: MissingClaudeCodeFlag[], +): string { + const lines = [ + `${command}: CLAUDECODE=1 detected. Interactive prompts are disabled.`, + "Re-run with explicit flags:", + ]; + for (const m of missing) { + lines.push(` ${m.flag}`); + lines.push(` ${m.description}`); + } + lines.push("Or pass -y/--yes to accept all defaults."); + return `${lines.join("\n")}\n`; +} diff --git a/packages/cli-internal/src/index.ts b/packages/cli-internal/src/index.ts index c0d2dcd0..8185cce7 100644 --- a/packages/cli-internal/src/index.ts +++ b/packages/cli-internal/src/index.ts @@ -24,3 +24,11 @@ export { type InitialCommitResult, } from "./git"; export { sanitise } from "./sanitise"; +export { + ClaudeCodeStrictExit, + isClaudeCode, + missingClaudeCodeFlags, + formatClaudeCodeMissingMessage, + type ClaudeCodeOptionsCheck, + type MissingClaudeCodeFlag, +} from "./claude-code"; diff --git a/packages/cli-internal/src/sanitise.test.ts b/packages/cli-internal/src/sanitise.test.ts index 8798d8ed..80da6964 100644 --- a/packages/cli-internal/src/sanitise.test.ts +++ b/packages/cli-internal/src/sanitise.test.ts @@ -6,10 +6,17 @@ describe("sanitise", () => { expect(sanitise("MyApp")).toBe("myapp"); }); - it("replaces non `[a-z0-9-]` characters with dashes", () => { + it("replaces non `[a-z0-9]` runs with a single dash", () => { expect(sanitise("hello world")).toBe("hello-world"); expect(sanitise("Hello! World")).toBe("hello-world"); expect(sanitise("foo_bar.baz")).toBe("foo-bar-baz"); + // Adjacent runs of non-alphanumerics (including mixed `-` and + // other punctuation) collapse to one dash in a single pass. + // This matters for the ReDoS-safety contract documented in + // `sanitise.ts`: the chain no longer relies on a separate `-+` + // collapse step that walked the same characters twice. + expect(sanitise("a---b")).toBe("a-b"); + expect(sanitise("a-_-b")).toBe("a-b"); }); it("collapses leading and trailing dashes after substitution", () => { @@ -17,6 +24,23 @@ describe("sanitise", () => { expect(sanitise("---abc---")).toBe("abc"); }); + it("returns the fallback on a 100k-char dash run (CodeQL hardening anchor)", () => { + // CodeQL's "polynomial regex on uncontrolled data" query flagged + // the previous chain because `sanitise()` sits on the + // `--name` / `[dir]` data flow from both CLI scaffolders. The + // original `+`-quantified replaces were already linear in + // practice, so this isn't a real ReDoS regression test; the + // 100k-char input is an output-correctness assertion that + // doubles as a smoke check that the rewritten chain still + // handles long, mostly-separator inputs (which was the alert's + // pathological shape) without any change in observable result. + // Vitest's per-file timeout catches a truly exponential rewrite + // for free, so no fixed wall-clock threshold is asserted here + // (PR #141 review: those make the suite flaky on overloaded CI). + const adversarial = "-".repeat(100_000); + expect(sanitise(adversarial)).toBe("arkor-project"); + }); + it("falls back to `arkor-project` when the result would be empty", () => { expect(sanitise("")).toBe("arkor-project"); expect(sanitise("!!!")).toBe("arkor-project"); @@ -28,6 +52,19 @@ describe("sanitise", () => { expect(sanitise(long)).toBe("a".repeat(60)); }); + it("does not leak a trailing dash when the 60-char cap cuts on a separator", () => { + // PR #141 review (Copilot): the trim used to run before the cap, + // so an input like 59 alphanumerics followed by `_b` ended up as + // `aaa…a-` (60 chars, ends with `-`) because the cap landed on + // the dash. The chain is now slice-then-trim, so the trailing + // separator gets stripped after the cap and the slug satisfies + // the documented contract. + const input = `${"a".repeat(59)}_b`; + const result = sanitise(input); + expect(result.endsWith("-")).toBe(false); + expect(result).toBe("a".repeat(59)); + }); + it("preserves digits and dashes", () => { expect(sanitise("v2-runner-01")).toBe("v2-runner-01"); }); diff --git a/packages/cli-internal/src/sanitise.ts b/packages/cli-internal/src/sanitise.ts index 93a2c4a1..9a7ef6d0 100644 --- a/packages/cli-internal/src/sanitise.ts +++ b/packages/cli-internal/src/sanitise.ts @@ -1,19 +1,42 @@ /** * Normalise a candidate project name into a safe lowercase slug: * - lowercased - * - non `[a-z0-9-]` characters replaced with `-` - * - consecutive dashes collapsed to a single `-` - * - leading / trailing `-` trimmed + * - non-alphanumeric runs (including existing `-`) collapsed to a single `-` * - capped at 60 characters + * - a single leading / trailing `-` trimmed (after the cap) * - falls back to `"arkor-project"` if the result is empty + * + * The original chain was three separate replaces (`/[^a-z0-9-]/g`, + * `/-+/g`, `/^-+|-+$/g`), each a linear character-class or literal + * scan. CodeQL's "polynomial-regex on uncontrolled data" query + * conservatively flagged the chain even though no pattern actually + * backtracks; what the query keys off is the *combination* of + * `+`-quantified regexes running over the same CLI-arg input from + * `arkor init` / `create-arkor` (on `--name`, `[dir]` basenames, and + * prompt values), where a pathological all-`-` input is consumed by + * the literal-keeping `/[^a-z0-9-]/g` and then re-scanned by the + * collapsing `/-+/g`. A single `+`-quantified replace on its own is + * not flagged (the rewrite below still uses `/[^a-z0-9]+/g` and + * passes the query). Rather than annotate a suppression, the chain + * was rewritten into one negated-class collapse plus an anchored + * trim: a run of any non-alphanumeric characters becomes one `-` in a + * single linear pass (`-` is inside the negated class now, so the + * separate run-collapse step is gone), which removes the chained-pass + * shape the query flagged. + * + * Trim runs *after* `.slice(0, 60)` so an alphanumeric-then-separator + * input that gets cut on the separator (e.g. 59 letters + `_b` after + * the run-collapse becomes `aaa…a-b`, then sliced to `aaa…a-`) + * doesn't leak a trailing dash into the final slug. The 60-char cap + * is a budget and the trim is the contract; doing them in this order + * makes the contract win. */ export function sanitise(name: string): string { return ( name .toLowerCase() - .replace(/[^a-z0-9-]/g, "-") - .replace(/-+/g, "-") - .replace(/^-+|-+$/g, "") - .slice(0, 60) || "arkor-project" + .replace(/[^a-z0-9]+/g, "-") + .slice(0, 60) + .replace(/^-|-$/g, "") || "arkor-project" ); } diff --git a/packages/create-arkor/src/bin.ts b/packages/create-arkor/src/bin.ts index 66340515..6a4cd184 100644 --- a/packages/create-arkor/src/bin.ts +++ b/packages/create-arkor/src/bin.ts @@ -6,9 +6,13 @@ import process from "node:process"; import * as clack from "@clack/prompts"; import { Command } from "commander"; import { + ClaudeCodeStrictExit, + formatClaudeCodeMissingMessage, gitInitialCommit, install, + isClaudeCode, isInGitRepo, + missingClaudeCodeFlags, resolvePackageManager, sanitise, scaffold, @@ -150,7 +154,13 @@ async function run(options: RunOptions): Promise { let name = sanitise(options.name ?? defaultName); let template: TemplateId = options.template ?? "triage"; - if (!options.yes && isInteractive()) { + // Under CLAUDECODE=1 the action handler below has already gated strict + // mode: by the time we get here either every flag is present or `--yes` + // was passed. Treat CLAUDECODE as an implicit "skip prompts" so a Claude + // Code TTY (which can't answer clack) never opens a real prompt; other + // CLIs keep their pre-existing interactive semantics because + // `isInteractive()` is no longer overridden globally. + if (!options.yes && !isClaudeCode() && isInteractive()) { // Re-prompt loop: when the project name is auto-derived into a fresh // `.//` subdirectory, refuse to merge into an existing non-empty // directory (likely a typo or a forgotten earlier scaffold). Explicit @@ -368,6 +378,42 @@ program "Pick one of --agents-md / --no-agents-md, not both.", ); } + // Under CLAUDECODE=1, refuse to fall through to interactive prompts + // (they'd hang) or to silent defaults (they'd hide decisions the + // agent should be making). Print the suggested re-invocation and + // exit 1 so the agent can re-run with explicit flags. `agentsMd` + // is checked from raw argv so default-on doesn't satisfy the + // requirement: the agent should opt in or out deliberately. + if (isClaudeCode()) { + const agentsMdSpecified = + flagsArgv.includes("--agents-md") || + flagsArgv.includes("--no-agents-md"); + const missing = missingClaudeCodeFlags({ + yes: opts.yes, + template: opts.template, + git: opts.git, + skipGit: opts.skipGit, + skipInstall: opts.skipInstall, + useNpm: opts.useNpm, + usePnpm: opts.usePnpm, + useYarn: opts.useYarn, + useBun: opts.useBun, + name: opts.name, + dir, + agentsMd: agentsMdSpecified ? opts.agentsMd ?? true : undefined, + requireProjectName: true, + }); + if (missing.length > 0) { + process.stderr.write( + formatClaudeCodeMissingMessage("create-arkor", missing), + ); + // Throw (don't `process.exit`) so the outer `program.parseAsync` + // catch block recognises this sentinel and exits silently. The + // "create-arkor failed:" prefix it normally adds would double up + // on our already-printed missing-flags block. + throw new ClaudeCodeStrictExit(); + } + } // Use `Object.hasOwn` (not `in`) so prototype keys like `toString` / // `__proto__` can't pass validation and crash later inside scaffold(). // Reject typos with an explicit error rather than silently coercing them @@ -405,6 +451,22 @@ program ); program.parseAsync(process.argv).catch((err) => { + // The strict-mode validator throws this sentinel after writing the + // missing-flags block; exit silently so the `create-arkor failed:` + // prefix below doesn't double up on the already-printed message. + // + // Use `process.exitCode` for the strict path (not `process.exit`): + // Node then lets the event loop drain naturally so the multi-line + // stderr block flushes on piped stdio. Generic failures still go + // through `process.exit(1)` because `run()` may have started a + // `clack.spinner()` whose internal interval would otherwise keep + // the event loop alive past the catch and stall the exit; a forced + // exit is safer there than waiting for unknown UI resources to + // tidy up (PR #141 review). + if (err instanceof ClaudeCodeStrictExit) { + process.exitCode = 1; + return; + } process.stderr.write( `create-arkor failed: ${err instanceof Error ? err.message : String(err)}\n`, );