Skip to content

Conversation

@dannysmith
Copy link
Owner

@dannysmith dannysmith commented Dec 30, 2025

Summary by CodeRabbit

  • New Features

    • Interactive release preparation and an installer to download, verify, and install the CLI (macOS/Linux).
  • Chores

    • Added CI for linting, cross-platform testing, and multi-target builds.
    • Added release workflow to build platform archives, generate checksums, and publish releases.
    • Added build/release helper scripts and package scripts; updated ignore patterns.
  • Bug Fixes

    • Avoid fatal write failures on Windows during atomic file replace.
    • Better handling for temporary-directory vault paths across platforms.
  • Tests

    • Improved cross-platform path tests and added a helper to detect archive paths.

✏️ Tip: You can customize this high-level summary in your review settings.

  - Add build:binary and build:binary:release scripts to package.json
  - Create scripts/build-release.sh for platform-specific local builds
  - Add *.bun-build to gitignore
  - Lint & typecheck on ubuntu
  - Tests on ubuntu, macos, windows
  - Binary builds for all 5 platform targets
  - Uploads artifacts for each platform
  - Release workflow triggered by tdn-cli-v* tags
  - Builds archives with checksums for all 5 platforms
  - Creates GitHub Release with auto-generated notes
  - Triggers Homebrew tap update for stable releases
  - Add curl-installable install.sh script
@coderabbitai
Copy link

coderabbitai bot commented Dec 30, 2025

Warning

Rate limit exceeded

@dannysmith has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 10 minutes and 58 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between 548c31f and 85000c8.

📒 Files selected for processing (1)
  • tdn-cli/crates/core/src/vault_index.rs
📝 Walkthrough

Walkthrough

Adds CI and release GitHub workflows, new CLI build/install/prepare scripts, package scripts and .gitignore entry, a Windows-tolerant disk-sync tweak in Rust writer, vault-path temp-dir acceptance, and several test/helper updates to centralize archive-path detection.

Changes

Cohort / File(s) Summary
GitHub Actions workflows
.github/workflows/ci-cli.yml, .github/workflows/release-cli.yml
New "CI - CLI" workflow (lint, multi-platform test matrix, multi-target build matrix, artifact uploads) and "Release - CLI" workflow (tag-triggered multi-platform builds, create archives and SHA256 checksums, create GitHub release, optional Homebrew dispatch).
CLI scripts & package
tdn-cli/scripts/build-release.sh, tdn-cli/scripts/install.sh, tdn-cli/scripts/prepare-release.js, tdn-cli/package.json, tdn-cli/.gitignore
New build/install/prepare scripts; added npm/bun scripts build:binary, build:binary:release, release:prepare; added *.bun-build to .gitignore.
Rust core change
tdn-cli/crates/core/src/writer.rs
Make temp-file fsync non-fatal on Windows (ignore sync errors there; keep fatal on non-Windows) while preserving atomic rename behavior.
Config path handling
tdn-cli/src/config/index.ts
Broadened temp-dir recognition (macOS /var/folders, Linux /tmp) and only warn for vault paths outside home when not in recognized temp dirs.
Tests & test helpers
tdn-cli/tests/helpers/cli.ts, tdn-cli/tests/e2e/list.test.ts, tdn-cli/tests/e2e/modify.test.ts, tdn-cli/tests/unit/bindings.test.ts, tdn-cli/tests/unit/config-security.test.ts
Added exported helper isArchivePath; replaced hardcoded 'archive/' checks with isArchivePath; made tests cross-platform by using path.join and adding Linux temp-dir acceptance tests; updated assertions to use regex/path helper.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Poem

🐰 I hopped the CI fields at dawn's first light,
Built binaries for day and night,
Tagged each crate with a tidy cheer,
Checked sums, packed archives — ready to steer,
Installer nibble — swift and bright.

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'CLI Build and Deployment Setup' accurately reflects the main purpose of this PR, which adds comprehensive GitHub Actions workflows for CI/CD, build scripts, release automation, and installation infrastructure for the tdn-cli tool.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

🧹 Nitpick comments (4)
tdn-cli/scripts/install.sh (1)

62-69: Fragile JSON parsing without jq.

Parsing JSON with grep and sed is brittle and may break if GitHub changes their API response formatting. Consider adding jq as an optional dependency with a fallback, or document this limitation.

🔎 Proposed improvement using jq with fallback
 if [ "$VERSION" = "latest" ]; then
   info "Fetching latest version..."
-  # Filter for tdn-cli releases specifically
-  VERSION=$(curl -sL "https://api.github.com/repos/${REPO}/releases" | \
-    grep '"tag_name":' | \
-    grep 'tdn-cli-v' | \
-    head -1 | \
-    sed -E 's/.*"tdn-cli-v([^"]+)".*/\1/')
+  # Filter for tdn-cli releases specifically
+  RELEASES_JSON=$(curl -sL "https://api.github.com/repos/${REPO}/releases")
+  if command -v jq >/dev/null 2>&1; then
+    VERSION=$(echo "$RELEASES_JSON" | jq -r '[.[] | select(.tag_name | startswith("tdn-cli-v"))][0].tag_name // empty' | sed 's/^tdn-cli-v//')
+  else
+    VERSION=$(echo "$RELEASES_JSON" | \
+      grep '"tag_name":' | \
+      grep 'tdn-cli-v' | \
+      head -1 | \
+      sed -E 's/.*"tdn-cli-v([^"]+)".*/\1/')
+  fi
   if [ -z "$VERSION" ]; then
     error "Failed to fetch latest version. Check your internet connection."
   fi
 fi
tdn-cli/scripts/prepare-release.js (1)

146-148: Add error handling for unhandled promise rejection.

The script calls prepareRelease() without catching errors, which could lead to unhandled promise rejection warnings in Node.js.

🔎 Proposed fix
-prepareRelease()
+prepareRelease().catch(error => {
+  console.error('Unexpected error:', error.message)
+  process.exit(1)
+})
tdn-cli/scripts/build-release.sh (1)

7-7: VERSION is unused in the output filename.

The VERSION variable is set but only used in the echo message, not in the actual output filename. This might be intentional for the build script (version is added during release packaging), but consider adding a comment to clarify this design decision.

.github/workflows/release-cli.yml (1)

109-109: Static analysis hint: Consider quoting the variable expansion.

While the ref value is unlikely to contain spaces, quoting ${GITHUB_REF#refs/tags/tdn-cli-v} is a good defensive practice.

🔎 Proposed fix
       - name: Extract version
         id: version
-        run: echo "version=${GITHUB_REF#refs/tags/tdn-cli-v}" >> $GITHUB_OUTPUT
+        run: echo "version=${GITHUB_REF#refs/tags/tdn-cli-v}" >> "$GITHUB_OUTPUT"
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0c2a045 and aa32564.

⛔ Files ignored due to path filters (2)
  • docs/tasks-todo/task-x-cli-distribution-and-releases.md is excluded by !**/*.md
  • tdn-cli/docs/developer/releases.md is excluded by !**/*.md
📒 Files selected for processing (7)
  • .github/workflows/ci-cli.yml
  • .github/workflows/release-cli.yml
  • tdn-cli/.gitignore
  • tdn-cli/package.json
  • tdn-cli/scripts/build-release.sh
  • tdn-cli/scripts/install.sh
  • tdn-cli/scripts/prepare-release.js
🧰 Additional context used
🧠 Learnings (13)
📓 Common learnings
Learnt from: CR
Repo: dannysmith/taskdn PR: 0
File: tdn-cli/CLAUDE.md:0-0
Timestamp: 2025-12-28T12:22:45.546Z
Learning: Follow TDD workflow for CLI features: write failing E2E test, add fixture files conforming to S1 spec, review test against specs, get user confirmation, implement until green, refactor, add unit tests (Rust for parsing, TS for formatters/utilities), run checks, update docs
📚 Learning: 2025-12-28T12:22:45.546Z
Learnt from: CR
Repo: dannysmith/taskdn PR: 0
File: tdn-cli/CLAUDE.md:0-0
Timestamp: 2025-12-28T12:22:45.546Z
Learning: Follow TDD workflow for CLI features: write failing E2E test, add fixture files conforming to S1 spec, review test against specs, get user confirmation, implement until green, refactor, add unit tests (Rust for parsing, TS for formatters/utilities), run checks, update docs

Applied to files:

  • tdn-cli/scripts/prepare-release.js
  • .github/workflows/ci-cli.yml
  • tdn-cli/package.json
📚 Learning: 2025-12-28T12:22:37.372Z
Learnt from: CR
Repo: dannysmith/taskdn PR: 0
File: archived-projects/taskdn-ts/CLAUDE.md:0-0
Timestamp: 2025-12-28T12:22:37.372Z
Learning: Clean build artifacts (*.node, index.js, index.d.ts) before rebuilding if experiencing build failures

Applied to files:

  • tdn-cli/scripts/build-release.sh
  • tdn-cli/package.json
📚 Learning: 2025-12-28T12:22:45.546Z
Learnt from: CR
Repo: dannysmith/taskdn PR: 0
File: tdn-cli/CLAUDE.md:0-0
Timestamp: 2025-12-28T12:22:45.546Z
Learning: Applies to tdn-cli/bindings/** : Never manually edit files in the `bindings/` directory as it is auto-generated by NAPI-RS

Applied to files:

  • tdn-cli/.gitignore
  • tdn-cli/package.json
📚 Learning: 2025-12-28T12:22:45.546Z
Learnt from: CR
Repo: dannysmith/taskdn PR: 0
File: tdn-cli/CLAUDE.md:0-0
Timestamp: 2025-12-28T12:22:45.546Z
Learning: Applies to tdn-cli/src/**/*.{ts,tsx} : Use path aliases in TypeScript imports: `@/*` maps to `src/*` (e.g., `import { foo } from '@/commands/list'`) and `bindings` maps to `bindings/` (e.g., `import { parseTask } from 'bindings`)

Applied to files:

  • tdn-cli/.gitignore
  • tdn-cli/package.json
📚 Learning: 2025-12-28T12:22:45.546Z
Learnt from: CR
Repo: dannysmith/taskdn PR: 0
File: tdn-cli/CLAUDE.md:0-0
Timestamp: 2025-12-28T12:22:45.546Z
Learning: Run `bun run fix` before `bun run check` when editing multiple TypeScript or Rust files to avoid formatting errors failing the check

Applied to files:

  • tdn-cli/package.json
📚 Learning: 2025-12-28T12:22:45.546Z
Learnt from: CR
Repo: dannysmith/taskdn PR: 0
File: tdn-cli/CLAUDE.md:0-0
Timestamp: 2025-12-28T12:22:45.546Z
Learning: Regenerate TypeScript bindings by running `bun run build` after modifying any `#[napi]` function signatures, structs, or enums in Rust code

Applied to files:

  • tdn-cli/package.json
📚 Learning: 2025-12-28T12:22:45.546Z
Learnt from: CR
Repo: dannysmith/taskdn PR: 0
File: tdn-cli/CLAUDE.md:0-0
Timestamp: 2025-12-28T12:22:45.546Z
Learning: Use `bun` (not pnpm/npm) for all JavaScript/TypeScript operations

Applied to files:

  • tdn-cli/package.json
📚 Learning: 2025-12-28T12:22:10.779Z
Learnt from: CR
Repo: dannysmith/taskdn PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-28T12:22:10.779Z
Learning: Use `bun` instead of `pnpm` or `npm` for JavaScript/TypeScript projects

Applied to files:

  • tdn-cli/package.json
📚 Learning: 2025-12-28T12:22:45.546Z
Learnt from: CR
Repo: dannysmith/taskdn PR: 0
File: tdn-cli/CLAUDE.md:0-0
Timestamp: 2025-12-28T12:22:45.546Z
Learning: Applies to tdn-cli/crates/**/*.rs : Rust code must use the `#[napi]` macro to expose functions that will be auto-generated as TypeScript bindings

Applied to files:

  • tdn-cli/package.json
📚 Learning: 2025-12-28T12:22:37.372Z
Learnt from: CR
Repo: dannysmith/taskdn PR: 0
File: archived-projects/taskdn-ts/CLAUDE.md:0-0
Timestamp: 2025-12-28T12:22:37.372Z
Learning: Use bun as the package manager and task runner for this project, not npm or pnpm

Applied to files:

  • tdn-cli/package.json
📚 Learning: 2025-12-28T12:22:26.336Z
Learnt from: CR
Repo: dannysmith/taskdn PR: 0
File: archived-projects/taskdn-rust/CLAUDE.md:0-0
Timestamp: 2025-12-28T12:22:26.336Z
Learning: Applies to archived-projects/taskdn-rust/{src/**/*.rs,tests/**/*.rs} : Place unit tests in the same file as code using #[cfg(test)] modules, integration tests in tests/ directory

Applied to files:

  • tdn-cli/package.json
📚 Learning: 2025-12-28T17:09:11.509Z
Learnt from: CR
Repo: dannysmith/taskdn PR: 0
File: tdn-desktop/AGENTS.md:0-0
Timestamp: 2025-12-28T17:09:11.509Z
Learning: Use bun only for package management and running scripts (not npm or pnpm)

Applied to files:

  • tdn-cli/package.json
🪛 actionlint (1.7.9)
.github/workflows/release-cli.yml

109-109: shellcheck reported issue in this script: SC2086:info:1:53: Double quote to prevent globbing and word splitting

(shellcheck)

.github/workflows/ci-cli.yml

91-91: shellcheck reported issue in this script: SC1001:info:1:2: This \d will be a regular 'd' in this context

(shellcheck)


91-91: shellcheck reported issue in this script: SC1012:warning:1:7: \t is just literal 't' here. For tab, use "$(printf '\t')" instead

(shellcheck)

🔇 Additional comments (8)
tdn-cli/.gitignore (1)

9-9: LGTM!

The new .bun-build pattern correctly excludes Bun's build artifacts, aligning with the release build scripts introduced in this PR.

tdn-cli/scripts/install.sh (2)

1-2: LGTM!

Good use of strict mode with set -euo pipefail for robust error handling.


111-118: LGTM!

Extraction and installation logic is straightforward with proper directory creation and executable permissions.

tdn-cli/package.json (1)

25-28: LGTM!

The new build and release scripts are well-structured. Using --sourcemap for release builds aids in debugging production issues while keeping the binary minified.

tdn-cli/scripts/build-release.sh (1)

21-25: LGTM!

The build sequence correctly ensures NAPI bindings are built before compiling the standalone binary.

.github/workflows/ci-cli.yml (2)

89-91: Static analysis false positive - Windows path syntax is correct.

The shellcheck warnings (SC1001/SC1012) about \d and \t are false positives. This step runs on Windows where backslashes are valid path separators. The syntax .\dist\tdn-... is correct for PowerShell/cmd.


49-66: LGTM!

The build matrix covers all major platforms with appropriate runners. Using macos-13 for x64 builds (Intel) and macos-latest for arm64 (Apple Silicon) is the correct approach.

.github/workflows/release-cli.yml (1)

120-127: Verify the Homebrew tap token secret exists.

The workflow references secrets.HOMEBREW_TAP_TOKEN which must be configured in the repository settings. Ensure this secret is created with appropriate permissions for the target repository.

  - Split upload artifact step for Unix/Windows file types
  - Fix working directory for release job steps
  - Improve checksum verification to not falsely report success
  - Quote variable expansion in version extraction
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (3)
.github/workflows/release-cli.yml (2)

77-95: LGTM! Conditional uploads resolve past artifact issue.

The conditional upload steps correctly separate Unix (tar.gz) and Windows (zip) artifacts, ensuring each target only uploads the archive type it produces. This addresses the previous review concern about mixed file patterns.


110-116: LGTM! Working directory overrides fix path issues.

Using working-directory: . correctly overrides the default tdn-cli working directory, ensuring paths are resolved from the repository root. This addresses previous review concerns.

tdn-cli/scripts/install.sh (1)

89-110: LGTM! Checksum verification is no longer misleading.

The improved logic correctly handles the case when no hash tool is available:

  • Warning is printed (line 101)
  • "Checksum verified" message only appears when verification actually succeeds (line 105)

This addresses the concern from previous review about silent bypass being misleading.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between aa32564 and 7fddb64.

⛔ Files ignored due to path filters (1)
  • docs/tasks-todo/task-x-cli-distribution-and-releases.md is excluded by !**/*.md
📒 Files selected for processing (3)
  • .github/workflows/ci-cli.yml
  • .github/workflows/release-cli.yml
  • tdn-cli/scripts/install.sh
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: dannysmith/taskdn PR: 0
File: tdn-cli/CLAUDE.md:0-0
Timestamp: 2025-12-28T12:22:45.546Z
Learning: Follow TDD workflow for CLI features: write failing E2E test, add fixture files conforming to S1 spec, review test against specs, get user confirmation, implement until green, refactor, add unit tests (Rust for parsing, TS for formatters/utilities), run checks, update docs
Learnt from: CR
Repo: dannysmith/taskdn PR: 0
File: tdn-cli/CLAUDE.md:0-0
Timestamp: 2025-12-28T12:22:45.546Z
Learning: Applies to tdn-cli/src/**/*.{ts,tsx} : Use path aliases in TypeScript imports: `@/*` maps to `src/*` (e.g., `import { foo } from '@/commands/list'`) and `bindings` maps to `bindings/` (e.g., `import { parseTask } from 'bindings`)
📚 Learning: 2025-12-28T12:22:45.546Z
Learnt from: CR
Repo: dannysmith/taskdn PR: 0
File: tdn-cli/CLAUDE.md:0-0
Timestamp: 2025-12-28T12:22:45.546Z
Learning: Follow TDD workflow for CLI features: write failing E2E test, add fixture files conforming to S1 spec, review test against specs, get user confirmation, implement until green, refactor, add unit tests (Rust for parsing, TS for formatters/utilities), run checks, update docs

Applied to files:

  • .github/workflows/ci-cli.yml
🪛 actionlint (1.7.9)
.github/workflows/release-cli.yml

120-120: shellcheck reported issue in this script: SC2086:info:1:57: Double quote to prevent globbing and word splitting

(shellcheck)

.github/workflows/ci-cli.yml

91-91: shellcheck reported issue in this script: SC1001:info:1:2: This \d will be a regular 'd' in this context

(shellcheck)


91-91: shellcheck reported issue in this script: SC1012:warning:1:7: \t is just literal 't' here. For tab, use "$(printf '\t')" instead

(shellcheck)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
  • GitHub Check: Build Binary - windows-x64
  • GitHub Check: Test - windows-latest
  • GitHub Check: Lint & Typecheck
  • GitHub Check: Test - ubuntu-latest
  • GitHub Check: Test - macos-latest
🔇 Additional comments (13)
.github/workflows/ci-cli.yml (5)

1-21: LGTM! Well-configured CI trigger and environment.

The workflow appropriately scopes triggers to tdn-cli/** paths and sets a working directory default, which is ideal for a monorepo structure.


23-48: LGTM! Comprehensive testing strategy.

Testing across Ubuntu, macOS, and Windows ensures the CLI works reliably on all major platforms. Building NAPI bindings before tests is the correct order of operations.


85-91: Shellcheck warnings are false positives.

The static analysis warnings on line 91 (SC1001, SC1012) are false positives. The Windows path syntax .\dist\ is correct for PowerShell, which is the default shell for Windows runners in GitHub Actions.


93-98: LGTM! Artifact upload path is correct.

The artifact path tdn-cli/dist/tdn-${{ matrix.target }}* is correctly specified relative to the repository root, since with: parameters in actions don't inherit the workflow's working-directory.


49-66: The ubuntu-24.04-arm runner is a valid, officially available GitHub-hosted runner for arm64 architecture on Ubuntu 24.04, announced by GitHub on June 24, 2024. The workflow configuration is correct and requires no changes.

.github/workflows/release-cli.yml (2)

1-36: LGTM! Proper release workflow configuration.

The tag trigger pattern tdn-cli-v* correctly scopes to CLI releases, and contents: write permission is necessary for creating GitHub releases.


55-66: LGTM! Platform-appropriate archive creation.

The conditional use of sha256sum (Linux) vs shasum (macOS) correctly handles platform differences in Unix systems.

tdn-cli/scripts/install.sh (6)

1-34: LGTM! Excellent error handling and user experience setup.

The use of set -euo pipefail ensures strict error handling, and the terminal detection for colors provides a good user experience both in interactive and non-interactive contexts.


35-56: LGTM! Robust platform detection.

The platform and architecture detection handles common variations (e.g., x86_64 vs amd64) and provides clear error messages for unsupported platforms.


58-71: LGTM! Version resolution handles monorepo releases correctly.

Filtering for tdn-cli-v prefixed tags ensures the script selects the correct release in a monorepo context where multiple packages may have releases.


73-87: LGTM! Proper temporary file handling.

The use of trap 'rm -rf "$TMPDIR"' EXIT ensures cleanup happens even if the script fails, preventing leftover temporary files.


112-127: LGTM! Installation includes verification.

Running the installed binary with --version provides immediate feedback that the installation succeeded and the binary is functional.


129-142: LGTM! Helpful PATH guidance.

The PATH check provides clear, actionable instructions for users to add the installation directory to their shell profile, improving the installation experience.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
.github/workflows/ci-cli.yml (2)

23-33: Consider caching dependencies for faster CI runs.

The lint job setup is correct, but adding dependency caching could significantly improve CI performance.

🔎 Suggested caching additions

Add these steps after the tool setup and before bun install:

      - uses: dtolnay/rust-toolchain@stable
+     - uses: Swatinem/rust-cache@v2
+       with:
+         workspaces: tdn-cli
+     - uses: actions/cache@v4
+       with:
+         path: ~/.bun/install/cache
+         key: ${{ runner.os }}-bun-${{ hashFiles('tdn-cli/bun.lockb') }}
      - run: bun install

85-92: Consider more comprehensive binary smoke tests.

The --version check verifies the binary starts, but consider adding a simple functionality test to catch runtime issues earlier.

🔎 Example additional smoke test

Add after the version test steps:

      - name: Smoke test binary (Unix)
        if: matrix.target != 'windows-x64'
        run: |
          ./dist/tdn-${{ matrix.target }} --help
          # Add a simple command test if applicable
          # ./dist/tdn-${{ matrix.target }} <some-safe-command>

      - name: Smoke test binary (Windows)
        if: matrix.target == 'windows-x64'
        run: |
          .\dist\tdn-${{ matrix.target }}.exe --help
          # Add a simple command test if applicable
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7fddb64 and 52c5a08.

📒 Files selected for processing (2)
  • .github/workflows/ci-cli.yml
  • .github/workflows/release-cli.yml
✅ Files skipped from review due to trivial changes (1)
  • .github/workflows/release-cli.yml
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: CR
Repo: dannysmith/taskdn PR: 0
File: tdn-cli/CLAUDE.md:0-0
Timestamp: 2025-12-28T12:22:45.546Z
Learning: Follow TDD workflow for CLI features: write failing E2E test, add fixture files conforming to S1 spec, review test against specs, get user confirmation, implement until green, refactor, add unit tests (Rust for parsing, TS for formatters/utilities), run checks, update docs
📚 Learning: 2025-12-28T12:22:45.546Z
Learnt from: CR
Repo: dannysmith/taskdn PR: 0
File: tdn-cli/CLAUDE.md:0-0
Timestamp: 2025-12-28T12:22:45.546Z
Learning: Follow TDD workflow for CLI features: write failing E2E test, add fixture files conforming to S1 spec, review test against specs, get user confirmation, implement until green, refactor, add unit tests (Rust for parsing, TS for formatters/utilities), run checks, update docs

Applied to files:

  • .github/workflows/ci-cli.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: Build Binary - windows-x64
  • GitHub Check: Test - ubuntu-latest
  • GitHub Check: Test - macos-latest
  • GitHub Check: Test - windows-latest
🔇 Additional comments (5)
.github/workflows/ci-cli.yml (5)

1-21: LGTM! Clean workflow configuration.

The trigger paths, default working directory, and environment setup are well-structured for a monorepo CLI project.


34-47: Good cross-platform test coverage.

The test matrix appropriately covers the major platforms. The same caching optimization mentioned for the lint job would benefit this job as well.


71-84: LGTM! Binary build steps properly handle platform differences.

The conditional Unix/Windows build steps correctly handle the .exe extension requirement and output naming conventions.


93-98: LGTM! Artifact upload configuration is correct.

The artifact path correctly references the repository root (not the working directory), and the wildcard pattern properly handles both Unix and Windows binary names. The if-no-files-found: error setting ensures build failures are caught.


49-70: No actionable issues identified. The runner labels macos-15-intel and ubuntu-24.04-arm are both valid and publicly available GitHub-hosted runners. The matrix configuration is correct and will work without workflow failures.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
tdn-cli/crates/core/src/writer.rs (1)

194-214: Consider adding debug logging for Windows sync failures.

While making sync_all non-fatal on Windows is a pragmatic solution to temp directory permission issues, silently ignoring the error at line 203 (let _ = e;) may hide legitimate I/O problems during development or debugging.

🔎 Suggested enhancement

Consider adding debug-level logging when sync fails on Windows:

             #[cfg(target_os = "windows")]
             {
-                // Silently ignore sync errors on Windows
-                let _ = e;
+                // Log but don't fail on Windows sync errors (common in temp dirs)
+                eprintln!("Warning: Failed to sync file (non-fatal on Windows): {}", e);
             }

This would help diagnose unexpected I/O issues while still allowing the operation to proceed on Windows.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0576d22 and 548c31f.

📒 Files selected for processing (8)
  • .github/workflows/ci-cli.yml
  • tdn-cli/crates/core/src/writer.rs
  • tdn-cli/src/config/index.ts
  • tdn-cli/tests/e2e/list.test.ts
  • tdn-cli/tests/e2e/modify.test.ts
  • tdn-cli/tests/helpers/cli.ts
  • tdn-cli/tests/unit/bindings.test.ts
  • tdn-cli/tests/unit/config-security.test.ts
🧰 Additional context used
📓 Path-based instructions (2)
tdn-cli/crates/**/*.rs

📄 CodeRabbit inference engine (tdn-cli/CLAUDE.md)

Rust code must use the #[napi] macro to expose functions that will be auto-generated as TypeScript bindings

Files:

  • tdn-cli/crates/core/src/writer.rs
tdn-cli/src/**/*.{ts,tsx}

📄 CodeRabbit inference engine (tdn-cli/CLAUDE.md)

Use path aliases in TypeScript imports: @/* maps to src/* (e.g., import { foo } from '@/commands/list') and @bindings maps to bindings/ (e.g., import { parseTask } from '@bindings)

Files:

  • tdn-cli/src/config/index.ts
🧠 Learnings (17)
📓 Common learnings
Learnt from: CR
Repo: dannysmith/taskdn PR: 0
File: tdn-cli/CLAUDE.md:0-0
Timestamp: 2025-12-28T12:22:45.546Z
Learning: Follow TDD workflow for CLI features: write failing E2E test, add fixture files conforming to S1 spec, review test against specs, get user confirmation, implement until green, refactor, add unit tests (Rust for parsing, TS for formatters/utilities), run checks, update docs
📚 Learning: 2025-12-28T12:22:45.546Z
Learnt from: CR
Repo: dannysmith/taskdn PR: 0
File: tdn-cli/CLAUDE.md:0-0
Timestamp: 2025-12-28T12:22:45.546Z
Learning: Applies to tdn-cli/src/**/*.{ts,tsx} : Use path aliases in TypeScript imports: `@/*` maps to `src/*` (e.g., `import { foo } from '@/commands/list'`) and `bindings` maps to `bindings/` (e.g., `import { parseTask } from 'bindings`)

Applied to files:

  • tdn-cli/tests/helpers/cli.ts
  • tdn-cli/tests/unit/config-security.test.ts
  • tdn-cli/tests/unit/bindings.test.ts
  • tdn-cli/tests/e2e/modify.test.ts
  • tdn-cli/tests/e2e/list.test.ts
  • tdn-cli/src/config/index.ts
📚 Learning: 2025-12-28T12:22:37.372Z
Learnt from: CR
Repo: dannysmith/taskdn PR: 0
File: archived-projects/taskdn-ts/CLAUDE.md:0-0
Timestamp: 2025-12-28T12:22:37.372Z
Learning: Applies to archived-projects/taskdn-ts/**/index.d.ts : Ensure TypeScript types in index.d.ts exactly match the Taskdn specification and maintain type fidelity with the Rust SDK API

Applied to files:

  • tdn-cli/tests/helpers/cli.ts
  • tdn-cli/tests/unit/config-security.test.ts
  • tdn-cli/tests/unit/bindings.test.ts
  • tdn-cli/tests/e2e/modify.test.ts
  • tdn-cli/tests/e2e/list.test.ts
📚 Learning: 2025-12-28T12:22:37.372Z
Learnt from: CR
Repo: dannysmith/taskdn PR: 0
File: archived-projects/taskdn-ts/CLAUDE.md:0-0
Timestamp: 2025-12-28T12:22:37.372Z
Learning: Applies to archived-projects/taskdn-ts/src/lib.rs : Implement all file operations as synchronous in the TypeScript SDK to match the Rust SDK design

Applied to files:

  • tdn-cli/crates/core/src/writer.rs
📚 Learning: 2025-12-28T12:22:45.546Z
Learnt from: CR
Repo: dannysmith/taskdn PR: 0
File: tdn-cli/CLAUDE.md:0-0
Timestamp: 2025-12-28T12:22:45.546Z
Learning: Applies to tdn-cli/**/*.test.*/**/* : Fixture files for tests must conform to the S1 specification

Applied to files:

  • tdn-cli/tests/unit/config-security.test.ts
  • tdn-cli/tests/unit/bindings.test.ts
  • tdn-cli/tests/e2e/modify.test.ts
  • tdn-cli/tests/e2e/list.test.ts
📚 Learning: 2025-12-28T12:22:45.546Z
Learnt from: CR
Repo: dannysmith/taskdn PR: 0
File: tdn-cli/CLAUDE.md:0-0
Timestamp: 2025-12-28T12:22:45.546Z
Learning: Follow TDD workflow for CLI features: write failing E2E test, add fixture files conforming to S1 spec, review test against specs, get user confirmation, implement until green, refactor, add unit tests (Rust for parsing, TS for formatters/utilities), run checks, update docs

Applied to files:

  • tdn-cli/tests/unit/config-security.test.ts
  • tdn-cli/tests/e2e/list.test.ts
  • .github/workflows/ci-cli.yml
📚 Learning: 2025-12-28T12:22:37.372Z
Learnt from: CR
Repo: dannysmith/taskdn PR: 0
File: archived-projects/taskdn-ts/CLAUDE.md:0-0
Timestamp: 2025-12-28T12:22:37.372Z
Learning: Use snapshot testing via api-snapshot.test.ts to verify that generated TypeScript type definitions (index.d.ts) have not unintentionally changed

Applied to files:

  • tdn-cli/tests/unit/config-security.test.ts
  • tdn-cli/tests/e2e/modify.test.ts
  • tdn-cli/tests/e2e/list.test.ts
📚 Learning: 2025-12-28T12:22:37.372Z
Learnt from: CR
Repo: dannysmith/taskdn PR: 0
File: archived-projects/taskdn-ts/CLAUDE.md:0-0
Timestamp: 2025-12-28T12:22:37.372Z
Learning: Applies to archived-projects/taskdn-ts/**/index.d.ts : Use tagged unions with a 'type' field for file references (possible values: 'wikilink', 'relativePath', 'filename') in the TypeScript API

Applied to files:

  • tdn-cli/tests/unit/bindings.test.ts
  • tdn-cli/tests/e2e/modify.test.ts
  • tdn-cli/tests/e2e/list.test.ts
📚 Learning: 2025-12-28T12:22:37.372Z
Learnt from: CR
Repo: dannysmith/taskdn PR: 0
File: archived-projects/taskdn-ts/CLAUDE.md:0-0
Timestamp: 2025-12-28T12:22:37.372Z
Learning: Applies to archived-projects/taskdn-ts/**/*.{ts,tsx,js} : Use camelCase for JavaScript/TypeScript identifiers (e.g., createdAt) while maintaining snake_case internally in Rust code

Applied to files:

  • tdn-cli/tests/unit/bindings.test.ts
  • tdn-cli/tests/e2e/modify.test.ts
  • tdn-cli/tests/e2e/list.test.ts
📚 Learning: 2025-12-28T12:22:26.336Z
Learnt from: CR
Repo: dannysmith/taskdn PR: 0
File: archived-projects/taskdn-rust/CLAUDE.md:0-0
Timestamp: 2025-12-28T12:22:26.336Z
Learning: Applies to archived-projects/taskdn-rust/{src/**/*.rs,tests/**/*.rs} : Place unit tests in the same file as code using #[cfg(test)] modules, integration tests in tests/ directory

Applied to files:

  • tdn-cli/tests/unit/bindings.test.ts
  • tdn-cli/tests/e2e/list.test.ts
📚 Learning: 2025-12-28T12:22:37.372Z
Learnt from: CR
Repo: dannysmith/taskdn PR: 0
File: archived-projects/taskdn-ts/CLAUDE.md:0-0
Timestamp: 2025-12-28T12:22:37.372Z
Learning: Applies to archived-projects/taskdn-ts/**/index.d.ts : Expose Rust status enums as TypeScript string literal unions (e.g., TaskStatus = 'ready' | 'completed') rather than as objects

Applied to files:

  • tdn-cli/tests/unit/bindings.test.ts
  • tdn-cli/tests/e2e/list.test.ts
📚 Learning: 2025-12-28T12:22:37.372Z
Learnt from: CR
Repo: dannysmith/taskdn PR: 0
File: archived-projects/taskdn-ts/CLAUDE.md:0-0
Timestamp: 2025-12-28T12:22:37.372Z
Learning: Update the api-snapshot.test.ts snapshot file when intentionally changing the NAPI bindings API using 'bun test --update-snapshots'

Applied to files:

  • tdn-cli/tests/unit/bindings.test.ts
  • tdn-cli/tests/e2e/modify.test.ts
📚 Learning: 2025-12-28T12:22:45.546Z
Learnt from: CR
Repo: dannysmith/taskdn PR: 0
File: tdn-cli/CLAUDE.md:0-0
Timestamp: 2025-12-28T12:22:45.546Z
Learning: Use `bun` (not pnpm/npm) for all JavaScript/TypeScript operations

Applied to files:

  • tdn-cli/tests/e2e/modify.test.ts
📚 Learning: 2025-12-28T12:22:37.372Z
Learnt from: CR
Repo: dannysmith/taskdn PR: 0
File: archived-projects/taskdn-ts/CLAUDE.md:0-0
Timestamp: 2025-12-28T12:22:37.372Z
Learning: Use bun as the package manager and task runner for this project, not npm or pnpm

Applied to files:

  • tdn-cli/tests/e2e/modify.test.ts
  • tdn-cli/tests/e2e/list.test.ts
📚 Learning: 2025-12-28T17:09:11.509Z
Learnt from: CR
Repo: dannysmith/taskdn PR: 0
File: tdn-desktop/AGENTS.md:0-0
Timestamp: 2025-12-28T17:09:11.509Z
Learning: Use bun only for package management and running scripts (not npm or pnpm)

Applied to files:

  • tdn-cli/tests/e2e/modify.test.ts
📚 Learning: 2025-12-28T12:22:10.779Z
Learnt from: CR
Repo: dannysmith/taskdn PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-28T12:22:10.779Z
Learning: Use `bun` instead of `pnpm` or `npm` for JavaScript/TypeScript projects

Applied to files:

  • tdn-cli/tests/e2e/modify.test.ts
📚 Learning: 2025-12-28T12:22:10.779Z
Learnt from: CR
Repo: dannysmith/taskdn PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-28T12:22:10.779Z
Learning: Never make changes to files in tdn-specs/ without clearly confirming with the user first, as specification changes affect all implementations

Applied to files:

  • tdn-cli/src/config/index.ts
🧬 Code graph analysis (5)
tdn-cli/crates/core/src/writer.rs (1)
tdn-cli/crates/core/src/error.rs (1)
  • write_error (89-97)
tdn-cli/tests/unit/config-security.test.ts (1)
tdn-cli/src/config/index.ts (1)
  • validateVaultPath (44-83)
tdn-cli/tests/unit/bindings.test.ts (1)
tdn-cli/tests/helpers/cli.ts (1)
  • isArchivePath (101-104)
tdn-cli/tests/e2e/modify.test.ts (1)
tdn-cli/tests/helpers/cli.ts (1)
  • isArchivePath (101-104)
tdn-cli/tests/e2e/list.test.ts (1)
tdn-cli/tests/helpers/cli.ts (2)
  • isArchivePath (101-104)
  • runCli (43-88)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
  • GitHub Check: Test - windows-latest
  • GitHub Check: Build Binary - darwin-x64
  • GitHub Check: Lint & Typecheck
  • GitHub Check: Test - ubuntu-latest
  • GitHub Check: Build Binary - windows-x64
  • GitHub Check: Test - macos-latest
🔇 Additional comments (16)
tdn-cli/tests/helpers/cli.ts (1)

97-104: LGTM! Clean cross-platform archive path detection.

The implementation correctly handles both Windows and Unix path separators using a simple regex pattern. The function is well-documented and serves as a centralized utility for archive path detection across the test suite.

tdn-cli/src/config/index.ts (1)

71-74: LGTM! Improved cross-platform temp directory handling.

The addition of Linux /tmp/ support alongside the existing macOS /var/folders/ exception makes the validation logic more robust across platforms. The introduction of the isTempDir variable improves code clarity.

.github/workflows/ci-cli.yml (2)

86-111: LGTM! Well-structured multi-platform build job.

The build job properly:

  • Separates Unix and Windows binary compilation steps
  • Includes smoke tests for each platform to validate basic functionality
  • Uploads artifacts with appropriate naming

The conditional execution based on target platform is handled correctly.


55-72: No action required. Both runner labels are valid and available in GitHub Actions as of December 2025:

  • macos-15-intel is the correct label for Intel-based macOS 15 runners
  • ubuntu-24.04-arm is the correct label for Ubuntu 24.04 ARM runners

The build matrix configuration is correct.

tdn-cli/tests/unit/bindings.test.ts (2)

10-10: LGTM! Importing centralized archive path helper.

Good refactor to use the shared isArchivePath utility from test helpers.


138-143: LGTM! Improved archive path detection.

Using isArchivePath(t.path) instead of a string match for 'archive/' makes the test more robust and cross-platform compatible (handles both forward slashes and backslashes).

tdn-cli/tests/e2e/modify.test.ts (2)

2-2: LGTM! Importing cross-platform path helper.

Good addition of the isArchivePath utility for consistent archive path detection.


385-387: LGTM! Improved cross-platform archive path validation.

The test now properly:

  1. Uses isArchivePath(output.to) to detect archive directories across platforms
  2. Validates the filename suffix separately with a regex

This separation of concerns makes the test more maintainable and cross-platform compatible.

tdn-cli/tests/e2e/list.test.ts (4)

2-2: LGTM! Importing cross-platform path helper.

Good addition of the isArchivePath utility for consistent archive path detection across the test suite.


904-904: LGTM! Cross-platform archive path detection.

Using isArchivePath(t.path) instead of a string substring check properly handles both Unix and Windows path separators.


910-910: LGTM! Consistent use of archive path helper.

The negated form !isArchivePath(t.path) correctly validates that archived tasks are excluded by default.


920-920: LGTM! Proper validation for --only-archived flag.

Correctly uses isArchivePath(t.path) to ensure all returned tasks are from archive directories.

tdn-cli/tests/unit/config-security.test.ts (4)

3-4: LGTM! Improved imports with static named imports.

Using explicit named imports from os and path modules is better than dynamic requires and provides better type safety.


36-42: LGTM! Cross-platform path construction.

Using path.join() for path construction is more robust and cross-platform compatible than string concatenation.


133-146: LGTM! Improved test coverage for temp directory handling.

The new tests properly validate:

  1. /tmp paths are accepted without warnings (Linux temp directory)
  2. Non-temp, outside-home paths still trigger warnings (e.g., /opt/tasks)

This aligns with the expanded temp directory logic in tdn-cli/src/config/index.ts that now accepts both /var/folders/ (macOS) and /tmp/ (Linux).


149-178: LGTM! Platform-aware path resolution tests.

The updated tests properly handle platform differences:

  • Windows: expects drive letters (e.g., C:\)
  • Unix: expects leading slash
  • Uses path.join() for cross-platform path construction

The test at lines 171-178 correctly validates that paths with .. are properly resolved to their canonical form.

Rust tests were using format!("{}/file.md", dir) which creates mixed path
separators on Windows (e.g., C:\Users\...\tasks/task1.md). Changed to use
Path::new().join() which uses the correct separator for each platform.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
@dannysmith dannysmith merged commit 27e5209 into main Dec 31, 2025
13 checks passed
@dannysmith dannysmith deleted the cli-deployment-setup branch December 31, 2025 00:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants