fix(upgrade): detect failed extraction and stop leaking temp directories - #561
Conversation
Expand-Archive reports a corrupt archive as a non-terminating error, so `powershell -Command` exits 0 and extraction failure went unnoticed. `-ErrorAction Stop` promotes it to terminating. A post-extraction existence check covers both branches, so an archive that extracts to nothing is reported where it happens rather than as a missing binary later. The extraction directory is removed on every failure path, and the caller removes it once the binary is installed. The tar path-traversal guard's backslash normalization is live, not dead: GNU tar lists a member stored as `..\evil` with the backslash doubled, and normalizing that yields `..//evil`, which contains `../`. bsdtar lists it verbatim, which normalizes to `../evil`. Both trip the guard, and neither does without the normalization. The traversal table gains the case it was missing. Closes #554 Closes #555 Closes #556 Signed-off-by: Rhuan Barreto <rhuan@barreto.work>
Signed-off-by: Rhuan Barreto <rhuan@barreto.work>
|
Warning Review limit reached
Next review available in: 17 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughArchive upgrades now validate tar and ZIP extraction, reject unsafe paths, require the expected binary, and clean temporary directories after failures or installation attempts. Tests cover cleanup, backslash traversal, PowerShell exit handling, 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
Deploying archgate-cli with
|
| Latest commit: |
41602f9
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e1b647fb.archgate-cli.pages.dev |
| Branch Preview URL: | https://claude-issues-554-555-556-8f.archgate-cli.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.claude/agent-memory/archgate-developer/feedback_verify_agent_claims.md:
- Around line 23-25: Update the guidance around reproducing prior-session
reports to require verification in the current checkout without claiming that
all prior analyses were never executed. Preserve the requirement to reproduce
the reported failure and separately validate the premise behind the proposed
fix, describing prior reports as unverified in the current checkout.
In `@src/helpers/binary-upgrade.ts`:
- Around line 239-287: Update the archive handling around the listing and
extraction subprocesses to consume each configured stdout and stderr stream
concurrently before awaiting process exit, preventing blocked pipes. For the tar
listing process, await its exit code and throw a clear UserError on non-zero
exit before validating listing entries; apply the same stream-draining pattern
to tar extraction and PowerShell extraction while preserving their existing
failure messages.
🪄 Autofix
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1ba000e2-9e6a-4d04-8a05-0a6021aec531
📒 Files selected for processing (5)
.claude/agent-memory/archgate-developer/MEMORY.md.claude/agent-memory/archgate-developer/feedback_verify_agent_claims.mdsrc/commands/upgrade.tssrc/helpers/binary-upgrade.tstests/helpers/binary-upgrade-archive.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Cloudflare Pages
🧰 Additional context used
📓 Path-based instructions (12)
{src,tests}/**/*.ts
📄 CodeRabbit inference engine (.archgate/adrs/LEGAL-001-spdx-license-headers.md)
{src,tests}/**/*.ts: Every TypeScript source file insrc/andtests/must begin with// SPDX-License-Identifier: Apache-2.0followed by// Copyright 2026 Archgate.
If a TypeScript file has a shebang line (for example#!/usr/bin/env buninsrc/cli.ts), the SPDX license header must appear immediately after the shebang.
Use single-line//comments for the SPDX header; do not use block comments (/* */) or alternate license identifiers.
Files:
src/helpers/binary-upgrade.tssrc/commands/upgrade.tstests/helpers/binary-upgrade-archive.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.archgate/adrs/ARCH-006-dependency-policy.md)
**/*.{ts,tsx}: Prefer Bun built-ins for file I/O, HTTP, globbing, testing, and subprocess execution; prefernode:built-in modules over npm alternatives when appropriate.
UseBun.spawnwith array-based arguments for all subprocess execution; do not useBun.$because it can hang on Windows.
Do not add npm packages for functionality already provided by Bun, such asglob,chalk, or utility libraries used for a single function.
Use Bun APIs such asBun.file()instead of Node.js-specific APIs such asfs.readFile()when Bun provides an equivalent.
Use relative imports with Bun's native module resolution; do not use TypeScript path aliases.Use TypeScript strict mode with ESNext and ES modules; derive schema types with
z.infer<>rather than defining separate interfaces.
Files:
src/helpers/binary-upgrade.tssrc/commands/upgrade.tstests/helpers/binary-upgrade-archive.test.ts
src/**/*.ts
📄 CodeRabbit inference engine (.archgate/adrs/ARCH-018-lazy-load-heavy-dependencies.md)
src/**/*.ts: Heavy runtime dependencies such asinquirer,posthog-node, and@sentry/*must be loaded with dynamicimport()at their point of use, never through top-level static value imports.
Type-only imports for heavy dependencies are allowed, but runtime values must be obtained through dynamicimport(); for example, useimport type { PostHog } from "posthog-node".
SDKs that require early initialization may use eager-start/lazy-await: begin initialization before command registration and await the result at first use, such as in apreActionhook.
src/**/*.ts: In all TypeScript source files undersrc/, useBun.envfor every environment-variable read and write; never referenceprocess.env, including in comments.
AccessBun.envproperties directly; do not create wrapper functions around it or destructure it.
Use nullish coalescing for environment-variable defaults, such asBun.env.NODE_ENV ?? "production".
For truthy environment-flag checks, useBoolean(Bun.env.FLAG)only inline, as part of a larger expression, or assign it to a variable before using it as a sole condition; otherwise use an explicit defined-and-nonempty comparison.
src/**/*.ts: Everyinquirer.prompt(...)call must be wrapped inwithPromptFix(() => ...)imported fromsrc/helpers/prompt.ts; keep the wrapper adjacent to the prompt invocation so automated checks can detect it.
Do not callinquirer.prompt(...)directly or reimplement cursor/newline fixes at individual call sites; route all prompt behavior throughwithPromptFix().Every call to
Bun.Glob#scan()(glob.scan(...)) in source must pass{ dot: true }in its options object, including scans whose patterns do not explicitly target dot-directories. Do not usedot: false; intentionally excluded dotfiles must be filtered explicitly after scanning with a comment. Normalize scanned path separators withfile.replaceAll("\\", "/")when performing cross-platform path comparisons.
src/**/*.ts: Use `sty...
Files:
src/helpers/binary-upgrade.tssrc/commands/upgrade.ts
{src,tests,lint,scripts,shims}/**/*.ts
📄 CodeRabbit inference engine (.archgate/adrs/GEN-004-concise-forward-only-code-comments.md)
{src,tests,lint,scripts,shims}/**/*.ts: Project-authored TypeScript comments must be concise, describe current behavior only, and never narrate history, relocations, refactors, or how the code came to be.
A contiguous run of whole-line comments must contain at most five lines of narrative prose; longer rationale belongs in an ADR, agent-memory file, issue, or PR with a pointer. Tests and fixtures follow the same limit.
Use structural TSDoc tags such as@param,@returns,@throws,@example, and@seefor structured documentation; tagged sections are exempt from the five-line narrative bound, while@remarks,@description,@summary,@notes,@todo, and@fixmeremain counted as prose.
Files:
src/helpers/binary-upgrade.tssrc/commands/upgrade.tstests/helpers/binary-upgrade-archive.test.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{ts,tsx}: For user-scope editors, resolve paths using the editor's actual path helper; do not assume Windows conventions. For opencode, mirrorxdg-basedir, which falls back to~/.configon all platforms.
For opencode-gated behavior, useisOpencodeAvailable()rather thanisOpencodeCliAvailable()alone because the Desktop distribution has no CLI binary and shares the config directory.
For Copilot-gated behavior, useisCopilotAvailable()rather thanisCopilotCliAvailable()alone because desktop and CLI distributions share~/.copilot/.
Files:
src/helpers/binary-upgrade.tssrc/commands/upgrade.ts
**/*.{js,ts,tsx,mjs,cjs}
📄 CodeRabbit inference engine (.archgate/adrs/GEN-003-tool-invocation-via-scripts.md)
Invoke linting, formatting, and validation through package scripts (
bun run lint,bun run format,bun run format:check, andbun run validate), rather than directly invoking tool binaries such asbunx prettier,bunx oxfmt,npx eslint, oroxlint.
Files:
src/helpers/binary-upgrade.tssrc/commands/upgrade.tstests/helpers/binary-upgrade-archive.test.ts
src/**/!(platform).ts
📄 CodeRabbit inference engine (.archgate/adrs/ARCH-009-platform-detection-helper.md)
All platform detection in
src/must go throughsrc/helpers/platform.ts(isWindows(),isMacOS(),isLinux(),isWSL(), orgetPlatformInfo()); directprocess.platformaccess and duplicated detection logic are forbidden outsideplatform.ts.
Files:
src/helpers/binary-upgrade.tssrc/commands/upgrade.ts
**
⚙️ CodeRabbit configuration file
**: This project uses Archgate — an AI governance framework based on
Architecture Decision Records (ADRs). The ADRs in.archgate/adrs/
are the authoritative rules for this codebase. Each ADR has a companion
.rules.tsfile with automated checks that run viaarchgate check.When reviewing, you must:
- Treat ADR violations as blocking issues, not suggestions.
- Cite the specific ADR ID when flagging a violation (e.g., "Violates ARCH-006").
- Focus on semantic and contextual violations that automated rules cannot catch —
the.rules.tsfiles already cover syntactic/structural patterns.- If you are unsure whether something violates an ADR, flag it as a question
rather than approving it.
Files:
src/helpers/binary-upgrade.tssrc/commands/upgrade.tstests/helpers/binary-upgrade-archive.test.ts
src/commands/**/*.ts
📄 CodeRabbit inference engine (.archgate/adrs/ARCH-011-consistent-project-root-resolution.md)
src/commands/**/*.ts: All commands that operate on.archgate/project resources must use the sharedfindProjectRoot()fromsrc/helpers/paths.ts; directprocess.cwd()project-root resolution is prohibited except ininit.
Commands that require a project must userequireProjectRoot()fromsrc/helpers/paths.tsinstead of implementing their own missing-project guard. Commands that can operate without a project may usefindProjectRoot()and handlenullthemselves.
When usingfindProjectRoot()directly, check for anullresult and provide a helpful error before exiting.
Pass the resolvedprojectRoottoprojectPaths()when constructing derived project paths.
Do not define localfindProjectRoot()variants; use the shared implementation fromsrc/helpers/paths.ts.Command modules must export a
register*Command(program)function, handle I/O only, and contain no business logic.
src/commands/**/*.ts: Each command module must export aregister*Command(program)function; each non-index.tscommand file must define exactly one command.
Command files must remain thin: parse arguments, call engine/helpers, and format output; business logic must reside insrc/engine/,src/helpers/, orsrc/formats/.
Commands must execute in-process and must not spawn child processes for subcommand execution.
Command files must not call.parse(); argument parsing is handled by the CLI entry point.
Commands should use typed Commander registration APIs, such as@commander-js/extra-typings, within theirregister*Commandfunctions.
src/commands/**/*.ts: In Commander.js command files, options requiring type narrowing beyond plain strings MUST usenew Option()from@commander-js/extra-typingsand register it with.addOption()instead of.option().
Use.choices([... ] as const)for options accepting a fixed set of values, and use.default(... as const)when providing a default, preserving literal type inference.
Use.argParser((value) => ...)...
Files:
src/commands/upgrade.ts
src/commands/{*.ts,*/index.ts}
📄 CodeRabbit inference engine (.archgate/adrs/ARCH-015-cli-command-documentation-coverage.md)
Top-level command modules must follow the
src/commands/<name>.tsorsrc/commands/<name>/index.tsconvention. Nested subcommand files do not count as top-level commands.
Files:
src/commands/upgrade.ts
tests/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Tests mirror the
src/structure; shared test fixtures belong undertests/fixtures/.
tests/**/*.ts: Use Bun's built-inbun:testrunner for all tests; do not importnode:test. Test files belong undertests/, mirrorsrc/, usetests/fixtures/for shared fixtures, and follow<module-name>.test.tsnaming.
Filesystem tests must use isolatedmkdtempdirectories and clean them up inafterEachorafterAll; do not touch real user-scope paths or leave temporary files behind.
Close external SDK instances, servers, clients, and transports in test hooks, such asawait server.close()inafterEachorafterAll.
Restore every captured environment variable withrestoreEnv(key, original); never restore with direct assignment such asBun.env.X = original, becauseundefinedbecomes the string"undefined".
Mockos.homedir()via an imported module namespace andspyOn; do not rely on overridingHOMEfor code usingos.homedir(), and keep filesystem writes inside temporary directories.
Shared test helpers, including non-test files undertests/, must restore every captured environment variable withrestoreEnv; isolation responsibilities apply across the entire shared Bun test process.Use
_resetAllCaches()fromsrc/helpers/platform.tsto simulate different platforms in tests rather than mockingprocess.platformdirectly.
Files:
tests/helpers/binary-upgrade-archive.test.ts
tests/**/*.test.ts
📄 CodeRabbit inference engine (.archgate/adrs/ARCH-025-idiomatic-bun-test-parametrization-and-matchers.md)
tests/**/*.test.ts: Usetest.each()ordescribe.each()for the same assertion logic against multiple independent inputs. Do not register tests or callexpect()once per case inside aforor.forEachloop.
Use array rows for positional destructuring and object rows for named fields when passing cases totest.each()ordescribe.each(). Choose descriptive title placeholders such as%s,%p,%d, or$field.
Assert derived facts with the most specific matcher available instead of passing a derived boolean to.toBe(true)or.toBe(false). Compare values directly with.toBe()or.toEqual().
Use specific matchers for common derived checks:.toContain()or.toMatch()for containment,.toBeInstanceOf()for type checks such asArray.isArray,.toHaveLength()for counts, and.find()with.toBeDefined()or.toBeUndefined()for predicate existence checks.
Do not precompute a boolean solely to assert it; assert the underlying values directly with matchers such as.toEqual()or.toBe().
When converting a loop totest.each()ordescribe.each(), preserve every assertion that ran per iteration; do not drop or merge assertions.
tests/**/*.test.ts: Every runnable test must contain anexpect()assertion; usetest.skiportest.todofor placeholders rather than assertion-less or silently skipped tests.
Test public interfaces with descriptive names rather than private implementation details.
Do not usemock.module()for first-party modules. Mock them withimport * as modplusspyOn(mod, "fn"), and restore mocks after each test.mock.module()may be used for approved external modules such asinquirerornode:readline.
For HTTP mocking, saveglobalThis.fetchbefore direct assignment and restore it inafterEach; do not usemock.module("node:fetch"), which does not intercept Bun's global fetch.
Wrap inlinespyOnormockImplementationlifecycles intry/finally, or manage them in hooks, somockRestore()runs wh...
Files:
tests/helpers/binary-upgrade-archive.test.ts
🧠 Learnings (16)
📚 Learning: 2026-06-11T12:50:28.661Z
Learnt from: rhuanbarreto
Repo: archgate/cli PR: 406
File: .claude/agent-memory/archgate-developer/feedback_prefer_tests_over_adr_rules.md:8-18
Timestamp: 2026-06-11T12:50:28.661Z
Learning: In `archgate/cli`, for markdown files under `.claude/agent-memory/`, follow the established convention: use YAML frontmatter (with a `name:` field used as the document title) and do not require a top-level `#` (H1) heading. During code review, do not flag missing first-line/first-top-level H1 headings (e.g., MD041) for these agent-memory files since markdownlint is not part of the repo’s `bun run validate` lint pipeline (oxlint/oxfmt only).
Applied to files:
.claude/agent-memory/archgate-developer/feedback_verify_agent_claims.md.claude/agent-memory/archgate-developer/MEMORY.md
📚 Learning: 2026-07-25T00:05:20.592Z
Learnt from: rhuanbarreto
Repo: archgate/cli PR: 496
File: .claude/agent-memory/archgate-developer/project_test_isolation_gotchas.md:10-10
Timestamp: 2026-07-25T00:05:20.592Z
Learning: When reviewing documentation/agent-memory entries under `.claude/agent-memory/**`, do not enforce GEN-004’s “forward-only” comment/narrative requirement. These entries are allowed to keep historical/past-tense incident narratives and dated markers (e.g., `Found YYYY-MM-DD`) because the context is intended to help future agents evaluate edge cases. Outside this scope, GEN-004’s forward-only rule should still apply.
Applied to files:
.claude/agent-memory/archgate-developer/feedback_verify_agent_claims.md.claude/agent-memory/archgate-developer/MEMORY.md
📚 Learning: 2026-07-02T16:03:33.031Z
Learnt from: rhuanbarreto
Repo: archgate/cli PR: 446
File: src/helpers/session-context-opencode.ts:81-100
Timestamp: 2026-07-02T16:03:33.031Z
Learning: For synchronous helper functions that use Bun’s sqlite sync API (i.e., they must remain synchronous), it’s acceptable to use `existsSync` from `node:fs` to check whether the SQLite database file exists. Avoid using `Bun.file(path).exists()` for this purpose because it’s async and would force the helper to become async (no equivalent synchronous Bun alternative). If the DB file is missing, throw/return a clear, actionable "No database found" error (per ARCH-006) rather than letting the sqlite open fail with a generic DB-open error.
Applied to files:
src/helpers/binary-upgrade.ts
📚 Learning: 2026-07-25T00:05:58.884Z
Learnt from: rhuanbarreto
Repo: archgate/cli PR: 496
File: tests/helpers/auth.test.ts:38-46
Timestamp: 2026-07-25T00:05:58.884Z
Learning: When reviewing the Archgate CLI repository’s GEN-004 “concise forward-only narration” comments, don’t rely only on the automated phrase-based narration checks. Those checks can pass even when the comment wording describes historical/transfer semantics rather than current behavior (e.g., saying a prior restore “leaked” a value or a later subprocess “inherited it”). Manually verify that the comment describes the code’s current, forward behavior; flag or adjust comments that imply past/historical state transfer even if GEN-004 enforcement passes.
Applied to files:
src/helpers/binary-upgrade.tssrc/commands/upgrade.tstests/helpers/binary-upgrade-archive.test.ts
📚 Learning: 2026-07-25T00:05:59.109Z
Learnt from: rhuanbarreto
Repo: archgate/cli PR: 496
File: src/cli.ts:0-0
Timestamp: 2026-07-25T00:05:59.109Z
Learning: Code comments may include a concise issue/PR reference (per GEN-004) when it’s used to point readers to fuller rationale instead of inlining that rationale. During review, flag surrounding comment prose that reads like historical context or narrates refactors/relocations; a bare GEN-004-style reference is allowed and should not be flagged by itself.
Applied to files:
src/helpers/binary-upgrade.tssrc/commands/upgrade.tstests/helpers/binary-upgrade-archive.test.ts
📚 Learning: 2026-08-04T19:58:05.877Z
Learnt from: rhuanbarreto
Repo: archgate/cli PR: 543
File: src/helpers/copilot-user-settings.ts:0-0
Timestamp: 2026-08-04T19:58:05.877Z
Learning: In archgate/cli TypeScript code, use `Bun.file(path).exists()` only to check whether a file exists; it must not be used for directory existence checks. For helpers such as `isCopilotAvailable()` that need to detect a configuration directory, use an appropriate directory-aware check such as `existsSync` from `node:fs`.
Applied to files:
src/helpers/binary-upgrade.tssrc/commands/upgrade.tstests/helpers/binary-upgrade-archive.test.ts
📚 Learning: 2026-08-05T06:56:33.435Z
Learnt from: rhuanbarreto
Repo: archgate/cli PR: 546
File: tests/integration/stream-guards.test.ts:3-9
Timestamp: 2026-08-05T06:56:33.435Z
Learning: When reviewing GEN-004 comment-block limits in the Archgate CLI repository, count only narrative prose lines within a block comment. Do not count a closing delimiter such as `*/` as a prose line; for example, in `tests/integration/stream-guards.test.ts`, Lines 4–8 contain five prose lines while Line 9 contains only the delimiter.
Applied to files:
src/helpers/binary-upgrade.tssrc/commands/upgrade.tstests/helpers/binary-upgrade-archive.test.ts
📚 Learning: 2026-07-25T22:03:14.216Z
Learnt from: rhuanbarreto
Repo: archgate/cli PR: 501
File: .archgate/adrs/ARCH-002-error-handling.md:0-0
Timestamp: 2026-07-25T22:03:14.216Z
Learning: In Archgate boundary-wrapped CLI command actions (the handlers that rely on `handleCommandError()` for user-facing error output), expected-failure guards should signal user errors by throwing `new UserError(<message/details>)` rather than directly calling `logError()` followed by `exitWith(1)`. This keeps user-facing logging and the exit path centralized in `handleCommandError()`. For normal/computed command outcomes (e.g., `const exitCode = getExitCode(await runChecks(...))`), use `await exitWith(exitCode)` instead of calling `process.exit(exitCode)` so telemetry/Sentry flushing and outcome tagging still run.
Applied to files:
src/helpers/binary-upgrade.tssrc/commands/upgrade.tstests/helpers/binary-upgrade-archive.test.ts
📚 Learning: 2026-07-25T22:03:22.236Z
Learnt from: rhuanbarreto
Repo: archgate/cli PR: 501
File: .archgate/adrs/ARCH-016-cli-subcommand-documentation-accuracy.md:64-67
Timestamp: 2026-07-25T22:03:22.236Z
Learning: When reviewing CLI subcommand documentation, don’t rely solely on the companion ARCH-016 enforcement rule’s limited path coverage (it only checks `src/commands/<parent>/*.ts` and `src/commands/<parent>/*/index.ts`). Manually verify that subcommands documented by convention in deeper paths (e.g., `src/commands/<parent>/**/add.ts` or `src/commands/adr/domain/add.ts`) have the required documentation, since future nested subcommands can drift without automated detection (tracked by ARCH-015 / GitHub `#503`).
Applied to files:
src/commands/upgrade.ts
📚 Learning: 2026-07-15T22:55:51.978Z
Learnt from: rhuanbarreto
Repo: archgate/cli PR: 476
File: tests/helpers/telemetry-config.test.ts:24-28
Timestamp: 2026-07-15T22:55:51.978Z
Learning: In this Bun/TypeScript codebase, when a unit under test spawns subprocesses via Bun.spawn (e.g., running `git credential ...`), prefer overriding relevant env vars (such as `HOME`, `GIT_CONFIG_GLOBAL`, `GIT_CONFIG_NOSYSTEM`) using `process.env` in the test and restoring them with the test utility (e.g., `restoreEnv` from `tests/test-utils.ts`). Avoid relying on `spyOn(os, 'homedir')` for this purpose, because it only affects in-process calls and does not change the environment inherited by subprocesses; env-var overrides should be used for subprocess-level isolation and must be applied at call time.
Applied to files:
tests/helpers/binary-upgrade-archive.test.ts
📚 Learning: 2026-07-15T22:56:35.415Z
Learnt from: rhuanbarreto
Repo: archgate/cli PR: 476
File: tests/commands/clean.test.ts:61-62
Timestamp: 2026-07-15T22:56:35.415Z
Learning: When reviewing tests that rely on src/helpers/paths.ts `internalPath()`, note that `internalPath()` intentionally reads `Bun.env.HOME ?? Bun.env.USERPROFILE` at call time and only uses `os.homedir()` if neither env var is set. Therefore, don’t suggest changing tests to `spyOn(os, "homedir")` for this behavior; instead, use per-test `Bun.env.HOME` / `Bun.env.USERPROFILE` overrides (as applicable) so the tests control `internalPath()`’s inputs.
Applied to files:
tests/helpers/binary-upgrade-archive.test.ts
📚 Learning: 2026-07-25T15:44:40.668Z
Learnt from: rhuanbarreto
Repo: archgate/cli PR: 501
File: .archgate/adrs/ARCH-005-testing-standards.md:0-0
Timestamp: 2026-07-25T15:44:40.668Z
Learning: In Archgate CLI test code governed by ARCH-007, only allow `Bun.$` in test suites that are explicitly restricted to a single platform. Any cross-platform test that runs on Linux, macOS, and Windows must avoid `Bun.$` and instead use array-based `Bun.spawn`. For shared git setup used by tests, import and use the `git()` helper from `tests/test-utils.ts` rather than duplicating git setup logic.
Applied to files:
tests/helpers/binary-upgrade-archive.test.ts
📚 Learning: 2026-08-05T16:54:13.117Z
Learnt from: rhuanbarreto
Repo: archgate/cli PR: 550
File: tests/commands/adr/domain/remove.test.ts:21-24
Timestamp: 2026-08-05T16:54:13.117Z
Learning: In the Archgate CLI test suite, continue using `z.object` for JSON output schemas unless a repository-wide testing policy explicitly adopts `z.strictObject`. Do not introduce strict CLI-output schema enforcement as an isolated change in a coverage-focused pull request; require coordinated updates and policy agreement across affected tests.
Applied to files:
tests/helpers/binary-upgrade-archive.test.ts
📚 Learning: 2026-07-25T23:21:49.190Z
Learnt from: rhuanbarreto
Repo: archgate/cli PR: 512
File: tests/engine/git-files.test.ts:98-100
Timestamp: 2026-07-25T23:21:49.190Z
Learning: When reviewing archgate/cli for ARCH-006 (per its ADR frontmatter), only enforce the production-dependency policy scoped to package.json. Do not treat test-only refactors or relocated `node:fs` fixture writes as an ARCH-006 violation (since ARCH-006 does not govern test-file I/O API selection). If there’s a broader/test-wide refactor that would migrate fixture writing to `Bun.write()`, evaluate it separately under the appropriate in-scope rule.
Applied to files:
tests/helpers/binary-upgrade-archive.test.ts
📚 Learning: 2026-07-27T16:05:38.683Z
Learnt from: rhuanbarreto
Repo: archgate/cli PR: 536
File: tests/commands/adr/sync-strict.test.ts:173-173
Timestamp: 2026-07-27T16:05:38.683Z
Learning: In this Bun + TypeScript repo, for rejected-promise assertions use the unawaited form: `expect(promise).rejects.toThrow(...)`. Do NOT add `await` to `expect(promise).rejects.toThrow(...)` (Bun’s types model this as `void`), because it will violate the type-aware oxlint rules `typescript(await-thenable)` and `typescript(no-confusing-void-expression)`. Only request an `await` if the repo adopts a typed, lint-compliant assertion helper or Bun’s typings change.
Applied to files:
tests/helpers/binary-upgrade-archive.test.ts
📚 Learning: 2026-08-05T16:54:50.574Z
Learnt from: rhuanbarreto
Repo: archgate/cli PR: 550
File: tests/helpers/plugin-install-cursor-hooks.test.ts:44-44
Timestamp: 2026-08-05T16:54:50.574Z
Learning: In this repository, every TypeScript module under `src/` must have a matching `<module-name>.test.ts` file under the mirrored `tests/` directory, as required by ARCH-005. Supplemental behavior-suffixed sibling test files are allowed only when the matching parent test file exists. Use such siblings to keep individual test files below the 500-line oxlint limit.
Applied to files:
tests/helpers/binary-upgrade-archive.test.ts
🔇 Additional comments (6)
.claude/agent-memory/archgate-developer/MEMORY.md (1)
28-35: LGTM!src/helpers/binary-upgrade.ts (2)
4-11: LGTM!
228-238: LGTM!Also applies to: 290-303
src/commands/upgrade.ts (2)
3-3: LGTM!
281-303: LGTM!tests/helpers/binary-upgrade-archive.test.ts (1)
12-13: LGTM!Also applies to: 109-143, 158-160, 176-176, 185-203, 224-224, 233-268
An empty listing from a failed `tar -tzf` read as "no unsafe entries" and waved the archive past the path-traversal guard; a non-zero listing exit now aborts before extraction. Subprocess output goes through one helper that drains stdout and stderr concurrently with the exit code, the pattern ARCH-007 prescribes. The tar extraction-failure branch is now covered on every runner rather than only where `tar -tzf` can read a Windows path. Signed-off-by: Rhuan Barreto <rhuan@barreto.work>
Deriving the directory to remove from `dirname(binaryPath)` deletes whichever directory the binary happens to sit in. downloadReleaseBinary now returns that directory alongside the binary, so the caller removes the one actually created rather than inferring it. The success-path cleanup gains an assertion against a real directory, which is what the mocked string path left uncovered. Signed-off-by: Rhuan Barreto <rhuan@barreto.work>
Signed-off-by: Rhuan Barreto <rhuan@barreto.work>
Code Coverage
Full HTML report available in workflow artifacts. Per-directory breakdown
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/helpers/binary-upgrade.ts`:
- Line 327: Update the binary extraction return path around binaryPath to
validate it with lstatSync(binaryPath).isFile() rather than existsSync(),
rejecting symbolic links and directories by throwing UserError before returning.
Add a regression test covering an archive whose archgate member extracts as a
symlink, ensuring it is not accepted or cached.
In `@tests/helpers/binary-upgrade.test.ts`:
- Line 384: Update the test around downloadReleaseBinary to retain tmpDir from
the returned result and wrap the extraction assertions in a finally block that
removes tmpDir, ensuring the archgate-upgrade-* directory is cleaned up after
successful or failed execution.
🪄 Autofix
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: cf2115ea-7b2c-4929-b82c-9220a96d4853
📒 Files selected for processing (8)
.claude/agent-memory/archgate-developer/MEMORY.md.claude/agent-memory/archgate-developer/feedback_verify_agent_claims.mdsrc/commands/upgrade.tssrc/helpers/binary-upgrade.tstests/commands/upgrade-action.test.tstests/commands/upgrade-dispatch.test.tstests/helpers/binary-upgrade-archive.test.tstests/helpers/binary-upgrade.test.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Cloudflare Pages
🧰 Additional context used
📓 Path-based instructions (12)
{src,tests}/**/*.ts
📄 CodeRabbit inference engine (.archgate/adrs/LEGAL-001-spdx-license-headers.md)
{src,tests}/**/*.ts: Every TypeScript source file insrc/andtests/must begin with// SPDX-License-Identifier: Apache-2.0followed by// Copyright 2026 Archgate.
If a TypeScript file has a shebang line (for example#!/usr/bin/env buninsrc/cli.ts), the SPDX license header must appear immediately after the shebang.
Use single-line//comments for the SPDX header; do not use block comments (/* */) or alternate license identifiers.
Files:
tests/helpers/binary-upgrade.test.tssrc/commands/upgrade.tstests/commands/upgrade-dispatch.test.tssrc/helpers/binary-upgrade.tstests/helpers/binary-upgrade-archive.test.tstests/commands/upgrade-action.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.archgate/adrs/ARCH-006-dependency-policy.md)
**/*.{ts,tsx}: Prefer Bun built-ins for file I/O, HTTP, globbing, testing, and subprocess execution; prefernode:built-in modules over npm alternatives when appropriate.
UseBun.spawnwith array-based arguments for all subprocess execution; do not useBun.$because it can hang on Windows.
Do not add npm packages for functionality already provided by Bun, such asglob,chalk, or utility libraries used for a single function.
Use Bun APIs such asBun.file()instead of Node.js-specific APIs such asfs.readFile()when Bun provides an equivalent.
Use relative imports with Bun's native module resolution; do not use TypeScript path aliases.Use TypeScript strict mode with ESNext and ES modules; derive schema types with
z.infer<>rather than defining separate interfaces.
Files:
tests/helpers/binary-upgrade.test.tssrc/commands/upgrade.tstests/commands/upgrade-dispatch.test.tssrc/helpers/binary-upgrade.tstests/helpers/binary-upgrade-archive.test.tstests/commands/upgrade-action.test.ts
{src,tests,lint,scripts,shims}/**/*.ts
📄 CodeRabbit inference engine (.archgate/adrs/GEN-004-concise-forward-only-code-comments.md)
{src,tests,lint,scripts,shims}/**/*.ts: Project-authored TypeScript comments must be concise, describe current behavior only, and never narrate history, relocations, refactors, or how the code came to be.
A contiguous run of whole-line comments must contain at most five lines of narrative prose; longer rationale belongs in an ADR, agent-memory file, issue, or PR with a pointer. Tests and fixtures follow the same limit.
Use structural TSDoc tags such as@param,@returns,@throws,@example, and@seefor structured documentation; tagged sections are exempt from the five-line narrative bound, while@remarks,@description,@summary,@notes,@todo, and@fixmeremain counted as prose.
Files:
tests/helpers/binary-upgrade.test.tssrc/commands/upgrade.tstests/commands/upgrade-dispatch.test.tssrc/helpers/binary-upgrade.tstests/helpers/binary-upgrade-archive.test.tstests/commands/upgrade-action.test.ts
tests/**/*.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Tests mirror the
src/structure; shared test fixtures belong undertests/fixtures/.
tests/**/*.ts: Use Bun's built-inbun:testrunner for all tests; do not importnode:test. Test files belong undertests/, mirrorsrc/, usetests/fixtures/for shared fixtures, and follow<module-name>.test.tsnaming.
Filesystem tests must use isolatedmkdtempdirectories and clean them up inafterEachorafterAll; do not touch real user-scope paths or leave temporary files behind.
Close external SDK instances, servers, clients, and transports in test hooks, such asawait server.close()inafterEachorafterAll.
Restore every captured environment variable withrestoreEnv(key, original); never restore with direct assignment such asBun.env.X = original, becauseundefinedbecomes the string"undefined".
Mockos.homedir()via an imported module namespace andspyOn; do not rely on overridingHOMEfor code usingos.homedir(), and keep filesystem writes inside temporary directories.
Shared test helpers, including non-test files undertests/, must restore every captured environment variable withrestoreEnv; isolation responsibilities apply across the entire shared Bun test process.Use
_resetAllCaches()fromsrc/helpers/platform.tsto simulate different platforms in tests rather than mockingprocess.platformdirectly.
Files:
tests/helpers/binary-upgrade.test.tstests/commands/upgrade-dispatch.test.tstests/helpers/binary-upgrade-archive.test.tstests/commands/upgrade-action.test.ts
**/*.{js,ts,tsx,mjs,cjs}
📄 CodeRabbit inference engine (.archgate/adrs/GEN-003-tool-invocation-via-scripts.md)
Invoke linting, formatting, and validation through package scripts (
bun run lint,bun run format,bun run format:check, andbun run validate), rather than directly invoking tool binaries such asbunx prettier,bunx oxfmt,npx eslint, oroxlint.
Files:
tests/helpers/binary-upgrade.test.tssrc/commands/upgrade.tstests/commands/upgrade-dispatch.test.tssrc/helpers/binary-upgrade.tstests/helpers/binary-upgrade-archive.test.tstests/commands/upgrade-action.test.ts
tests/**/*.test.ts
📄 CodeRabbit inference engine (.archgate/adrs/ARCH-025-idiomatic-bun-test-parametrization-and-matchers.md)
tests/**/*.test.ts: Usetest.each()ordescribe.each()for the same assertion logic against multiple independent inputs. Do not register tests or callexpect()once per case inside aforor.forEachloop.
Use array rows for positional destructuring and object rows for named fields when passing cases totest.each()ordescribe.each(). Choose descriptive title placeholders such as%s,%p,%d, or$field.
Assert derived facts with the most specific matcher available instead of passing a derived boolean to.toBe(true)or.toBe(false). Compare values directly with.toBe()or.toEqual().
Use specific matchers for common derived checks:.toContain()or.toMatch()for containment,.toBeInstanceOf()for type checks such asArray.isArray,.toHaveLength()for counts, and.find()with.toBeDefined()or.toBeUndefined()for predicate existence checks.
Do not precompute a boolean solely to assert it; assert the underlying values directly with matchers such as.toEqual()or.toBe().
When converting a loop totest.each()ordescribe.each(), preserve every assertion that ran per iteration; do not drop or merge assertions.
tests/**/*.test.ts: Every runnable test must contain anexpect()assertion; usetest.skiportest.todofor placeholders rather than assertion-less or silently skipped tests.
Test public interfaces with descriptive names rather than private implementation details.
Do not usemock.module()for first-party modules. Mock them withimport * as modplusspyOn(mod, "fn"), and restore mocks after each test.mock.module()may be used for approved external modules such asinquirerornode:readline.
For HTTP mocking, saveglobalThis.fetchbefore direct assignment and restore it inafterEach; do not usemock.module("node:fetch"), which does not intercept Bun's global fetch.
Wrap inlinespyOnormockImplementationlifecycles intry/finally, or manage them in hooks, somockRestore()runs wh...
Files:
tests/helpers/binary-upgrade.test.tstests/commands/upgrade-dispatch.test.tstests/helpers/binary-upgrade-archive.test.tstests/commands/upgrade-action.test.ts
**
⚙️ CodeRabbit configuration file
**: This project uses Archgate — an AI governance framework based on
Architecture Decision Records (ADRs). The ADRs in.archgate/adrs/
are the authoritative rules for this codebase. Each ADR has a companion
.rules.tsfile with automated checks that run viaarchgate check.When reviewing, you must:
- Treat ADR violations as blocking issues, not suggestions.
- Cite the specific ADR ID when flagging a violation (e.g., "Violates ARCH-006").
- Focus on semantic and contextual violations that automated rules cannot catch —
the.rules.tsfiles already cover syntactic/structural patterns.- If you are unsure whether something violates an ADR, flag it as a question
rather than approving it.
Files:
tests/helpers/binary-upgrade.test.tssrc/commands/upgrade.tstests/commands/upgrade-dispatch.test.tssrc/helpers/binary-upgrade.tstests/helpers/binary-upgrade-archive.test.tstests/commands/upgrade-action.test.ts
src/commands/**/*.ts
📄 CodeRabbit inference engine (.archgate/adrs/ARCH-011-consistent-project-root-resolution.md)
src/commands/**/*.ts: All commands that operate on.archgate/project resources must use the sharedfindProjectRoot()fromsrc/helpers/paths.ts; directprocess.cwd()project-root resolution is prohibited except ininit.
Commands that require a project must userequireProjectRoot()fromsrc/helpers/paths.tsinstead of implementing their own missing-project guard. Commands that can operate without a project may usefindProjectRoot()and handlenullthemselves.
When usingfindProjectRoot()directly, check for anullresult and provide a helpful error before exiting.
Pass the resolvedprojectRoottoprojectPaths()when constructing derived project paths.
Do not define localfindProjectRoot()variants; use the shared implementation fromsrc/helpers/paths.ts.Command modules must export a
register*Command(program)function, handle I/O only, and contain no business logic.
src/commands/**/*.ts: Each command module must export aregister*Command(program)function; each non-index.tscommand file must define exactly one command.
Command files must remain thin: parse arguments, call engine/helpers, and format output; business logic must reside insrc/engine/,src/helpers/, orsrc/formats/.
Commands must execute in-process and must not spawn child processes for subcommand execution.
Command files must not call.parse(); argument parsing is handled by the CLI entry point.
Commands should use typed Commander registration APIs, such as@commander-js/extra-typings, within theirregister*Commandfunctions.
src/commands/**/*.ts: In Commander.js command files, options requiring type narrowing beyond plain strings MUST usenew Option()from@commander-js/extra-typingsand register it with.addOption()instead of.option().
Use.choices([... ] as const)for options accepting a fixed set of values, and use.default(... as const)when providing a default, preserving literal type inference.
Use.argParser((value) => ...)...
Files:
src/commands/upgrade.ts
src/**/*.ts
📄 CodeRabbit inference engine (.archgate/adrs/ARCH-018-lazy-load-heavy-dependencies.md)
src/**/*.ts: Heavy runtime dependencies such asinquirer,posthog-node, and@sentry/*must be loaded with dynamicimport()at their point of use, never through top-level static value imports.
Type-only imports for heavy dependencies are allowed, but runtime values must be obtained through dynamicimport(); for example, useimport type { PostHog } from "posthog-node".
SDKs that require early initialization may use eager-start/lazy-await: begin initialization before command registration and await the result at first use, such as in apreActionhook.
src/**/*.ts: In all TypeScript source files undersrc/, useBun.envfor every environment-variable read and write; never referenceprocess.env, including in comments.
AccessBun.envproperties directly; do not create wrapper functions around it or destructure it.
Use nullish coalescing for environment-variable defaults, such asBun.env.NODE_ENV ?? "production".
For truthy environment-flag checks, useBoolean(Bun.env.FLAG)only inline, as part of a larger expression, or assign it to a variable before using it as a sole condition; otherwise use an explicit defined-and-nonempty comparison.
src/**/*.ts: Everyinquirer.prompt(...)call must be wrapped inwithPromptFix(() => ...)imported fromsrc/helpers/prompt.ts; keep the wrapper adjacent to the prompt invocation so automated checks can detect it.
Do not callinquirer.prompt(...)directly or reimplement cursor/newline fixes at individual call sites; route all prompt behavior throughwithPromptFix().Every call to
Bun.Glob#scan()(glob.scan(...)) in source must pass{ dot: true }in its options object, including scans whose patterns do not explicitly target dot-directories. Do not usedot: false; intentionally excluded dotfiles must be filtered explicitly after scanning with a comment. Normalize scanned path separators withfile.replaceAll("\\", "/")when performing cross-platform path comparisons.
src/**/*.ts: Use `sty...
Files:
src/commands/upgrade.tssrc/helpers/binary-upgrade.ts
src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.{ts,tsx}: For user-scope editors, resolve paths using the editor's actual path helper; do not assume Windows conventions. For opencode, mirrorxdg-basedir, which falls back to~/.configon all platforms.
For opencode-gated behavior, useisOpencodeAvailable()rather thanisOpencodeCliAvailable()alone because the Desktop distribution has no CLI binary and shares the config directory.
For Copilot-gated behavior, useisCopilotAvailable()rather thanisCopilotCliAvailable()alone because desktop and CLI distributions share~/.copilot/.
Files:
src/commands/upgrade.tssrc/helpers/binary-upgrade.ts
src/commands/{*.ts,*/index.ts}
📄 CodeRabbit inference engine (.archgate/adrs/ARCH-015-cli-command-documentation-coverage.md)
Top-level command modules must follow the
src/commands/<name>.tsorsrc/commands/<name>/index.tsconvention. Nested subcommand files do not count as top-level commands.
Files:
src/commands/upgrade.ts
src/**/!(platform).ts
📄 CodeRabbit inference engine (.archgate/adrs/ARCH-009-platform-detection-helper.md)
All platform detection in
src/must go throughsrc/helpers/platform.ts(isWindows(),isMacOS(),isLinux(),isWSL(), orgetPlatformInfo()); directprocess.platformaccess and duplicated detection logic are forbidden outsideplatform.ts.
Files:
src/commands/upgrade.tssrc/helpers/binary-upgrade.ts
🧠 Learnings (17)
📚 Learning: 2026-07-15T22:55:51.978Z
Learnt from: rhuanbarreto
Repo: archgate/cli PR: 476
File: tests/helpers/telemetry-config.test.ts:24-28
Timestamp: 2026-07-15T22:55:51.978Z
Learning: In this Bun/TypeScript codebase, when a unit under test spawns subprocesses via Bun.spawn (e.g., running `git credential ...`), prefer overriding relevant env vars (such as `HOME`, `GIT_CONFIG_GLOBAL`, `GIT_CONFIG_NOSYSTEM`) using `process.env` in the test and restoring them with the test utility (e.g., `restoreEnv` from `tests/test-utils.ts`). Avoid relying on `spyOn(os, 'homedir')` for this purpose, because it only affects in-process calls and does not change the environment inherited by subprocesses; env-var overrides should be used for subprocess-level isolation and must be applied at call time.
Applied to files:
tests/helpers/binary-upgrade.test.tstests/helpers/binary-upgrade-archive.test.ts
📚 Learning: 2026-07-15T22:56:35.415Z
Learnt from: rhuanbarreto
Repo: archgate/cli PR: 476
File: tests/commands/clean.test.ts:61-62
Timestamp: 2026-07-15T22:56:35.415Z
Learning: When reviewing tests that rely on src/helpers/paths.ts `internalPath()`, note that `internalPath()` intentionally reads `Bun.env.HOME ?? Bun.env.USERPROFILE` at call time and only uses `os.homedir()` if neither env var is set. Therefore, don’t suggest changing tests to `spyOn(os, "homedir")` for this behavior; instead, use per-test `Bun.env.HOME` / `Bun.env.USERPROFILE` overrides (as applicable) so the tests control `internalPath()`’s inputs.
Applied to files:
tests/helpers/binary-upgrade.test.tstests/commands/upgrade-dispatch.test.tstests/helpers/binary-upgrade-archive.test.tstests/commands/upgrade-action.test.ts
📚 Learning: 2026-07-25T00:05:58.884Z
Learnt from: rhuanbarreto
Repo: archgate/cli PR: 496
File: tests/helpers/auth.test.ts:38-46
Timestamp: 2026-07-25T00:05:58.884Z
Learning: When reviewing the Archgate CLI repository’s GEN-004 “concise forward-only narration” comments, don’t rely only on the automated phrase-based narration checks. Those checks can pass even when the comment wording describes historical/transfer semantics rather than current behavior (e.g., saying a prior restore “leaked” a value or a later subprocess “inherited it”). Manually verify that the comment describes the code’s current, forward behavior; flag or adjust comments that imply past/historical state transfer even if GEN-004 enforcement passes.
Applied to files:
tests/helpers/binary-upgrade.test.tssrc/commands/upgrade.tstests/commands/upgrade-dispatch.test.tssrc/helpers/binary-upgrade.tstests/helpers/binary-upgrade-archive.test.tstests/commands/upgrade-action.test.ts
📚 Learning: 2026-07-25T00:05:59.109Z
Learnt from: rhuanbarreto
Repo: archgate/cli PR: 496
File: src/cli.ts:0-0
Timestamp: 2026-07-25T00:05:59.109Z
Learning: Code comments may include a concise issue/PR reference (per GEN-004) when it’s used to point readers to fuller rationale instead of inlining that rationale. During review, flag surrounding comment prose that reads like historical context or narrates refactors/relocations; a bare GEN-004-style reference is allowed and should not be flagged by itself.
Applied to files:
tests/helpers/binary-upgrade.test.tssrc/commands/upgrade.tstests/commands/upgrade-dispatch.test.tssrc/helpers/binary-upgrade.tstests/helpers/binary-upgrade-archive.test.tstests/commands/upgrade-action.test.ts
📚 Learning: 2026-08-04T19:58:05.877Z
Learnt from: rhuanbarreto
Repo: archgate/cli PR: 543
File: src/helpers/copilot-user-settings.ts:0-0
Timestamp: 2026-08-04T19:58:05.877Z
Learning: In archgate/cli TypeScript code, use `Bun.file(path).exists()` only to check whether a file exists; it must not be used for directory existence checks. For helpers such as `isCopilotAvailable()` that need to detect a configuration directory, use an appropriate directory-aware check such as `existsSync` from `node:fs`.
Applied to files:
tests/helpers/binary-upgrade.test.tssrc/commands/upgrade.tstests/commands/upgrade-dispatch.test.tssrc/helpers/binary-upgrade.tstests/helpers/binary-upgrade-archive.test.tstests/commands/upgrade-action.test.ts
📚 Learning: 2026-08-05T06:56:33.435Z
Learnt from: rhuanbarreto
Repo: archgate/cli PR: 546
File: tests/integration/stream-guards.test.ts:3-9
Timestamp: 2026-08-05T06:56:33.435Z
Learning: When reviewing GEN-004 comment-block limits in the Archgate CLI repository, count only narrative prose lines within a block comment. Do not count a closing delimiter such as `*/` as a prose line; for example, in `tests/integration/stream-guards.test.ts`, Lines 4–8 contain five prose lines while Line 9 contains only the delimiter.
Applied to files:
tests/helpers/binary-upgrade.test.tssrc/commands/upgrade.tstests/commands/upgrade-dispatch.test.tssrc/helpers/binary-upgrade.tstests/helpers/binary-upgrade-archive.test.tstests/commands/upgrade-action.test.ts
📚 Learning: 2026-08-06T21:09:28.014Z
Learnt from: rhuanbarreto
Repo: archgate/cli PR: 561
File: src/helpers/binary-upgrade.ts:239-287
Timestamp: 2026-08-06T21:09:28.014Z
Learning: In archgate/cli TypeScript code, follow ARCH-007 for Bun subprocess stream capture; ARCH-017 does not govern subprocess pipe handling. When Bun.spawn() uses piped stdout or stderr, use a shared capture helper where practical, consume configured streams concurrently to avoid deadlocks, and verify the subprocess exit code before trusting captured output.
Applied to files:
tests/helpers/binary-upgrade.test.tssrc/commands/upgrade.tstests/commands/upgrade-dispatch.test.tssrc/helpers/binary-upgrade.tstests/helpers/binary-upgrade-archive.test.tstests/commands/upgrade-action.test.ts
📚 Learning: 2026-07-25T15:44:40.668Z
Learnt from: rhuanbarreto
Repo: archgate/cli PR: 501
File: .archgate/adrs/ARCH-005-testing-standards.md:0-0
Timestamp: 2026-07-25T15:44:40.668Z
Learning: In Archgate CLI test code governed by ARCH-007, only allow `Bun.$` in test suites that are explicitly restricted to a single platform. Any cross-platform test that runs on Linux, macOS, and Windows must avoid `Bun.$` and instead use array-based `Bun.spawn`. For shared git setup used by tests, import and use the `git()` helper from `tests/test-utils.ts` rather than duplicating git setup logic.
Applied to files:
tests/helpers/binary-upgrade.test.tstests/commands/upgrade-dispatch.test.tstests/helpers/binary-upgrade-archive.test.tstests/commands/upgrade-action.test.ts
📚 Learning: 2026-08-05T16:54:13.117Z
Learnt from: rhuanbarreto
Repo: archgate/cli PR: 550
File: tests/commands/adr/domain/remove.test.ts:21-24
Timestamp: 2026-08-05T16:54:13.117Z
Learning: In the Archgate CLI test suite, continue using `z.object` for JSON output schemas unless a repository-wide testing policy explicitly adopts `z.strictObject`. Do not introduce strict CLI-output schema enforcement as an isolated change in a coverage-focused pull request; require coordinated updates and policy agreement across affected tests.
Applied to files:
tests/helpers/binary-upgrade.test.tstests/commands/upgrade-dispatch.test.tstests/helpers/binary-upgrade-archive.test.tstests/commands/upgrade-action.test.ts
📚 Learning: 2026-07-25T22:03:14.216Z
Learnt from: rhuanbarreto
Repo: archgate/cli PR: 501
File: .archgate/adrs/ARCH-002-error-handling.md:0-0
Timestamp: 2026-07-25T22:03:14.216Z
Learning: In Archgate boundary-wrapped CLI command actions (the handlers that rely on `handleCommandError()` for user-facing error output), expected-failure guards should signal user errors by throwing `new UserError(<message/details>)` rather than directly calling `logError()` followed by `exitWith(1)`. This keeps user-facing logging and the exit path centralized in `handleCommandError()`. For normal/computed command outcomes (e.g., `const exitCode = getExitCode(await runChecks(...))`), use `await exitWith(exitCode)` instead of calling `process.exit(exitCode)` so telemetry/Sentry flushing and outcome tagging still run.
Applied to files:
tests/helpers/binary-upgrade.test.tssrc/commands/upgrade.tstests/commands/upgrade-dispatch.test.tssrc/helpers/binary-upgrade.tstests/helpers/binary-upgrade-archive.test.tstests/commands/upgrade-action.test.ts
📚 Learning: 2026-07-25T23:21:49.190Z
Learnt from: rhuanbarreto
Repo: archgate/cli PR: 512
File: tests/engine/git-files.test.ts:98-100
Timestamp: 2026-07-25T23:21:49.190Z
Learning: When reviewing archgate/cli for ARCH-006 (per its ADR frontmatter), only enforce the production-dependency policy scoped to package.json. Do not treat test-only refactors or relocated `node:fs` fixture writes as an ARCH-006 violation (since ARCH-006 does not govern test-file I/O API selection). If there’s a broader/test-wide refactor that would migrate fixture writing to `Bun.write()`, evaluate it separately under the appropriate in-scope rule.
Applied to files:
tests/helpers/binary-upgrade.test.tstests/commands/upgrade-dispatch.test.tstests/helpers/binary-upgrade-archive.test.tstests/commands/upgrade-action.test.ts
📚 Learning: 2026-07-27T16:05:38.683Z
Learnt from: rhuanbarreto
Repo: archgate/cli PR: 536
File: tests/commands/adr/sync-strict.test.ts:173-173
Timestamp: 2026-07-27T16:05:38.683Z
Learning: In this Bun + TypeScript repo, for rejected-promise assertions use the unawaited form: `expect(promise).rejects.toThrow(...)`. Do NOT add `await` to `expect(promise).rejects.toThrow(...)` (Bun’s types model this as `void`), because it will violate the type-aware oxlint rules `typescript(await-thenable)` and `typescript(no-confusing-void-expression)`. Only request an `await` if the repo adopts a typed, lint-compliant assertion helper or Bun’s typings change.
Applied to files:
tests/helpers/binary-upgrade.test.tstests/commands/upgrade-dispatch.test.tstests/helpers/binary-upgrade-archive.test.tstests/commands/upgrade-action.test.ts
📚 Learning: 2026-08-05T16:54:50.574Z
Learnt from: rhuanbarreto
Repo: archgate/cli PR: 550
File: tests/helpers/plugin-install-cursor-hooks.test.ts:44-44
Timestamp: 2026-08-05T16:54:50.574Z
Learning: In this repository, every TypeScript module under `src/` must have a matching `<module-name>.test.ts` file under the mirrored `tests/` directory, as required by ARCH-005. Supplemental behavior-suffixed sibling test files are allowed only when the matching parent test file exists. Use such siblings to keep individual test files below the 500-line oxlint limit.
Applied to files:
tests/helpers/binary-upgrade.test.tstests/commands/upgrade-dispatch.test.tstests/helpers/binary-upgrade-archive.test.tstests/commands/upgrade-action.test.ts
📚 Learning: 2026-07-25T22:03:22.236Z
Learnt from: rhuanbarreto
Repo: archgate/cli PR: 501
File: .archgate/adrs/ARCH-016-cli-subcommand-documentation-accuracy.md:64-67
Timestamp: 2026-07-25T22:03:22.236Z
Learning: When reviewing CLI subcommand documentation, don’t rely solely on the companion ARCH-016 enforcement rule’s limited path coverage (it only checks `src/commands/<parent>/*.ts` and `src/commands/<parent>/*/index.ts`). Manually verify that subcommands documented by convention in deeper paths (e.g., `src/commands/<parent>/**/add.ts` or `src/commands/adr/domain/add.ts`) have the required documentation, since future nested subcommands can drift without automated detection (tracked by ARCH-015 / GitHub `#503`).
Applied to files:
src/commands/upgrade.ts
📚 Learning: 2026-06-11T12:50:28.661Z
Learnt from: rhuanbarreto
Repo: archgate/cli PR: 406
File: .claude/agent-memory/archgate-developer/feedback_prefer_tests_over_adr_rules.md:8-18
Timestamp: 2026-06-11T12:50:28.661Z
Learning: In `archgate/cli`, for markdown files under `.claude/agent-memory/`, follow the established convention: use YAML frontmatter (with a `name:` field used as the document title) and do not require a top-level `#` (H1) heading. During code review, do not flag missing first-line/first-top-level H1 headings (e.g., MD041) for these agent-memory files since markdownlint is not part of the repo’s `bun run validate` lint pipeline (oxlint/oxfmt only).
Applied to files:
.claude/agent-memory/archgate-developer/MEMORY.md.claude/agent-memory/archgate-developer/feedback_verify_agent_claims.md
📚 Learning: 2026-07-25T00:05:20.592Z
Learnt from: rhuanbarreto
Repo: archgate/cli PR: 496
File: .claude/agent-memory/archgate-developer/project_test_isolation_gotchas.md:10-10
Timestamp: 2026-07-25T00:05:20.592Z
Learning: When reviewing documentation/agent-memory entries under `.claude/agent-memory/**`, do not enforce GEN-004’s “forward-only” comment/narrative requirement. These entries are allowed to keep historical/past-tense incident narratives and dated markers (e.g., `Found YYYY-MM-DD`) because the context is intended to help future agents evaluate edge cases. Outside this scope, GEN-004’s forward-only rule should still apply.
Applied to files:
.claude/agent-memory/archgate-developer/MEMORY.md.claude/agent-memory/archgate-developer/feedback_verify_agent_claims.md
📚 Learning: 2026-07-02T16:03:33.031Z
Learnt from: rhuanbarreto
Repo: archgate/cli PR: 446
File: src/helpers/session-context-opencode.ts:81-100
Timestamp: 2026-07-02T16:03:33.031Z
Learning: For synchronous helper functions that use Bun’s sqlite sync API (i.e., they must remain synchronous), it’s acceptable to use `existsSync` from `node:fs` to check whether the SQLite database file exists. Avoid using `Bun.file(path).exists()` for this purpose because it’s async and would force the helper to become async (no equivalent synchronous Bun alternative). If the DB file is missing, throw/return a clear, actionable "No database found" error (per ARCH-006) rather than letting the sqlite open fail with a generic DB-open error.
Applied to files:
src/helpers/binary-upgrade.ts
🔇 Additional comments (10)
.claude/agent-memory/archgate-developer/MEMORY.md (1)
28-28: LGTM!Also applies to: 35-36
.claude/agent-memory/archgate-developer/feedback_verify_agent_claims.md (1)
25-27: LGTM!src/helpers/binary-upgrade.ts (4)
124-129: Use an accurate ARCH-007 rationale.Line 127 states that current Bun leaves the child blocked and prevents
proc.exitedfrom resolving. The recorded verification found that current Bun buffers these pipes. State that ARCH-007 requires concurrent capture without asserting this unsupported runtime behavior.Based on learnings, current Bun buffers piped output while ARCH-007 still requires concurrent capture.
Source: Learnings
130-166: LGTM!
269-312: LGTM!
328-333: LGTM!src/commands/upgrade.ts (1)
281-301: LGTM!tests/helpers/binary-upgrade-archive.test.ts (1)
128-157: LGTM!Also applies to: 223-262, 320-326
tests/commands/upgrade-action.test.ts (1)
19-20: LGTM!Also applies to: 48-52, 72-75, 104-104, 138-140
tests/commands/upgrade-dispatch.test.ts (1)
467-470: LGTM!
existsSync follows symlinks and accepts directories, so an archive member that is either passed the post-extraction check. replaceBinary renames without following, which would leave ~/.archgate/bin/ holding a symlink aimed wherever the archive chose. lstat rejects both. The zip extraction test removes the directory the download created, which is not the fixture's own tmpDir the surrounding finally already handles. Signed-off-by: Rhuan Barreto <rhuan@barreto.work>
1 participant
Fixes #554, #555 and #556 — all three in
binary-upgrade.ts.#554 —
Expand-Archivefailure swallowed on WindowsConfirmed against the exact production command with a deliberately corrupt archive:
0+ -ErrorAction Stop1+ -ErrorAction Stop, valid archive0, extracted-ErrorAction Stoppromotes the non-terminating error, and a valid archive still extracts — the guard is not over-rejecting.Added a post-extraction existence check as well. It covers the tar branch too, so an archive that extracts to nothing is reported where it happens rather than as a missing binary later.
#555 — the backslash branch is not dead
The issue states that GNU tar's escaping yields
..//evil, "which matches none of the three guard conditions". That is incorrect:"..//evil".includes("../")istrue— the../needle matches at index 0.Verified end-to-end against real GNU tar 1.35 rather than by reading:
..\evilas..\evil..//evil..\evil../evilSo the normalization is load-bearing, not decoration. Fire-tested under WSL: deleting
.replaceAll("\\", "/")makes the new case fail, restoring it makes it pass.No production change here. What was actually wrong was the test file, which documented the backslash row as deliberately absent and gave this false reason. The row is now present with the comment corrected; it asserts
/\.\.\+evil/uso it holds for both tar implementations.#556 — temp directory leak
Every failure path now removes the extraction directory.
The issue says "only the success path's caller cleans up" — the caller never cleaned up at all, so a successful upgrade leaked the directory plus the ~100 MB archive inside it.
upgradeBinarynow removes it once the binary is installed, from an innerfinallythat runs before the handler callingexitWith()(which ends the process, so a trailingfinallywould be skipped).Verification
Each fix was fire-tested by removing it and confirming the corresponding test fails:
-ErrorAction StoprmSyncThe tar-branch tests are
skipIf(win32), so they were run under WSL Ubuntu rather than left to CI — 11/11 pass on Linux.bun run validategreen: 2390 pass / 0 fail,archgate check51/51 with 0 warnings under strict mode.