Sync skills - #8
Conversation
Encode the inner PowerShell command with paths embedded as single-quoted literals and pass it via -EncodedCommand with no user data on the command line. The previous version interpolated srcPath/destPath into the outer script, which could break on spaces/quotes and allowed injection.
skills: analyze v1.1.0
|
Warning Review limit reached
Next review available in: 68 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThe project replaces shell and PowerShell setup scripts with a Bun/Node CLI. It adds interactive initialization, repository configuration, pull, publish, and sync workflows with shared manifest, configuration, prompt, download, hashing, and symlink utilities. ChangesCodewiser CLI migration
Estimated code review effort: 5 (Critical) | ~90+ minutes Mergeability Score: 🔴 Critical · up to This change replaces the existing synchronization scripts with a new CLI, but the current behavior can write files outside the selected project, redirect users to a contributor-specific source, target the wrong directory, overwrite local configuration, and produce incomplete or misleading publish and download results. These concrete security, data-integrity, and runtime issues make the PR unsafe to merge until fixed. Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (7)
.gitignore-13-16 (1)
13-16: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the log-file ignore pattern.
Line 15 uses
_instead of*. Standard files such asdebug.logare not ignored and can be committed accidentally.Proposed fix
-_.log +*.log🤖 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 @.gitignore around lines 13 - 16, Correct the log-file pattern in the logs section of .gitignore by replacing the underscore wildcard usage with asterisks so standard files such as debug.log are ignored.README.md-137-139 (1)
137-139: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect the skill-installation description and link.
Line 137 still says that a setup script creates symlinks. The CLI now creates them. Line 139 links to
.agents/skills/shared/git-worktrees/SKILL.md, butcodewiser.jsonlists.agents/skills/git-worktrees/SKILL.md.Proposed fix
-... The setup script symlinks this directory into each agent's private config ... +... The CLI symlinks this directory into each agent's private config ... -Example: the [git-worktrees skill](.agents/skills/shared/git-worktrees/SKILL.md) ... +Example: the [git-worktrees skill](.agents/skills/git-worktrees/SKILL.md) ...🤖 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 `@README.md` around lines 137 - 139, Update the skill-installation description in README.md to state that the CLI creates the symlinks instead of the setup script, and correct the git-worktrees example link to use the path listed in codewiser.json: .agents/skills/git-worktrees/SKILL.md.src/commands/repo.ts-19-25 (1)
19-25: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winValidate the manifest top-level value before returning it.
Line 23 casts any valid JSON value to
Manifest. A primitive causesrepoSetto throw when it assignsmanifest.repo. An array makes the command report success, butJSON.stringifydrops the assignedrepoandbranchproperties.Proposed fix
export function readManifest(dir: string): Manifest | null { const p = getManifestPath(dir); if (!existsSync(p)) return null; try { - return JSON.parse(readFileSync(p, "utf-8")) as Manifest; + const parsed: unknown = JSON.parse(readFileSync(p, "utf-8")); + if (typeof parsed !== "object" || parsed === null || Array.isArray(parsed)) { + return null; + } + return parsed as Manifest; } catch { return null; } }🤖 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 `@src/commands/repo.ts` around lines 19 - 25, Update readManifest to validate the parsed JSON top-level value before casting and returning it: accept only a non-null, non-array object as a Manifest, and return null for primitives, arrays, or invalid JSON so repoSet receives a usable manifest.src/commands/pull.ts-52-82 (1)
52-82: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winTreat
EXITas an abort instead of an empty selection.Each callback maps both
BACKandEXITto[]. The sync then continues, prints results, and rewritescodewiser.jsonat lines 112-117. A user who chooses exit still gets a config write.publishinsrc/commands/publish.ts(lines 112-115) aborts withprocess.exitCode = 1for the same state, so the two commands behave differently.Propagate a cancel flag from the callbacks and skip the report and the
writeConfigcall when the user exits.🤖 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 `@src/commands/pull.ts` around lines 52 - 82, Update the onNew, onUpdate, and onUpdateDirty callbacks to distinguish EXIT from BACK by propagating a cancellation state rather than converting both to an empty selection. In the pull command flow, detect that cancellation and abort before printing the report or calling writeConfig, matching publish’s process.exitCode = 1 behavior; preserve BACK as a normal empty selection. Apply the same fix in `@src/commands/pull.ts` around lines 23 - 31.src/utils/remote.ts-27-36 (1)
27-36: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winA missing named mode silently falls back to every mode.
Line 30 checks
modeName && modeEntries[modeName]. If the caller passes amodeNamethat the remote manifest no longer contains (renamed or removed mode), control moves to theelsebranch and collects the union of all modes.pullthen presents every other mode's skills as new or updated files for the project.Treat an unknown named mode as an explicit condition instead of falling through to the union.
🔧 Proposed fix
const modes = raw.modes; if (modes && typeof modes === "object") { const modeEntries = modes as Record<string, { files?: Record<string, unknown> }>; - if (modeName && modeEntries[modeName]) { - collect(modeEntries[modeName]?.files ?? {}); + if (modeName) { + const entry = modeEntries[modeName]; + if (entry) collect(entry.files ?? {}); } else { for (const entry of Object.values(modeEntries)) collect(entry.files ?? {}); } if (Object.keys(result).length > 0) return result; }If a requested mode is absent, the caller should surface that to the user rather than receive a different mode's file set.
🤖 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 `@src/utils/remote.ts` around lines 27 - 36, Update the mode selection logic in the remote manifest handling around modeEntries so a provided modeName that is absent is treated explicitly and surfaced to the caller, rather than entering the all-modes union branch. Preserve the existing named-mode collection for known modes and union behavior when no modeName is requested.src/utils/hash.ts-20-22 (1)
20-22: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse raw response bytes for remote hashing.
src/commands/pull.tspassesres.text()tosyncFiles, which hashes the decoded string withsha256Text.download()stores the response bytes withBun.write, andsha256File()hashes those bytes. A UTF-8 BOM is removed byResponse.text()but remains in the stored file, soremoteHashcannot equal the baseline hash for BOM-prefixed files. Fetch the response witharrayBuffer()and hash it withsha256Bytes.🤖 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 `@src/utils/hash.ts` around lines 20 - 22, Update the remote download flow in download() and its syncFiles call to read the response with arrayBuffer() and compute remoteHash using sha256Bytes, matching the raw bytes written by Bun.write; stop passing decoded res.text() content or sha256Text for remote hashing.src/utils/symlinks.ts-23-42 (1)
23-42: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winConfirm before replacing an existing symbolic link.
Lines 25 through 27 remove any existing symbolic link without checking its target or asking the user. A project can use
.claude/skillsor.cursor/skillsto link to a different shared skill directory. Initialization silently replaces that setup.If the existing link does not already point to
destPath, ask for confirmation before unlinking it.🤖 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 `@src/utils/symlinks.ts` around lines 23 - 42, The existing symbolic-link branch in the symlink initialization flow must compare the link target with destPath before unlinking. Preserve matching links without prompting; when the target differs, request confirmation using the existing confirm/isCancel handling, and only unlink after explicit approval.
🧹 Nitpick comments (5)
src/commands/publish.ts (1)
158-165: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueEncode the repository slug in the API path.
branchis encoded, butrepois interpolated raw into thegh apipath. A malformed value incodewiser.jsonor in the global config (for example one that contains..) changes the requested API route. Validate theowner/reposhape once afterresolveRepo, or encode both segments.🤖 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 `@src/commands/publish.ts` around lines 158 - 165, Update the repository API path in the syncBranchOnRemote check to safely handle the resolved owner/repo slug: validate the owner/repo shape once after resolveRepo, or split the slug and encode each segment before interpolation. Preserve the existing branch lookup, fallback to defaultBranch, and warning behavior.src/commands/sync.ts (1)
5-11: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStop after a failed pull.
pullsetsprocess.exitCode = 1when no synced project exists, andpublishthen prints the same two error lines. Check the exit code afterpulland return early.♻️ Proposed refactor
stepHeader(1, "Sync From Team (pull)"); await pull(dir); + if (process.exitCode) return; stepHeader(2, "Publish Local Changes (pull request)"); await publish(dir);🤖 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 `@src/commands/sync.ts` around lines 5 - 11, Update sync after await pull(dir) to check process.exitCode and return immediately when the pull fails, preventing publish(dir) from running and duplicating its error output. Preserve the existing publish flow when pull succeeds.src/utils/remote.ts (1)
19-21: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winGuard against non-semver version values.
If a manifest entry is an object whose
versionis not a string (for exampleundefinedornull),String(...)produces"undefined"or"null".versionGtthen parsesNaNand every comparison returnsfalse, so the bad value can be stored and later compared as if it were a version. The nested conditional is also hard to read.Extract a small resolver and fall back to
"0.0.0"when the value is not a string.♻️ Proposed refactor
const collect = (files: Record<string, unknown>): void => { for (const [path, val] of Object.entries(files)) { - const version = typeof val === "string" ? val : (val && typeof val === "object" && "version" in (val as Record<string, unknown>)) - ? String((val as Record<string, string>).version) - : "0.0.0"; + const version = resolveVersion(val); const prev = result[path]; if (!prev || versionGt(version, prev)) result[path] = version; } };Add the helper:
function resolveVersion(val: unknown): string { if (typeof val === "string") return val; if (val && typeof val === "object") { const v = (val as { version?: unknown }).version; if (typeof v === "string") return v; if (typeof v === "number") return String(v); } return "0.0.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 `@src/utils/remote.ts` around lines 19 - 21, Replace the nested version conditional with a small resolveVersion helper that accepts unknown values, returns string values directly, converts numeric object.version values to strings, and falls back to "0.0.0" for all other cases. Update the manifest version resolution to use resolveVersion, ensuring undefined, null, and other non-semver values are not stringified.src/utils/sync-files.ts (2)
135-166: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse sets for candidate membership, and copy entries instead of sharing references.
Two clarity items in this block.
resyncedCandidates.includes(path),keptDirtyCandidates.includes(path), andnewCandidates.includes(path)run a linear scan for every remote path. Sets make the intent explicit and remove the quadratic scan as manifests grow.files = { ...localFiles }is a shallow copy, sofiles[path]andlocalFiles[path]reference the same object. Line 81 mutatesstored.sha256in place, which also changes the entry visible throughfiles. The behaviour is intended today, but the aliasing is implicit and easy to break later.Build the membership sets once above this loop and assign fresh entry objects.
🤖 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 `@src/utils/sync-files.ts` around lines 135 - 166, Build Sets from resyncedCandidates, keptDirtyCandidates, and newCandidates before the remoteFiles loop, then replace each includes check with Set.has. Ensure files entries are copied into fresh objects rather than sharing localFiles entry references, including the initial files construction and any assignments derived from stored.
88-96: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftContent verification fetches files one at a time.
The loop awaits
contentFetcher(path)for every tracked file before classification finishes. Each call is a separate HTTP request to the raw host, and the requests run strictly in sequence.syncBatchat Line 124 downloads sequentially too. Total time grows linearly with manifest size and with round-trip latency.Collect the paths that need verification first, then resolve the hashes with bounded concurrency, and classify afterwards.
🤖 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 `@src/utils/sync-files.ts` around lines 88 - 96, Refactor the verification flow around the per-file `contentFetcher(path)` call to collect all paths requiring remote verification before classification, then fetch and hash them with bounded concurrency. Store each resolved hash or unverified result by path, and have the later classification logic reuse those results; also update `syncBatch` to avoid sequential downloads while preserving existing error and candidate handling. Apply the same fix in `@src/commands/pull.ts` around lines 35 - 43.
🤖 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 `@codewiser.json`:
- Around line 146-148: Remove the contributor-specific repo and branch overrides
from the manifest so resolveRepo and resolveBranch fall back to the documented
canonical source yallma3/codewiser on main; do not leave the sync target
pointing to mostafamm44/codewiser or sync/codewiser.json.
In `@package.json`:
- Around line 3-5: Add the Bun shebang as the first line of src/index.ts before
its imports, declare Bun as the intended runtime via the package.json engines
field, and ensure the package files configuration includes src/ so the bin
target is published.
In `@src/commands/init.ts`:
- Around line 285-290: Update the initialization flow around
generateOpenCodeConfig, generateClaudeMD, generateAntigravityConfig,
generateKiloConfig, and addExecutionProtocolToAgentsMD to check each destination
for an existing file before generation and prompt for explicit overwrite
consent. Only invoke generation for approved files, preserving any configuration
the user declines to replace.
- Around line 36-43: Update the repository and branch resolution in the init
command to use existingConfig?.repo and existingConfig?.branch as the target
project’s existing values, while retaining cliRepo and cliBranch as the
highest-precedence inputs. Apply the same target-based sources to
describeRepoSource and describeBranchSource so logging matches the resolved
settings; leave process.cwd()-based manifest and global configuration handling
unchanged.
- Around line 167-170: Update the "files" case in the initialization flow to
preserve object-form file versions by using extractVersion(val) for each entry
instead of defaulting non-string values to "0.0.0". Keep direct string versions
unchanged.
In `@src/commands/publish.ts`:
- Around line 187-194: Update the publish loop over selected paths to create
each destination file’s parent directory before cpSync, preventing ENOENT for
new nested skills. When a source path is missing, report the skipped path and
remove it from the publish set so later manifest and config updates do not
include it.
Apply the same fix in `@src/utils/remote.ts` around lines 54 - 65.
- Around line 327-345: Update the fallback after the found check to add each
path/version from versions to the top-level manifest.files map when no valid
modes object exists, preserving the existing modes.*.files updates for manifests
with modes. Reuse the files validation and version lookup used around bumpFiles
and ensure new entries are written to the manifest’s top-level files record.
- Around line 228-255: Update the fork fallback in the publish flow to run gh
repo fork from the temporary clone directory tmp, removing the repo argument and
specifying --remote-name codewiser-fork alongside --remote. Keep the subsequent
remote discovery and fork push behavior unchanged.
In `@src/index.ts`:
- Around line 69-70: Update the targetDir resolution in the CLI flow to resolve
first relative to process.cwd() without adding the parent-directory segment.
Preserve the existing init(targetDir, cli.flags.repo, cli.flags.branch)
invocation so project paths like “my-project” and “.” operate from the current
directory.
In `@src/utils/download.ts`:
- Around line 10-17: Update the download function around fetch and Bun.write to
use an abort timeout consistent with the existing remote request pattern, write
the response to a temporary path rather than dest, and rename it into place only
after the write succeeds. Ensure any failed or aborted download removes the
temporary file while leaving the destination unchanged, and continue returning
false on failure.
In `@src/utils/manifest.ts`:
- Around line 1-12: The versionLt function must handle SemVer prerelease
identifiers instead of converting them with Number and producing NaN. Replace
its numeric-only comparison with a SemVer-compliant comparator that orders
prerelease versions before their corresponding stable versions, or validate and
reject unsupported formats before syncFiles uses the result.
In `@src/utils/remote.ts`:
- Around line 54-65: In src/utils/remote.ts lines 54-65, remove the duplicate
versionGt helper and reuse a single exported comparison helper from
src/utils/manifest.ts that rejects or normalizes non-numeric segments; in
src/utils/remote.ts lines 19-21, resolve manifest versions only when the value
is a string or number, otherwise use "0.0.0"; in src/utils/prompts.ts lines
115-125, validate the entered version as semver and return it only when it is
greater than current.
In `@src/utils/sync-files.ts`:
- Around line 64-69: Validate each remote manifest path once before
classification, ensuring its resolved destination remains inside targetDir; skip
rejected entries and report them through SyncOutcome. Reuse the validated
safeDest for the download destination and hash-baseline lookup instead of
rebuilding paths from the raw key, including the flows around remoteFiles
classification and the download/hash logic.
- Around line 168-173: Update syncBatch and SyncOutcome to track paths accepted
for download whose downloads return false, expose them as failedDownloads, and
exclude them from keptUpdates and conflicts. In pull, warn when failedDownloads
is non-empty and set a non-zero exit code while preserving existing reporting
for actual user choices.
---
Minor comments:
In @.gitignore:
- Around line 13-16: Correct the log-file pattern in the logs section of
.gitignore by replacing the underscore wildcard usage with asterisks so standard
files such as debug.log are ignored.
In `@README.md`:
- Around line 137-139: Update the skill-installation description in README.md to
state that the CLI creates the symlinks instead of the setup script, and correct
the git-worktrees example link to use the path listed in codewiser.json:
.agents/skills/git-worktrees/SKILL.md.
In `@src/commands/pull.ts`:
- Around line 52-82: Update the onNew, onUpdate, and onUpdateDirty callbacks to
distinguish EXIT from BACK by propagating a cancellation state rather than
converting both to an empty selection. In the pull command flow, detect that
cancellation and abort before printing the report or calling writeConfig,
matching publish’s process.exitCode = 1 behavior; preserve BACK as a normal
empty selection.
Apply the same fix in `@src/commands/pull.ts` around lines 23 - 31.
In `@src/commands/repo.ts`:
- Around line 19-25: Update readManifest to validate the parsed JSON top-level
value before casting and returning it: accept only a non-null, non-array object
as a Manifest, and return null for primitives, arrays, or invalid JSON so
repoSet receives a usable manifest.
In `@src/utils/hash.ts`:
- Around line 20-22: Update the remote download flow in download() and its
syncFiles call to read the response with arrayBuffer() and compute remoteHash
using sha256Bytes, matching the raw bytes written by Bun.write; stop passing
decoded res.text() content or sha256Text for remote hashing.
In `@src/utils/remote.ts`:
- Around line 27-36: Update the mode selection logic in the remote manifest
handling around modeEntries so a provided modeName that is absent is treated
explicitly and surfaced to the caller, rather than entering the all-modes union
branch. Preserve the existing named-mode collection for known modes and union
behavior when no modeName is requested.
In `@src/utils/symlinks.ts`:
- Around line 23-42: The existing symbolic-link branch in the symlink
initialization flow must compare the link target with destPath before unlinking.
Preserve matching links without prompting; when the target differs, request
confirmation using the existing confirm/isCancel handling, and only unlink after
explicit approval.
---
Nitpick comments:
In `@src/commands/publish.ts`:
- Around line 158-165: Update the repository API path in the syncBranchOnRemote
check to safely handle the resolved owner/repo slug: validate the owner/repo
shape once after resolveRepo, or split the slug and encode each segment before
interpolation. Preserve the existing branch lookup, fallback to defaultBranch,
and warning behavior.
In `@src/commands/sync.ts`:
- Around line 5-11: Update sync after await pull(dir) to check process.exitCode
and return immediately when the pull fails, preventing publish(dir) from running
and duplicating its error output. Preserve the existing publish flow when pull
succeeds.
In `@src/utils/remote.ts`:
- Around line 19-21: Replace the nested version conditional with a small
resolveVersion helper that accepts unknown values, returns string values
directly, converts numeric object.version values to strings, and falls back to
"0.0.0" for all other cases. Update the manifest version resolution to use
resolveVersion, ensuring undefined, null, and other non-semver values are not
stringified.
In `@src/utils/sync-files.ts`:
- Around line 135-166: Build Sets from resyncedCandidates, keptDirtyCandidates,
and newCandidates before the remoteFiles loop, then replace each includes check
with Set.has. Ensure files entries are copied into fresh objects rather than
sharing localFiles entry references, including the initial files construction
and any assignments derived from stored.
- Around line 88-96: Refactor the verification flow around the per-file
`contentFetcher(path)` call to collect all paths requiring remote verification
before classification, then fetch and hash them with bounded concurrency. Store
each resolved hash or unverified result by path, and have the later
classification logic reuse those results; also update `syncBatch` to avoid
sequential downloads while preserving existing error and candidate handling.
Apply the same fix in `@src/commands/pull.ts` around lines 35 - 43.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8b0e2115-b5c4-4c44-b21c-28403baf44ae
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
.gitignoreREADME.mdcodewiser.jsoncodewiser.ps1codewiser.shpackage.jsonsrc/commands/init.tssrc/commands/publish.tssrc/commands/pull.tssrc/commands/repo.tssrc/commands/sync.tssrc/index.tssrc/utils/config.tssrc/utils/download.tssrc/utils/generate-configs.tssrc/utils/hash.tssrc/utils/manifest.tssrc/utils/prompts.tssrc/utils/remote.tssrc/utils/symlinks.tssrc/utils/sync-files.tssrc/utils/ui.tstsconfig.json
💤 Files with no reviewable changes (2)
- codewiser.ps1
- codewiser.sh
| }, | ||
| "repo": "mostafamm44/codewiser", | ||
| "branch": "sync/codewiser.json" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not ship a contributor-specific synchronization override.
Lines 147-148 make this manifest resolve to mostafamm44/codewiser @ sync/codewiser.json. resolveRepo and resolveBranch give project-manifest values precedence over the bundled source. This redirects sync operations away from the documented yallma3/codewiser @ main source after merge.
Remove these overrides, or set them to the intended canonical release source.
Proposed fix
- },
- "repo": "mostafamm44/codewiser",
- "branch": "sync/codewiser.json"
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| }, | |
| "repo": "mostafamm44/codewiser", | |
| "branch": "sync/codewiser.json" | |
| } |
🤖 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 `@codewiser.json` around lines 146 - 148, Remove the contributor-specific repo
and branch overrides from the manifest so resolveRepo and resolveBranch fall
back to the documented canonical source yallma3/codewiser on main; do not leave
the sync target pointing to mostafamm44/codewiser or sync/codewiser.json.
| "bin": { | ||
| "codewiser": "src/index.ts" | ||
| }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add a shebang to the bin entry and declare the runtime.
The bin field points at a TypeScript file. The provided src/index.ts starts with import meow from "meow";, so it has no interpreter line. When a package manager links this bin, the shim relies on the shebang to select the interpreter. Without #!/usr/bin/env bun, codewiser fails on install for consumers. Node also cannot run .ts directly on older versions.
Add the shebang to src/index.ts and declare the intended runtime with engines. Consider also adding a files field so src/ ships in the published tarball.
🔧 Proposed changes
Add to the top of src/index.ts:
#!/usr/bin/env bun
import meow from "meow"; "type": "module",
"version": "0.1.2",
+ "files": ["src"],
+ "engines": {
+ "bun": ">=1.2.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 `@package.json` around lines 3 - 5, Add the Bun shebang as the first line of
src/index.ts before its imports, declare Bun as the intended runtime via the
package.json engines field, and ensure the package files configuration includes
src/ so the bin target is published.
| const existingConfig = readConfig(targetDir); | ||
| const localManifest = readManifest(process.cwd()); | ||
| const globalConfig = readGlobalConfig(); | ||
| const repo = resolveRepo(process.cwd(), cliRepo, localManifest?.repo, globalConfig?.repo); | ||
| const branch = resolveBranch(process.cwd(), cliBranch, localManifest?.branch, globalConfig?.branch); | ||
| const RAW_BASE = buildRawBase(repo, branch); | ||
| info(`Repo: ${repo} (branch: ${branch})`); | ||
| info(` from ${describeRepoSource(process.cwd(), cliRepo, localManifest?.repo, globalConfig?.repo)} / ${describeBranchSource(process.cwd(), cliBranch, localManifest?.branch, globalConfig?.branch)}`); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Resolve repository settings from the target project.
Line 36 reads the target project's existing manifest, but lines 37 through 43 resolve repo and branch from process.cwd(). If the user runs codewiser init /other/project from a configured directory, initialization can pull from the current directory's repository and replace /other/project/codewiser.json with that repository selection.
Use existingConfig?.repo and existingConfig?.branch for an existing target. Keep the CLI flags as the highest-precedence source.
🤖 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 `@src/commands/init.ts` around lines 36 - 43, Update the repository and branch
resolution in the init command to use existingConfig?.repo and
existingConfig?.branch as the target project’s existing values, while retaining
cliRepo and cliBranch as the highest-precedence inputs. Apply the same
target-based sources to describeRepoSource and describeBranchSource so logging
matches the resolved settings; leave process.cwd()-based manifest and global
configuration handling unchanged.
| case "files": { | ||
| for (const [path, val] of Object.entries(format.files)) { | ||
| remoteFiles[path] = typeof val === "string" ? val : "0.0.0"; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve object-form file versions.
A merged codewiser.json can contain { "version": "...", "sha256": "..." } entries. These lines convert every such entry to "0.0.0". The written local manifest then has an incorrect baseline, so later pulls can report false updates.
Use extractVersion(val) here.
Proposed fix
import {
+ extractVersion,
versionLt,
flattenModeFiles,
flattenWorkflowFiles,
detectManifestFormat,
} from "../utils/manifest";
case "files": {
for (const [path, val] of Object.entries(format.files)) {
- remoteFiles[path] = typeof val === "string" ? val : "0.0.0";
+ remoteFiles[path] = extractVersion(val);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case "files": { | |
| for (const [path, val] of Object.entries(format.files)) { | |
| remoteFiles[path] = typeof val === "string" ? val : "0.0.0"; | |
| } | |
| case "files": { | |
| for (const [path, val] of Object.entries(format.files)) { | |
| remoteFiles[path] = extractVersion(val); | |
| } |
🤖 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 `@src/commands/init.ts` around lines 167 - 170, Update the "files" case in the
initialization flow to preserve object-form file versions by using
extractVersion(val) for each entry instead of defaulting non-string values to
"0.0.0". Keep direct string versions unchanged.
| stepHeader(5, "Generate Configs"); | ||
| generateOpenCodeConfig(targetDir, skillDirs, agents?.opencode ?? false); | ||
| generateClaudeMD(targetDir, agents?.claude ?? false); | ||
| generateAntigravityConfig(targetDir, agents?.antigravity ?? false); | ||
| generateKiloConfig(targetDir, skillDirs, agents?.kilo ?? false); | ||
| addExecutionProtocolToAgentsMD(targetDir, selectedMode); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not overwrite existing agent configuration without consent.
These calls overwrite opencode.json, CLAUDE.md, .antigravity/workflows.json, and .kilo/config.json. The earlier agent detection makes this reachable when initialization runs in an existing project. Local agent settings are lost without a prompt.
Before generation, detect existing destination files and require an explicit overwrite choice. Preserve files that the user declines to replace.
🤖 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 `@src/commands/init.ts` around lines 285 - 290, Update the initialization flow
around generateOpenCodeConfig, generateClaudeMD, generateAntigravityConfig,
generateKiloConfig, and addExecutionProtocolToAgentsMD to check each destination
for an existing file before generation and prompt for explicit overwrite
consent. Only invoke generation for approved files, preserving any configuration
the user declines to replace.
| try { | ||
| const res = await fetch(url); | ||
| if (!res.ok) return false; | ||
| await Bun.write(dest, res); | ||
| return true; | ||
| } catch { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add a timeout, and write atomically.
Two problems exist in this block.
fetch(url)has no abort signal.src/utils/remote.tsalready appliesAbortSignal.timeout(MANIFEST_TIMEOUT_MS)to the manifest request, but file downloads have no limit.syncFilescallsdownloadsequentially for every selected path, so one stalled connection hangs the CLI with no output and no way to recover other than interrupting the process.Bun.write(dest, res)streams directly to the final path. If the body errors part way through, thecatchreturnsfalse, but a truncated file stays on disk.syncFilesthen sees the file exist on the next run and classifies it as a local edit, because its hash no longer matches the tracked baseline. That corrupts the sync baseline.
Download with a timeout, write to a temporary path, then rename into place and remove the temporary file on failure.
🔧 Proposed fix
-import { existsSync, mkdirSync } from "fs";
-import { dirname } from "path";
+import { existsSync, mkdirSync, renameSync, rmSync } from "fs";
+import { dirname } from "path";
+
+const DOWNLOAD_TIMEOUT_MS = 30000;
export async function download(url: string, dest: string): Promise<boolean> {
const dir = dirname(dest);
if (!existsSync(dir)) {
mkdirSync(dir, { recursive: true });
}
-
+
+ const tmp = `${dest}.codewiser-tmp`;
try {
- const res = await fetch(url);
- if (!res.ok) return false;
- await Bun.write(dest, res);
+ const res = await fetch(url, { signal: AbortSignal.timeout(DOWNLOAD_TIMEOUT_MS) });
+ if (!res.ok) return false;
+ await Bun.write(tmp, res);
+ renameSync(tmp, dest);
return true;
} catch {
+ rmSync(tmp, { force: true });
return false;
}
}🤖 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 `@src/utils/download.ts` around lines 10 - 17, Update the download function
around fetch and Bun.write to use an abort timeout consistent with the existing
remote request pattern, write the response to a temporary path rather than dest,
and rename it into place only after the write succeeds. Ensure any failed or
aborted download removes the temporary file while leaving the destination
unchanged, and continue returning false on failure.
| export function versionLt(v1: string, v2: string): boolean { | ||
| const p1 = v1.split(".").map(Number); | ||
| const p2 = v2.split(".").map(Number); | ||
| const len = Math.max(p1.length, p2.length); | ||
| for (let i = 0; i < len; i++) { | ||
| const a = p1[i] ?? 0; | ||
| const b = p2[i] ?? 0; | ||
| if (a < b) return true; | ||
| if (a > b) return false; | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Handle SemVer prerelease versions correctly.
Line 2 converts identifiers such as 1.2.0-beta.1 to NaN. Lines 8 and 9 then return no ordering result. syncFiles can therefore skip an upstream prerelease update or retain an incorrect tracked version.
Use a SemVer-compliant comparison, or reject unsupported version formats before synchronization.
🤖 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 `@src/utils/manifest.ts` around lines 1 - 12, The versionLt function must
handle SemVer prerelease identifiers instead of converting them with Number and
producing NaN. Replace its numeric-only comparison with a SemVer-compliant
comparator that orders prerelease versions before their corresponding stable
versions, or validate and reject unsupported formats before syncFiles uses the
result.
| function versionGt(a: string, b: string): boolean { | ||
| const pa = a.split(".").map(Number); | ||
| const pb = b.split(".").map(Number); | ||
| const len = Math.max(pa.length, pb.length); | ||
| for (let i = 0; i < len; i++) { | ||
| const x = pa[i] ?? 0; | ||
| const y = pb[i] ?? 0; | ||
| if (x > y) return true; | ||
| if (x < y) return false; | ||
| } | ||
| return false; | ||
| } No newline at end of file |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Version strings have no single validated parser. versionGt here and versionLt in src/utils/manifest.ts both parse segments with Number and never reject a non-numeric value. Any malformed version produces NaN segments, every comparison returns false, and the affected skill silently stops appearing as an update. Manifest input and user input both reach these comparisons unvalidated.
src/utils/remote.ts#L54-L65: remove this duplicate and export one comparison helper fromsrc/utils/manifest.tsthat rejects or normalizes non-numeric segments.src/utils/remote.ts#L19-L21: resolve a manifest entry to a version only when the value is a string or number, and fall back to"0.0.0"otherwise.src/utils/prompts.ts#L115-L125: validate the entered value as semver and require it to be greater thancurrentbefore returning it.
📍 Affects 2 files
src/utils/remote.ts#L54-L65(this comment)src/utils/remote.ts#L19-L21src/utils/prompts.ts#L115-L125
🤖 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 `@src/utils/remote.ts` around lines 54 - 65, In src/utils/remote.ts lines
54-65, remove the duplicate versionGt helper and reuse a single exported
comparison helper from src/utils/manifest.ts that rejects or normalizes
non-numeric segments; in src/utils/remote.ts lines 19-21, resolve manifest
versions only when the value is a string or number, otherwise use "0.0.0"; in
src/utils/prompts.ts lines 115-125, validate the entered version as semver and
return it only when it is greater than current.
| for (const [path, remoteVer] of Object.entries(remoteFiles)) { | ||
| const dest = join(targetDir, ...path.split("/")); | ||
| if (!existsSync(dest)) { | ||
| newCandidates.push(path); | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
Reject manifest paths that escape the target directory.
remoteFiles keys come from the remote codewiser.json through flattenRemoteManifest. join normalizes .. segments, so a key such as ../../.ssh/authorized_keys resolves outside targetDir. download then creates the parent directory with mkdirSync(..., { recursive: true }) and writes the file. The same unvalidated path is reused at Line 126 for the download destination and at Line 137 for the hash baseline.
The repo and branch are user-configurable, so the manifest is not always trusted input. A hostile or compromised repo can write arbitrary files outside the project.
Validate each path once, before classification, and skip anything that does not resolve inside targetDir.
🔒️ Proposed fix
-import { existsSync } from "fs";
-import { join } from "path";
+import { existsSync } from "fs";
+import { join, relative, resolve, isAbsolute } from "path";+ const root = resolve(targetDir);
+ const safeDest = (path: string): string | null => {
+ const dest = resolve(root, ...path.split("/"));
+ const rel = relative(root, dest);
+ if (rel === "" || rel.startsWith("..") || isAbsolute(rel)) return null;
+ return dest;
+ };
+
for (const [path, remoteVer] of Object.entries(remoteFiles)) {
- const dest = join(targetDir, ...path.split("/"));
+ const dest = safeDest(path);
+ if (dest === null) continue; // Reject paths that escape targetDir.
if (!existsSync(dest)) {
newCandidates.push(path);
continue;
}Apply safeDest at Line 126 and Line 137 as well, and report rejected paths in SyncOutcome so the user sees that entries were skipped.
🤖 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 `@src/utils/sync-files.ts` around lines 64 - 69, Validate each remote manifest
path once before classification, ensuring its resolved destination remains
inside targetDir; skip rejected entries and report them through SyncOutcome.
Reuse the validated safeDest for the download destination and hash-baseline
lookup instead of rebuilding paths from the raw key, including the flows around
remoteFiles classification and the download/hash logic.
| const keptDirty = [...keptDirtyCandidates]; | ||
| const skippedNew = [...newCandidates.filter((p) => !downloadedNew.has(p))]; | ||
| // "downloadedUpdates" merges clean updates and overwritten dirty files; since a | ||
| // path can only ever be one kind of candidate, the filtering is unambiguous. | ||
| const keptUpdates = [...updateCandidates.filter((p) => !downloadedUpdates.has(p))]; | ||
| const conflicts = [...dirtyUpdateCandidates.filter((p) => !downloadedUpdates.has(p))]; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Failed downloads are reported as user choices.
syncBatch adds a path to the target set only when download returns true. A download failure therefore leaves the path out of downloadedUpdates, and these filters classify it as keptUpdates or conflicts. src/commands/pull.ts prints "Team updated upstream — you kept your local copy" and "Changed on both sides — you kept your version" for those buckets. The user sees a message about a decision they did not make, and the failure is never surfaced. The exit code stays 0.
Record the paths the caller accepted but that failed to download, and report them separately.
🔧 Proposed fix
const downloadedNew = new Set<string>();
const downloadedUpdates = new Set<string>();
+ const failed = new Set<string>();
const syncBatch = async (paths: string[], target: Set<string>): Promise<void> => {
for (const path of paths) {
const dest = join(targetDir, ...path.split("/"));
const ok = await download(`${rawBase}/${path}`, dest);
- if (ok) target.add(path);
+ if (ok) target.add(path);
+ else failed.add(path);
}
}; const keptDirty = [...keptDirtyCandidates];
- const skippedNew = [...newCandidates.filter((p) => !downloadedNew.has(p))];
+ const skippedNew = [...newCandidates.filter((p) => !downloadedNew.has(p) && !failed.has(p))];
// "downloadedUpdates" merges clean updates and overwritten dirty files; since a
// path can only ever be one kind of candidate, the filtering is unambiguous.
- const keptUpdates = [...updateCandidates.filter((p) => !downloadedUpdates.has(p))];
- const conflicts = [...dirtyUpdateCandidates.filter((p) => !downloadedUpdates.has(p))];
+ const keptUpdates = [...updateCandidates.filter((p) => !downloadedUpdates.has(p) && !failed.has(p))];
+ const conflicts = [...dirtyUpdateCandidates.filter((p) => !downloadedUpdates.has(p) && !failed.has(p))];
+ const failedDownloads = [...failed];Add failedDownloads: string[] to SyncOutcome, return it, and have pull warn on it and set a non-zero exit code.
🤖 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 `@src/utils/sync-files.ts` around lines 168 - 173, Update syncBatch and
SyncOutcome to track paths accepted for download whose downloads return false,
expose them as failedDownloads, and exclude them from keptUpdates and conflicts.
In pull, warn when failedDownloads is non-empty and set a non-zero exit code
while preserving existing reporting for actual user choices.
1 participant
Summary by CodeRabbit
New Features
Documentation
Chores