refactor: migrate shell scripts to TypeScript#75
Open
forggy0113 wants to merge 8 commits into
Open
Conversation
refactor bash script to js script, and make some tweaks to skills
refactor bash script to js script, and make some tweaks to skills
|
Lost in the diff? Review this PR in Change Stack to follow the change map from intent to exact ranges. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR migrates Comet automation from Bash to Node/TypeScript: new TS source under src/scripts, compiled JS in assets/skills/comet/scripts, a shared comet-lib, state/guard/handoff/archive/yaml-validate CLIs, updated build/CI, tests converted to Node-run, and documentation revised to reference the JS scripts. ChangesComet Node.js Script Migration
Estimated code review effort: 🎯 3 (Moderate) | ⏱️ ~25 minutes
✨ Finishing Touches🧪 Generate unit tests (beta)
|
forggy0113
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (8)
test/ts/comet-scripts.test.ts (1)
801-806: ⚡ Quick winAllow direct Node nested invocations in this assertion.
Line 805 currently hard-codes
runNode, but directnode/process.execPathscript invocation is also valid under the migration contract. This test can reject compliant implementations.Suggested test adjustment
- expect(content, `${name} should invoke nested scripts through runNode`).toContain('runNode'); + expect( + content, + `${name} should invoke nested scripts through runNode or direct Node execution`, + ).toMatch(/\brunNode\s*\(|process\.execPath|['"]node['"]/);As per coding guidelines, script-to-script invocations must use
node <script>.jsor a sharedrunNodehelper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/ts/comet-scripts.test.ts` around lines 801 - 806, The test currently asserts that nested script invocations must contain the helper runNode (in the loop over 'comet-archive.js', 'comet-guard.js', 'comet-handoff.js'), which wrongly rejects direct Node invocations; update the assertion in the test to accept either the shared runNode helper OR direct node invocations (e.g., "node " or process.execPath) while keeping the negative assertion that they must not shell out to bash; locate the expectations referencing runNode and the regex banning bash in the test and change the positive expectation to check for runNode OR a pattern matching direct node/process.execPath invocation.assets/skills/comet-hotfix/SKILL.md (1)
32-35: ⚡ Quick winConsider simplifying JSON extraction pattern.
Same verbose JSON parsing. Use simpler
node -papproach as recommended in other files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@assets/skills/comet-hotfix/SKILL.md` around lines 32 - 35, The four verbose JSON extraction lines for COMET_STATE, COMET_GUARD, COMET_HANDOFF and COMET_ARCHIVE should be simplified by replacing the multi-line node -e streaming parse with the concise node -p JSON evaluation pattern used elsewhere: invoke node -p to parse the incoming COMET_ENV_JSON and directly print the desired property (COMET_STATE / COMET_GUARD / COMET_HANDOFF / COMET_ARCHIVE) for each variable, preserving the same printf piping but using the shorter node -p form to make the code clearer and consistent.assets/skills-zh/comet/SKILL.md (1)
242-245: ⚡ Quick winConsider simplifying JSON extraction pattern.
The inline Node script for JSON parsing is verbose and fragile. A simpler approach would improve maintainability:
COMET_STATE="$(node -p "JSON.parse(require('fs').readFileSync(0, 'utf-8')).COMET_STATE" <<< "$COMET_ENV_JSON")" COMET_GUARD="$(node -p "JSON.parse(require('fs').readFileSync(0, 'utf-8')).COMET_GUARD" <<< "$COMET_ENV_JSON")" COMET_HANDOFF="$(node -p "JSON.parse(require('fs').readFileSync(0, 'utf-8')).COMET_HANDOFF" <<< "$COMET_ENV_JSON")" COMET_ARCHIVE="$(node -p "JSON.parse(require('fs').readFileSync(0, 'utf-8')).COMET_ARCHIVE" <<< "$COMET_ENV_JSON")"Or even simpler with
-pand direct property access:COMET_STATE="$(node -p "const o=JSON.parse(process.argv[1]);o.COMET_STATE" "$COMET_ENV_JSON")"This reduces complexity and improves cross-platform reliability.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@assets/skills-zh/comet/SKILL.md` around lines 242 - 245, Replace the verbose inline node -e JSON parsing for COMET_STATE, COMET_GUARD, COMET_HANDOFF and COMET_ARCHIVE with a simpler, more robust node -p/readFileSync(0,'utf-8') or node -p using process.argv[1] pattern; specifically update the shell assignments for the symbols COMET_STATE, COMET_GUARD, COMET_HANDOFF and COMET_ARCHIVE to call node -p and parse stdin or a passed argument (e.g., JSON.parse(require('fs').readFileSync(0,'utf-8')).PROPERTY or JSON.parse(process.argv[1]).PROPERTY) instead of the current process.stdin on('data')... on('end') handlers to reduce verbosity and improve cross-platform reliability.assets/skills/comet-archive/SKILL.md (1)
27-30: ⚡ Quick winConsider simplifying JSON extraction pattern.
Same verbose JSON parsing as in other files. See the suggestion in
comet/SKILL.mdfor a simpler approach usingnode -pwith process.argv or readFileSync(0).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@assets/skills/comet-archive/SKILL.md` around lines 27 - 30, The four lines setting COMET_STATE, COMET_GUARD, COMET_HANDOFF, and COMET_ARCHIVE use a verbose node stdin parser; replace each with a simpler node command that reads stdin or argv (e.g., using require('fs').readFileSync(0,'utf8') or node -p with process.argv) to parse COMET_ENV_JSON and extract the respective fields more concisely and consistently across this file.assets/skills/comet-open/SKILL.md (1)
62-65: ⚡ Quick winConsider simplifying JSON extraction pattern.
Same verbose JSON parsing approach. Suggest using simpler
node -pmethod as recommended in other files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@assets/skills/comet-open/SKILL.md` around lines 62 - 65, Replace the verbose node -e stdin parsing for COMET_STATE, COMET_GUARD, COMET_HANDOFF, and COMET_ARCHIVE with the simpler node -p pattern used elsewhere: read stdin via fs.readFileSync(0, "utf8") and JSON.parse it, then access the respective property (e.g., use node -p 'JSON.parse(require("fs").readFileSync(0,"utf8")).COMET_STATE') for each variable COMET_STATE, COMET_GUARD, COMET_HANDOFF and COMET_ARCHIVE so the four assignments become concise and consistent.assets/skills/comet-build/SKILL.md (1)
26-29: ⚡ Quick winConsider simplifying JSON extraction pattern.
Same verbose JSON parsing. Suggest using
node -pwith simpler inline approach as recommended in other files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@assets/skills/comet-build/SKILL.md` around lines 26 - 29, The four long inline node parsers that assign COMET_STATE, COMET_GUARD, COMET_HANDOFF, and COMET_ARCHIVE should be replaced with the simpler node -p style JSON extraction used elsewhere: call node with a short one-liner that parses the COMET_ENV_JSON value and prints the requested property (COMET_STATE/COMET_GUARD/COMET_HANDOFF/COMET_ARCHIVE) instead of the current multi-line stdin handler; update each assignment to use that concise pattern to keep consistency and reduce verbosity.assets/skills/comet-tweak/SKILL.md (1)
35-38: ⚡ Quick winConsider simplifying JSON extraction pattern.
Same verbose JSON parsing. Use simpler
node -papproach as recommended across all files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@assets/skills/comet-tweak/SKILL.md` around lines 35 - 38, Replace the verbose node -e streaming JSON parsing used to populate COMET_STATE, COMET_GUARD, COMET_HANDOFF and COMET_ARCHIVE from COMET_ENV_JSON with the simpler node -p JSON evaluation pattern; specifically, update each line that pipes COMET_ENV_JSON into node -e to use node -p to directly evaluate JSON.parse(...) and extract the named property (COMET_STATE/COMET_GUARD/COMET_HANDOFF/COMET_ARCHIVE) for clarity and consistency with other files.assets/skills/comet-design/SKILL.md (1)
26-29: ⚡ Quick winConsider simplifying JSON extraction pattern.
Repeated verbose JSON parsing pattern. Recommend simpler
node -papproach as suggested in other files.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@assets/skills/comet-design/SKILL.md` around lines 26 - 29, The four lines that set COMET_STATE, COMET_GUARD, COMET_HANDOFF and COMET_ARCHIVE use a repeated verbose node -e stdin parsing pattern; replace each with a concise node -p command that reads JSON from the COMET_ENV_JSON environment variable and returns the specific key (COMET_STATE, COMET_GUARD, COMET_HANDOFF, COMET_ARCHIVE) so the shell variables are populated directly from process.env.COMET_ENV_JSON instead of piping and manually accumulating stdin.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@assets/skills/comet/scripts/comet-yaml-validate.js`:
- Around line 31-42: Update the required fields array in comet-yaml-validate.js
(the const required declaration) to include the state-owned keys that
comet-state.js init always writes: base_ref, build_pause, verification_report,
branch_status, and created_at; ensure the validation list mirrors the fields
created by comet-state (and update test/ts/comet-scripts.test.ts as needed) so
missing values aren't skipped and incomplete .comet.yaml files fail validation.
In `@CHANGELOG.md`:
- Around line 9-13: The 0.3.6 changelog entry contains unresolved Git conflict
markers (<<<<<<<, =======, >>>>>>>); remove those markers and merge the two
conflicting lines into a single well-formed "Added" bullet (e.g., combine the
TypeScript note and the cross-platform Node scripts note into one clear
sentence) and ensure the entry adheres to the repository changelog structure by
including the version header "0.3.6" and required sections (Added/Changed/etc.)
for that release.
In `@CLAUDE.md`:
- Line 25: The fenced code block in CLAUDE.md currently uses plain backticks
(```) with no language tag; update the opening fence to include a language
identifier (for example change the opening ``` to ```text) so the code block is
tagged for markdown linting and readable by linters/renderers.
- Around line 10-18: Remove the unresolved Git merge markers and unify the
script guidance into a single canonical paragraph: keep the rule that scripts
are implemented in TypeScript under src/scripts/*.ts and are compiled via
build.js to assets/skills/comet/scripts/ (JavaScript) for cross-platform
compatibility; delete the alternate ".js only / Node.js" wording and any
leftover conflict markers (e.g., <<<<<<<, =======, >>>>>>>) throughout the file
(also check and fix the duplicate/conflicting blocks referenced around the other
ranges mentioned).
In `@src/scripts/comet-archive.ts`:
- Around line 112-113: The annotateFrontmatter function currently injects
"status: final" unconditionally into the constructed frontmatter (see
construction of next using archiveName, extraFields and text), which mutates
plan files when extraFields is empty; change the logic so "status: final" is
only added when appropriate — e.g., append `status: final` only if extraFields
is truthy or if a newStatus flag/condition indicates the file should be
finalized; update the code that builds next (the template that uses archiveName,
extraFields, and text) to conditionally include the status line rather than
always concatenating `status: final`.
In `@src/scripts/comet-state.ts`:
- Around line 74-77: The warning when field === 'phase' should be suppressed for
internal transitions invoked via transition(); update the check in set() (the
block that logs yellow("WARNING: Setting 'phase'...")) to only emit the warning
when not called with the quiet flag (i.e., if field === 'phase' && !quiet), so
internal calls from transition(..., quiet=true) do not show the warning; keep
the existing fallback that still validates SET_FIELDS when not phase.
---
Nitpick comments:
In `@assets/skills-zh/comet/SKILL.md`:
- Around line 242-245: Replace the verbose inline node -e JSON parsing for
COMET_STATE, COMET_GUARD, COMET_HANDOFF and COMET_ARCHIVE with a simpler, more
robust node -p/readFileSync(0,'utf-8') or node -p using process.argv[1] pattern;
specifically update the shell assignments for the symbols COMET_STATE,
COMET_GUARD, COMET_HANDOFF and COMET_ARCHIVE to call node -p and parse stdin or
a passed argument (e.g.,
JSON.parse(require('fs').readFileSync(0,'utf-8')).PROPERTY or
JSON.parse(process.argv[1]).PROPERTY) instead of the current process.stdin
on('data')... on('end') handlers to reduce verbosity and improve cross-platform
reliability.
In `@assets/skills/comet-archive/SKILL.md`:
- Around line 27-30: The four lines setting COMET_STATE, COMET_GUARD,
COMET_HANDOFF, and COMET_ARCHIVE use a verbose node stdin parser; replace each
with a simpler node command that reads stdin or argv (e.g., using
require('fs').readFileSync(0,'utf8') or node -p with process.argv) to parse
COMET_ENV_JSON and extract the respective fields more concisely and consistently
across this file.
In `@assets/skills/comet-build/SKILL.md`:
- Around line 26-29: The four long inline node parsers that assign COMET_STATE,
COMET_GUARD, COMET_HANDOFF, and COMET_ARCHIVE should be replaced with the
simpler node -p style JSON extraction used elsewhere: call node with a short
one-liner that parses the COMET_ENV_JSON value and prints the requested property
(COMET_STATE/COMET_GUARD/COMET_HANDOFF/COMET_ARCHIVE) instead of the current
multi-line stdin handler; update each assignment to use that concise pattern to
keep consistency and reduce verbosity.
In `@assets/skills/comet-design/SKILL.md`:
- Around line 26-29: The four lines that set COMET_STATE, COMET_GUARD,
COMET_HANDOFF and COMET_ARCHIVE use a repeated verbose node -e stdin parsing
pattern; replace each with a concise node -p command that reads JSON from the
COMET_ENV_JSON environment variable and returns the specific key (COMET_STATE,
COMET_GUARD, COMET_HANDOFF, COMET_ARCHIVE) so the shell variables are populated
directly from process.env.COMET_ENV_JSON instead of piping and manually
accumulating stdin.
In `@assets/skills/comet-hotfix/SKILL.md`:
- Around line 32-35: The four verbose JSON extraction lines for COMET_STATE,
COMET_GUARD, COMET_HANDOFF and COMET_ARCHIVE should be simplified by replacing
the multi-line node -e streaming parse with the concise node -p JSON evaluation
pattern used elsewhere: invoke node -p to parse the incoming COMET_ENV_JSON and
directly print the desired property (COMET_STATE / COMET_GUARD / COMET_HANDOFF /
COMET_ARCHIVE) for each variable, preserving the same printf piping but using
the shorter node -p form to make the code clearer and consistent.
In `@assets/skills/comet-open/SKILL.md`:
- Around line 62-65: Replace the verbose node -e stdin parsing for COMET_STATE,
COMET_GUARD, COMET_HANDOFF, and COMET_ARCHIVE with the simpler node -p pattern
used elsewhere: read stdin via fs.readFileSync(0, "utf8") and JSON.parse it,
then access the respective property (e.g., use node -p
'JSON.parse(require("fs").readFileSync(0,"utf8")).COMET_STATE') for each
variable COMET_STATE, COMET_GUARD, COMET_HANDOFF and COMET_ARCHIVE so the four
assignments become concise and consistent.
In `@assets/skills/comet-tweak/SKILL.md`:
- Around line 35-38: Replace the verbose node -e streaming JSON parsing used to
populate COMET_STATE, COMET_GUARD, COMET_HANDOFF and COMET_ARCHIVE from
COMET_ENV_JSON with the simpler node -p JSON evaluation pattern; specifically,
update each line that pipes COMET_ENV_JSON into node -e to use node -p to
directly evaluate JSON.parse(...) and extract the named property
(COMET_STATE/COMET_GUARD/COMET_HANDOFF/COMET_ARCHIVE) for clarity and
consistency with other files.
In `@test/ts/comet-scripts.test.ts`:
- Around line 801-806: The test currently asserts that nested script invocations
must contain the helper runNode (in the loop over 'comet-archive.js',
'comet-guard.js', 'comet-handoff.js'), which wrongly rejects direct Node
invocations; update the assertion in the test to accept either the shared
runNode helper OR direct node invocations (e.g., "node " or process.execPath)
while keeping the negative assertion that they must not shell out to bash;
locate the expectations referencing runNode and the regex banning bash in the
test and change the positive expectation to check for runNode OR a pattern
matching direct node/process.execPath invocation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1bf810eb-699b-4bcb-8938-a3788d153145
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (55)
.github/PULL_REQUEST_TEMPLATE.md.github/workflows/ci.ymlAGENTS.mdCHANGELOG.mdCLAUDE.mdCONTRIBUTING.mdREADME-zh.mdREADME.mdassets/manifest.jsonassets/skills-zh/comet-archive/SKILL.mdassets/skills-zh/comet-build/SKILL.mdassets/skills-zh/comet-design/SKILL.mdassets/skills-zh/comet-hotfix/SKILL.mdassets/skills-zh/comet-open/SKILL.mdassets/skills-zh/comet-tweak/SKILL.mdassets/skills-zh/comet-verify/SKILL.mdassets/skills-zh/comet/SKILL.mdassets/skills/comet-archive/SKILL.mdassets/skills/comet-build/SKILL.mdassets/skills/comet-design/SKILL.mdassets/skills/comet-hotfix/SKILL.mdassets/skills/comet-open/SKILL.mdassets/skills/comet-tweak/SKILL.mdassets/skills/comet-verify/SKILL.mdassets/skills/comet/SKILL.mdassets/skills/comet/scripts/comet-archive.jsassets/skills/comet/scripts/comet-archive.shassets/skills/comet/scripts/comet-env.jsassets/skills/comet/scripts/comet-env.shassets/skills/comet/scripts/comet-guard.jsassets/skills/comet/scripts/comet-guard.shassets/skills/comet/scripts/comet-handoff.jsassets/skills/comet/scripts/comet-handoff.shassets/skills/comet/scripts/comet-lib.jsassets/skills/comet/scripts/comet-state.jsassets/skills/comet/scripts/comet-state.shassets/skills/comet/scripts/comet-yaml-validate.jsassets/skills/comet/scripts/comet-yaml-validate.shbuild.jspackage.jsonscripts/run-bats.jssrc/scripts/comet-archive.tssrc/scripts/comet-env.tssrc/scripts/comet-guard.tssrc/scripts/comet-handoff.tssrc/scripts/comet-lib.tssrc/scripts/comet-state.tssrc/scripts/comet-yaml-validate.tssrc/scripts/tsconfig.jsontest/shell/comet-state.batstest/ts/comet-scripts.test.tstest/ts/detect.test.tstest/ts/init-e2e.test.tstest/ts/run-bats.test.tstest/ts/skills.test.ts
💤 Files with no reviewable changes (10)
- AGENTS.md
- scripts/run-bats.js
- assets/skills/comet/scripts/comet-state.sh
- assets/skills/comet/scripts/comet-handoff.sh
- assets/skills/comet/scripts/comet-yaml-validate.sh
- test/shell/comet-state.bats
- assets/skills/comet/scripts/comet-guard.sh
- assets/skills/comet/scripts/comet-env.sh
- assets/skills/comet/scripts/comet-archive.sh
- test/ts/run-bats.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
test/ts/detect.test.ts (1)
162-163:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix environment variable restoration to handle undefined correctly.
Same issue as lines 138-139: undefined env vars must be deleted, not assigned.
🔧 Proposed fix
} finally { - process.env.HOME = origHome; - process.env.USERPROFILE = origUserProfile; + if (origHome !== undefined) { + process.env.HOME = origHome; + } else { + delete process.env.HOME; + } + if (origUserProfile !== undefined) { + process.env.USERPROFILE = origUserProfile; + } else { + delete process.env.USERPROFILE; + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/ts/detect.test.ts` around lines 162 - 163, Restore environment variables correctly by checking origHome and origUserProfile before assignment: if the original value is undefined, delete process.env.HOME / process.env.USERPROFILE instead of assigning undefined; otherwise reassign the original string. Apply the same pattern used for the earlier fix (lines that handled origHome/origUserProfile at 138-139) to the restoration calls in detect.test.ts where process.env.HOME and process.env.USERPROFILE are being restored.
🧹 Nitpick comments (1)
test/ts/init-e2e.test.ts (1)
176-232: 💤 Low valueInconsistent homedir mocking approach.
This test uses
process.env.HOMEandprocess.env.USERPROFILEoverrides, while other tests in this file (lines 113, 241, 264, 297) usevi.spyOn(os, 'homedir'). Mixing approaches creates maintenance confusion and suggests the mocking strategy is not settled.Consider either:
- Adopting
process.envconsistently across all tests (and fixing the undefined handling), or- Reverting this test to
vi.spyOn(os, 'homedir')to match the rest of the file.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/ts/init-e2e.test.ts` around lines 176 - 232, The test "installs all platforms from clean directory with --yes" should stop mutating process.env.HOME/USERPROFILE and instead mock os.homedir like the other tests: remove the process.env assignments and related origHome/origUserprofile restore, add vi.spyOn(os, 'homedir').mockReturnValue(tmpDir) before calling initCommand and ensure you restore the spy (vi.spyOn(...).mockRestore() or use afterEach) so the homedir mock is isolated; locate the test by the it block name in test/ts/init-e2e.test.ts and update it accordingly to match the other tests' homedir mocking pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/ts/detect.test.ts`:
- Around line 138-139: The test restores HOME and USERPROFILE incorrectly by
assigning possibly undefined values (process.env.HOME = origHome), which sets
the string "undefined"; update the restoration to mirror the existing pattern
used for CLAUDE_CONFIG_DIR: check origHome and origUserProfile for undefined and
either reassign process.env.HOME/process.env.USERPROFILE when defined or delete
the properties when undefined so environment is properly unset between tests
(use the origHome and origUserProfile variables referenced in the diff).
In `@test/ts/init-e2e.test.ts`:
- Around line 230-231: Restore environment variables without coercing undefined
to the string: instead of unconditionally assigning process.env.HOME = origHome
and process.env.USERPROFILE = origUserProfile, mirror the pattern used earlier
(lines ~217-221) by checking origHome and origUserProfile — if the original
value is undefined delete process.env.HOME / delete process.env.USERPROFILE,
otherwise set process.env.HOME = origHome and process.env.USERPROFILE =
origUserProfile; update in init-e2e.test.ts where those two restores occur.
---
Duplicate comments:
In `@test/ts/detect.test.ts`:
- Around line 162-163: Restore environment variables correctly by checking
origHome and origUserProfile before assignment: if the original value is
undefined, delete process.env.HOME / process.env.USERPROFILE instead of
assigning undefined; otherwise reassign the original string. Apply the same
pattern used for the earlier fix (lines that handled origHome/origUserProfile at
138-139) to the restoration calls in detect.test.ts where process.env.HOME and
process.env.USERPROFILE are being restored.
---
Nitpick comments:
In `@test/ts/init-e2e.test.ts`:
- Around line 176-232: The test "installs all platforms from clean directory
with --yes" should stop mutating process.env.HOME/USERPROFILE and instead mock
os.homedir like the other tests: remove the process.env assignments and related
origHome/origUserprofile restore, add vi.spyOn(os,
'homedir').mockReturnValue(tmpDir) before calling initCommand and ensure you
restore the spy (vi.spyOn(...).mockRestore() or use afterEach) so the homedir
mock is isolated; locate the test by the it block name in
test/ts/init-e2e.test.ts and update it accordingly to match the other tests'
homedir mocking pattern.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9b615732-8df6-4a74-9b8a-2887af155503
📒 Files selected for processing (11)
CHANGELOG.mdCLAUDE.mdassets/skills/comet/scripts/comet-archive.jsassets/skills/comet/scripts/comet-state.jsassets/skills/comet/scripts/comet-yaml-validate.jssrc/scripts/comet-archive.tssrc/scripts/comet-state.tssrc/scripts/comet-yaml-validate.tstest/ts/comet-scripts.test.tstest/ts/detect.test.tstest/ts/init-e2e.test.ts
🚧 Files skipped from review as they are similar to previous changes (9)
- src/scripts/comet-yaml-validate.ts
- src/scripts/comet-archive.ts
- assets/skills/comet/scripts/comet-archive.js
- assets/skills/comet/scripts/comet-yaml-validate.js
- src/scripts/comet-state.ts
- CHANGELOG.md
- CLAUDE.md
- assets/skills/comet/scripts/comet-state.js
- test/ts/comet-scripts.test.ts
Contributor
|
Thank you for your contribution. I considered this solution a while ago, and this refactoring is actually in the works. However, considering the numerous script modifications currently being made to the features under development, it's not a high priority for now. Merging it would cause significant conflicts with other features, making things complicated. Therefore, I suggest postponing this project for now. If you'd like, you can start working on other community tasks. |
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
2 participants
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
Refactor all Comet shell scripts from Bash (
.sh) to TypeScript (.ts) compiled to Node.js (.js). Script source now lives undersrc/scripts/with strict type annotations, compiled bybuild.jstoassets/skills/comet/scripts/. This eliminates the Bash dependency for bundled Comet automation — all phase guards, state management, handoff, archive, YAML validation, and environment discovery now run cross-platform vianode <script>.js.Why: Bash scripts fail on Windows without Git Bash, creating a broken experience for Windows users. TypeScript source provides strict type safety and linting while still producing zero-dependency JavaScript that Skills invoke identically.
🎯 Scope
init,status,doctor,update)assets/skills/,assets/skills-zh/)assets/skills/comet/scripts/)build.js, CI workflow,src/scripts/TypeScript source🧪 Testing
pnpm build— passpnpm lint— passpnpm format:check— passpnpm test— 178/178 passpnpm test -- test/ts/comet-scripts.test.ts— 45/45 passpnpm test:shell— removed (no more shell scripts; Bats runner deleted)✅ Checklist
README.md,README-zh.md, andCONTRIBUTING.mdCHANGELOG.mdupdatedassets/manifest.jsonand relevant tests👀 Notes for Reviewers
.tssource files insrc/scripts/— compiled toassets/skills/comet/scripts/*.jsbybuild.js.shscripts deleted —comet-state.sh,comet-guard.sh,comet-handoff.sh,comet-archive.sh,comet-env.sh,comet-yaml-validate.shcomet-lib.ts/.js(shared utilities)shellcheckand Bats jobs; updated script paths from.sh→.jsscripts-src/contents moved intosrc/scripts/so every TypeScript source lives undersrc/.sh→.js, shell sourcing →nodeinvocation with JSON pipe extraction for env discoveryvi.spyOn(os, 'homedir')replaced withprocess.env.HOME/USERPROFILEoverrides for more reliable cross-platform homedir mockingSummary by CodeRabbit
New Features
Documentation
Chores / Tests