Sync skills - #9
Open
mostafamm44 wants to merge 31 commits into
Open
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
Contributor
📝 WalkthroughWalkthroughThe pull request replaces shell bootstrap scripts with a TypeScript CLI. It adds interactive initialization, repository configuration, manifest processing, pull synchronization, GitHub-based publishing, generated agent configurations, symlink handling, caching, and project documentation. ChangesCodewiser CLI migration
Estimated code review effort: 5 (Critical) | ~120 minutes Mergeability Score: 🔴 Critical · up to This PR replaces the existing scripts with a new CLI, but the current implementation can write manifest-controlled paths outside the target project, overwrite user files, complete initialization with missing or stale content, and fail on supported publish or Node.js paths. These are concrete security, data-integrity, and availability risks, so the PR is not merge-ready and should be blocked 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 19
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (4)
.gitignore-15-16 (1)
15-16: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the log ignore patterns.
_.logmatches only a file named_.log. The report pattern also requires literal underscores and exactly one digit in each segment. These rules will not ignore ordinary log files or standard diagnostic reports.Proposed fix
-_.log -report.[0-9]_.[0-9]_.[0-9]_.[0-9]_.json +*.log +report.[0-9]*.[0-9]*.[0-9]*.[0-9]*.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 @.gitignore around lines 15 - 16, Update the .gitignore patterns for log and diagnostic report files: replace the literal _.log rule with a wildcard pattern matching ordinary log filenames, and change the report pattern to use wildcard separators and digit sequences rather than literal underscores and single-digit segments.src/utils/sync-files.ts-140-149 (1)
140-149: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFailed downloads are reported as user choices.
syncBatchignores afalseresult fromdownload. A path the caller accepted but that failed to download ends up inkeptUpdatesorconflicts.src/commands/pull.tsthen prints "you kept your local copy", which hides the network failure. Track the failures separately and surface them.🤖 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 140 - 149, Update syncBatch and its callers to track paths where download returns false separately from successfully downloaded paths; ensure failed accepted downloads are removed from keptUpdates/conflicts classification and surfaced as download failures instead of being reported as “you kept your local copy.”src/commands/sync.ts-5-11 (1)
5-11: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winStop after a failed pull.
pullreports failure by settingprocess.exitCode = 1and returning; it does not throw.syncthen runspublish, which repeats the same "No synced codewiser project found here." error, or publishes against state that was never synced. Check the exit code between the two steps.♻️ Proposed fix
export async function sync(dir: string = process.cwd()): Promise<void> { 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 to inspect process.exitCode immediately after await pull(dir) and return without calling publish when the pull reports failure. Preserve the existing step headers and publish flow for successful pulls, and ensure the check reflects pull’s non-throwing failure behavior.README.md-137-137 (1)
137-137: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRefer to the CLI instead of a removed setup script.
The CLI creates the agent symlinks. This line tells contributors that a setup script creates them. Replace “The setup script” with “The CLI”.
🤖 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` at line 137, Update the skill setup instructions in README.md to state that the CLI symlinks the skills directory into each agent’s private config, replacing the outdated “setup script” reference while preserving the rest of the guidance.
🧹 Nitpick comments (2)
src/commands/publish.ts (1)
448-456: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated remote-content fetch in
src/commands/publish.tsandsrc/commands/pull.ts. Both commands implement the same raw-content fetch with the same timeout and the same null-on-failure contract. The shared root cause is a missing helper insrc/utils/remote.ts.
src/commands/publish.ts#L448-L456: movefetchRemoteContentintosrc/utils/remote.tsand import it here.src/commands/pull.ts#L38-L46: delete the localcontentFetcherclosure and pass the imported helper instead.🤖 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 448 - 456, Move fetchRemoteContent from src/commands/publish.ts#L448-L456 into src/utils/remote.ts, preserving its timeout and null-on-failure behavior, then import and use it in publish.ts. In src/commands/pull.ts#L38-L46, remove the local contentFetcher closure and pass the shared imported helper instead.src/utils/sync-files.ts (1)
82-88: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid mutating the caller's
localFilesentry.Line 86 assigns
stored.sha256, which mutates the object owned by the caller (config.filesinsrc/commands/pull.ts). The baseline is already re-derived in the rebuild loop at lines 151-182, so this mutation is not needed. Use a local variable instead.♻️ Proposed refactor
let dirty = false; + let effectiveStoredHash = storedHash; if (storedHash) { dirty = currentHash !== null && currentHash !== storedHash; } else { // Legacy entry without a tracked hash: adopt the current hash as baseline. - if (stored) stored.sha256 = currentHash ?? undefined; + effectiveStoredHash = currentHash; dirty = 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/sync-files.ts` around lines 82 - 88, In the hash comparison flow, remove the mutation of the caller-owned entry via stored.sha256 in the storedHash fallback. Keep the legacy-entry behavior by deriving the current hash into a local value if needed, while leaving the rebuild loop’s baseline handling unchanged.
🤖 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 `@package.json`:
- Around line 3-5: Update package.json so bin.codewiser targets a shipped,
executable JavaScript CLI rather than src/index.ts, and ensure the publish path
replaces Bun.write with a Node-compatible implementation or otherwise ships
Node-compatible output; update README.md lines 51-52 and 141-144 to retain npx
codewiser guidance only once npm execution works and remove or revise the
Node.js requirement accordingly.
In `@src/commands/init.ts`:
- Around line 236-250: Update the onUpdate handler in the syncFiles flow to
process every tracked remote file rather than skipping paths that do not end in
/SKILL.md, applying the existing version comparison and overwrite confirmation
consistently. If non-skill files are locally generated and should not be
synchronized, filter them out before they are added to remoteFiles instead of
silently omitting them here.
- Around line 272-276: Update the initialization flow after processing the
download outcome to check whether outcome.skippedNew is non-empty; report the
failed paths and return before writeConfig or successful completion. Keep the
existing downloaded-file and all-up-to-date messaging for outcomes without
skipped new files.
- Around line 37-42: Update the resolveRepo and resolveBranch calls in the
initialization flow to include existingConfig?.repo and existingConfig?.branch
immediately after the explicit CLI values, before local manifest, global config,
and defaults, preserving target project settings during reinitialization.
- Around line 228-252: Before the syncFiles call, validate every path in
remoteFiles and localFiles (including paths handled by the callbacks) to reject
absolute or empty manifest paths and any path whose resolved destination escapes
targetDir. Resolve each manifest path against targetDir using the platform path
utilities, compare it with the resolved targetDir boundary, and abort with an
appropriate error before invoking syncFiles.
In `@src/commands/publish.ts`:
- Around line 411-438: Update bumpManifestVersions to traverse
workflows[*].stages[*].files in the same order and shape supported by
flattenRemoteManifest, including both version lookup and fallback insertion
behavior. Preserve existing modes and top-level files handling, and report an
error when found remains zero after all manifest entries are processed.
- Around line 174-191: Update the failed-merge branch in publish around
mergeThreeWay: only overwrite the working file when result.merged contains
actual conflict markers, and create a backup of the original file before doing
so. For other non-ok merge failures, preserve the local file and report the
failure without claiming conflict markers were written; keep dirty-path handling
consistent with the resulting outcome.
- Around line 279-286: Update the staging loop in the publish command around
selected, src, and dest to create dest’s parent directory recursively before
calling cpSync. Preserve the existing force-copy and “Staged” logging behavior
for both existing and newly added skill paths.
- Around line 319-347: Update the fork fallback in the publish flow to run gh
repo fork from tmp without passing repo, using a fixed remote name such as fork
via --remote-name. Remove discovery of the first non-origin remote and push
directly to the known fork remote, preserving the existing failure handling and
head assignment.
In `@src/commands/pull.ts`:
- Around line 54-86: Update the selection callbacks around selectFilesToUpdate
and the pull command flow so EXIT propagates as an abort rather than becoming an
empty selection, while BACK retains its existing empty-selection behavior. Catch
the abort around syncFiles, print the cancellation message consistent with init,
and return before writeConfig.
In `@src/index.ts`:
- Around line 57-62: Update the CLI dispatch around pull, publish, and sync so
parsed --repo and --branch values are forwarded to each command instead of
relying only on process.cwd() and persisted configuration. Extend the
corresponding command interfaces and ensure pull, publish, and sync all receive
and apply these optional source overrides.
- Around line 69-70: Update the targetDir resolution in the CLI entrypoint to
resolve the selected directory relative to the current working directory only,
without adding "..". Ensure commands such as "codewiser my-project" from "/work"
pass "/work/my-project" to init.
In `@src/utils/cache.ts`:
- Around line 15-16: Update cachePath to validate path before calling join,
rejecting empty, "." or ".." segments, backslashes, and absolute paths; apply
the same canonical relative-path validation at the download-destination mapping
in sync-files.ts. Preserve valid manifest paths while preventing any filesystem
sink from escaping the selected cache or target directory.
In `@src/utils/download.ts`:
- Around line 4-16: Update download so directory creation and the existing
fetch/write flow are inside the try block, ensuring any filesystem or network
failure returns false per the boolean contract. In download, add a bounded
AbortSignal timeout to fetch so stalled requests terminate while preserving the
current success and failure results.
In `@src/utils/generate-configs.ts`:
- Line 20: Preserve existing agent configuration across all writers in
src/utils/generate-configs.ts: at lines 20-20 merge managed OpenCode fields or
prompt and back up before replacement; at lines 27-37 retain user-managed
CLAUDE.md content outside the marked Codewiser block; at lines 58-58 merge
managed Antigravity workflow entries or prompt and back up before replacement;
and at lines 80-80 merge managed Kilo fields or prompt and back up before
replacement.
In `@src/utils/prompts.ts`:
- Around line 115-124: Validate the version in enterNewVersion before returning
it, rejecting values outside the supported grammar. Add and reuse a shared
validated version parser in src/utils/manifest.ts#L1-L11 and
src/utils/remote.ts#L54-L65, replacing direct Number conversion so comparison
and synchronization use identical ordering rules; update all three sites
accordingly.
In `@src/utils/remote.ts`:
- Around line 30-34: Update the mode selection logic around modeName and
modeEntries so an explicitly provided but missing mode is treated as a stale
configuration: report the missing configured mode and stop synchronization
without collecting files. Only collect all mode files when modeName is omitted,
and preserve the existing named-mode collection for valid entries.
- Around line 3-8: Update fetchManifest to validate the decoded JSON before
returning it: accept only non-null objects that are not arrays, and return null
for null, arrays, and scalar values. Remove the unconditional Record cast from
the return path while preserving the existing response-status handling.
In `@src/utils/sync-files.ts`:
- Around line 95-103: Update the sync flow around contentFetcher and the
classification loop to prefetch upstream content for tracked files through a
bounded-concurrency pool, then reuse the results instead of issuing sequential
requests. Record paths in unverifiedContentCandidates whenever sha256File(dest)
returns null and storedHash is absent, even when baselineHash is null, so they
are not classified as up to date.
---
Minor comments:
In @.gitignore:
- Around line 15-16: Update the .gitignore patterns for log and diagnostic
report files: replace the literal _.log rule with a wildcard pattern matching
ordinary log filenames, and change the report pattern to use wildcard separators
and digit sequences rather than literal underscores and single-digit segments.
In `@README.md`:
- Line 137: Update the skill setup instructions in README.md to state that the
CLI symlinks the skills directory into each agent’s private config, replacing
the outdated “setup script” reference while preserving the rest of the guidance.
In `@src/commands/sync.ts`:
- Around line 5-11: Update sync to inspect process.exitCode immediately after
await pull(dir) and return without calling publish when the pull reports
failure. Preserve the existing step headers and publish flow for successful
pulls, and ensure the check reflects pull’s non-throwing failure behavior.
In `@src/utils/sync-files.ts`:
- Around line 140-149: Update syncBatch and its callers to track paths where
download returns false separately from successfully downloaded paths; ensure
failed accepted downloads are removed from keptUpdates/conflicts classification
and surfaced as download failures instead of being reported as “you kept your
local copy.”
---
Nitpick comments:
In `@src/commands/publish.ts`:
- Around line 448-456: Move fetchRemoteContent from
src/commands/publish.ts#L448-L456 into src/utils/remote.ts, preserving its
timeout and null-on-failure behavior, then import and use it in publish.ts. In
src/commands/pull.ts#L38-L46, remove the local contentFetcher closure and pass
the shared imported helper instead.
In `@src/utils/sync-files.ts`:
- Around line 82-88: In the hash comparison flow, remove the mutation of the
caller-owned entry via stored.sha256 in the storedHash fallback. Keep the
legacy-entry behavior by deriving the current hash into a local value if needed,
while leaving the rebuild loop’s baseline handling unchanged.
🪄 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: 6e9b3a31-71b7-48ed-b479-21f20d5669b3
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
.gitignoreREADME.mdcodewiser.jsoncodewiser.ps1codewiser.shpackage.jsonsrc/commands/init.tssrc/commands/publish.tssrc/commands/pull.tssrc/commands/repo.tssrc/commands/sync.tssrc/index.tssrc/utils/cache.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.sh
- codewiser.ps1
Comment on lines
+3
to
+5
| "bin": { | ||
| "codewiser": "src/index.ts" | ||
| }, |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
jq '{bin, scripts}' package.json
sed -n '1,3p' src/index.ts
rg -n --glob '*.ts' '\bBun\.' srcRepository: yallma3/codewiser
Length of output: 436
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- package.json ---'
cat -n package.json
printf '%s\n' '--- src/index.ts ---'
cat -n src/index.ts
printf '%s\n' '--- Bun references ---'
rg -n --glob '*.ts' --glob '!node_modules/**' '\bBun\b|from "bun"|from '\''bun'\''' .
printf '%s\n' '--- TypeScript and package metadata ---'
find . -maxdepth 2 -type f \( -name 'tsconfig*.json' -o -name '.npmignore' -o -name 'bun.lockb' -o -name 'bun.lock' -o -name 'package-lock.json' -o -name 'npm-shrinkwrap.json' \) -print
printf '%s\n' '--- README requirements and npm usage ---'
sed -n '45,58p;136,150p' README.md
printf '%s\n' '--- source imports and entrypoint syntax ---'
rg -n --glob '*.ts' '^(import|export)|(^|[^[:alnum:]_])interface |(^|[^[:alnum:]_])type |:\s*[A-Za-z_$][A-Za-z0-9_$<>\[\]| ]*([,)=;{])' src/index.ts src/commands src/utilsRepository: yallma3/codewiser
Length of output: 35311
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("src/index.ts")
text = p.read_text()
print("has_shebang:", text.startswith("#!"))
print("first_line:", repr(text.splitlines()[0] if text.splitlines() else ""))
print("has_typescript_markers:", any(x in text for x in ("interface ", "type ", " as ", ":", "<")))
PYRepository: yallma3/codewiser
Length of output: 242
Make the npm and Node.js CLI path executable.
bin.codewiser points to a TypeScript file without a shebang. Node.js 20.12+ cannot launch it. The publish path also calls the Bun-only Bun.write API.
- Point
bin.codewiserto a shipped JavaScript executable. - Keep the
npx codewiserexample only after npm execution works. - Remove the Node.js requirement or replace
Bun.writeand ship Node-compatible JavaScript.
📍 Affects 2 files
package.json#L3-L5(this comment)README.md#L51-L52README.md#L141-L144
🤖 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, Update package.json so bin.codewiser
targets a shipped, executable JavaScript CLI rather than src/index.ts, and
ensure the publish path replaces Bun.write with a Node-compatible implementation
or otherwise ships Node-compatible output; update README.md lines 51-52 and
141-144 to retain npx codewiser guidance only once npm execution works and
remove or revise the Node.js requirement accordingly.
Comment on lines
+37
to
+42
| 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); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect source-resolution precedence and callers that pass a target directory.
ast-grep outline src/utils/config.ts --items all
rg -n -C 5 'function resolveRepo|function resolveBranch|resolveRepo\(|resolveBranch\(|readConfig\(|writeConfig\(' \
src/utils/config.ts src/commands src/index.tsRepository: yallma3/codewiser
Length of output: 12070
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect target-directory resolution, configuration writes, and tests/docs that define init precedence.
sed -n '1,70p' src/commands/init.ts
sed -n '245,305p' src/commands/init.ts
sed -n '45,125p' src/utils/config.ts
rg -n -C 4 'init|targetDir|existingConfig|repo.*branch|resolveRepo|resolveBranch' test tests README.md docs 2>/dev/null || trueRepository: yallma3/codewiser
Length of output: 13403
Preserve the target project's repository source on reinitialization.
When targetDir differs from the current directory, resolveRepo and resolveBranch ignore the target project's existing configuration. Add existingConfig?.repo and existingConfig?.branch below explicit CLI options and before current-directory, global, and default sources. Otherwise, writeConfig(targetDir, ...) can overwrite the target project's source settings.
🤖 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 37 - 42, Update the resolveRepo and
resolveBranch calls in the initialization flow to include existingConfig?.repo
and existingConfig?.branch immediately after the explicit CLI values, before
local manifest, global config, and defaults, preserving target project settings
during reinitialization.
Comment on lines
+228
to
+252
| let outcome: SyncOutcome; | ||
| try { | ||
| outcome = await syncFiles({ | ||
| targetDir, | ||
| rawBase: RAW_BASE, | ||
| remoteFiles, | ||
| localFiles, | ||
| callbacks: { | ||
| async onNew(paths) { return paths; }, | ||
| async onUpdate(paths) { | ||
| const accepted: string[] = []; | ||
| for (const path of paths) { | ||
| if (!path.endsWith("/SKILL.md")) continue; | ||
| const remoteVer = remoteFiles[path] ?? "0.0.0"; | ||
| const localVer = localFiles[path]?.version ?? "0.0.0"; | ||
| if (!versionLt(localVer, remoteVer)) continue; | ||
| const overwrite = await confirmOverwrite(path, localVer, remoteVer); | ||
| if (overwrite === EXIT) throw abort; | ||
| if (overwrite) accepted.push(path); | ||
| } | ||
| return accepted; | ||
| }, | ||
| async onUpdateDirty() { return []; }, | ||
| }, | ||
| }); |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Trace manifest-path handling and find an existing shared validator.
rg -n -C 4 'remoteFiles|path\.split\("/"\)|join\(targetDir|resolve\(targetDir|relative\(targetDir' \
src/commands/init.ts src/utils/sync-files.ts src/utilsRepository: yallma3/codewiser
Length of output: 18328
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- sync-files.ts ---'
cat -n src/utils/sync-files.ts | sed -n '1,190p'
printf '%s\n' '--- init manifest and download flow ---'
cat -n src/commands/init.ts | sed -n '1,290p'
printf '%s\n' '--- manifest flattening and download helpers ---'
rg -n -C 6 'flattenModeFiles|flattenWorkflowFiles|manifest|download\(|writeFileSync|mkdirSync' srcRepository: yallma3/codewiser
Length of output: 50373
🏁 Script executed:
node - <<'JS'
const path = require('path');
const target = '/tmp/project';
for (const manifestPath of [
'../../.ssh/authorized_keys',
'../outside.txt',
'/etc/passwd',
'',
'a/../../outside.txt',
'nested/../file.txt',
'nested//file.txt',
'C:/outside.txt',
]) {
const destination = path.join(target, ...manifestPath.split('/'));
console.log(JSON.stringify({ manifestPath, destination, inside: destination === target || destination.startsWith(target + path.sep) }));
}
JSRepository: yallma3/codewiser
Length of output: 848
Reject manifest paths that escape targetDir.
syncFiles downloads every new manifest path to join(targetDir, ...path.split("/")). A path containing .. can resolve outside targetDir and overwrite user-writable files. Validate every manifest path before calling syncFiles. Reject absolute paths, empty paths, and destinations outside targetDir.
🤖 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 228 - 252, Before the syncFiles call,
validate every path in remoteFiles and localFiles (including paths handled by
the callbacks) to reject absolute or empty manifest paths and any path whose
resolved destination escapes targetDir. Resolve each manifest path against
targetDir using the platform path utilities, compare it with the resolved
targetDir boundary, and abort with an appropriate error before invoking
syncFiles.
Comment on lines
+236
to
+250
| async onNew(paths) { return paths; }, | ||
| async onUpdate(paths) { | ||
| const accepted: string[] = []; | ||
| for (const path of paths) { | ||
| if (!path.endsWith("/SKILL.md")) continue; | ||
| const remoteVer = remoteFiles[path] ?? "0.0.0"; | ||
| const localVer = localFiles[path]?.version ?? "0.0.0"; | ||
| if (!versionLt(localVer, remoteVer)) continue; | ||
| const overwrite = await confirmOverwrite(path, localVer, remoteVer); | ||
| if (overwrite === EXIT) throw abort; | ||
| if (overwrite) accepted.push(path); | ||
| } | ||
| return accepted; | ||
| }, | ||
| async onUpdateDirty() { return []; }, |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not silently skip existing non-skill updates.
Line 240 rejects every updated file that does not end in /SKILL.md. syncFiles keeps the old version and hash for those files. The command then continues without reporting outcome.keptUpdates.
Prompt for, or consistently apply, updates for all remote files that this command tracks. If non-skill files are intentionally generated locally, exclude them before they enter remoteFiles.
🤖 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 236 - 250, Update the onUpdate handler in
the syncFiles flow to process every tracked remote file rather than skipping
paths that do not end in /SKILL.md, applying the existing version comparison and
overwrite confirmation consistently. If non-skill files are locally generated
and should not be synchronized, filter them out before they are added to
remoteFiles instead of silently omitting them here.
Comment on lines
+272
to
+276
| if (outcome.downloadedNew.length + outcome.downloadedUpdates.length > 0) { | ||
| success(`${outcome.downloadedNew.length + outcome.downloadedUpdates.length} file(s) downloaded`); | ||
| } else { | ||
| info("All files up to date"); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fail initialization when an accepted new-file download fails.
onNew accepts every new path. Therefore, a non-empty outcome.skippedNew indicates a failed download in this command. The code still prints All files up to date, writes configuration, and completes initialization with missing files.
Stop before writeConfig and report the failed paths when outcome.skippedNew is non-empty.
🤖 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 272 - 276, Update the initialization flow
after processing the download outcome to check whether outcome.skippedNew is
non-empty; report the failed paths and return before writeConfig or successful
completion. Keep the existing downloaded-file and all-up-to-date messaging for
outcomes without skipped new files.
| config.instructions = [".agents/skills/**/SKILL.md", "AGENTS.md"]; | ||
| } | ||
|
|
||
| writeFileSync(dest, JSON.stringify(config, null, 2), "utf-8"); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Preserve existing agent configuration before generation.
Initialization can target an existing project. Each writer replaces the complete configuration file and can delete user-managed agent settings.
src/utils/generate-configs.ts#L20-L20: merge managed OpenCode fields, or prompt and back up before replacement.src/utils/generate-configs.ts#L27-L37: preserve user-managedCLAUDE.mdcontent outside a marked Codewiser block.src/utils/generate-configs.ts#L58-L58: merge managed Antigravity workflow entries, or prompt and back up before replacement.src/utils/generate-configs.ts#L80-L80: merge managed Kilo fields, or prompt and back up before replacement.
📍 Affects 1 file
src/utils/generate-configs.ts#L20-L20(this comment)src/utils/generate-configs.ts#L27-L37src/utils/generate-configs.ts#L58-L58src/utils/generate-configs.ts#L80-L80
🤖 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/generate-configs.ts` at line 20, Preserve existing agent
configuration across all writers in src/utils/generate-configs.ts: at lines
20-20 merge managed OpenCode fields or prompt and back up before replacement; at
lines 27-37 retain user-managed CLAUDE.md content outside the marked Codewiser
block; at lines 58-58 merge managed Antigravity workflow entries or prompt and
back up before replacement; and at lines 80-80 merge managed Kilo fields or
prompt and back up before replacement.
Comment on lines
+115
to
+124
| export async function enterNewVersion(path: string, current: string): Promise<string | typeof EXIT> { | ||
| const next = await textPrompt({ | ||
| message: `New version for ${path}:`, | ||
| placeholder: current, | ||
| initialValue: current, | ||
| }); | ||
| if (next === EXIT) return EXIT; | ||
| const trimmed = next.trim(); | ||
| if (!trimmed) return EXIT; | ||
| return trimmed; |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate versions before persistence and comparison. enterNewVersion accepts arbitrary text, but both comparison functions convert dot-separated components with Number. Values such as 1.0.x or 1.0.0-beta produce NaN, so neither side is newer and synchronization can miss updates.
src/utils/prompts.ts#L115-L124: reject versions outside the supported grammar before publishing metadata.src/utils/manifest.ts#L1-L11: use a shared validated parser instead of directNumberconversion.src/utils/remote.ts#L54-L65: use the same parser so pull and publish apply identical ordering rules.
📍 Affects 3 files
src/utils/prompts.ts#L115-L124(this comment)src/utils/manifest.ts#L1-L11src/utils/remote.ts#L54-L65
🤖 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/prompts.ts` around lines 115 - 124, Validate the version in
enterNewVersion before returning it, rejecting values outside the supported
grammar. Add and reuse a shared validated version parser in
src/utils/manifest.ts#L1-L11 and src/utils/remote.ts#L54-L65, replacing direct
Number conversion so comparison and synchronization use identical ordering
rules; update all three sites accordingly.
Comment on lines
+3
to
+8
| export async function fetchManifest(rawBase: string): Promise<Record<string, unknown> | null> { | ||
| const url = `${rawBase}/codewiser.json`; | ||
| try { | ||
| const res = await fetch(url, { signal: AbortSignal.timeout(MANIFEST_TIMEOUT_MS) }); | ||
| if (!res.ok) return null; | ||
| return (await res.json()) as Record<string, unknown>; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate the decoded manifest before returning it.
A valid JSON response can be null, an array, or a scalar. Line 8 casts those values to a record. pull then calls flattenRemoteManifest, which reads raw.modes and crashes for null.
Accept only a non-null, non-array object. Return null for every other JSON value.
🤖 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 3 - 8, Update fetchManifest to validate the
decoded JSON before returning it: accept only non-null objects that are not
arrays, and return null for null, arrays, and scalar values. Remove the
unconditional Record cast from the return path while preserving the existing
response-status handling.
Comment on lines
+30
to
+34
| if (modeName && modeEntries[modeName]) { | ||
| collect(modeEntries[modeName]?.files ?? {}); | ||
| } else { | ||
| for (const entry of Object.values(modeEntries)) collect(entry.files ?? {}); | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not fall back to every mode when the configured mode is missing.
If modeName is set but the upstream manifest removed or renamed that mode, this branch collects files from all modes. pull can then offer unrelated files for installation and update shared paths with another mode's version.
Distinguish an omitted modeName from a missing named mode. Report the stale configuration and stop synchronization when the named mode does not exist.
🤖 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 30 - 34, Update the mode selection logic
around modeName and modeEntries so an explicitly provided but missing mode is
treated as a stale configuration: report the missing configured mode and stop
synchronization without collecting files. Only collect all mode files when
modeName is omitted, and preserve the existing named-mode collection for valid
entries.
Comment on lines
+95
to
+103
| // Fetch the upstream file once and compare it against the tracked baseline, | ||
| // NOT the on-disk file. Comparing against the disk would misattribute a | ||
| // local-only edit as an upstream change (and vice versa). | ||
| let remoteHash: string | null = null; | ||
| if (contentFetcher && baselineHash) { | ||
| const remoteContent = await contentFetcher(path); | ||
| if (remoteContent !== null) remoteHash = sha256Text(remoteContent); | ||
| else unverifiedContentCandidates.push(path); | ||
| } |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Fetch upstream content concurrently, and record hash failures.
Two concerns in this block:
contentFetcherruns one HTTP request per tracked file, sequentially.src/commands/pull.tsalways passes a fetcher, so a manifest with N tracked files costs N serial round trips. Batch these with a bounded concurrency pool before the classification loop.- When
sha256File(dest)returnsnulland nostoredHashexists,baselineHashisnull, so no comparison runs and the path is not added tounverifiedContentCandidates. The file is then reported as up to date. That contradicts the documented purpose ofunverifiedContent.
♻️ Proposed fix for the unverified-content gap
let remoteHash: string | null = null;
if (contentFetcher && baselineHash) {
const remoteContent = await contentFetcher(path);
if (remoteContent !== null) remoteHash = sha256Text(remoteContent);
else unverifiedContentCandidates.push(path);
+ } else if (contentFetcher) {
+ // No baseline hash available (unreadable file): content comparison is impossible.
+ unverifiedContentCandidates.push(path);
}🤖 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 95 - 103, Update the sync flow around
contentFetcher and the classification loop to prefetch upstream content for
tracked files through a bounded-concurrency pool, then reuse the results instead
of issuing sequential requests. Record paths in unverifiedContentCandidates
whenever sha256File(dest) returns null and storedHash is absent, even when
baselineHash is null, so they are not classified as up to date.
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 by CodeRabbit
codewiserCLI for interactive project setup and configuration.