[CU-86ewpu7j2] Create sample Bun Library template#3
Open
adelphi-liong wants to merge 18 commits intomainfrom
Open
[CU-86ewpu7j2] Create sample Bun Library template#3adelphi-liong wants to merge 18 commits intomainfrom
adelphi-liong wants to merge 18 commits intomainfrom
Conversation
Create sample Bun Library template with: - Testing patterns from imine/boron - Release patterns from sulfone/helium (adapted for Bun) - Skill validation for all 15 skills - ESM + CJS module output
Implement a production-ready Bun (TypeScript) library template:
- Add src/lib/ with structures.ts, interfaces.ts, services.ts
- Add test/unit/ with 104 tests, 100% line coverage
- Add test/int/ with 12 tests, 86.3% line coverage
- Add test/fixtures/ with mock implementations
- Add scripts/test.sh, scripts/ci/{test,build,publish}.sh
- Add bunfig.unit.toml, bunfig.int.toml for coverage config
- Add tasks/test.tasks.yaml with test tasks
- Update package.json with proper exports (ESM + CJS)
- Update tsconfig.json for declaration generation
- Add GitHub workflows (ci, reusable-test, reusable-build)
- Add README.md and LICENSE (MIT)
- Update atomi_release.yaml with @semantic-release/npm
Push Cycle 3 fixes:
- Add dist/ and coverage/ to .gitignore (CI-generated artifacts)
- Fix update_version.sh $2 unbound variable with ${2:-./package.json}
- Ensure release.sh has || true for semantic-release
- Fix truncate() edge case when suffix.length >= maxLength
- Fix reverse() for Unicode surrogate pairs using spread operator
- Update JSDoc comments for clarity
- Add missing test cases for edge conditions
Tests: 118/118 passing
Build: succeeds
Push Cycle 4-5 fixes: - Fix ESM version resolution with fallback path for bundled code - Add mktemp for safe temp file handling in update_version.sh - Fix shift without args in test.sh - Document cache parameter as reserved for future use - Fix grammar in CommitConventions.md - Update test.tasks.yaml description for accuracy - Update all tests for dynamic version reading Tests: 106/118 passing (unit), 12 (int) Build: succeeds with correct version in both ESM and CJS
The correct order is `set -euo pipefail`, not `set -eou pipefail`. Bash parses `-eou` as `-e -o u` (sets nounset only). It does NOT set pipefail because that becomes a separate argument. This fix ensures all scripts follow the convention in docs/developer/standard/shell-scripts.md. Scripts updated: - scripts/ci/build.sh, publish.sh, release.sh, test.sh - scripts/ci/update_version.sh - scripts/local/secrets.sh - scripts/test.sh
- Fix documentation formatting and content issues - Update skill documentation - Sync cyan state and release config
Code changes: - Add JSDoc for StringProcessOutput.length property - Handle negative maxLength in truncate() method - Document TTL=0 and stale entry behavior in MockCacheAdapter - Fix cache test to verify actual cache behavior - Update Taskfile test description Configuration: - Order package.json exports correctly (types first) - Add if-no-files-found config for artifact upload - Add explicit access: public for semantic-release/npm Documentation: - Add language specifiers to fenced code blocks - Fix style section description in CommitConventions - Clarify lodash tree-shaking and testing guidance - Update utilities skill with .NET version info By Claude Code Kagent Autopilot 🤖
- Add checkout step to reusable pre-commit workflow - Remove || true from release script to propagate failures - Fix shell conventions in publish script (strict mode ordering) - Add API documentation link in README - Remove unnecessary knip configuration entries - Update services.ts header to document I/O usage
Contributor
Author
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a production-ready Bun TypeScript library template: public API and immutable types, DI services, fixtures and extensive tests, CI/CD reusable workflows and publish pipeline, build/publish scripts for ESM+CJS, docs, license, and developer tooling/configuration. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer (PR)
participant GH as GitHub Actions
participant Runner as Self-hosted Runner / Nix
participant Cache as Bun/Nix Cache
participant Artifact as Actions Artifact Store
participant Registry as npm Registry
Dev->>GH: push PR (precommit)
GH->>Runner: run precommit job (checkout, pre-commit)
GH->>Runner: run reusable-test (test-unit)
Runner->>Cache: restore bun cache
Runner->>Runner: run unit tests + coverage
Runner->>Artifact: upload coverage and artifacts
GH->>Runner: run reusable-test (test-int)
Runner->>Runner: run integration tests
Runner->>Artifact: upload integration artifacts
GH->>Runner: run reusable-build
Runner->>Runner: build ESM+CJS -> dist/
Runner->>Artifact: upload dist/
GH->>GH: CD workflow polls CI success
GH->>Artifact: download dist artifacts
GH->>Runner: run publish script (uses NPM_API_KEY)
Runner->>Registry: publish package
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (10)
.claude/skills/datetime/SKILL.md (1)
59-59: Tighten thetime.Sleepguidance to avoid a misleading recommendation.Line 59 says to “Use
time.Durationcalculations,” buttime.Sleepalready usestime.Duration. Consider recommending timers/context cancellation for precision/control instead.Proposed doc fix
-| `time.Sleep` for durations | Not precise, blocks thread | Use `time.Duration` calculations | +| `time.Sleep` for scheduling | Hard to cancel/control drift | Use `time.Timer`/`time.Ticker` + context cancellation |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/datetime/SKILL.md at line 59, Update the guidance for the "time.Sleep" row: replace the vague "Use `time.Duration` calculations" with a recommendation to prefer timers and context cancellation for precision and control (e.g., `time.NewTimer`/`time.After`, `time.Ticker`, and `context.WithTimeout`/`WithDeadline`) and note that `time.Sleep` blocks the goroutine and is less suitable for cancellable/precise scheduling; reference `time.Sleep` and `time.Duration` in the wording so readers can find the related API.scripts/ci/update_version.sh (1)
7-9: Consider adding cleanup trap for the temp file.If
jqfails, the temp file created bymktempwill remain on disk. Adding a trap ensures cleanup on error.🧹 Proposed fix to add cleanup trap
+cleanup() { + rm -f "$tmpfile" +} + tmpfile=$(mktemp) +trap cleanup EXIT jq --arg new_version "${version}" '.version = $new_version' "${package_path}" >"$tmpfile" mv "$tmpfile" "${package_path}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/ci/update_version.sh` around lines 7 - 9, The tmpfile created by mktemp (variable tmpfile used in the update_version.sh script) is not cleaned up if jq fails; add a trap that removes "$tmpfile" on EXIT or ERR and clear the trap after a successful mv to ensure the temporary file is always deleted on error but not removed after success; locate the mktemp assignment and the jq/mv sequence and wrap them with a trap cleanup and trap - to unset it after moving the temp file (use trap 'rm -f "$tmpfile"' EXIT or trap 'rm -f "$tmpfile"' ERR/EXIT as appropriate)..cyan_state.yaml (1)
45-88: Duplicate version entries detected.There are two entries with
version: 27(lines 45 and 67) with different timestamps and values (notablyenable-secretchanges fromtruetofalse). If this is an auto-generated state file from the Cyan templating tool, this may be expected behavior. Otherwise, consider deduplicating to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.cyan_state.yaml around lines 45 - 88, The .cyan_state.yaml contains duplicate state entries with the same "version: 27" but different timestamps and answers (see the two version: 27 blocks and differing keys such as atomi/workspace/enable-secret); remove or consolidate the duplicate entry so each version key is unique or merge the differing answers into the intended canonical state (e.g., keep the most recent timestamped block or merge values for atomi/workspace/enable-secret, atomi/workspace/enable-docker, atomi/workspace/runtime, atomi/workspace/service, atomi/workspace/platform, and atomi/workspace/enable-helm) to avoid ambiguity in state processing.scripts/test.sh (1)
20-35: Remove unnecessaryshiftstatements in the for loop.The
shiftcalls inside theforloop (lines 24, 27) have no effect on the loop iteration sincefor flag in "$@"captures a snapshot of the arguments at loop start. These shifts are dead code and should be removed for clarity.♻️ Proposed fix
for flag in "$@"; do case $flag in --cover) COVER=true - shift ;; --watch) WATCH=true - shift ;; *) echo "❌ Unknown flag: $flag" exit 1 ;; esac done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/test.sh` around lines 20 - 35, The for-loop that iterates arguments using for flag in "$@" contains dead shift statements; remove the two shift lines inside the case block so the loop relies solely on the for iteration. Specifically, edit the loop that declares for flag in "$@" and the case branches setting COVER=true and WATCH=true to delete the shift statements associated with those branches, leaving the echo/exit fallback unchanged.README.md (2)
142-142: Consider updating the reference to TypeScript declarations.The link to
./dist/index.d.tsreferences a build artifact that won't exist until the project is built. For repository visitors, this link will be broken. Consider either:
- Pointing to the source types in
src/lib/instead, or- Noting that users should run the build first
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 142, Update the README reference that links to ./dist/index.d.ts (a build artifact) so it doesn't point to a missing file for repo visitors: either change the link to point to the source TypeScript declarations under src/lib/ (replace ./dist/index.d.ts with the appropriate src/lib path) or keep the ./dist link but add a short note instructing users to run the build first (e.g., "Run npm build to generate ./dist/index.d.ts"). Ensure the updated sentence remains in the same spot and clearly directs readers to the source types or the build step.
3-4: Consider using a dynamic coverage badge.The coverage badge is hardcoded to 100%. If coverage changes, this badge will become stale. Consider integrating with a coverage service (e.g., Codecov, Coveralls) for automatic badge updates, or document that this badge should be manually updated after coverage changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 3 - 4, The README currently contains a hardcoded coverage badge showing "100%" which will become stale; replace the static badge line with a dynamic badge from a coverage service (e.g., Codecov or Coveralls) by adding the service's markdown badge URL and linking to the repo's coverage report, or alternatively add a short note near the badge explaining it is manually maintained; update the badge markdown in README.md where the coverage badge string appears so it references the chosen service's project token/slug.src/lib/index.ts (1)
5-7: Consider explicit exports for API stability.Wildcard re-exports make it easy to leak new internals as public API. For a publishable library template, explicit exports are usually safer for semver control.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/index.ts` around lines 5 - 7, Replace the wildcard re-exports in src/lib/index.ts with explicit named exports to avoid unintentionally exposing internals; open structures.ts, interfaces.ts and services.ts, identify the intended public symbols (types, classes, functions, constants) and change the three lines to export only those named identifiers (e.g., export { Foo, Bar } from './structures.ts'), updating any downstream imports/tests as needed so only the chosen API surface is exported from index.ts.package.json (1)
62-62: Consider pinning@types/bunversion for CI stability.Using
"latest"can cause unexpected CI failures when type definitions change. While this is common practice for Bun projects (types are tightly coupled to runtime), pinning to a specific version provides more reproducible builds.♻️ Suggested change
- "@types/bun": "latest", + "@types/bun": "^1.2.0",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 62, Replace the floating "latest" version for the `@types/bun` dependency with a pinned semver to stabilize CI: locate the package.json dependency entry for "@types/bun" and change its version from "latest" to a specific version string (e.g., "0.1.23" or the current tested release) so installs are reproducible across CI runs.src/lib/services.ts (2)
37-38: Consider adding a defensive check for undefined version.The type assertion
as stringassumesversionexists. Ifpackage.jsonis malformed (missing version field), this will returnundefinedtyped asstring. While the try-catch provides a fallback for parsing errors, a missing field won't throw.🛡️ Suggested defensive check
const packageJson = JSON.parse(readFileSync(packageJsonPath, 'utf-8')); - return packageJson.version as string; + return typeof packageJson.version === 'string' ? packageJson.version : '0.0.0-development';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/services.ts` around lines 37 - 38, The code reads packageJson and asserts packageJson.version as string which can mask a missing version field; update the logic around the JSON parsing (the block that reads packageJsonPath and returns packageJson.version) to defensively check that packageJson.version exists and is a string (e.g., typeof packageJson.version === 'string') and if not either throw a clear error or return a safe default/fallback version so the function never returns undefined typed as string; ensure you reference packageJson and packageJsonPath in the updated check and keep the existing try-catch fallback behavior.
195-214: JSDoc claims "grapheme clusters" but implementation uses code point splitting.The
reverse()method uses spread syntax[...text]which splits by Unicode code points, not grapheme clusters. For complex sequences like emoji with ZWJ (e.g.,👨👩👧👦), this will produce incorrect results. The JSDoc onisPalindromementions "grapheme clusters" but the underlyingreverse()doesn't handle them.If grapheme cluster support is needed, consider using
Intl.Segmenter. Otherwise, update the JSDoc to clarify the limitation.📝 Option 1: Update JSDoc to clarify limitation
/** * Check if a string is a palindrome - * Handles Unicode characters by normalizing and comparing grapheme clusters + * Handles Unicode characters by normalizing and comparing code points. + * Note: Complex grapheme clusters (e.g., ZWJ emoji sequences) may not be handled correctly. */♻️ Option 2: Use Intl.Segmenter for true grapheme support
public reverse(text: string): string { const segmenter = new Intl.Segmenter(undefined, { granularity: 'grapheme' }); return [...segmenter.segment(text)].map(s => s.segment).reverse().join(''); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/services.ts` around lines 195 - 214, The JSDoc on isPalindrome falsely claims grapheme-cluster handling while reverse(text: string) uses [...text] (codepoint splitting); either update the JSDoc to state it only handles code points/Unicode scalar values, or change reverse to use Intl.Segmenter with granularity 'grapheme' (and add a clear fallback for environments without Intl.Segmenter) so reverse and isPalindrome actually compare grapheme clusters; update comments on both reverse and isPalindrome to reflect the chosen approach.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/skills/datetime/SKILL.md:
- Line 31: Update the Go timezone entry in the reference table: replace the
incorrect `carbon.Carbon` with Go's timezone type `*time.Location`, and mention
canonical ways to obtain it (e.g. `time.LoadLocation()`, `time.UTC`,
`time.Local`); modify the row containing "Timezone info" / “Temporal.TimeZone”
so the Go column reads `*time.Location` and optionally add the functions
`time.LoadLocation`, `time.UTC`, `time.Local` as examples to guide proper API
usage.
In @.claude/skills/utilities/SKILL.md:
- Line 23: The docs mention Lodash and "tree-shakeable imports" but don't show
the correct import pattern; update the TypeScript/Bun Lodash recommendation (the
table row "TypeScript/Bun | Lodash" and the paragraph that mentions
"tree-shakeable imports") to include concrete examples: prefer ESM named imports
like `import { debounce } from "lodash-es"` and as a fallback show subpath
imports like `import debounce from "lodash/debounce"`, explicitly warn against
`import _ from "lodash"` or `import * as _` and against using per-method
packages (e.g., `lodash.debounce`) which are deprecated; keep the wording short
and add both example import lines so readers can copy-paste the correct
tree-shakeable patterns.
In `@scripts/ci/build.sh`:
- Line 23: The CI script invokes the TypeScript compiler directly using the
command "tsc --emitDeclarationOnly --declaration --outDir ./dist" which fails in
the Nix CI environment; update the command in scripts/ci/build.sh to run the
same tsc invocation through bun (use "bunx") so it becomes "bunx tsc
--emitDeclarationOnly --declaration --outDir ./dist", preserving all flags and
behavior.
In `@scripts/ci/publish.sh`:
- Around line 17-24: The script currently unconditionally removes any existing
.npmrc (rm .npmrc) and never restores it; change publish.sh to detect an
existing .npmrc before overwriting, move it to a temporary backup (e.g.,
.npmrc.backup or mktemp) instead of rm, write the new auth .npmrc, and update
the trap handler so EXIT will remove only the generated .npmrc and restore the
backup if one existed (and clean up the backup). Keep the chmod 600 for the
generated file and ensure the trap handles both restore and cleanup paths.
In `@scripts/ci/test.sh`:
- Around line 15-18: The script only checks that the COVERAGE_TARGET environment
variable is set but not that it is numeric, causing later failures; update the
validation in scripts/ci/test.sh to first ensure COVERAGE_TARGET is present and
then validate its format (e.g., integer or numeric percentage) using a regex
check such as [[ ${COVERAGE_TARGET} =~ ^[0-9]+$ ]] (or your desired numeric
pattern), and if the check fails emit a clear error like "❌ COVERAGE_TARGET must
be a numeric value" and exit 1 so downstream coverage checks get a clear, early
failure.
In `@test/fixtures/index.ts`:
- Around line 17-31: In getPackageVersion(), after parsing packageJson (from
packageJsonPath) add a defensive check that packageJson.version is a non-empty
string and if not return the fallback '0.0.0-development'; specifically validate
the version value returned from JSON.parse (variable packageJson) before casting
and returning it, so missing or undefined version fields won’t be returned as an
asserted string.
In `@test/int/library.integration.test.ts`:
- Around line 7-15: Tests are importing internal module src/lib/services.ts
directly; update the test to import the public API export that re-exports
createLibraryService, createDefaultConfigProvider, CalculatorService,
StringService, ConfigService, and LibraryService from the package entrypoint (so
the test exercises the package surface and catches export wiring regressions).
Replace the import source for those named symbols with the library's public
entry (the module that consumers import) and keep the same named imports
(createLibraryService, createDefaultConfigProvider, CalculatorService,
StringService, ConfigService, LibraryService) so tests validate the exported
API.
---
Nitpick comments:
In @.claude/skills/datetime/SKILL.md:
- Line 59: Update the guidance for the "time.Sleep" row: replace the vague "Use
`time.Duration` calculations" with a recommendation to prefer timers and context
cancellation for precision and control (e.g., `time.NewTimer`/`time.After`,
`time.Ticker`, and `context.WithTimeout`/`WithDeadline`) and note that
`time.Sleep` blocks the goroutine and is less suitable for cancellable/precise
scheduling; reference `time.Sleep` and `time.Duration` in the wording so readers
can find the related API.
In @.cyan_state.yaml:
- Around line 45-88: The .cyan_state.yaml contains duplicate state entries with
the same "version: 27" but different timestamps and answers (see the two
version: 27 blocks and differing keys such as atomi/workspace/enable-secret);
remove or consolidate the duplicate entry so each version key is unique or merge
the differing answers into the intended canonical state (e.g., keep the most
recent timestamped block or merge values for atomi/workspace/enable-secret,
atomi/workspace/enable-docker, atomi/workspace/runtime, atomi/workspace/service,
atomi/workspace/platform, and atomi/workspace/enable-helm) to avoid ambiguity in
state processing.
In `@package.json`:
- Line 62: Replace the floating "latest" version for the `@types/bun` dependency
with a pinned semver to stabilize CI: locate the package.json dependency entry
for "@types/bun" and change its version from "latest" to a specific version
string (e.g., "0.1.23" or the current tested release) so installs are
reproducible across CI runs.
In `@README.md`:
- Line 142: Update the README reference that links to ./dist/index.d.ts (a build
artifact) so it doesn't point to a missing file for repo visitors: either change
the link to point to the source TypeScript declarations under src/lib/ (replace
./dist/index.d.ts with the appropriate src/lib path) or keep the ./dist link but
add a short note instructing users to run the build first (e.g., "Run npm build
to generate ./dist/index.d.ts"). Ensure the updated sentence remains in the same
spot and clearly directs readers to the source types or the build step.
- Around line 3-4: The README currently contains a hardcoded coverage badge
showing "100%" which will become stale; replace the static badge line with a
dynamic badge from a coverage service (e.g., Codecov or Coveralls) by adding the
service's markdown badge URL and linking to the repo's coverage report, or
alternatively add a short note near the badge explaining it is manually
maintained; update the badge markdown in README.md where the coverage badge
string appears so it references the chosen service's project token/slug.
In `@scripts/ci/update_version.sh`:
- Around line 7-9: The tmpfile created by mktemp (variable tmpfile used in the
update_version.sh script) is not cleaned up if jq fails; add a trap that removes
"$tmpfile" on EXIT or ERR and clear the trap after a successful mv to ensure the
temporary file is always deleted on error but not removed after success; locate
the mktemp assignment and the jq/mv sequence and wrap them with a trap cleanup
and trap - to unset it after moving the temp file (use trap 'rm -f "$tmpfile"'
EXIT or trap 'rm -f "$tmpfile"' ERR/EXIT as appropriate).
In `@scripts/test.sh`:
- Around line 20-35: The for-loop that iterates arguments using for flag in "$@"
contains dead shift statements; remove the two shift lines inside the case block
so the loop relies solely on the for iteration. Specifically, edit the loop that
declares for flag in "$@" and the case branches setting COVER=true and
WATCH=true to delete the shift statements associated with those branches,
leaving the echo/exit fallback unchanged.
In `@src/lib/index.ts`:
- Around line 5-7: Replace the wildcard re-exports in src/lib/index.ts with
explicit named exports to avoid unintentionally exposing internals; open
structures.ts, interfaces.ts and services.ts, identify the intended public
symbols (types, classes, functions, constants) and change the three lines to
export only those named identifiers (e.g., export { Foo, Bar } from
'./structures.ts'), updating any downstream imports/tests as needed so only the
chosen API surface is exported from index.ts.
In `@src/lib/services.ts`:
- Around line 37-38: The code reads packageJson and asserts packageJson.version
as string which can mask a missing version field; update the logic around the
JSON parsing (the block that reads packageJsonPath and returns
packageJson.version) to defensively check that packageJson.version exists and is
a string (e.g., typeof packageJson.version === 'string') and if not either throw
a clear error or return a safe default/fallback version so the function never
returns undefined typed as string; ensure you reference packageJson and
packageJsonPath in the updated check and keep the existing try-catch fallback
behavior.
- Around line 195-214: The JSDoc on isPalindrome falsely claims grapheme-cluster
handling while reverse(text: string) uses [...text] (codepoint splitting);
either update the JSDoc to state it only handles code points/Unicode scalar
values, or change reverse to use Intl.Segmenter with granularity 'grapheme' (and
add a clear fallback for environments without Intl.Segmenter) so reverse and
isPalindrome actually compare grapheme clusters; update comments on both reverse
and isPalindrome to reflect the chosen approach.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (15)
bun.lockis excluded by!**/*.lockdocs/developer/standard/datetime/index.mdis excluded by!docs/developer/standard/**/*.mddocs/developer/standard/datetime/languages/csharp.mdis excluded by!docs/developer/standard/**/*.mddocs/developer/standard/datetime/languages/go.mdis excluded by!docs/developer/standard/**/*.mddocs/developer/standard/datetime/languages/typescript.mdis excluded by!docs/developer/standard/**/*.mddocs/developer/standard/shell-scripts.mdis excluded by!docs/developer/standard/**/*.mddocs/developer/standard/utilities/index.mdis excluded by!docs/developer/standard/**/*.mddocs/developer/standard/utilities/languages/csharp.mdis excluded by!docs/developer/standard/**/*.mddocs/developer/standard/utilities/languages/go.mdis excluded by!docs/developer/standard/**/*.mddocs/developer/standard/utilities/languages/typescript.mdis excluded by!docs/developer/standard/**/*.mddocs/developer/standard/validation/index.mdis excluded by!docs/developer/standard/**/*.mddocs/developer/standard/validation/languages/csharp.mdis excluded by!docs/developer/standard/**/*.mddocs/developer/standard/validation/languages/go.mdis excluded by!docs/developer/standard/**/*.mddocs/developer/standard/validation/languages/typescript.mdis excluded by!docs/developer/standard/**/*.mdspec/CU-86ewpu7j2/task-spec.mdis excluded by!spec/**/*.md
📒 Files selected for processing (45)
.claude/skills/datetime/SKILL.md.claude/skills/utilities/SKILL.md.claude/skills/validation/SKILL.md.coderabbit.yaml.cyan_state.yaml.github/workflows/ci.yaml.github/workflows/⚡reusable-build.yaml.github/workflows/⚡reusable-precommit.yaml.github/workflows/⚡reusable-release.yaml.github/workflows/⚡reusable-test.yaml.gitignoreLICENSEREADME.mdTaskfile.yamlatomi_release.yamlbuild.config.tsbunfig.int.tomlbunfig.unit.tomldocs/api.mddocs/developer/CommitConventions.mdnix/env.nixnix/packages.nixpackage.jsonscripts/ci/build.shscripts/ci/pre-commit.shscripts/ci/publish.shscripts/ci/release.shscripts/ci/test.shscripts/ci/update_version.shscripts/local/secrets.shscripts/test.shsrc/index.tssrc/lib/index.tssrc/lib/interfaces.tssrc/lib/services.tssrc/lib/structures.tstasks/test.tasks.yamltest/fixtures/index.tstest/int/library.integration.test.tstest/unit/calculator.service.test.tstest/unit/config.service.test.tstest/unit/fixtures.test.tstest/unit/library.service.test.tstest/unit/string.service.test.tstsconfig.json
CodeRabbit identified that artifact verification would not fail the workflow if artifacts were missing. Updated to use explicit exit codes and error messages for each required artifact file. Co-authored-by: CodeRabbit AI Review <review@coderabbit.ai>
Additional requirements implementation: - Add codecov.yaml with coverage threshold blocking (unit 100%, int 80%) - Add lcov coverage generation and upload to codecov - Remove dependency installation from CI scripts (Nix provides everything) - Add default task to test.tasks.yaml for cleaner interface - Update cd.yaml with CI polling and artifact download By Claude Code Kagent Autopilot 🤖
Contributor
Author
|
@coderabbitai I have attempted to resolve all the issues mentioned, and replied to conversations that need further discussion. Please:
By Claude Code Kagent Autopilot 🤖 |
🧩 Analysis chain🏁 Script executed: Length of output: 67534
Here's a summary of what was addressed:
Your responses showed good judgment—many of the suggestions were false positives given your project's context (CI-first scripts, project conventions, etc.). Now I'll perform a comprehensive re-review to check for any other issues across all the changes. ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
scripts/ci/publish.sh (1)
20-23: Harden.npmrccreation so the token file is restricted from first write.Line 20 writes the auth token before Line 23 applies
chmod 600. Prefer creating the file under restrictive umask immediately.🔐 Suggested hardening
echo "🛠️ Generating .npmrc" rm .npmrc || true -echo "//registry.npmjs.org/:_authToken=${NPM_API_KEY}" >.npmrc -echo "registry=https://registry.npmjs.org/" >>.npmrc -echo "always-auth=true" >>.npmrc -chmod 600 .npmrc +umask 077 +{ + echo "//registry.npmjs.org/:_authToken=${NPM_API_KEY}" + echo "registry=https://registry.npmjs.org/" + echo "always-auth=true" +} > .npmrc +chmod 600 .npmrc trap 'rm -f .npmrc' EXIT echo "✅ .npmrc generated!"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/ci/publish.sh` around lines 20 - 23, The .npmrc is written with sensitive token before permissions are set; modify the script so the file is created under restrictive permissions (e.g., set umask 077 or create the file with mode 600) before writing the token instead of relying on a later chmod; update the block that writes the auth token and registry (the echo statements that write to ".npmrc" and the later "chmod 600 .npmrc") so the file is created securely first and then the token/lines are written to it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/cd.yaml:
- Around line 38-70: The jq expressions used to extract databaseId (for RUN_ID,
RUNNING_ID, FAILED_ID) can return the literal "null" when no runs exist; update
each gh run list --jq argument (the occurrences inside the RUN_ID, RUNNING_ID,
and FAILED_ID commands) to append // empty (e.g. '.[0].databaseId // empty') so
jq emits an empty string instead of "null", ensuring the bash checks ([[ -n
"$RUN_ID" ]], etc.) correctly treat absent runs as empty.
In `@scripts/ci/build.sh`:
- Around line 24-30: The current file-checks (e.g., the commands "test -f
dist/index.js", "test -f dist/index.cjs", etc.) use the "&&" pattern which can
silently allow failures under "set -e"; update each artifact check to explicitly
fail the script when missing by replacing the "test ... && echo ..." pattern
with a check that echoes a clear error and exits non-zero on missing files (for
example use "test -f <file> || { echo 'Missing <file>'; exit 1; }" for each
artifact or aggregate checks and exit with a failure if any are missing) so the
final "✅ All build outputs verified!" is only printed when all files exist.
---
Nitpick comments:
In `@scripts/ci/publish.sh`:
- Around line 20-23: The .npmrc is written with sensitive token before
permissions are set; modify the script so the file is created under restrictive
permissions (e.g., set umask 077 or create the file with mode 600) before
writing the token instead of relying on a later chmod; update the block that
writes the auth token and registry (the echo statements that write to ".npmrc"
and the later "chmod 600 .npmrc") so the file is created securely first and then
the token/lines are written to it.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
spec/CU-86ewpu7j2/additonal.mdis excluded by!spec/**/*.mdspec/CU-86ewpu7j2/task-spec.mdis excluded by!spec/**/*.md
📒 Files selected for processing (10)
.github/workflows/cd.yaml.github/workflows/ci.yaml.github/workflows/⚡reusable-test.yamlTaskfile.yamlcodecov.yamlscripts/ci/build.shscripts/ci/pre-commit.shscripts/ci/publish.shscripts/ci/test.shtasks/test.tasks.yaml
✅ Files skipped from review due to trivial changes (1)
- codecov.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- scripts/ci/pre-commit.sh
- scripts/ci/test.sh
- Taskfile.yaml
- tasks/test.tasks.yaml
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
scripts/ci/build.sh (1)
24-30:⚠️ Potential issue | 🟠 MajorBuild output verification can silently pass when files are missing.
With
set -e, a failedtestin an&&list does not abort the script, so the final "All build outputs verified!" message can print even when required artifacts are missing. This was flagged in a previous review and remains unresolved.🔧 Suggested fix
echo "🔍 Verifying build outputs..." -test -f dist/index.js && echo " ✓ dist/index.js (ESM)" -test -f dist/index.cjs && echo " ✓ dist/index.cjs (CJS)" -test -f dist/index.d.ts && echo " ✓ dist/index.d.ts (Types)" -test -f dist/index.js.map && echo " ✓ dist/index.js.map (Sourcemap)" -test -f dist/index.cjs.map && echo " ✓ dist/index.cjs.map (Sourcemap)" +test -f dist/index.js || { echo " ✗ dist/index.js (ESM) MISSING"; exit 1; } +echo " ✓ dist/index.js (ESM)" +test -f dist/index.cjs || { echo " ✗ dist/index.cjs (CJS) MISSING"; exit 1; } +echo " ✓ dist/index.cjs (CJS)" +test -f dist/index.d.ts || { echo " ✗ dist/index.d.ts (Types) MISSING"; exit 1; } +echo " ✓ dist/index.d.ts (Types)" +test -f dist/index.js.map || { echo " ✗ dist/index.js.map (Sourcemap) MISSING"; exit 1; } +echo " ✓ dist/index.js.map (Sourcemap)" +test -f dist/index.cjs.map || { echo " ✗ dist/index.cjs.map (Sourcemap) MISSING"; exit 1; } +echo " ✓ dist/index.cjs.map (Sourcemap)" echo "✅ All build outputs verified!"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/ci/build.sh` around lines 24 - 30, The current "test -f ... && echo ..." lines can silently succeed even when files are missing; update the checks so a failing test aborts the script and reports which artifact is missing. Replace each "test -f dist/..." invocation (the lines checking dist/index.js, dist/index.cjs, dist/index.d.ts, dist/index.js.map, dist/index.cjs.map) with a check that on failure prints a clear error for that artifact and exits non‑zero (e.g., use "|| { echo 'Missing dist/...'; exit 1; }" or an if/then that echoes the missing file to stderr and exits), then keep the final echo only after all checks succeed..github/workflows/cd.yaml (1)
38-68:⚠️ Potential issue | 🟠 MajorNormalize empty
gh run listresults before bash non-empty checks.
--jq '.[0].databaseId'can return literalnull;[[ -n "$RUN_ID" ]]then passes and may emitrun_id=null, causing downstream artifact download failures.🔧 Proposed fix
RUN_ID=$(gh run list \ --workflow=ci.yaml \ --commit="$COMMIT_SHA" \ --status=success \ --json databaseId \ - --jq '.[0].databaseId') + --jq '.[0].databaseId // empty') @@ RUNNING_ID=$(gh run list \ --workflow=ci.yaml \ --commit="$COMMIT_SHA" \ --status=in_progress \ --json databaseId \ - --jq '.[0].databaseId') + --jq '.[0].databaseId // empty') @@ FAILED_ID=$(gh run list \ --workflow=ci.yaml \ --commit="$COMMIT_SHA" \ --status=failure \ - --json databaseId,conclusion \ - --jq '.[0].databaseId') + --json databaseId \ + --jq '.[0].databaseId // empty')#!/usr/bin/env bash # Verify jq filters are hardened. # Expected after fix: # - 0 matches for the non-hardened form # - 3 matches for the hardened form rg -n --fixed-strings "--jq '.[0].databaseId'" .github/workflows/cd.yaml rg -n --fixed-strings "--jq '.[0].databaseId // empty'" .github/workflows/cd.yaml🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/cd.yaml around lines 38 - 68, The gh run list jq filters can yield literal "null" which makes [[ -n "$VAR" ]] true; update each gh run list invocation that uses --jq '.[0].databaseId' (for RUN_ID, RUNNING_ID, and FAILED_ID) to coerce null to an empty string by changing the jq expression to '.[0].databaseId // empty' so the bash non-empty checks behave correctly and won't emit run_id=null or similar.
🧹 Nitpick comments (8)
docs/developer/CommitConventions.md (1)
186-186: Use consistent heading level for “Special Scopes.”Line 186 uses
# Special Scopeswhile the type sections use##. Consider making this##for a consistent document outline.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/developer/CommitConventions.md` at line 186, The "Special Scopes" heading uses a top-level heading marker (# Special Scopes) which breaks the document outline; change that heading to a second-level heading (## Special Scopes) so it matches the other type sections and preserves a consistent document structure, updating only the heading token for "Special Scopes".bunfig.unit.toml (1)
3-9: Comment wording should match the configured ignore scope.The config excludes
test/**(all test paths), not only fixtures. Updating the comment would make intent clearer for future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bunfig.unit.toml` around lines 3 - 9, Update the top comment so it accurately reflects the configured ignore scope: replace "exclude adapters, cli, and test fixtures" with wording that indicates tests in general are excluded (e.g., "exclude adapters, cli, and all tests") to match the coveragePathIgnorePatterns entry "test/**" and the settings under coverageSkipTestFiles and coveragePathIgnorePatterns; reference the existing keys coverageSkipTestFiles and coveragePathIgnorePatterns and the pattern "test/**" when locating the comment to change.bunfig.int.toml (1)
3-7: Inline comment is broader than stated exclusion.The comment says “only exclude test fixtures,” but the pattern excludes all
test/**. Consider rewording the comment to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bunfig.int.toml` around lines 3 - 7, The inline comment above coverageSkipTestFiles and coveragePathIgnorePatterns is inaccurate: it says "only exclude test fixtures" while the pattern "test/**" excludes the entire test directory; either change the comment to accurately describe the behavior (e.g., "exclude all test files under test/**") or adjust coveragePathIgnorePatterns to target only fixtures (e.g., refine the pattern instead of "test/**"); update the comment text immediately above the coverageSkipTestFiles and coveragePathIgnorePatterns keys to match the actual pattern you choose.package.json (1)
62-62: Replace"latest"with a pinned or semver-bounded version for@types/bunto maintain deterministic dependency resolution.All other devDependencies in this file use caret ranges (e.g.,
^6.0.3). Using a moving dist-tag for@types/buncreates inconsistency and risks type breakage when lockfiles are refreshed. Align the version specifier to your Bun baseline (e.g.,^1.x.y).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 62, The devDependency entry for "@types/bun" in package.json uses the moving tag "latest"; replace it with a pinned or semver-bounded version (e.g., change "@types/bun": "latest" to a caret or exact version like "^1.x.y" or "1.x.y") so dependency resolution is deterministic and consistent with other devDependencies; update the package.json "@types/bun" value accordingly and run your lockfile update (npm/yarn/pnpm) to commit the deterministic version.test/unit/calculator.service.test.ts (1)
11-190: Optional: convert repetitive arithmetic cases to table-driven tests.This would keep the suite easier to extend while preserving clarity.
🧪 Example pattern
+const addCases = [ + { a: 5, b: 3, expected: 8 }, + { a: -5, b: -3, expected: -8 }, + { a: 5, b: -3, expected: 2 }, + { a: 5, b: 0, expected: 5 }, +]; + +for (const c of addCases) { + it(`add(${c.a}, ${c.b}) => ${c.expected}`, () => { + const result = service.add({ a: c.a, b: c.b }); + expect(result.success).toBe(true); + if (result.success) expect(result.data.result).toBe(c.expected); + }); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/calculator.service.test.ts` around lines 11 - 190, Tests repeat the same arrange/act/assert pattern for service.add, service.subtract, service.multiply, and service.divide; convert these to table-driven tests using Jest's test.each (or equivalent) to reduce duplication. Create a table (array) of cases per operation containing inputs {a,b}, expected result, and optional expected operation string or error code, then replace the individual it(...) blocks for each operation group with a single test.each that calls service.add/service.subtract/service.multiply/service.divide and asserts result.success, result.data.result, and result.data.operation or error fields as appropriate; reference the existing test names and functions (service.add, service.subtract, service.multiply, service.divide) to locate and refactor the cases.src/lib/structures.ts (1)
48-51: Narrowoperationto a literal union for stronger API safety.Using
stringhere allows invalid operation names to flow through typed code.♻️ Proposed type tightening
+export type CalculatorOperation = 'add' | 'subtract' | 'multiply' | 'divide'; + export interface CalculatorOutput { readonly result: number; - readonly operation: string; + readonly operation: CalculatorOperation; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/structures.ts` around lines 48 - 51, The CalculatorOutput.operation is too permissive as string; tighten it to a literal union (e.g., 'add' | 'subtract' | 'multiply' | 'divide' or whatever operation names your code uses) so only valid operation names are allowed; update the CalculatorOutput interface declaration and then update any callers (functions returning CalculatorOutput or constructing objects typed as CalculatorOutput) to use the literal union values or map their inputs to those literals to satisfy the new type.docs/api.md (1)
14-395: Consider generating API docs from source to reduce drift risk.Given the breadth of this file, auto-generated API references (or a generation check in CI) would keep docs synchronized as signatures evolve.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/api.md` around lines 14 - 395, The API doc has high drift risk—automate generation from the source and/or add a CI check to prevent divergence: add a doc generation step (e.g., TypeDoc or your project's chosen generator) that outputs the public API for symbols like createLibraryService, createDefaultConfigProvider, LibraryService, CalculatorService, StringService, ConfigService and export types (LibConfig, Result, OperationResult, OperationError, CalculatorInput, CalculatorOutput, StringProcessInput, StringProcessOutput, ILoggerAdapter, IConfigProvider, ICacheAdapter); commit the generated output or add a CI job that regenerates docs and fails the build if the checked-in docs (the current API reference) differ from the generated output so signatures stay in sync..github/workflows/⚡reusable-test.yaml (1)
42-51: Consider adjusting the Codecov upload condition.Using
if: ${{ always() }}combined withfail_ci_if_error: truemeans that if tests fail (and potentially don't produce the coverage file), the Codecov upload will also fail and obscure the original test failure in the workflow summary.Consider using
if: ${{ !cancelled() }}orif: ${{ success() || failure() }}to skip on cancellation, or conditionally check if the coverage file exists before attempting upload.💡 Suggested alternatives
Option 1: Skip on cancellation only:
- name: Upload coverage to Codecov - if: ${{ always() }} + if: ${{ !cancelled() }} uses: codecov/codecov-action@v5Option 2: Only upload on test step success:
- name: Upload coverage to Codecov - if: ${{ always() }} + if: ${{ steps.run_all.outcome == 'success' }} uses: codecov/codecov-action@v5🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/⚡reusable-test.yaml around lines 42 - 51, The Codecov upload step named "Upload coverage to Codecov" currently uses if: ${{ always() }} with fail_ci_if_error: true which can surface an upload failure over the original test failure; change the step condition to skip uploads on cancellation or only run when tests produced results (e.g., use if: ${{ !cancelled() }} or if: ${{ success() || failure() }}) or add a pre-check to ensure the coverage file (files: coverage/${{ inputs.test_type }}/lcov.filtered.info) exists before running the codecov/codecov-action and keep fail_ci_if_error as desired; update the step condition in the workflow and/or add a conditional file-existence check job/step that gates the "Upload coverage to Codecov" step.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.cyan_state.yaml:
- Around line 45-111: The file contains duplicate snapshot versions (two entries
with "version: 27") under the atomi/workspace history block, which must be
unique and monotonic; update the duplicate "version" fields so each snapshot
under atomi/workspace.history has a strictly increasing integer (e.g., change
the second "version: 27" to 28, bump the following "version: 28" to 29, and
"version: 29" to 30) to preserve unique, monotonic history ordering.
In `@docs/developer/CommitConventions.md`:
- Line 24: Change the prose occurrences of the platform name to "GitHub" while
leaving the example scope tokens unchanged; specifically update the table row
for the [ci] entry and any other prose lines that read "github" (e.g., the
descriptive text mentioning github_workflow and github_action) so that the
platform is capitalized as "GitHub" but the literal scope tokens remain exactly
as shown (github_workflow, github_action).
In `@scripts/ci/update_version.sh`:
- Around line 4-9: The script currently accepts empty version values and can
leave behind the temporary file if jq or mv fails; add an explicit usage guard
that checks the version (variable "version") is non-empty and that
"package_path" exists and is writable, and return a non-zero exit if validation
fails; create the temp file in "tmpfile" as before but immediately install a
trap (trap 'rm -f "$tmpfile"' EXIT) to ensure cleanup on error or exit; after
running jq, check its exit status before mv and exit non-zero on failure so the
trap removes the temp file; reference the variables and commands ("version",
"package_path", "tmpfile", "jq", "mv") when implementing these checks and the
trap.
In `@scripts/test.sh`:
- Around line 8-13: The current logic always treats $1 as TEST_TYPE which
rejects flag-first invocations; change it so TEST_TYPE defaults to "unit" and
only consumes/shift $1 when it equals "unit" or "int". Concretely, update the
assignment/condition around TEST_TYPE (the TEST_TYPE variable and the existing
if [[ ... ]]) to: set TEST_TYPE="unit" by default, then if [[ "$1" == "unit" ||
"$1" == "int" ]]; then set TEST_TYPE="$1" and shift; else leave TEST_TYPE as
"unit" and do not shift so flags like --cover are preserved for later parsing.
In `@src/lib/services.ts`:
- Around line 15-23: Remove the unused type import OperationError from the
import list at the top of the file; edit the import statement that currently
lists CalculatorInput, CalculatorOutput, LibConfig, OperationError, Result,
StringProcessInput, StringProcessOutput to drop OperationError, and then run
type checking/TS compile to confirm there are no remaining references (e.g.,
methods like divide construct inline error objects rather than using
OperationError).
In `@test/int/library.integration.test.ts`:
- Around line 7-14: The import list includes an unused symbol ConfigService
causing a lint/pipeline error; remove ConfigService from the import declaration
that currently contains createLibraryService, createDefaultConfigProvider,
CalculatorService, StringService and LibraryService so only the actually used
services (CalculatorService and StringService) and helpers remain imported;
confirm tests still reference createLibraryService/createDefaultConfigProvider
and that only CalculatorService and StringService are asserted in "Service
composition".
---
Duplicate comments:
In @.github/workflows/cd.yaml:
- Around line 38-68: The gh run list jq filters can yield literal "null" which
makes [[ -n "$VAR" ]] true; update each gh run list invocation that uses --jq
'.[0].databaseId' (for RUN_ID, RUNNING_ID, and FAILED_ID) to coerce null to an
empty string by changing the jq expression to '.[0].databaseId // empty' so the
bash non-empty checks behave correctly and won't emit run_id=null or similar.
In `@scripts/ci/build.sh`:
- Around line 24-30: The current "test -f ... && echo ..." lines can silently
succeed even when files are missing; update the checks so a failing test aborts
the script and reports which artifact is missing. Replace each "test -f
dist/..." invocation (the lines checking dist/index.js, dist/index.cjs,
dist/index.d.ts, dist/index.js.map, dist/index.cjs.map) with a check that on
failure prints a clear error for that artifact and exits non‑zero (e.g., use "||
{ echo 'Missing dist/...'; exit 1; }" or an if/then that echoes the missing file
to stderr and exits), then keep the final echo only after all checks succeed.
---
Nitpick comments:
In @.github/workflows/⚡reusable-test.yaml:
- Around line 42-51: The Codecov upload step named "Upload coverage to Codecov"
currently uses if: ${{ always() }} with fail_ci_if_error: true which can surface
an upload failure over the original test failure; change the step condition to
skip uploads on cancellation or only run when tests produced results (e.g., use
if: ${{ !cancelled() }} or if: ${{ success() || failure() }}) or add a pre-check
to ensure the coverage file (files: coverage/${{ inputs.test_type
}}/lcov.filtered.info) exists before running the codecov/codecov-action and keep
fail_ci_if_error as desired; update the step condition in the workflow and/or
add a conditional file-existence check job/step that gates the "Upload coverage
to Codecov" step.
In `@bunfig.int.toml`:
- Around line 3-7: The inline comment above coverageSkipTestFiles and
coveragePathIgnorePatterns is inaccurate: it says "only exclude test fixtures"
while the pattern "test/**" excludes the entire test directory; either change
the comment to accurately describe the behavior (e.g., "exclude all test files
under test/**") or adjust coveragePathIgnorePatterns to target only fixtures
(e.g., refine the pattern instead of "test/**"); update the comment text
immediately above the coverageSkipTestFiles and coveragePathIgnorePatterns keys
to match the actual pattern you choose.
In `@bunfig.unit.toml`:
- Around line 3-9: Update the top comment so it accurately reflects the
configured ignore scope: replace "exclude adapters, cli, and test fixtures" with
wording that indicates tests in general are excluded (e.g., "exclude adapters,
cli, and all tests") to match the coveragePathIgnorePatterns entry "test/**" and
the settings under coverageSkipTestFiles and coveragePathIgnorePatterns;
reference the existing keys coverageSkipTestFiles and coveragePathIgnorePatterns
and the pattern "test/**" when locating the comment to change.
In `@docs/api.md`:
- Around line 14-395: The API doc has high drift risk—automate generation from
the source and/or add a CI check to prevent divergence: add a doc generation
step (e.g., TypeDoc or your project's chosen generator) that outputs the public
API for symbols like createLibraryService, createDefaultConfigProvider,
LibraryService, CalculatorService, StringService, ConfigService and export types
(LibConfig, Result, OperationResult, OperationError, CalculatorInput,
CalculatorOutput, StringProcessInput, StringProcessOutput, ILoggerAdapter,
IConfigProvider, ICacheAdapter); commit the generated output or add a CI job
that regenerates docs and fails the build if the checked-in docs (the current
API reference) differ from the generated output so signatures stay in sync.
In `@docs/developer/CommitConventions.md`:
- Line 186: The "Special Scopes" heading uses a top-level heading marker (#
Special Scopes) which breaks the document outline; change that heading to a
second-level heading (## Special Scopes) so it matches the other type sections
and preserves a consistent document structure, updating only the heading token
for "Special Scopes".
In `@package.json`:
- Line 62: The devDependency entry for "@types/bun" in package.json uses the
moving tag "latest"; replace it with a pinned or semver-bounded version (e.g.,
change "@types/bun": "latest" to a caret or exact version like "^1.x.y" or
"1.x.y") so dependency resolution is deterministic and consistent with other
devDependencies; update the package.json "@types/bun" value accordingly and run
your lockfile update (npm/yarn/pnpm) to commit the deterministic version.
In `@src/lib/structures.ts`:
- Around line 48-51: The CalculatorOutput.operation is too permissive as string;
tighten it to a literal union (e.g., 'add' | 'subtract' | 'multiply' | 'divide'
or whatever operation names your code uses) so only valid operation names are
allowed; update the CalculatorOutput interface declaration and then update any
callers (functions returning CalculatorOutput or constructing objects typed as
CalculatorOutput) to use the literal union values or map their inputs to those
literals to satisfy the new type.
In `@test/unit/calculator.service.test.ts`:
- Around line 11-190: Tests repeat the same arrange/act/assert pattern for
service.add, service.subtract, service.multiply, and service.divide; convert
these to table-driven tests using Jest's test.each (or equivalent) to reduce
duplication. Create a table (array) of cases per operation containing inputs
{a,b}, expected result, and optional expected operation string or error code,
then replace the individual it(...) blocks for each operation group with a
single test.each that calls
service.add/service.subtract/service.multiply/service.divide and asserts
result.success, result.data.result, and result.data.operation or error fields as
appropriate; reference the existing test names and functions (service.add,
service.subtract, service.multiply, service.divide) to locate and refactor the
cases.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (16)
bun.lockis excluded by!**/*.lockdocs/developer/standard/datetime/index.mdis excluded by!docs/developer/standard/**/*.mddocs/developer/standard/datetime/languages/csharp.mdis excluded by!docs/developer/standard/**/*.mddocs/developer/standard/datetime/languages/go.mdis excluded by!docs/developer/standard/**/*.mddocs/developer/standard/datetime/languages/typescript.mdis excluded by!docs/developer/standard/**/*.mddocs/developer/standard/shell-scripts.mdis excluded by!docs/developer/standard/**/*.mddocs/developer/standard/utilities/index.mdis excluded by!docs/developer/standard/**/*.mddocs/developer/standard/utilities/languages/csharp.mdis excluded by!docs/developer/standard/**/*.mddocs/developer/standard/utilities/languages/go.mdis excluded by!docs/developer/standard/**/*.mddocs/developer/standard/utilities/languages/typescript.mdis excluded by!docs/developer/standard/**/*.mddocs/developer/standard/validation/index.mdis excluded by!docs/developer/standard/**/*.mddocs/developer/standard/validation/languages/csharp.mdis excluded by!docs/developer/standard/**/*.mddocs/developer/standard/validation/languages/go.mdis excluded by!docs/developer/standard/**/*.mddocs/developer/standard/validation/languages/typescript.mdis excluded by!docs/developer/standard/**/*.mdspec/CU-86ewpu7j2/additonal.mdis excluded by!spec/**/*.mdspec/CU-86ewpu7j2/task-spec.mdis excluded by!spec/**/*.md
📒 Files selected for processing (47)
.claude/skills/datetime/SKILL.md.claude/skills/utilities/SKILL.md.claude/skills/validation/SKILL.md.coderabbit.yaml.cyan_state.yaml.github/workflows/cd.yaml.github/workflows/ci.yaml.github/workflows/⚡reusable-build.yaml.github/workflows/⚡reusable-precommit.yaml.github/workflows/⚡reusable-release.yaml.github/workflows/⚡reusable-test.yaml.gitignoreLICENSEREADME.mdTaskfile.yamlatomi_release.yamlbuild.config.tsbunfig.int.tomlbunfig.unit.tomlcodecov.yamldocs/api.mddocs/developer/CommitConventions.mdnix/env.nixnix/packages.nixpackage.jsonscripts/ci/build.shscripts/ci/pre-commit.shscripts/ci/publish.shscripts/ci/release.shscripts/ci/test.shscripts/ci/update_version.shscripts/local/secrets.shscripts/test.shsrc/index.tssrc/lib/index.tssrc/lib/interfaces.tssrc/lib/services.tssrc/lib/structures.tstasks/test.tasks.yamltest/fixtures/index.tstest/int/library.integration.test.tstest/unit/calculator.service.test.tstest/unit/config.service.test.tstest/unit/fixtures.test.tstest/unit/library.service.test.tstest/unit/string.service.test.tstsconfig.json
- Fix knip hook to use bunx instead of bun (resolves binary from node_modules) - Add // empty to jq expressions in cd.yaml to handle null arrays - Fix build.sh verification to fail on missing files By Claude Code Kagent Autopilot 🤖
Contributor
Author
|
@coderabbitai I have addressed all the feedback: Fixed in 207555a:
Please review the changes. By Claude Code Kagent Autopilot 🤖 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 4122
Verified fixes:
The changes are well-structured, properly handle edge cases, and include clear error messages. No issues found! 🎉 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/cd.yaml (1)
67-68: Minor: Remove unusedconclusionfield from JSON query.The
--json databaseId,conclusionfetches theconclusionfield, but onlydatabaseIdis used. For consistency with the othergh run listcalls (lines 42, 56), consider removing it.♻️ Suggested fix
FAILED_ID=$(gh run list \ --workflow=ci.yaml \ --commit="$COMMIT_SHA" \ --status=failure \ - --json databaseId,conclusion \ + --json databaseId \ --jq '.[0].databaseId // empty')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/cd.yaml around lines 67 - 68, The gh run list command currently uses the JSON selector "--json databaseId,conclusion" but only databaseId is consumed; update the command so the JSON output only requests databaseId (remove the ",conclusion" token) to match the other gh run list calls and avoid returning unused fields, i.e., change the "--json databaseId,conclusion" invocation to "--json databaseId" wherever it appears in this workflow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/cd.yaml:
- Around line 61-76: The workflow currently only queries CI runs for failure via
FAILED_ID and ignores cancelled runs; update the check that calls gh run list
(for --workflow=ci.yaml and --commit="$COMMIT_SHA") to also detect cancelled
runs (e.g., query --status=cancelled or inspect the returned conclusion field
for "cancelled") and treat a cancelled run the same as failure — echo a message
like "❌ CI run <id> cancelled for commit <sha>" and exit 1; modify the FAILED_ID
logic or add a new CANCELLED_ID lookup that uses the same pattern so cancelled
runs fail fast instead of waiting the full timeout.
---
Nitpick comments:
In @.github/workflows/cd.yaml:
- Around line 67-68: The gh run list command currently uses the JSON selector
"--json databaseId,conclusion" but only databaseId is consumed; update the
command so the JSON output only requests databaseId (remove the ",conclusion"
token) to match the other gh run list calls and avoid returning unused fields,
i.e., change the "--json databaseId,conclusion" invocation to "--json
databaseId" wherever it appears in this workflow.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/cd.yamlnix/pre-commit.nixscripts/ci/build.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- scripts/ci/build.sh
- Add ignoreBinaries for knip in package.json - Remove unused OperationError import from services.ts - Remove unused ConfigService import from integration test By Claude Code Kagent Autopilot 🤖
There was a problem hiding this comment.
🧹 Nitpick comments (2)
package.json (1)
65-65: Consider pinning@types/bunto a specific version range.Using
"latest"for@types/buncan cause non-reproducible builds when type definitions change unexpectedly. Consider pinning to a version range (e.g.,"^1.x.x") to ensure consistent CI behavior while still receiving compatible updates.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 65, In package.json replace the dependency entry that uses "@types/bun": "latest" with a pinned semver range (for example "^1.0.0" or whatever major version is appropriate) to ensure reproducible builds; open package.json, locate the dependency key "@types/bun" and update its version string to a fixed semver range, then run npm install / yarn install and commit the updated package-lock.json / yarn.lock to lock the resolved version.src/lib/services.ts (1)
36-37: Type assertion assumesversionfield exists.If
package.jsonexists but lacks aversionfield, this returnsundefinedcast asstring. Consider adding a fallback:- return packageJson.version as string; + return (packageJson.version as string) ?? '0.0.0-development';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/services.ts` around lines 36 - 37, The current return of packageJson.version is asserted as string and can be undefined; update the logic around reading packageJson (reference packageJsonPath, packageJson and the return of packageJson.version) to guard against a missing version field and provide a safe fallback or error: check that packageJson.version is a non-empty string and return that, otherwise return a sensible default (e.g. '0.0.0' or empty string) or throw a clear error, ensuring consumers always receive a valid string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@package.json`:
- Line 65: In package.json replace the dependency entry that uses "@types/bun":
"latest" with a pinned semver range (for example "^1.0.0" or whatever major
version is appropriate) to ensure reproducible builds; open package.json, locate
the dependency key "@types/bun" and update its version string to a fixed semver
range, then run npm install / yarn install and commit the updated
package-lock.json / yarn.lock to lock the resolved version.
In `@src/lib/services.ts`:
- Around line 36-37: The current return of packageJson.version is asserted as
string and can be undefined; update the logic around reading packageJson
(reference packageJsonPath, packageJson and the return of packageJson.version)
to guard against a missing version field and provide a safe fallback or error:
check that packageJson.version is a non-empty string and return that, otherwise
return a sensible default (e.g. '0.0.0' or empty string) or throw a clear error,
ensuring consumers always receive a valid string.
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. ℹ️ You can also turn on project coverage checks and project coverage reporting on Pull Request comment Thanks for integrating Codecov - We've got you covered ☂️ |
The build script now runs `bun install --frozen-lockfile` before building to ensure all dependencies (including @types/bun) are available when TypeScript generates type declarations. Fixes TS2688: Cannot find type definition file for '@types/bun'.
- Fix duplicate version in .cyan_state.yaml workspace history - Add argument validation and cleanup trap in update_version.sh - Allow flag-first invocation in test.sh (e.g., ./scripts/test.sh --cover) - Add cancelled CI run detection in cd.yaml By Claude Code Kagent Autopilot 🤖
Contributor
Author
|
@coderabbitai I have addressed all the review feedback: Fixed in 155bb55:
Previously addressed:
Please resolve any remaining threads or provide new feedback. By Claude Code Kagent Autopilot 🤖 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 9913
Verified fixes:
Combined with your previous fixes (bunx knip, jq null handling, build verification), this PR is now in excellent shape! The code quality, error handling, and edge case coverage are all solid. 🎉 No further issues found. All review threads can be considered resolved! 🧠 Learnings used |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Ticket
Changes
Added
src/lib/- Library architecture with structures.ts, interfaces.ts, services.tstest/unit/- Unit tests with 100% coverage (108 tests)test/int/- Integration tests with 87.6% coverage (12 tests)test/fixtures/- Test fixtures and mock implementationsscripts/test.sh- Test runner with CLI-style flagsscripts/ci/test.sh- CI test script with coverage threshold checkscripts/ci/build.sh- CI build scriptscripts/ci/publish.sh- npm publishing scriptscripts/ci/release.sh- Semantic release scripttasks/test.tasks.yaml- Test-related tasksbunfig.unit.toml- Unit test coverage configurationbunfig.int.toml- Integration test coverage configurationdocs/api.md- API documentationLICENSE- MIT licenseModified
package.json- Proper exports for ESM/CJS, metadata, knip configREADME.md- Installation, usage, API docs, badges.github/workflows/ci.yaml- Complete CI pipeline.github/workflows/⚡reusable-*.yaml- Reusable workflows for test, build, pre-commit, releaseTest Plan
Checklist
Summary by CodeRabbit
New Features
Documentation
Chores
CI / Tests