feat(paths/cli/setup): unify env load + write on three-path model (#1302, #1303)#1304
Merged
Conversation
, #1303) Key env handling on directory ownership rather than filename. `.archon/` (at `~/` or `<cwd>/`) is archon-owned; anything else is the user's. - `<repo>/.env` — stripped at boot (guard kept), never loaded, never written - `<repo>/.archon/.env` — loaded at repo scope (wins over home), writable via `archon setup --scope project` - `~/.archon/.env` — loaded at home scope, writable via `--scope home` (default) Read side (#1302): - New `@archon/paths/env-loader` with `loadArchonEnv(cwd)` shared by CLI and server entry points. Loads both archon-owned files with `override: true`; repo scope wins. - Replaced `[dotenv@17.3.1] injecting env (0) from .env` (always lied about stripped keys) with `[archon] stripped N keys from <cwd> (...)` and `[archon] loaded N keys from <path>` lines, emitted only when N > 0. `quiet: true` passed to dotenv to silence its own output. - `stripCwdEnv` unchanged in semantics — still the only source that deletes keys from `process.env`; now logs what it did. Write side (#1303): - `archon setup` never writes to `<repo>/.env`. Writing there was incoherent because `stripCwdEnv` deletes those keys on every run. - New `--scope home|project` (default home) targets exactly one archon-owned file. New `--force` overrides the merge; backup still written. - Merge-only by default: existing non-empty values win, user-added custom keys survive, `<path>.archon-backup-<ISO-ts>` written before every rewrite. Fixes silent PostgreSQL→SQLite downgrade and silent token loss in Add mode. - One-time migration note emitted when `<cwd>/.env` exists at setup start. Tests: new `env-loader.test.ts` (6), extended `strip-cwd-env.test.ts` (+4 for the log line), extended `setup.test.ts` (+10 for scope/merge/backup/force/ repo-untouched), extended `cli.test.ts` (+5 for flag parsing). Docs: configuration.md, cli.md, security.md, cli-internals.md, setup skill — all updated to the three-path model.
📝 WalkthroughWalkthroughArchon now treats Changes
Sequence DiagramsequenceDiagram
participant Operator
participant Bun as Bun (auto-load .env*)
participant Boot as strip-cwd-env-boot
participant Loader as loadArchonEnv()
participant CLI as Archon CLI / Server
participant FS as Filesystem
Operator->>Bun: run `archon` / CLI command
Bun->>Boot: Bun injects <cwd>/.env* into process.env
Boot->>Boot: parse CWD .env* (quiet), collect keys & filenames
Boot->>Boot: delete keys from process.env
Boot->>Operator: stderr "[archon] stripped N keys from <cwd> (a, b)" (if N>0)
Boot->>Loader: call loadArchonEnv(process.cwd())
Loader->>FS: read `~/.archon/.env`
Loader->>Operator: stderr "[archon] loaded M keys from ~/.archon/.env" (if M>0)
Loader->>FS: read `<cwd>/.archon/.env`
Loader->>Operator: stderr "[archon] loaded K keys from <cwd>/.archon/.env (repo scope)" (if K>0)
Loader->>CLI: process.env populated (home + repo, repo wins)
CLI->>FS: on `archon setup` write target via writeScopedEnv (backup -> merge or force overwrite)
CLI->>Operator: final messages (backup path, preserved keys)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/paths/src/strip-cwd-env.ts (1)
62-80:⚠️ Potential issue | 🟡 MinorCount only keys actually removed from
process.env.
cwdKeys.sizecounts parsed keys, sostripCwdEnv(tmpDir)can log “stripped 1 keys” even when that key was never present inprocess.env. Track keys that exist before deletion to keep the operator log accurate.🛠️ Proposed adjustment
- for (const key of cwdKeys) { - Reflect.deleteProperty(process.env, key); - } + const strippedKeys: string[] = []; + for (const key of cwdKeys) { + if (Object.prototype.hasOwnProperty.call(process.env, key)) { + strippedKeys.push(key); + Reflect.deleteProperty(process.env, key); + } + } // Tell the operator what we just did — otherwise the delete loop is silent // and users think their env file was loaded (see `#1302`). - if (cwdKeys.size > 0) { + if (strippedKeys.length > 0) { process.stderr.write( - `[archon] stripped ${cwdKeys.size} keys from ${cwd} (${strippedFiles.join(', ')}) to prevent target repo env from leaking into Archon processes\n` + `[archon] stripped ${strippedKeys.length} keys from ${cwd} (${strippedFiles.join(', ')}) to prevent target repo env from leaking into Archon processes\n` ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/paths/src/strip-cwd-env.ts` around lines 62 - 80, The log currently uses cwdKeys.size which counts parsed env keys even if they weren't present on process.env; change the deletion loop that iterates over cwdKeys (where Reflect.deleteProperty(process.env, key) is called) to first check if Object.prototype.hasOwnProperty.call(process.env, key) (or process.env[key] !== undefined) and only then delete and record the key into a new removedKeys set (or increment a removedCount). Update the final message to report removedKeys.size (or removedCount) and, if you include key names in the message, list keys from removedKeys instead of cwdKeys; keep strippedFiles as-is for the filenames part and retain cwd in the message.packages/cli/src/commands/setup.ts (1)
1593-1597:⚠️ Potential issue | 🟠 MajorPropagate
--scopeand--forceinto spawned setup sessions.
archon setup --spawn --scope projectcurrently opens a barearchon setup, which falls back to home scope and drops--force. That can write the wrong env file.🛠️ Proposed direction
-export function spawnTerminalWithSetup(repoPath: string): SpawnResult { +export function spawnTerminalWithSetup( + repoPath: string, + options: { scope?: 'home' | 'project'; force?: boolean } = {} +): SpawnResult { + const setupArgs = [ + 'setup', + ...(options.scope === 'project' ? ['--scope', 'project'] : []), + ...(options.force ? ['--force'] : []), + ]; + const setupCommand = `archon ${setupArgs.join(' ')}`; const platform = process.platform;Then pass
setupCommandthrough the platform-specific spawn helpers instead of hardcodingarchon setup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/setup.ts` around lines 1593 - 1597, The spawned setup currently invokes a hardcoded "archon setup" and drops CLI flags; change the spawn flow to construct a full setupCommand that includes options.scope (e.g., "--scope project") and options.force (e.g., "--force") along with repoPath or other relevant flags, then pass that setupCommand into the platform spawn helper instead of calling a fixed "archon setup". Locate the spawn logic in the options.spawn branch and the call to spawnTerminalWithSetup (and any platform-specific helpers it delegates to) and update them to accept and use the computed setupCommand string so the child session receives the same --scope and --force flags as the parent.
🧹 Nitpick comments (1)
packages/cli/src/commands/setup.test.ts (1)
544-713: Solid coverage of the three-path write invariants.The suite locks in the key guarantees (merge preserves non-empty values, tokens & DB URL survive re-runs,
<repo>/.envnever touched across all four scope/force combinations, backup written before rewrite). Use ofARCHON_HOME+tmpdir()keeps it deterministic.One minor observation (line 664): asserting
result.forced === falsewhen caller passedforce: truebecause the target didn't exist is a subtle API contract — worth a JSDoc onWriteScopedEnvResult.forced(insetup.ts) clarifying that it reflects "overwrote an existing file", not "user passed --force". Otherwise it reads like a bug to a future maintainer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/setup.test.ts` around lines 544 - 713, Add a JSDoc comment to the WriteScopedEnvResult.forced field in setup.ts clarifying that this flag indicates whether the call actually overwrote an existing target file (true) versus a force request on a non-existent target (false); update the WriteScopedEnvResult interface/type declaration near writeScopedEnv and, if present, the writeScopedEnv() JSDoc to echo that semantic so callers understand it reflects "overwrote existing file" not merely "user passed --force".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/skills/archon/guides/setup.md:
- Line 126: Update the reference section at the bottom of this guide to match
the new Step 4 behavior: document that Archon reads two archon-owned env files
(~/.archon/.env for user scope and <cwd>/.archon/.env for repo/project scope
which overrides user), remove the old two-row table and the “symlink/copy to
<archon-repo>/.env” best-practice, and add a note that the CLI calls
loadArchonEnv(cwd) (so the project-scoped <cwd>/.archon/.env is loaded); ensure
any sentence that claimed the CLI does not load .env from the current working
directory is deleted or corrected.
In `@packages/cli/src/cli.ts`:
- Around line 293-303: The setup command currently passes the literal cwd to
setupCommand causing --scope project run from a subdirectory to write to the
wrong path; change the handler so that when scope === 'project' you normalize
cwd to the repository root using the same repo-root resolution used by the other
git-gated commands (the normalization block used elsewhere in this file) and
pass that repo-root as repoPath to setupCommand (keep raw cwd for 'home' scope),
ensuring you await the helper and preserve force/spawn flags.
In `@packages/cli/src/commands/setup.ts`:
- Around line 1629-1630: The setup flow currently calls checkExistingConfig()
against the default home target; update the code so the resolved scoped target
(the path for --scope project vs global) is passed into checkExistingConfig (or
its equivalent parameter) and used for both initial existence checks and later
Add/Update/Fresh decisions; change the checkExistingConfig signature to accept a
targetPath or scope string (e.g., checkExistingConfig(targetPath)) and replace
all calls in the setup command (the initial existing = checkExistingConfig() and
the later check at the Add/Update/Fresh branch) to pass the resolved scoped
target so the decision uses the correct <repo>/.archon/.env when --scope project
is selected.
- Around line 1389-1422: The env file and its backup are written with
potentially permissive permissions; ensure both targetPath and backupPath are
restricted to owner-only read/write (mode 0o600). After calling
copyFileSync(targetPath, backupPath) (the backup created in the exists branch
using backupPath/backupTimestamp), immediately call chmodSync(backupPath, 0o600)
to tighten backup permissions; and when writing the final env contents via
writeFileSync(targetPath, finalContent) use the fs mode option or call
chmodSync(targetPath, 0o600) afterward so the created/overwritten target has
mode 0o600 regardless of umask. Apply this for all paths where writeFileSync or
copyFileSync are used (targetPath, backupPath) to ensure secrets are not
group/world-readable.
- Around line 1330-1332: resolveScopedEnvPath currently builds the home-scoped
path by calling getArchonHome() directly which duplicates logic and misses
`@archon/paths` behavior (Docker’s /.archon handling and the literal "undefined"
guard); update resolveScopedEnvPath to use the shared path helper from
`@archon/paths` (the exported scoped env path resolver provided by that package)
instead of join(getArchonHome(), '.env'), and import that helper at the top of
the file so project-scoped behavior (join(repoPath, '.archon', '.env')) remains
but home-scoped resolution delegates to the `@archon/paths` helper to preserve
Docker and undefined-guard semantics.
In `@packages/docs-web/src/content/docs/reference/cli.md`:
- Line 85: Update the stale "Environment" section that currently claims
"~/.archon/.env" is the sole trusted source to reflect the new scope model:
describe that the CLI loads the CWD `.env` (user-owned, untrusted) first, then
loads the archon-owned file chosen by `--scope` (e.g., global `~/.archon/.env`
or project-scoped archon file), merges the archon-owned keys into the combined
env (preserving user-added keys from CWD), and that `archon setup` writes only
to the archon-owned file and creates a timestamped backup before rewriting;
reference `archon setup`, `--scope project`, and `cli-internals.md` to ensure
wording matches the two-file load/merge order and backup behavior.
---
Outside diff comments:
In `@packages/cli/src/commands/setup.ts`:
- Around line 1593-1597: The spawned setup currently invokes a hardcoded "archon
setup" and drops CLI flags; change the spawn flow to construct a full
setupCommand that includes options.scope (e.g., "--scope project") and
options.force (e.g., "--force") along with repoPath or other relevant flags,
then pass that setupCommand into the platform spawn helper instead of calling a
fixed "archon setup". Locate the spawn logic in the options.spawn branch and the
call to spawnTerminalWithSetup (and any platform-specific helpers it delegates
to) and update them to accept and use the computed setupCommand string so the
child session receives the same --scope and --force flags as the parent.
In `@packages/paths/src/strip-cwd-env.ts`:
- Around line 62-80: The log currently uses cwdKeys.size which counts parsed env
keys even if they weren't present on process.env; change the deletion loop that
iterates over cwdKeys (where Reflect.deleteProperty(process.env, key) is called)
to first check if Object.prototype.hasOwnProperty.call(process.env, key) (or
process.env[key] !== undefined) and only then delete and record the key into a
new removedKeys set (or increment a removedCount). Update the final message to
report removedKeys.size (or removedCount) and, if you include key names in the
message, list keys from removedKeys instead of cwdKeys; keep strippedFiles as-is
for the filenames part and retain cwd in the message.
---
Nitpick comments:
In `@packages/cli/src/commands/setup.test.ts`:
- Around line 544-713: Add a JSDoc comment to the WriteScopedEnvResult.forced
field in setup.ts clarifying that this flag indicates whether the call actually
overwrote an existing target file (true) versus a force request on a
non-existent target (false); update the WriteScopedEnvResult interface/type
declaration near writeScopedEnv and, if present, the writeScopedEnv() JSDoc to
echo that semantic so callers understand it reflects "overwrote existing file"
not merely "user passed --force".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 187ecd65-42b6-446f-8c96-a4b7945075a7
📒 Files selected for processing (18)
.claude/skills/archon/guides/setup.mdCHANGELOG.mdpackages/cli/src/cli.test.tspackages/cli/src/cli.tspackages/cli/src/commands/setup.test.tspackages/cli/src/commands/setup.tspackages/docs-web/src/content/docs/contributing/cli-internals.mdpackages/docs-web/src/content/docs/reference/cli.mdpackages/docs-web/src/content/docs/reference/configuration.mdpackages/docs-web/src/content/docs/reference/security.mdpackages/paths/package.jsonpackages/paths/src/archon-paths.tspackages/paths/src/env-loader.test.tspackages/paths/src/env-loader.tspackages/paths/src/index.tspackages/paths/src/strip-cwd-env.test.tspackages/paths/src/strip-cwd-env.tspackages/server/src/index.ts
Collaborator
Author
PR Review Summary (7 specialized agents)Critical Issues (2)
Both collapse to a single fix: wrap the Important Issues (7)
Suggestions (8)
Test Coverage Gaps
Strengths
Documentation Issues
Type Design Ratings
The main enforcement gap is the duplicated VerdictNEEDS FIXES — Two critical issues (both resolvable by a single try/catch at the spinner site) plus one confirmed round-trip bug ( Recommended Actions
|
…ases
- cli: resolve --scope project to git repo root so running setup from a
subdir writes to <repo-root>/.archon/.env (what loadArchonEnv reads at
boot), not <subdir>/.archon/.env. Fail fast with a useful message when
--scope project is used outside a git repo.
- setup: resolveScopedEnvPath() now delegates to @archon/paths helpers
(getArchonEnvPath / getRepoArchonEnvPath) so Docker's /.archon home,
ARCHON_HOME overrides, and the "undefined" literal guard all behave
identically between the loader and the writer.
- setup: wrap the writeScopedEnv call in try/catch so an fs exception
(permission denied, read-only FS, backup copy failure) stops the clack
spinner cleanly and emits an actionable error instead of a raw stack
trace after the user has completed the entire wizard.
- setup: checkExistingConfig(envPath?) — scope-aware existing-config read.
Add/Update/Fresh now reflects the actual write target, not an
unconditional ~/.archon/.env.
- setup: serializeEnv escapes \r (was only \n) so values with bare CR or
CRLF round-trip through dotenv.parse without corruption. Regression
test added.
- setup: merge path treats whitespace-only existing values (' ') as
empty, so a copy-paste stray space doesn't silently defeat the wizard
update for that key forever. Regression test added.
- setup: 0o600 mode on the written env file AND on backup copies —
writeFileSync+copyFileSync default to 0o666 & ~umask, which can leave
secrets group/world-readable on a permissive umask.
- docs/cli.md + setup skill: appendix sections that still described the
pre-#1303 two-file symlink model now reflect the three-path model.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/cli/src/commands/setup.ts (2)
1934-1942:⚠️ Potential issue | 🟡 Minor"Additional Options" note hardcodes
~/.archon/.env— wrong under--scope project.The summary above (lines 1915-1916) correctly surfaces
writeResult.targetPath, but this trailing note still tells the user to edit~/.archon/.enveven when they just wrote to<repo>/.archon/.env. Confusing for project-scoped setups.📝 Proposed fix
note( - 'Other settings you can customize in ~/.archon/.env:\n' + + `Other settings you can customize in ${writeResult.targetPath}:\n` + ' - PORT (default: 3090)\n' + ' - MAX_CONCURRENT_CONVERSATIONS (default: 10)\n' + ' - *_STREAMING_MODE (stream | batch per platform)\n\n' + 'These defaults work well for most users.', 'Additional Options' );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/setup.ts` around lines 1934 - 1942, The trailing "Additional Options" note hardcodes "~/.archon/.env" which is incorrect for project-scoped installs; update the note in the setup flow (the call to note(..., 'Additional Options')) to mention the actual target path written (use writeResult.targetPath) instead of the hardcoded "~/.archon/.env", so the message reflects whether the file was created in the user home or the project (reference: writeResult.targetPath and the note(...) invocation).
145-154: 🛠️ Refactor suggestion | 🟠 MajorEliminate the local
getArchonHome()fallback incheckExistingConfig— it diverges from@archon/paths.The private
getArchonHome()at lines 145-154 lacks the Docker/.archonbranch and theARCHON_HOME === 'undefined'literal guard present inpackages/paths/src/archon-paths.ts. Today, all in-tree callers pass an explicitenvPathintocheckExistingConfig, but the defaultenvPath ?? join(getArchonHome(), '.env')is a latent hazard: any future caller invokingcheckExistingConfig()with no argument in a Docker container or with a corruptedARCHON_HOMEwill read a different file thanwriteScopedEnv(which goes throughpathsGetArchonEnvPath) writes.🛠 Proposed fix — delegate to `@archon/paths` and drop the local helper
-export function checkExistingConfig(envPath?: string): ExistingConfig | null { - const path = envPath ?? join(getArchonHome(), '.env'); +export function checkExistingConfig(envPath?: string): ExistingConfig | null { + const path = envPath ?? pathsGetArchonEnvPath();Then remove the unused private
getArchonHome()at lines 145-154 if no other callers remain.Based on learnings,
@archon/pathsprovides path resolution utilities (includinggetArchonEnvPath), so setup should delegate rather than duplicate.Also applies to: 336-343
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/setup.ts` around lines 145 - 154, Replace the local getArchonHome() fallback in setup's checkExistingConfig with the canonical path helper from `@archon/paths`: call getArchonEnvPath (or the exported equivalent) to compute the env file path instead of using envPath ?? join(getArchonHome(), '.env'), and then remove the now-unused private getArchonHome() function; ensure checkExistingConfig and any other callers rely on getArchonEnvPath so behavior matches writeScopedEnv and includes the Docker '/.archon' branch and the ARCHON_HOME === 'undefined' guard.
🧹 Nitpick comments (1)
packages/cli/src/commands/setup.ts (1)
1372-1379: ExportWriteScopedEnvResultalongside the exportedwriteScopedEnv.The function is
exported but its return-type interface is file-private, forcing external callers (or tests) to either rely onReturnType<typeof writeScopedEnv>(as line 1787 does) or re-declare the shape. Exporting the interface is a trivial consistency win.♻️ Proposed change
-interface WriteScopedEnvResult { +export interface WriteScopedEnvResult { targetPath: string; backupPath: string | null; /** Keys present in the existing file that were preserved against the proposed set. */ preservedKeys: string[]; /** True when `--force` overrode the merge. */ forced: boolean; }And simplify line 1787:
- let writeResult: ReturnType<typeof writeScopedEnv>; + let writeResult: WriteScopedEnvResult;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/setup.ts` around lines 1372 - 1379, The interface WriteScopedEnvResult is currently file-private while writeScopedEnv is exported; export the interface so callers/tests can reference it directly instead of using ReturnType<typeof writeScopedEnv>. Locate the interface declaration named WriteScopedEnvResult and add an export modifier (export interface WriteScopedEnvResult) and then update any imports/exports as needed so external modules can import WriteScopedEnvResult alongside writeScopedEnv.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/cli/src/commands/setup.ts`:
- Around line 1934-1942: The trailing "Additional Options" note hardcodes
"~/.archon/.env" which is incorrect for project-scoped installs; update the note
in the setup flow (the call to note(..., 'Additional Options')) to mention the
actual target path written (use writeResult.targetPath) instead of the hardcoded
"~/.archon/.env", so the message reflects whether the file was created in the
user home or the project (reference: writeResult.targetPath and the note(...)
invocation).
- Around line 145-154: Replace the local getArchonHome() fallback in setup's
checkExistingConfig with the canonical path helper from `@archon/paths`: call
getArchonEnvPath (or the exported equivalent) to compute the env file path
instead of using envPath ?? join(getArchonHome(), '.env'), and then remove the
now-unused private getArchonHome() function; ensure checkExistingConfig and any
other callers rely on getArchonEnvPath so behavior matches writeScopedEnv and
includes the Docker '/.archon' branch and the ARCHON_HOME === 'undefined' guard.
---
Nitpick comments:
In `@packages/cli/src/commands/setup.ts`:
- Around line 1372-1379: The interface WriteScopedEnvResult is currently
file-private while writeScopedEnv is exported; export the interface so
callers/tests can reference it directly instead of using ReturnType<typeof
writeScopedEnv>. Locate the interface declaration named WriteScopedEnvResult and
add an export modifier (export interface WriteScopedEnvResult) and then update
any imports/exports as needed so external modules can import
WriteScopedEnvResult alongside writeScopedEnv.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 11fe84d8-ed69-4c86-9847-cb7d34909d9b
📒 Files selected for processing (5)
.claude/skills/archon/guides/setup.mdpackages/cli/src/cli.tspackages/cli/src/commands/setup.test.tspackages/cli/src/commands/setup.tspackages/docs-web/src/content/docs/reference/cli.md
✅ Files skipped from review due to trivial changes (1)
- packages/cli/src/commands/setup.test.ts
The test asserted the log line contained `from ~/`, which is opportunistic tilde-shortening that only happens when the tmpdir lives under `homedir()`. On Windows CI the tmpdir is on `D:\\` while homedir is `C:\\Users\\...`, so the path renders absolute and the `~/` never appears. Match on the count and the archon-home tmpdir segment instead — robust on both Unix tilde-short paths and Windows absolute paths.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/paths/src/env-loader.test.ts (1)
15-17: Use a per-test temp directory instead of a fixed source-tree path.A shared
__env-loader-test-tmp__can retain stale.envfiles after interrupted runs and can collide if the suite is invoked concurrently. PrefermkdtempSync(tmpdir())inbeforeEach.Suggested isolation change
-import { writeFileSync, mkdirSync, rmSync } from 'fs'; +import { writeFileSync, mkdirSync, rmSync, mkdtempSync } from 'fs'; import { join } from 'path'; +import { tmpdir } from 'os'; import { loadArchonEnv } from './env-loader'; @@ -const tmpRoot = join(import.meta.dir, '__env-loader-test-tmp__'); -const archonHomeDir = join(tmpRoot, 'archon-home'); -const repoDir = join(tmpRoot, 'repo'); +let tmpRoot: string; +let archonHomeDir: string; +let repoDir: string; @@ beforeEach(() => { + tmpRoot = mkdtempSync(join(tmpdir(), 'archon-env-loader-')); + archonHomeDir = join(tmpRoot, 'archon-home'); + repoDir = join(tmpRoot, 'repo'); + mkdirSync(archonHomeDir, { recursive: true }); mkdirSync(join(repoDir, '.archon'), { recursive: true });As per coding guidelines, prefer reproducible commands and keep tests deterministic without flaky timing or environment dependence.
Also applies to: 29-31, 53-53
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/paths/src/env-loader.test.ts` around lines 15 - 17, Tests currently use a fixed tmpRoot (tmpRoot, archonHomeDir, repoDir) which can retain state between runs and collide in concurrent execution; change the test to create a fresh per-test temp directory using mkdtempSync(os.tmpdir()) in beforeEach, initialize archonHomeDir and repoDir relative to that temp root, and remove the temp directory in afterEach to ensure isolation and cleanup (update any uses at lines referencing tmpRoot, archonHomeDir, repoDir to use the per-test variables).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/paths/src/env-loader.test.ts`:
- Around line 38-47: Add a stdout spy similar to the existing stderr and
console.error spies to capture any writes to process.stdout (e.g., create
stdoutWrites array and stdoutSpy = spyOn(process.stdout,
'write').mockImplementation(...)); then in the relevant tests that assert
successful dotenv loads (the ones using stderrWrites/stderrSpy and
consoleErrorMessages/consoleErrorSpy), also assert that stdoutWrites is empty
after the load to catch regressions where dotenv prints to stdout instead of
being quiet; ensure you restore/mock cleanup exactly like the other spies.
- Around line 119-135: The test currently only verifies that process.exit was
invoked in the home-scope failure; update it to assert process.exit was called
with a non-zero code (e.g. check exitSpy.mock.calls[0][0] !== 0) so we confirm
an error exit, and add a second sub-case that creates a directory at the repo
.env path and runs loadArchonEnv(repoDir) (or reuse loadArchonEnv) to ensure the
repo-scope read failure also triggers the same non-zero exit and logs an "Error
loading .env" message; be sure to reset/restore the process.exit spy between the
two scenarios so calls don't mix.
---
Nitpick comments:
In `@packages/paths/src/env-loader.test.ts`:
- Around line 15-17: Tests currently use a fixed tmpRoot (tmpRoot,
archonHomeDir, repoDir) which can retain state between runs and collide in
concurrent execution; change the test to create a fresh per-test temp directory
using mkdtempSync(os.tmpdir()) in beforeEach, initialize archonHomeDir and
repoDir relative to that temp root, and remove the temp directory in afterEach
to ensure isolation and cleanup (update any uses at lines referencing tmpRoot,
archonHomeDir, repoDir to use the per-test variables).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6577b916-51bf-4c9f-b5fd-f37a0ec5484d
📒 Files selected for processing (1)
packages/paths/src/env-loader.test.ts
Comment on lines
+38
to
+47
| stderrWrites = []; | ||
| stderrSpy = spyOn(process.stderr, 'write').mockImplementation((chunk: unknown) => { | ||
| stderrWrites.push(typeof chunk === 'string' ? chunk : String(chunk)); | ||
| return true; | ||
| }); | ||
|
|
||
| consoleErrorMessages = []; | ||
| consoleErrorSpy = spyOn(console, 'error').mockImplementation((msg: unknown) => { | ||
| consoleErrorMessages.push(String(msg)); | ||
| }); |
There was a problem hiding this comment.
Capture stdout so dotenv noise regressions fail.
These tests only observe stderr/error output. Add a stdout spy and assert the loaded cases do not write there; otherwise a missing quiet/suppression regression can pass while still printing dotenv internals.
Suggested stdout guard
let stderrSpy: ReturnType<typeof spyOn>;
let stderrWrites: string[];
+let stdoutSpy: ReturnType<typeof spyOn>;
+let stdoutWrites: string[];
let consoleErrorSpy: ReturnType<typeof spyOn>;
let consoleErrorMessages: string[];
@@
stderrSpy = spyOn(process.stderr, 'write').mockImplementation((chunk: unknown) => {
stderrWrites.push(typeof chunk === 'string' ? chunk : String(chunk));
return true;
});
+
+ stdoutWrites = [];
+ stdoutSpy = spyOn(process.stdout, 'write').mockImplementation((chunk: unknown) => {
+ stdoutWrites.push(typeof chunk === 'string' ? chunk : String(chunk));
+ return true;
+ });
@@
afterEach(() => {
stderrSpy.mockRestore();
+ stdoutSpy.mockRestore();
consoleErrorSpy.mockRestore();Then assert after representative successful loads:
loadArchonEnv(repoDir);
+ expect(stdoutWrites.join('')).toBe('');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/paths/src/env-loader.test.ts` around lines 38 - 47, Add a stdout spy
similar to the existing stderr and console.error spies to capture any writes to
process.stdout (e.g., create stdoutWrites array and stdoutSpy =
spyOn(process.stdout, 'write').mockImplementation(...)); then in the relevant
tests that assert successful dotenv loads (the ones using stderrWrites/stderrSpy
and consoleErrorMessages/consoleErrorSpy), also assert that stdoutWrites is
empty after the load to catch regressions where dotenv prints to stdout instead
of being quiet; ensure you restore/mock cleanup exactly like the other spies.
Comment on lines
+119
to
+135
| it('exits with error when env file has a dotenv-unparseable layout', () => { | ||
| // dotenv.parse is very permissive — lines without `=` are silently ignored, | ||
| // so syntactic errors that actually surface are rare. We instead simulate | ||
| // a permission-style failure by writing a path that cannot be read: pass a | ||
| // directory in place of a file. dotenv.config returns an error for EISDIR. | ||
| // (Use the home slot since the repo path derives from cwd inside the fn.) | ||
| rmSync(join(archonHomeDir, '.env'), { force: true }); | ||
| mkdirSync(join(archonHomeDir, '.env'), { recursive: true }); // directory at .env path | ||
|
|
||
| const exitSpy = spyOn(process, 'exit').mockImplementation((() => { | ||
| throw new Error('process.exit called'); | ||
| }) as never); | ||
|
|
||
| try { | ||
| expect(() => loadArchonEnv(repoDir)).toThrow('process.exit called'); | ||
| const msg = consoleErrorMessages.find(s => s.startsWith('Error loading .env')); | ||
| expect(msg).toBeDefined(); |
There was a problem hiding this comment.
Assert the non-zero exit and cover repo-scope read failures too.
This currently proves process.exit was reached, but not that it exits as an error. It also only exercises the home path; the project path is the override load and should fail just as strictly.
Suggested coverage additions
- it('exits with error when env file has a dotenv-unparseable layout', () => {
+ it('exits non-zero when the home-scope env path cannot be read', () => {
@@
expect(() => loadArchonEnv(repoDir)).toThrow('process.exit called');
+ expect(exitSpy).toHaveBeenCalledWith(1);
const msg = consoleErrorMessages.find(s => s.startsWith('Error loading .env'));
expect(msg).toBeDefined();
@@
});
+
+ it('exits non-zero when the repo-scope env path cannot be read', () => {
+ mkdirSync(join(repoDir, '.archon', '.env'), { recursive: true });
+
+ const exitSpy = spyOn(process, 'exit').mockImplementation((() => {
+ throw new Error('process.exit called');
+ }) as never);
+
+ try {
+ expect(() => loadArchonEnv(repoDir)).toThrow('process.exit called');
+ expect(exitSpy).toHaveBeenCalledWith(1);
+ const msg = consoleErrorMessages.find(s => s.startsWith('Error loading .env'));
+ expect(msg).toBeDefined();
+ } finally {
+ exitSpy.mockRestore();
+ }
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/paths/src/env-loader.test.ts` around lines 119 - 135, The test
currently only verifies that process.exit was invoked in the home-scope failure;
update it to assert process.exit was called with a non-zero code (e.g. check
exitSpy.mock.calls[0][0] !== 0) so we confirm an error exit, and add a second
sub-case that creates a directory at the repo .env path and runs
loadArchonEnv(repoDir) (or reuse loadArchonEnv) to ensure the repo-scope read
failure also triggers the same non-zero exit and logs an "Error loading .env"
message; be sure to reset/restore the process.exit spy between the two scenarios
so calls don't mix.
5 tasks
prospapledge88
added a commit
to prospapledge88/Archon
that referenced
this pull request
May 5, 2026
* feat(paths/cli/setup): unify env load + write on three-path model (coleam00#1302, coleam00#1303) (coleam00#1304) * feat(paths/cli/setup): unify env load + write on three-path model (coleam00#1302, coleam00#1303) Key env handling on directory ownership rather than filename. `.archon/` (at `~/` or `<cwd>/`) is archon-owned; anything else is the user's. - `<repo>/.env` — stripped at boot (guard kept), never loaded, never written - `<repo>/.archon/.env` — loaded at repo scope (wins over home), writable via `archon setup --scope project` - `~/.archon/.env` — loaded at home scope, writable via `--scope home` (default) Read side (coleam00#1302): - New `@archon/paths/env-loader` with `loadArchonEnv(cwd)` shared by CLI and server entry points. Loads both archon-owned files with `override: true`; repo scope wins. - Replaced `[dotenv@17.3.1] injecting env (0) from .env` (always lied about stripped keys) with `[archon] stripped N keys from <cwd> (...)` and `[archon] loaded N keys from <path>` lines, emitted only when N > 0. `quiet: true` passed to dotenv to silence its own output. - `stripCwdEnv` unchanged in semantics — still the only source that deletes keys from `process.env`; now logs what it did. Write side (coleam00#1303): - `archon setup` never writes to `<repo>/.env`. Writing there was incoherent because `stripCwdEnv` deletes those keys on every run. - New `--scope home|project` (default home) targets exactly one archon-owned file. New `--force` overrides the merge; backup still written. - Merge-only by default: existing non-empty values win, user-added custom keys survive, `<path>.archon-backup-<ISO-ts>` written before every rewrite. Fixes silent PostgreSQL→SQLite downgrade and silent token loss in Add mode. - One-time migration note emitted when `<cwd>/.env` exists at setup start. Tests: new `env-loader.test.ts` (6), extended `strip-cwd-env.test.ts` (+4 for the log line), extended `setup.test.ts` (+10 for scope/merge/backup/force/ repo-untouched), extended `cli.test.ts` (+5 for flag parsing). Docs: configuration.md, cli.md, security.md, cli-internals.md, setup skill — all updated to the three-path model. * fix(cli/setup): address PR review — scope/path/secret-handling edge cases - cli: resolve --scope project to git repo root so running setup from a subdir writes to <repo-root>/.archon/.env (what loadArchonEnv reads at boot), not <subdir>/.archon/.env. Fail fast with a useful message when --scope project is used outside a git repo. - setup: resolveScopedEnvPath() now delegates to @archon/paths helpers (getArchonEnvPath / getRepoArchonEnvPath) so Docker's /.archon home, ARCHON_HOME overrides, and the "undefined" literal guard all behave identically between the loader and the writer. - setup: wrap the writeScopedEnv call in try/catch so an fs exception (permission denied, read-only FS, backup copy failure) stops the clack spinner cleanly and emits an actionable error instead of a raw stack trace after the user has completed the entire wizard. - setup: checkExistingConfig(envPath?) — scope-aware existing-config read. Add/Update/Fresh now reflects the actual write target, not an unconditional ~/.archon/.env. - setup: serializeEnv escapes \r (was only \n) so values with bare CR or CRLF round-trip through dotenv.parse without corruption. Regression test added. - setup: merge path treats whitespace-only existing values (' ') as empty, so a copy-paste stray space doesn't silently defeat the wizard update for that key forever. Regression test added. - setup: 0o600 mode on the written env file AND on backup copies — writeFileSync+copyFileSync default to 0o666 & ~umask, which can leave secrets group/world-readable on a permissive umask. - docs/cli.md + setup skill: appendix sections that still described the pre-coleam00#1303 two-file symlink model now reflect the three-path model. * fix(paths/env-loader): Windows-safe assertion for home-scope load line The test asserted the log line contained `from ~/`, which is opportunistic tilde-shortening that only happens when the tmpdir lives under `homedir()`. On Windows CI the tmpdir is on `D:\\` while homedir is `C:\\Users\\...`, so the path renders absolute and the `~/` never appears. Match on the count and the archon-home tmpdir segment instead — robust on both Unix tilde-short paths and Windows absolute paths. * feat(paths,workflows): unify ~/.archon/{workflows,commands,scripts} + drop globalSearchPath (closes coleam00#1136) (coleam00#1315) * feat(paths,workflows): unify ~/.archon/{workflows,commands,scripts} + drop globalSearchPath Collapses the awkward `~/.archon/.archon/workflows/` convention to a direct `~/.archon/workflows/` child (matching `workspaces/`, `archon.db`, etc.), adds home-scoped commands and scripts with the same loading story, and kills the opt-in `globalSearchPath` parameter so every call site gets home-scope for free. Closes coleam00#1136 (supersedes @jonasvanderhaegen's tactical fix — the bug was the primitive itself: an easy-to-forget parameter that five of six call sites on dev dropped). Primitive changes: - Home paths are direct children of `~/.archon/`. New helpers in `@archon/paths`: `getHomeWorkflowsPath()`, `getHomeCommandsPath()`, `getHomeScriptsPath()`, and `getLegacyHomeWorkflowsPath()` (detection-only for migration). - `discoverWorkflowsWithConfig(cwd, loadConfig)` reads home-scope internally. The old `{ globalSearchPath }` option is removed. Chat command handler, Web UI workflow picker, orchestrator resolve path — all inherit home-scope for free without maintainer patches at every new site. - `discoverScriptsForCwd(cwd)` merges home + repo scripts (repo wins on name collision). dag-executor and validator use it; the hardcoded `resolve(cwd, '.archon', 'scripts')` single-scope path is gone. - Command resolution is now walked-by-basename in each scope. `loadCommand` and `resolveCommand` walk 1 subfolder deep and match by `.md` basename, so `.archon/commands/triage/review.md` resolves as `review` — closes the latent bug where subfolder commands were listed but unresolvable. - All three (`workflows/`, `commands/`, `scripts/`) enforce a 1-level subfolder cap (matches the existing `defaults/` convention). Deeper nesting is silently skipped. - `WorkflowSource` gains `'global'` alongside `'bundled'` and `'project'`. Web UI node palette shows a dedicated "Global (~/.archon/commands/)" section; badges updated. Migration (clean cut — no fallback read): - First use after upgrade: if `~/.archon/.archon/workflows/` exists, Archon logs a one-time WARN per process with the exact `mv` command: `mv ~/.archon/.archon/workflows ~/.archon/workflows && rmdir ~/.archon/.archon` The legacy path is NOT read — users migrate manually. Rollback caveat noted in CHANGELOG. Tests: - `@archon/paths/archon-paths.test.ts`: new helper tests (default HOME, ARCHON_HOME override, Docker), plus regression guards for the double-`.archon/` path. - `@archon/workflows/loader.test.ts`: home-scoped workflows, precedence, subfolder 1-depth cap, legacy-path deprecation warning fires exactly once per process. - `@archon/workflows/validator.test.ts`: home-scoped commands + subfolder resolution. - `@archon/workflows/script-discovery.test.ts`: depth cap + merge semantics (repo wins, home-missing tolerance). - Existing CLI + orchestrator tests updated to drop `globalSearchPath` assertions. E2E smoke (verified locally, before cleanup): - `.archon/workflows/e2e-home-scope.yaml` + scratch repo at /tmp - Home-scoped workflow discovered from an unrelated git repo - Home-scoped script (`~/.archon/scripts/*.ts`) executes inside a script node - 1-level subfolder workflow (`~/.archon/workflows/triage/*.yaml`) listed - Legacy path warning fires with actionable `mv` command; workflows there are NOT loaded Docs: `CLAUDE.md`, `docs-web/guides/global-workflows.md` (full rewrite for three-type scope + subfolder convention + migration), `docs-web/reference/ configuration.md` (directory tree), `docs-web/reference/cli.md`, `docs-web/guides/authoring-workflows.md`. Co-authored-by: Jonas Vanderhaegen <7755555+jonasvanderhaegen@users.noreply.github.com> * test(script-discovery): normalize path separators in mocks for Windows The 4 new tests in `scanScriptDir depth cap` and `discoverScriptsForCwd — merge repo + home with repo winning` compared incoming mock paths with hardcoded forward-slash strings (`if (path === '/scripts/triage')`). On Windows, `path.join('/scripts', 'triage')` produces `\scripts\triage`, so those branches never matched, readdir returned `[]`, and the tests failed. Added a `norm()` helper at module scope and wrapped the incoming `path` argument in every `mockImplementation` before comparing. Stored paths go through `normalizeSep()` in production code, so the existing equality assertions on `script.path` remain OS-independent. Fixes Windows CI job `test (windows-latest)` on PR coleam00#1315. * address review feedback: home-scope error handling, depth cap, and tests Critical fixes: - api.ts: add `maxDepth: 1` to all 3 findMarkdownFilesRecursive calls in GET /api/commands (bundled/home/project). Without this the UI palette surfaced commands from deep subfolders that the executor (capped at 1) could not resolve — silent "command not found" at runtime. - validator.ts: wrap home-scope findMarkdownFilesRecursive and resolveCommandInDir calls in try/catch so EACCES/EPERM on ~/.archon/commands/ doesn't crash the validator with a raw filesystem error. ENOENT still returns [] via the underlying helper. Error handling fixes: - workflow-discovery.ts: maybeWarnLegacyHomePath now sets the "warned-once" flag eagerly before `await access()`, so concurrent discovery calls (server startup with parallel codebase resolution) can't double-warn. Non-ENOENT probe errors (EACCES/EPERM) now log at WARN instead of DEBUG so permission issues on the legacy dir are visible in default operation. - dag-executor.ts: wrap discoverScriptsForCwd in its own try/catch so an EACCES on ~/.archon/scripts/ routes through safeSendMessage / logNodeError with a dedicated "failed to discover scripts" message instead of being mis-attributed by the outer catch's "permission denied (check cwd permissions)" branch. Tests: - load-command-prompt.test.ts (new): 6 tests covering the executor's command resolution hot path — home-scope resolves when repo misses, repo shadows home, 1-level subfolder resolvable by basename, 2-level rejected, not-found, empty-file. Runs in its own bun test batch. - archon-paths.test.ts: add getHomeScriptsPath describe block to match the existing getHomeCommandsPath / getHomeWorkflowsPath coverage. Comment clarity: - workflow-discovery.ts: MAX_DISCOVERY_DEPTH comment now leads with the actual value (1) before describing what 0 would mean. - script-discovery.ts: copy the "routing ambiguity" rationale from MAX_DISCOVERY_DEPTH to MAX_SCRIPT_DISCOVERY_DEPTH. Cleanup: - Remove .archon/workflows/e2e-home-scope.yaml — one-off smoke test that would ship permanently in every project's workflow list. Equivalent coverage exists in loader.test.ts. Addresses all blocking and important feedback from the multi-agent review on PR coleam00#1315. --------- Co-authored-by: Jonas Vanderhaegen <7755555+jonasvanderhaegen@users.noreply.github.com> * feat(isolation,workflows): worktree location + per-workflow isolation policy (coleam00#1310) * feat(isolation): per-project worktree.path + collapse to two layouts Adds an opt-in `worktree.path` to .archon/config.yaml so a repo can co-locate worktrees with its own checkout (`<repoRoot>/<path>/<branch>`) instead of the default `~/.archon/workspaces/<owner>/<repo>/worktrees/<branch>`. Requested in joelsb's coleam00#1117. Primitive changes (clean up the graveyard rather than add parallel code paths): - Collapse worktree layouts from three to two. The old "legacy global" layout (`~/.archon/worktrees/<owner>/<repo>/<branch>`) is gone — every repo resolves to the workspace-scoped layout (`~/.archon/workspaces/<owner>/<repo>/worktrees/<branch>`), whether it was archon-cloned or locally registered. `extractOwnerRepo()` on the repo path is the stable identity fallback. Ends the divergence where workspace-cloned and local repos had visibly different worktree trees. - `getWorktreeBase()` in @archon/git now returns `{ base, layout }` and accepts an optional `{ repoLocal }` override. The layout value replaces the old `isProjectScopedWorktreeBase()` classification at the call sites (`isProjectScopedWorktreeBase` stays exported as deprecated back-compat). - `WorktreeCreateConfig.path` carries the validated override from repo config. `resolveRepoLocalOverride()` fails loudly on absolute paths, `..` escapes, and resolve-escape edge cases (Fail Fast — no silent default fallback when the config is syntactically wrong). - `WorktreeProvider.create()` now loads repo config exactly once and threads it through `getWorktreePath()` + `createWorktree()`. Replaces the prior swallow-then-retry pattern flagged on coleam00#1117. `generateEnvId()` is gone — envId is assigned directly from the resolved path (the invariant was already documented on `destroy(envId)`). Tests (packages/git + packages/isolation): - Update the pre-existing `getWorktreeBase` / `isProjectScopedWorktreeBase` suite for the new two-layout return shape and precedence. - Add 8 tests for `worktree.path`: default fallthrough, empty/whitespace ignored, override wins for workspace-scoped repos, rejects absolute, rejects `../` escapes (three variants), accepts nested relative paths. Docs: add `worktree.path` to the repo config reference with explicit precedence and the `.gitignore` responsibility note. Co-authored-by: Joel Bastos <joelsb2001@gmail.com> * feat(workflows): per-workflow worktree.enabled policy Introduces a declarative top-level `worktree:` block on a workflow so authors can pin isolation behavior regardless of invocation surface. Solves the case where read-only workflows (e.g. `repo-triage`) should always run in the live checkout, without every CLI/web/scheduled-trigger caller having to remember to set the right flag. Schema (packages/workflows/src/schemas/workflow.ts + loader.ts): - New optional `worktree.enabled: boolean` on `workflowBaseSchema`. Loader parses with the same warn-and-ignore discipline used for `interactive` and `modelReasoningEffort` — invalid shapes log and drop rather than killing workflow discovery. Policy reconciliation (packages/cli/src/commands/workflow.ts): - Three hard-error cases when YAML policy contradicts invocation flags: • `enabled: false` + `--branch` (worktree required by flag, forbidden by policy) • `enabled: false` + `--from` (start-point only meaningful with worktree) • `enabled: true` + `--no-worktree` (policy requires worktree, flag forbids it) - `enabled: false` + `--no-worktree` is redundant, accepted silently. - `--resume` ignores the pinned policy (it reuses the existing run's worktree even when policy would disable — avoids disturbing a paused run). Orchestrator wiring (packages/core/src/orchestrator/orchestrator-agent.ts): - `dispatchOrchestratorWorkflow` short-circuits `validateAndResolveIsolation` when `workflow.worktree?.enabled === false` and runs directly in `codebase.default_cwd`. Web chat/slack/telegram callers have no flag equivalent to `--no-worktree`, so the YAML field is their only control. - Logged as `workflow.worktree_disabled_by_policy` for operator visibility. First consumer (.archon/workflows/repo-triage.yaml): - `worktree: { enabled: false }` — triage reads issues/PRs and writes gh labels; no code mutations, no reason to spin up a worktree per run. Tests: - Loader: parses `worktree.enabled: true|false`, omits block when absent. - CLI: four new integration tests for the reconciliation matrix (skip when policy false, three hard-error cases, redundant `--no-worktree` accepted, `--no-worktree` + `enabled: true` rejected). Docs: authoring-workflows.md gets the new top-level field in the schema example with a comment explaining the precedence and the `enabled: true|false` semantics. * fix(isolation): use path.sep for repo-containment check on Windows resolveRepoLocalOverride was hardcoding '/' as the separator in the startsWith check, so on Windows (where `resolve()` returns backslash paths like `D:\Users\dev\Projects\myapp`) every otherwise-valid relative `worktree.path` was rejected with "resolves outside the repo root". Fixed by importing `path.sep` and using it in the sentinel. Fixes the 3 Windows CI failures in `worktree.path repo-local override`. --------- Co-authored-by: Joel Bastos <joelsb2001@gmail.com> * docs(worktree): fix stale rename example + document copyFiles properly (coleam00#1328) Three related fixes around the `worktree.copyFiles` primitive: 1. Remove the `.env.example -> .env` rename example from reference/configuration.md and getting-started/overview.md. The `->` parser was removed in coleam00#739 (2026-03-19) because it caused the stale-credentials production bug in coleam00#228 — but the docs kept advertising it. A user writing `.env.example -> .env` today gets `parseCopyFileEntry` returning `{source: '.env.example -> .env', destination: '.env.example -> .env'}`, stat() fails with ENOENT, and the copy silently no-ops at debug level. 2. Replace the single-line "Default behavior: .archon/ is always copied" note with a proper "Worktree file copying" subsection that explains: - Why this exists (git worktree add = tracked files only; gitignored workflow inputs need this hook) - The `.archon/` default (no config needed for the common case) - Common entries: .env, .vscode/, .claude/, plans/, reports/, data fixtures - Semantics: source=destination, ENOENT silently skipped, per-entry error isolation, path-traversal rejected - Interaction with `worktree.path` (both layouts get the same treatment) 3. Update the overview example to drop the `.env.example + .env` pair (which implied rename semantics) in favor of `.env + plans/`, and call out that `.archon/` is auto-copied so users don't list it. No code changes. `bun run format:check` and `bun run lint` green. * fix(workflows): archon-assist runs in live checkout (worktree.enabled: false) — closes coleam00#1546 (coleam00#1555) Co-authored-by: Zolto <zolto@zhome.local> * chore(changelog): document Tier 4 paths/env unification cherry-pick batch (5 commits) Adds a CHANGELOG entry under [Unreleased] / Fixed summarizing the five upstream commits picked in this batch: - 28908f0 — env load/write three-path model + loadArchonEnv helper (coleam00#1302/coleam00#1303/coleam00#1304) - 7be4d0a — unify ~/.archon/{workflows,commands,scripts} (coleam00#1315) - 5ed38dc — worktree.path config + per-workflow worktree.enabled policy (coleam00#1310) - ba4b9b4 — docs follow-up to 5ed38dc (coleam00#1328) - e33e0de — archon-assist worktree.enabled: false (deferred from PR #8, now unblocked) Notes that cc78071 (worktree timeout 5m) was already absorbed in earlier batches. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Rasmus Widing <152263317+Wirasm@users.noreply.github.com> Co-authored-by: Jonas Vanderhaegen <7755555+jonasvanderhaegen@users.noreply.github.com> Co-authored-by: Joel Bastos <joelsb2001@gmail.com> Co-authored-by: ztech-gthb <ztech-001@gmx.net> Co-authored-by: Zolto <zolto@zhome.local> Co-authored-by: cjnprospa <sirhcle.j23@gmail.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
(0) from .envinstead ofstripped N keys#1302) CLI silently stripped<repo>/.envkeys with a misleading[dotenv@17.3.1] injecting env (0) from .envlog; no repo-scoped archon.envexisted. (bug(cli/setup): archon setup silently overwrites repo .env and loses secrets #1303)archon setupunconditionally overwrote both~/.archon/.envand<repo>/.envon every run, destroying user-added secrets and silently downgrading PostgreSQL to SQLite in Add mode.loadArchonEnv(cwd)+ archon-owned log lines. Addedarchon setup --scope home|projectand--force, merge-only by default, timestamped backups, never touches<repo>/.env.stripCwdEnv()still strips<repo>/.env*on boot (guard still protects Archon from target-repo var leaks). Claude/Codex subprocess env plumbing unchanged. No new dependencies. No database migration.UX Journey
Before
After
Architecture Diagram
Before
After
Connection inventory:
cli.tsstrip-cwd-env-bootcli.ts@archon/paths/env-loader[+]~/.archon/.envload blockserver/src/index.ts@archon/paths/env-loader[+]strip-cwd-env.ts(dotenv)[archon] stripped …injecting (0)tipsetup.ts → writeEnvFiles<repo>/.envwritesetup.ts → writeScopedEnv[+]--forceopt-in full regenLabel Snapshot
risk: medium(operator-facing log format change + setup write-path refactor; balanced by comprehensive tests)size: M(~770 lines added / 85 removed across 18 files)paths+cli+server+docspaths:env-loader,paths:strip-cwd-env,cli:cli,cli:setup,server:indexChange Metadata
feature(primary) +bug(fixes CLI silently strips repo-local .env vars; logs say(0) from .envinstead ofstripped N keys#1302 and bug(cli/setup): archon setup silently overwrites repo .env and loses secrets #1303)multi(paths + cli + server)Linked Issue
(0) from .envinstead ofstripped N keys#1302Validation Evidence (required)
bun run validate # all 5 phases: check:bundled, type-check, lint, format:check, testResult: EXIT=0. Every package test batch reported
0 fail. Specifically:@archon/paths: 16 strip tests + 6 new env-loader tests → all pass@archon/cli: 42 CLI parsing tests (5 new for--scope/--force) + 36 setup tests (10 new for scope/merge/backup/force/repo-untouched) → all pass@archon/server: 62 tests → all passSecurity Impact (required)
archon setupno longer silently destroys secrets by overwriting<repo>/.env.<path>.archon-backup-<ISO-ts>) — even--forcewrites a backup.<repo>/.envstrip guard retained — still prevents target-repo env vars from leaking into Archon subprocesses.archon setupnow writes to<cwd>/.archon/.envwhen--scope projectis passed (in addition to~/.archon/.env). Both are archon-owned directory conventions already in use forconfig.yaml.Compatibility / Migration
archon setupno longer writes to<repo>/.env. Pre-existing<repo>/.envfiles are left untouched. Users who put archon secrets there will see a one-time migration note at setup start pointing them to~/.archon/.envor<cwd>/.archon/.env.~/.archon/.envcontents survive because of the new merge-only default.<cwd>/.archon/.envis now loaded. Users who place archon env vars there get them loaded at repo scope. Pre-existing setups keep working (home-scope only).<cwd>/.archon/.env. Users who had secrets in<repo>/.envshould move them to<cwd>/.archon/.env(or~/.archon/.env) — the migration note surfaces this at the nextarchon setuprun.Human Verification (required)
bun run validategreen (type-check + lint + format + test on all 10 packages).packages/pathsandpackages/cligreen with the new cases added.--forceoverwrites + backs up;--forceon missing target writes cleanly;--scope projectcreates<cwd>/.archon/;<repo>/.envuntouched across all scope/force combos; empty env files emit no log lines; malformed env file exits with error./test-releaseflow — deferred to the release step. No functional surprises expected sinceloadArchonEnvuses the same primitives the previous inline block used.Side Effects / Blast Radius (required)
archoninvocation), server boot sequence (everybun run dev:serverandarchon serve),archon setupwizard write path. No workflow engine, provider, adapter, or web-UI code touched..envsilently stripping were already broken (CLI silently strips repo-local .env vars; logs say(0) from .envinstead ofstripped N keys#1302). This PR makes the behavior visible via logs but does not change it.archon setupa second time will now see a new "Preserved N existing value(s)" message and a backup file appear alongside their env. Informational only.[archon] loaded …lines are new noise on stderr for anyone with archon-owned env files. Silent when N=0.Rollback Plan (required)
git revert 2e2861a6). Single atomic commit with a clear scope — revert is safe and symmetric.@archon/pathsor@archon/clion the log-assertion tests, or boot-time crashes in the CLI/server entry points if theloadArchonEnvimport path is broken.Risks and Mitigations
#, spaces, or newlines in user-added custom keys.serializeEnvquotes any value containing whitespace/#/"/newlines using dotenv-compatible escaping; round-trip testserializeEnv → dotenv.parseverified.rm ~/.archon/.env.archon-backup-*.<cwd>/.archon/.envbut write side still targets<repo>/.env.(0) from .envinstead ofstripped N keys#1302 / bug(cli/setup): archon setup silently overwrites repo .env and loses secrets #1303 discussion).Summary by CodeRabbit
New Features
archon setupadds--scope(home/project) and--forceflags; default writes to home (~/.archon/.env). Project scope writes to a per-repo archon file.Improvements
--forceforces overwrite.Bug Fixes
archon setupno longer writes to repository<cwd>/.env.