Launch Zaparoo CLI v2 - #2
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughChangesThe project changes from an MCP server to the Zaparoo CLI foundation
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant CredentialStore
participant ZaparooClient
participant Core
User->>CLI: invoke command
CLI->>CredentialStore: resolve credentials
CLI->>ZaparooClient: connect and request
ZaparooClient->>Core: WebSocket JSON-RPC
Core-->>ZaparooClient: response or notification
ZaparooClient-->>CLI: command result
CLI-->>User: human, JSON, or JSONL output
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (17)
src/cli/commands/online.test.ts-151-171 (1)
151-171: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRestore the stdout spy in
afterEach.Line 170 restores the spy only when the test reaches the end. If an assertion between line 166 and line 169 fails, the spy on
process.stdout.writestays active for the rest of the file and suppresses output. Addvi.restoreAllMocks()to the existingafterEachblock.💚 Proposed fix
afterEach(() => { vi.unstubAllEnvs(); + vi.restoreAllMocks(); for (const directory of directories) rmSync(directory, { recursive: true, force: true }); directories.length = 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/cli/commands/online.test.ts` around lines 151 - 171, Update the existing afterEach block in the online command tests to call vi.restoreAllMocks(), ensuring the process.stdout.write spy created in the watch test is restored even when assertions fail. Remove the test-local write.mockRestore() if appropriate to avoid redundant cleanup.src/online/client.test.ts-90-98 (1)
90-98: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert that the error contains
[REDACTED].rejects.toThrowcorrectly handles the rejectedOnlineApiError, but the current assertion only checks thatsuper-secretis absent. It also passes if the response body is dropped. Assert both[REDACTED]and the absence ofsuper-secreton the same error.🤖 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/online/client.test.ts` around lines 90 - 98, Update the “redacts keys from error bodies” test around OnlineClient.get so the rejected error assertion requires both the presence of “[REDACTED]” and the absence of “super-secret” on the same OnlineApiError, preserving the existing rejection check.skills/zaparoo-online/SKILL.md-43-43 (1)
43-43: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winUse the contract’s exact
403wording. Change “account is unavailable” to “account is suspended.” The contract defines403as a missing required scope or a suspended account. Keep the instruction not to request broader scope unless required.🤖 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 `@skills/zaparoo-online/SKILL.md` at line 43, Update the 403 description in the relevant instruction to say the key lacks the required scope or the account is suspended, replacing “account is unavailable.” Preserve the existing guidance not to request broader scope unless the task requires it.Source: Coding guidelines
src/crypto/pairing.ts-79-90 (1)
79-90: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winConstrain the server-supplied error message.
Line 83 accepts any string from an unauthenticated remote host and line 87 returns it as the error message.
pairCommandinsrc/cli/commands/pair.tspasses that message to CLI output. A hostile host can inject ANSI escape sequences or a very long string into the user's terminal.Bound the length and strip control characters before you use the server message.
🛡️ Proposed fix
const body = (await response.json()) as { error?: unknown }; - if (typeof body.error === 'string') serverMessage = body.error; + if (typeof body.error === 'string') { + // biome-ignore lint/suspicious/noControlCharactersInRegex: strip terminal control sequences + serverMessage = body.error.replace(/[\u0000-\u001f\u007f]/g, ' ').slice(0, 200); + }🤖 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/crypto/pairing.ts` around lines 79 - 90, Sanitize the serverMessage handling in pairingError before returning it: strip control characters, including ANSI escape sequences, and cap the resulting message to a reasonable maximum length. Apply the sanitized bounded value only when selecting the server-supplied message, while preserving the existing status-based and fallback error behavior.src/cli/commands/readers.ts-13-21 (1)
13-21: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winGate the NFC write behind an explicit confirmation flag.
readers writeoverwrites the contents of the physical tag. The command runs the write with no confirmation step. Add a required--force(or--yes) flag, or prompt when the session is interactive. Theforceflag is already inBOOLEAN_FLAGSinsrc/cli/args.ts.Based on learnings: "Ask for confirmation before launching or stopping media, sending input or NFC writes, changing mappings/settings/profiles, applying updates, clearing the inbox, or causing downtime".
🤖 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/cli/commands/readers.ts` around lines 13 - 21, Update the readers write case to require explicit confirmation before calling request for Methods.ReadersWrite: accept the existing force boolean flag (or an interactive confirmation for interactive sessions), and otherwise fail with a usage error. Preserve the current text validation and request arguments after confirmation succeeds.Source: Learnings
src/cli/commands/logs.ts-19-26 (1)
19-26: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate
--outputbefore the download request.The code requests
Methods.SettingsLogsDownloadfirst, then checks--output. If the user omits--output, the CLI opens a Core session, downloads the whole log archive, and then throws a usage error. The payload is discarded.src/cli/commands/admin.tslines 19-22 validate first. Use the same order here.🐛 Proposed fix
if (action === 'download') { + const output = flag(args.flags, 'output'); + if (!output) throw new CliError('logs download requires --output <path>', ExitCode.Usage); const response = await withClient(args.options, (client) => client.request<BinaryResponse>(Methods.SettingsLogsDownload), ); - const output = flag(args.flags, 'output'); - if (!output) throw new CliError('logs download requires --output <path>', ExitCode.Usage); return { data: writeBase64Output(response, output) }; }🤖 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/cli/commands/logs.ts` around lines 19 - 26, Move the `flag(args.flags, 'output')` lookup and missing-output `CliError` validation before the `withClient` call in the `action === 'download'` branch, then perform `Methods.SettingsLogsDownload` and pass the validated output to `writeBase64Output`. Match the validation order used by the admin command.src/cli/commands/screenshot.ts-9-15 (1)
9-15: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winValidate
--outputbefore you open the session.The handler requests the screenshot at line 10 and validates
--outputat line 14. If the user omits--output, the command still connects to Core, triggers a screenshot capture on the device, and then fails with a usage error. Move the check to the top of the function.As per coding guidelines: "Validate required command parameters and add colocated tests for non-trivial mapping."
🐛 Proposed fix
export async function screenshotCommand(args: ParsedArgs): Promise<CommandResult> { + const output = flag(args.flags, 'output'); + if (!output) throw new CliError('screenshot requires --output <path>', ExitCode.Usage); const response = await withClient(args.options, (client) => client.request<BinaryResponse>(Methods.Screenshot), ); - const output = flag(args.flags, 'output'); - if (!output) throw new CliError('screenshot requires --output <path>', ExitCode.Usage); return { data: writeBase64Output(response, output) }; }🤖 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/cli/commands/screenshot.ts` around lines 9 - 15, Move the required --output validation to the beginning of screenshotCommand, before withClient initiates the screenshot request; preserve the existing CliError and Usage exit behavior, then use the validated output value for writeBase64Output.Source: Coding guidelines
src/cli/commands/devices.ts-83-95 (1)
83-95: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
stateCommandresolves the device twice.Line 84 calls
resolveDevice(args.options).withClientcallsresolveDeviceagain at line 12 ofsrc/cli/commands/common.ts. If resolution falls back to mDNS discovery, the command waits for the discovery timeout twice and doubles the network work.Use
client.device.idinside the callback, asdevices pingdoes at line 39. Also replace the literal method names withMethods.*entries.♻️ Proposed change
export async function stateCommand(args: ParsedArgs): Promise<CommandResult> { - const device = await resolveDevice(args.options); const data = await withClient(args.options, async (client) => ({ - device: device.id, + device: client.device.id, version: client.info, readers: await client.request(Methods.Readers).catch((error) => ({ error: String(error) })), activeMedia: await client .request(Methods.MediaActive) .catch((error) => ({ error: String(error) })), tokenHistory: await client .request(Methods.TokensHistory) .catch((error) => ({ error: String(error) })), })); - return { data, human: `State snapshot for ${device.id}` }; + return { data, human: `State snapshot for ${data.device}` }; }🤖 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/cli/commands/devices.ts` around lines 83 - 95, Update stateCommand to avoid resolving the device before withClient; use client.device.id inside its callback for the data.device value, matching the existing ping command pattern. Replace the literal 'readers', 'media.active', and 'tokens.history' request names with their corresponding Methods.* entries.src/client/config.ts-102-112 (1)
102-112: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReport a malformed config file with the file path.
Line 105 parses the config file without error handling. A truncated or hand-edited
config.jsonmakes every CLI command fail with a rawSyntaxErrormessage.classifyErrorinsrc/cli/errors.tsmaps that toExitCode.Generaland the message contains no file path. Wrap the read and parse, and report the path.🔧 Proposed fix
export function loadCliConfig(configPath?: string): CliConfig { const path = configPath ?? defaultConfigPath(); - const fileConfig = existsSync(path) - ? (JSON.parse(readFileSync(path, 'utf8')) as Partial<CliConfig>) - : {}; + let fileConfig: Partial<CliConfig> = {}; + if (existsSync(path)) { + try { + fileConfig = JSON.parse(readFileSync(path, 'utf8')) as Partial<CliConfig>; + } catch (error) { + const reason = error instanceof Error ? error.message : String(error); + throw new Error(`Failed to read config file "${path}": ${reason}`); + } + }🤖 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/client/config.ts` around lines 102 - 112, Update loadCliConfig around the JSON.parse/readFileSync call to catch malformed config-file errors and report them with the config path, while preserving the existing configuration fallback behavior. Use the existing classifyError/error-reporting flow from the CLI error handling rather than allowing a raw SyntaxError to escape.src/client/trace.ts-60-68 (1)
60-68: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winSkip unparsable trace lines.
readLastthrows if any selected line is not complete JSON. An interrupted append or a concurrenttruncateSyncat line 55 can leave a partial line. Thelogs tracecommand insrc/cli/commands/logs.tsthen fails instead of returning the readable entries. Skip lines that fail to parse.🔧 Proposed fix
readLast(count: number): TraceEntry[] { if (!existsSync(this.path)) return []; - return readFileSync(this.path, 'utf8') - .trim() - .split('\n') - .filter(Boolean) - .slice(-count) - .map((line) => JSON.parse(line) as TraceEntry); + const entries: TraceEntry[] = []; + for (const line of readFileSync(this.path, 'utf8').trim().split('\n').filter(Boolean)) { + try { + entries.push(JSON.parse(line) as TraceEntry); + } catch { + // Skip a partially written trace line. + } + } + return entries.slice(-count); }🤖 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/client/trace.ts` around lines 60 - 68, Update TraceStore.readLast to parse selected lines defensively, skipping any line that throws during JSON.parse while preserving all successfully parsed TraceEntry values. Keep the existing file-reading, filtering, and count behavior unchanged so logs trace returns readable entries despite partial or invalid lines.src/cli/errors.ts-54-65 (1)
54-65: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAnchor the fallback patterns to whole words.
The fallback patterns match substrings.
/pair/iat line 56 also matches "repair" and "unpaired data"./connect/iat line 62 also matches "disconnected" and "reconnecting". An unrelated error then returnsExitCode.EncryptionRequiredorExitCode.Connection. Scripts consume these exit codes as a contract. Add word boundaries.🔧 Proposed fix
- if (/encryption required|not paired|pair/i.test(message)) { + if (/encryption required|not paired|\bpair(ing)?\b/i.test(message)) { return new CliError(message, ExitCode.EncryptionRequired); }🤖 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/cli/errors.ts` around lines 54 - 65, Update the encryption and connection fallback regexes in the error-classification logic to use word boundaries around standalone terms, especially “pair” and “connect,” so substrings such as “repair,” “unpaired,” “disconnected,” and “reconnecting” do not match. Preserve the existing classification order and all other patterns.src/cli/output.ts-10-15 (1)
10-15: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
--jsoncan printundefinedinstead of valid JSON.
JSON.stringify(undefined)returns the JavaScript valueundefined, not a string. Template interpolation then writes the textundefinedto stdout. Core methods that return no result produce this case, for example the pass-through results insrc/cli/commands/backup.tsLine 41 andsrc/cli/commands/tokens.tsLine 20. A machine consumer that parses stdout then fails.Normalize the value before serialization.
🔧 Proposed fix
export function printResult(result: CommandResult, options: GlobalOptions): void { if (options.json || !result.human) { const space = options.pretty === false ? 0 : 2; - process.stdout.write(`${JSON.stringify(result.data, null, space)}\n`); + process.stdout.write(`${JSON.stringify(result.data ?? null, null, space)}\n`); return; } process.stdout.write(`${result.human}\n`); }🤖 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/cli/output.ts` around lines 10 - 15, Update printResult to normalize result.data before JSON.stringify so undefined is converted to a valid JSON value, such as null. Preserve the existing pretty-print spacing, stdout output, and return behavior for all other result values.Source: Coding guidelines
src/cli/commands/ui.ts-23-30 (1)
23-30: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
ui respond --action selectaccepts a missing--choice-id.The handler validates the action name but not the choice. For the
selectaction,pickDefineddropschoiceIdwhen the flag is absent, and the request reaches Core without a selection. Validate the pairing locally and fail withExitCode.Usage.As per coding guidelines: "Validate required command parameters and add colocated tests for non-trivial mapping."
🔧 Proposed fix
if (!['dismiss', 'select', 'confirm'].includes(responseAction)) { throw new CliError('--action must be dismiss, select, or confirm', ExitCode.Usage); } + const choiceId = flag(args.flags, 'choice-id'); + if (responseAction === 'select' && !choiceId) { + throw new CliError('--action select requires --choice-id', ExitCode.Usage); + } const data = await withClient(args.options, (client) => client.request( Methods.UIRespond, - pickDefined({ id, action: responseAction, choiceId: flag(args.flags, 'choice-id') }), + pickDefined({ id, action: responseAction, choiceId }), ), );🤖 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/cli/commands/ui.ts` around lines 23 - 30, Update the UI response validation before the withClient request to require a non-missing choice-id whenever responseAction is “select”; throw CliError with ExitCode.Usage when that pairing is invalid, while preserving other action behavior. Add colocated tests covering select without choice-id and valid non-select actions.Source: Coding guidelines
src/cli/commands/watch.ts-27-32 (1)
27-32: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
entriesgrows without bound in JSONL mode.In JSONL mode each notification is written to stdout immediately, and Line 40 returns only
entries.length. The handler still retains every notification object in memory. A long watch on a busy device then holds notifications that are never read. Keep a counter for JSONL mode.🔧 Proposed fix
const entries: unknown[] = []; + let streamed = 0; client.on('notification', (method, params, deviceId) => { if (methods.size > 0 && !methods.has(method)) return; const entry = { timestamp: new Date().toISOString(), deviceId, method, params }; - entries.push(entry); - if (args.options.jsonl) process.stdout.write(`${JSON.stringify(entry)}\n`); + if (args.options.jsonl) { + streamed += 1; + process.stdout.write(`${JSON.stringify(entry)}\n`); + return; + } + entries.push(entry); });Then return
{ data: { notifications: streamed } }at Line 40.🤖 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/cli/commands/watch.ts` around lines 27 - 32, Update the notification handler in the watch command to avoid retaining entries in JSONL mode: keep writing each entry immediately, but increment a dedicated streamed counter instead of pushing into entries. Update the command’s return path to report notifications using that counter, while preserving the existing entries collection and result behavior for non-JSONL mode.src/cli/commands/doctor.ts-39-43 (1)
39-43: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
doctorignores the global--traceoption.
withClientinsrc/cli/commands/common.tsLine 20 passestrace: options.trace ? new TraceWriter() : undefined. This handler omits it. A user who runszaparoo-cli doctor --traceto collect diagnostics gets no trace JSONL, which is the case where a trace is most useful.🔧 Proposed fix
+import { TraceWriter } from '../../client/trace.js'; @@ const client = new ZaparooClient(device, { credentials, connectTimeoutMs: args.options.timeoutSeconds * 1000, requestTimeoutMs: args.options.timeoutSeconds * 1000, + trace: args.options.trace ? new TraceWriter() : undefined, });🤖 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/cli/commands/doctor.ts` around lines 39 - 43, Update the ZaparooClient options in the doctor command to honor the global trace setting, matching the trace handling in withClient from common.ts: create and pass a TraceWriter when args.options.trace is enabled, otherwise pass undefined.src/cli/commands/watch.ts-24-25 (1)
24-25: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winBound the watch duration.
Reject
secondsvalues above2_147_483before callingsetTimeout; larger values are clamped to 1 ms.🤖 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/cli/commands/watch.ts` around lines 24 - 25, Update the seconds validation in the watch command to reject values greater than 2_147_483, in addition to the existing positive-value check, before the setTimeout call. Preserve the current CliError usage and usage exit code for invalid durations.src/cli/commands/systems.ts-16-21 (1)
16-21: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRemove or remap
systems refresh.The CLI describes this action as refreshing system metadata, but it calls
Methods.LaunchersRefresh, which refreshes launcher configuration. Remove the action, or add a registeredSystemsRefreshmethod and use 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/cli/commands/systems.ts` around lines 16 - 21, Update the systems command’s refresh branch to avoid calling Methods.LaunchersRefresh: either remove the refresh action and its CLI registration, or register a SystemsRefresh method and invoke that method from the action === 'refresh' handler.Source: Coding guidelines
🤖 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 @.github/workflows/ci.yml:
- Line 16: Update the checkout steps in .github/workflows/ci.yml at lines 16-16
and .github/workflows/release.yml at lines 14-14 to set persist-credentials to
false, preserving the existing actions/checkout configuration.
In @.github/workflows/release.yml:
- Around line 24-26: Update the release workflow’s version-check block to assign
github.event.release.tag_name to a RELEASE_TAG environment variable, then use
quoted shell references to RELEASE_TAG for both the comparison and error
message. Remove direct interpolation of the tag into shell expressions while
preserving the existing mismatch validation.
In `@scripts/audit-user-api.mjs`:
- Around line 34-39: Replace the backtick-based missingScopes check with
validation against authoritative structured scope metadata, such as each
operation’s x-required-scope field, and ensure the audit does not infer scopes
from descriptive text. Update the relevant logic around EXPECTED_SCOPES and
discoveredPaths while preserving the existing non-GET path validation.
In `@scripts/validate-skills.mjs`:
- Around line 25-39: Update validateSkill to detect a skill-local build
directory and append a validation error when it exists, alongside the existing
SKILL.md checks. Use the directory path already available in validateSkill and
preserve the current validation behavior for other checks.
In `@skills/zaparoo-library/SKILL.md`:
- Line 14: Remove the explicit-request exception from
skills/zaparoo-library/SKILL.md line 14 so confirmation is always required
before media launch, control, or stop commands. Apply the corresponding change
in skills/zaparoo-nfc/SKILL.md line 14 so confirmation remains required before
NFC writes, mapping changes, launch confirmation, or media launch, even when
explicitly requested.
In `@src/cli/commands/auth.ts`:
- Around line 11-15: Update the claim branch in the command handler to accept
only the `-` sentinel for `--token`; when the option is missing or equals `-`,
obtain the token via `readSecret`, and reject any other provided value. Continue
passing the resolved token through the existing `request` call to
`Methods.SettingsAuthClaim`, preserving its redacted trace handling.
In `@src/cli/commands/backup.ts`:
- Around line 17-28: Add a shared requireConfirmation(args, description) helper
in src/cli/commands/common.ts that requires an explicit --yes flag, then invoke
it before dispatching the state-changing requests in src/cli/commands/backup.ts
lines 17-28 for delete, restore, and remote-restore, src/cli/commands/input.ts
lines 7-21 for keyboard and gamepad input, and src/cli/commands/playtime.ts
lines 16-29 for playtime limits update; leave non-state-changing commands
unchanged.
In `@src/cli/commands/pair.ts`:
- Around line 96-105: Update the forget case to attempt store.deleteCredentials
for device.id and every alias without skipping later entries after a successful
deletion. Combine the individual deletion results into deleted so the response
accurately reflects whether any credentials were removed.
In `@src/cli/secret.ts`:
- Around line 26-59: Update readSecret’s Promise handling to register end,
close, and error listeners on input, and ensure finish removes all data and
stream listeners before restoring terminal state. Settle the promise when the
stream ends or closes, and reject it with the emitted error on error, while
preserving the existing data, cancellation, and length-limit behavior.
In `@src/client/client.ts`:
- Around line 281-290: Update decryptIncoming to reject any frame without an
encrypted e field when this.encryptedSession is active, while preserving
decryption for valid encrypted frames and plaintext handling before session
establishment. Ensure plaintext frames cannot be accepted once the encrypted
session exists.
In `@src/client/config.ts`:
- Around line 90-100: Update parseDeviceList so apiKeys preserves the original
comma-separated positions while treating empty entries as missing values. Remove
the filter(Boolean) behavior from the keys mapping, keep trimming each entry,
and ensure parseDevice receives apiKeys[index] for the corresponding host.
In `@src/client/redact.ts`:
- Around line 1-15: Update SECRET_KEYS and URL_KEYS in redact so their patterns
accept an optional trailing “s”, covering plural secret and URL key names. In
redact’s array handling, pass the current key to each recursive redact call so
key-based rules remain effective for array values while preserving object-entry
behavior.
In `@src/client/resolver.ts`:
- Around line 18-25: Update the configured-device merge in the options.device
branch of the resolver so fields omitted by the user are preserved from
configured, matching the existing apiKey override behavior. In particular, do
not let parseDevice’s default scheme or undefined apiPath overwrite configured
values; only apply parsed values for fields explicitly supplied by the user,
while retaining parsed values for an unconfigured device.
In `@src/crypto/pairing.ts`:
- Around line 192-205: The unauthenticated pairing response fields are cast
without runtime validation, causing raw internal errors and unsafe HMAC
comparison. In src/crypto/pairing.ts lines 192-205, validate that
finishResult.authToken, finishResult.clientId, and finishResult.confirm are
strings, and verify serverHmac.length matches expectedServerHmac.length before
timingSafeEqual. In src/crypto/pairing.ts lines 152-153, validate that
startResult.session and startResult.pake are strings before decoding pake; use
the pairing error path for invalid responses.
- Around line 152-153: Validate the parsed `/api/pair/start` response before
using it in the pairing flow around `startResult` and `msgB`. Confirm that the
body is an object containing non-empty string `session` and `pake` fields, and
convert invalid responses into the pairing failure error path rather than
passing an unchecked value to `Buffer.from`; preserve normal processing for
valid responses.
In `@src/crypto/pake.ts`:
- Around line 57-76: Update the PakeClient constructor’s randomBytes handling to
store a defensive copy in this.alpha rather than the caller-provided Uint8Array
reference. Preserve the existing generated-key path when randomBytes is absent
and keep update()’s zeroization behavior unchanged.
---
Minor comments:
In `@skills/zaparoo-online/SKILL.md`:
- Line 43: Update the 403 description in the relevant instruction to say the key
lacks the required scope or the account is suspended, replacing “account is
unavailable.” Preserve the existing guidance not to request broader scope unless
the task requires it.
In `@src/cli/commands/devices.ts`:
- Around line 83-95: Update stateCommand to avoid resolving the device before
withClient; use client.device.id inside its callback for the data.device value,
matching the existing ping command pattern. Replace the literal 'readers',
'media.active', and 'tokens.history' request names with their corresponding
Methods.* entries.
In `@src/cli/commands/doctor.ts`:
- Around line 39-43: Update the ZaparooClient options in the doctor command to
honor the global trace setting, matching the trace handling in withClient from
common.ts: create and pass a TraceWriter when args.options.trace is enabled,
otherwise pass undefined.
In `@src/cli/commands/logs.ts`:
- Around line 19-26: Move the `flag(args.flags, 'output')` lookup and
missing-output `CliError` validation before the `withClient` call in the `action
=== 'download'` branch, then perform `Methods.SettingsLogsDownload` and pass the
validated output to `writeBase64Output`. Match the validation order used by the
admin command.
In `@src/cli/commands/online.test.ts`:
- Around line 151-171: Update the existing afterEach block in the online command
tests to call vi.restoreAllMocks(), ensuring the process.stdout.write spy
created in the watch test is restored even when assertions fail. Remove the
test-local write.mockRestore() if appropriate to avoid redundant cleanup.
In `@src/cli/commands/readers.ts`:
- Around line 13-21: Update the readers write case to require explicit
confirmation before calling request for Methods.ReadersWrite: accept the
existing force boolean flag (or an interactive confirmation for interactive
sessions), and otherwise fail with a usage error. Preserve the current text
validation and request arguments after confirmation succeeds.
In `@src/cli/commands/screenshot.ts`:
- Around line 9-15: Move the required --output validation to the beginning of
screenshotCommand, before withClient initiates the screenshot request; preserve
the existing CliError and Usage exit behavior, then use the validated output
value for writeBase64Output.
In `@src/cli/commands/systems.ts`:
- Around line 16-21: Update the systems command’s refresh branch to avoid
calling Methods.LaunchersRefresh: either remove the refresh action and its CLI
registration, or register a SystemsRefresh method and invoke that method from
the action === 'refresh' handler.
In `@src/cli/commands/ui.ts`:
- Around line 23-30: Update the UI response validation before the withClient
request to require a non-missing choice-id whenever responseAction is “select”;
throw CliError with ExitCode.Usage when that pairing is invalid, while
preserving other action behavior. Add colocated tests covering select without
choice-id and valid non-select actions.
In `@src/cli/commands/watch.ts`:
- Around line 27-32: Update the notification handler in the watch command to
avoid retaining entries in JSONL mode: keep writing each entry immediately, but
increment a dedicated streamed counter instead of pushing into entries. Update
the command’s return path to report notifications using that counter, while
preserving the existing entries collection and result behavior for non-JSONL
mode.
- Around line 24-25: Update the seconds validation in the watch command to
reject values greater than 2_147_483, in addition to the existing positive-value
check, before the setTimeout call. Preserve the current CliError usage and usage
exit code for invalid durations.
In `@src/cli/errors.ts`:
- Around line 54-65: Update the encryption and connection fallback regexes in
the error-classification logic to use word boundaries around standalone terms,
especially “pair” and “connect,” so substrings such as “repair,” “unpaired,”
“disconnected,” and “reconnecting” do not match. Preserve the existing
classification order and all other patterns.
In `@src/cli/output.ts`:
- Around line 10-15: Update printResult to normalize result.data before
JSON.stringify so undefined is converted to a valid JSON value, such as null.
Preserve the existing pretty-print spacing, stdout output, and return behavior
for all other result values.
In `@src/client/config.ts`:
- Around line 102-112: Update loadCliConfig around the JSON.parse/readFileSync
call to catch malformed config-file errors and report them with the config path,
while preserving the existing configuration fallback behavior. Use the existing
classifyError/error-reporting flow from the CLI error handling rather than
allowing a raw SyntaxError to escape.
In `@src/client/trace.ts`:
- Around line 60-68: Update TraceStore.readLast to parse selected lines
defensively, skipping any line that throws during JSON.parse while preserving
all successfully parsed TraceEntry values. Keep the existing file-reading,
filtering, and count behavior unchanged so logs trace returns readable entries
despite partial or invalid lines.
In `@src/crypto/pairing.ts`:
- Around line 79-90: Sanitize the serverMessage handling in pairingError before
returning it: strip control characters, including ANSI escape sequences, and cap
the resulting message to a reasonable maximum length. Apply the sanitized
bounded value only when selecting the server-supplied message, while preserving
the existing status-based and fallback error behavior.
In `@src/online/client.test.ts`:
- Around line 90-98: Update the “redacts keys from error bodies” test around
OnlineClient.get so the rejected error assertion requires both the presence of
“[REDACTED]” and the absence of “super-secret” on the same OnlineApiError,
preserving the existing rejection check.
---
Nitpick comments:
In `@src/cli/args.ts`:
- Around line 136-142: Update takeValue to treat only an undefined next argument
as missing, while continuing to reject values beginning with “--”. Preserve
empty-string values so separated and inline option forms behave consistently.
- Around line 208-211: Update the switch cases around the trace option so help
no longer shares the trace-handling body: keep help’s existing behavior separate
and handle trace alone by preserving or enabling options.trace without using the
name comparison.
In `@src/cli/commands/admin.ts`:
- Around line 18-27: Extract the shared SettingsLogsDownload request and
writeBase64Output handling from the admin logs-download case and the logs
download dispatcher into one helper. Have both dispatchers call that helper,
while preserving the required --output validation and ensuring both paths use
the same validation order.
In `@src/cli/commands/backup.ts`:
- Around line 40-42: The request helper is duplicated across three command
handlers. Add the shared request implementation to src/cli/commands/common.ts,
then remove the local helper and import the shared helper in
src/cli/commands/backup.ts lines 40-42, src/cli/commands/tokens.ts lines 19-22,
and src/cli/commands/playtime.ts lines 34-36; preserve the existing withClient
and client.request behavior.
In `@src/cli/commands/clients.ts`:
- Around line 20-25: Update the `pair-begin` branch in the clients command
handler to validate the `role` argument against the same `member` and `admin`
values accepted by `pair.ts`, rejecting invalid values before calling `request`.
Reuse the existing validation behavior or shared symbol so both entry points
fail consistently, and add colocated tests covering invalid-role rejection and
the existing valid mappings.
In `@src/cli/commands/commands.test.ts`:
- Around line 36-62: Add colocated tests in the command-to-RPC mapping suite for
runCommand with --unsafe, screenshotCommand when --output is missing, and a
representative CliError usage path. Assert the unsafe option maps correctly,
missing required output rejects with the expected usage error, and each path
verifies the relevant RPC request or confirms no request is sent.
In `@src/cli/commands/devices.ts`:
- Around line 37-44: Update the ping handler in the `withClient` callback to
replace the literal `version` and `health` request names with the corresponding
entries from the `Methods` registry, matching the established usage in other
command handlers. Keep the existing fallback and health error handling
unchanged.
In `@src/cli/commands/doctor.ts`:
- Around line 120-143: Define a shared union type for all supported doctor error
kinds, then use it for DoctorCheck.kind and the return type of errorKind. Update
checkFailure and the kind extraction logic to narrow unknown values to that
union, so doctorExitCode’s string comparisons are compile-time checked and
unsupported kinds cannot silently pass through.
In `@src/cli/commands/inbox.ts`:
- Around line 18-19: Update the `case 'clear'` branch in the inbox command to
require explicit confirmation before calling `request(args, Methods.InboxClear).
Accept a `--yes` flag, or prompt interactively when stdin is a TTY; otherwise
abort without clearing, preserving non-interactive safety for agent callers.
In `@src/cli/commands/input.ts`:
- Line 1: Use the authoritative Methods registry from ../../api/methods.js in
the handlers at src/cli/commands/input.ts:1, src/cli/commands/systems.ts:1, and
src/cli/commands/tokens.ts:1; remove any competing registry re-export from
src/types.ts if present, while keeping src/api/methods.ts synchronized with
registered Core methods.
In `@src/cli/commands/media.ts`:
- Around line 216-232: Move the shared rawRequest and request helpers from the
media command module into common.ts, preserving the generic raw request behavior
and pickDefined parameter handling. Update media.ts, admin.ts, and readers.ts to
import and use the common helpers, removing their duplicate local definitions.
In `@src/cli/commands/online.ts`:
- Around line 317-318: Update the existing-output hashing near createContext and
currentHash to use a streaming or chunked file read, incrementally updating the
hash instead of calling readFileSync on the entire backup. Preserve the
existsSync check and undefined result when outputPath is absent, and keep the
resulting digest equivalent to hashing the complete file.
- Around line 274-279: Update the online request handler around createContext
and the URL construction to import and use ONLINE_API_ORIGIN instead of the
hardcoded origin, and convert invalid URL inputs such as “http://” into the
command’s established CliError usage-error path rather than allowing a raw
TypeError to escape.
- Around line 366-404: Update the sleep interval in watchActiveSessions to honor
the server-provided retryAfter value when context.client.get returns a 429,
rather than relying only on response.pollInterval. Use retryAfter as the backoff
for rate-limited responses while preserving the existing poll interval and
jitter behavior otherwise, and continue allowing persistent client errors to
propagate and terminate the loop.
In `@src/cli/commands/pair.ts`:
- Around line 14-15: Update the pair command’s validation error flow around
requestedAction and action so invoking pair start without --pin reports the
user-typed start action instead of complete, or add explicit help text
documenting start as an alias for complete. Preserve the existing alias behavior
while ensuring the user-facing guidance is unambiguous.
- Around line 55-58: Update the `complete` case in the pair command so that when
`--pin` is absent, it obtains the PIN through the existing `src/cli/secret.ts`
prompt/stdin mechanism instead of reading `args.positionals[2]`; retain the
usage error when no PIN is provided or received, and leave the existing `--pin`
behavior unchanged.
In `@src/cli/commands/settings.ts`:
- Around line 66-83: Move the parseJson and parseJsonObject helpers out of the
settings command into a shared module such as common.ts or args.ts, preserving
their current validation and CliError behavior. Update settings.ts and
profiles.ts to import these helpers from the shared module, leaving command
handlers independent of one another.
In `@src/cli/commands/update.ts`:
- Around line 7-13: Update updateCommand so the Methods.UpdateApply path
requires explicit user confirmation, supporting a --yes override for
non-interactive or intentional callers before invoking client.request. Keep the
check action unchanged, and abort with the existing CLI error/usage mechanism
when confirmation is not provided.
In `@src/cli/files.ts`:
- Around line 34-50: Update writeBytesOutput to generate a randomized temporary
suffix in addition to process.pid, and pass the exclusive-create wx flag to
writeFileSync so pre-existing files or symlinks cause the write to fail.
Preserve the existing cleanup and rename behavior.
- Around line 19-31: Update the decoded payload handling near writeBytesOutput
to compare bytes.length with response.size when response.size is present, and
reject mismatches before writing the output. Preserve the existing behavior when
size is absent and continue returning the current metadata for valid payloads.
In `@src/cli/index.test.ts`:
- Around line 6-9: Restore the PACKAGE_VERSION global stub after the test suite
by adding an afterAll cleanup that calls vi.unstubAllGlobals() alongside the
existing beforeAll setup in the suite containing the dynamic index.js import. Do
not change the stub value or import behavior.
In `@src/cli/index.ts`:
- Around line 246-266: Update main and run so the already-created ParsedArgs
from parseCliArgs is passed into run instead of parsing argv again. Export an
overload or compatible run signature that accepts ParsedArgs while preserving
the existing argv form for tests, and ensure execution reuses the same parsed
object and validation result.
In `@src/cli/output.test.ts`:
- Around line 38-62: Move the entire “error classification” describe block from
output.test.ts into errors.test.ts alongside classifyError. Preserve all
existing test cases and assertions, and remove the duplicated block from
output.test.ts.
In `@src/cli/secret.test.ts`:
- Around line 5-19: Extend the readSecret tests with a fake TTY input exposing
isTTY, setRawMode, isRaw, and isPaused, then emit data chunks to exercise
raw-mode input handling. Cover cancellation with \u0003, backspace editing, the
maximum-length limit, and restoration of raw mode, while keeping the existing
non-TTY test unchanged.
In `@src/client/client.test.ts`:
- Around line 216-236: Add a test alongside the existing connection rejection
tests that starts ZaparooClient.connect(), emits the socket’s
unexpected-response event with statusCode 401, and asserts the connection
promise rejects with kind 'api-auth'. Use the existing client, socket, and timer
setup patterns without changing production behavior.
In `@src/client/resolver.ts`:
- Around line 6-14: Update scanDevices so the timeout wait and device collection
execute within a try block, with discovery.stop() in a finally block that always
runs when the wait or discovery flow rejects; preserve returning the collected
devices after successful completion.
In `@src/client/trace.ts`:
- Around line 16-28: Update SENSITIVE_REQUEST_METHODS and SENSITIVE_DATA_METHODS
in trace.ts to use the corresponding Methods enum members from src/types.ts
instead of string literals, preserving the existing set membership. Ensure every
referenced method is type-checked so renaming a Methods member causes a build
failure rather than disabling trace redaction.
In `@src/crypto/pairing.test.ts`:
- Around line 108-132: Remove the unused fetchMock definition and its initial
vi.stubGlobal call from the 401 test; retain fetchMock401 and the existing
performPairing assertion so the test contains only the effective mock setup.
- Around line 156-164: The 429 retry test incurs real retry delays, slowing the
suite. Update the test around performPairing to use fake timers, advance them
enough to complete all fetchWithRetry retries before asserting the rejection,
and restore real timers in an afterEach hook.
- Around line 39-41: Update the afterEach cleanup in pairing tests to call
vi.unstubAllGlobals() alongside vi.restoreAllMocks(), ensuring global fetch
stubs installed by vi.stubGlobal are removed after every test.
In `@src/crypto/storage.test.ts`:
- Around line 157-203: Add three focused tests covering CredentialStore.load
rejection branches: a credentials file with version greater than 3 must throw
“Unsupported credentials file version,” a device entry with an invalid
pairingKey must throw “Invalid credentials entry for <deviceId>,” and an online
API key not matching the zpk1_ format must throw “Invalid Online User API
credentials entry.” Follow the existing tempPath, paths, writeFileSync, and
assertion patterns in the surrounding tests.
In `@src/crypto/storage.ts`:
- Around line 148-212: Protect each mutating load–modify–write sequence with an
exclusive lock file created using wx, covering the full operation from load
through write. Reuse the storage class’s mutation entry points and ensure the
lock is always removed in a finally block, including when load or write fails,
so concurrent processes cannot overwrite each other’s credential changes.
In `@src/discovery/mdns.ts`:
- Line 58: Extract the repeated IPv6-aware host/port formatting from the
discovery and removal paths into one private helper, then use that helper
wherever the IDs are built near the existing declarations. Preserve the current
bracketed-IPv6 and host:port output exactly so discovered and removed IDs
continue to match.
- Around line 2-3: Update the type imports in the mDNS implementation to use the
package-root value exports for Bonjour, Browser, and Service, and derive their
instance types with InstanceType<typeof ...> wherever they are used as types.
Remove the deep direct type imports from bonjour-service/dist/lib/bonjour.js
while preserving the existing runtime constructors and behavior.
In `@src/online/contract.ts`:
- Around line 15-21: Update the CLI path definitions in the online command flow
to derive or validate them against the canonical OnlineOperations contract,
preventing independent path lists from drifting. Prefer reusing OnlineOperations
when constructing request paths; otherwise add coverage that asserts every CLI
path template, including /v1/play-sessions/summary, exists in the contract.
In `@src/online/pagination.ts`:
- Around line 13-18: Separate the configurable default from the validation
ceiling in the max-pages handling: declare a MAX_ALLOWED_PAGES constant and use
it for the upper-bound check and error message, while keeping DEFAULT_MAX_PAGES
only as the default value.
In `@src/version.ts`:
- Around line 1-3: Update src/cli/index.ts to import and use packageVersion from
version.ts, removing its duplicate PACKAGE_VERSION declaration and any local
version fallback. Preserve the existing CLI version behavior for both injected
build versions and direct module imports.
🪄 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 Plus
Run ID: 10089d21-7f91-4e96-955e-a369a81d8856
⛔ Files ignored due to path filters (2)
package-lock.jsonis excluded by!**/package-lock.jsonpnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (150)
.agents/skills.github/workflows/ci.yml.github/workflows/release.yml.gitignoreAGENTS.mdREADME.mdSECURITY.mddocs/cli-output.mddocs/skill-scenarios.mdpackage.jsonscripts/audit-core-api.mjsscripts/audit-user-api.mjsscripts/smoke-packed-package.mjsscripts/validate-skills.mjsskills/zaparoo-artifacts/SKILL.mdskills/zaparoo-artifacts/references/database-safety.mdskills/zaparoo-artifacts/references/platform-paths.mdskills/zaparoo-development/SKILL.mdskills/zaparoo-development/references/integration.mdskills/zaparoo-library/SKILL.mdskills/zaparoo-nfc/SKILL.mdskills/zaparoo-online/SKILL.mdskills/zaparoo-online/references/user-api.mdskills/zaparoo-troubleshooting/SKILL.mdskills/zaparoo-troubleshooting/references/cli.mdskills/zaparoo-zapscript/SKILL.mdskills/zaparoo-zapscript/references/zapscript.mdsrc/api/baseline.tssrc/api/methods.test.tssrc/api/methods.tssrc/cli/args.test.tssrc/cli/args.tssrc/cli/commands/admin.tssrc/cli/commands/auth.tssrc/cli/commands/backup.tssrc/cli/commands/clients.tssrc/cli/commands/commands.test.tssrc/cli/commands/common.tssrc/cli/commands/devices.tssrc/cli/commands/doctor.tssrc/cli/commands/inbox.tssrc/cli/commands/input.tssrc/cli/commands/logs.tssrc/cli/commands/mappings.tssrc/cli/commands/media.tssrc/cli/commands/online.test.tssrc/cli/commands/online.tssrc/cli/commands/pair.tssrc/cli/commands/playtime.tssrc/cli/commands/profiles.tssrc/cli/commands/readers.tssrc/cli/commands/rpc.tssrc/cli/commands/run.tssrc/cli/commands/screenshot.tssrc/cli/commands/settings.tssrc/cli/commands/systems.tssrc/cli/commands/tokens.tssrc/cli/commands/ui.tssrc/cli/commands/update.tssrc/cli/commands/watch.tssrc/cli/errors.test.tssrc/cli/errors.tssrc/cli/files.test.tssrc/cli/files.tssrc/cli/index.test.tssrc/cli/index.tssrc/cli/output.test.tssrc/cli/output.tssrc/cli/secret.test.tssrc/cli/secret.tssrc/client/client.test.tssrc/client/client.tssrc/client/config.test.tssrc/client/config.tssrc/client/endpoint.tssrc/client/errors.tssrc/client/redact.tssrc/client/resolver.test.tssrc/client/resolver.tssrc/client/trace.test.tssrc/client/trace.tssrc/config.test.tssrc/config.tssrc/connection/device.test.tssrc/connection/device.tssrc/connection/manager.test.tssrc/connection/manager.tssrc/connection/trace.test.tssrc/connection/trace.tssrc/connection/types.tssrc/crypto/fixtures/core-v2.16-pake.jsonsrc/crypto/index.tssrc/crypto/pairing.test.tssrc/crypto/pairing.tssrc/crypto/pake.test.tssrc/crypto/pake.tssrc/crypto/session.test.tssrc/crypto/session.tssrc/crypto/storage.test.tssrc/crypto/storage.tssrc/discovery/mdns.test.tssrc/discovery/mdns.tssrc/index.tssrc/notifications/buffer.test.tssrc/notifications/buffer.tssrc/notifications/handler.test.tssrc/notifications/handler.tssrc/notifications/state.test.tssrc/notifications/state.tssrc/online/client.test.tssrc/online/client.tssrc/online/contract.test.tssrc/online/contract.tssrc/online/credentials.test.tssrc/online/credentials.tssrc/online/errors.tssrc/online/pagination.test.tssrc/online/pagination.tssrc/prompts/index.tssrc/resources/device-state.tssrc/resources/zapscript-ref.tssrc/server.tssrc/tools/admin-manage.tssrc/tools/admin.tssrc/tools/devices.tssrc/tools/helpers.test.tssrc/tools/helpers.tssrc/tools/inbox.tssrc/tools/index.test.tssrc/tools/index.tssrc/tools/input.tssrc/tools/logs.test.tssrc/tools/logs.tssrc/tools/mappings.tssrc/tools/media-control.tssrc/tools/media-index.tssrc/tools/media.tssrc/tools/notifications.test.tssrc/tools/notifications.tssrc/tools/readers-write.tssrc/tools/readers.tssrc/tools/run.tssrc/tools/screenshot.tssrc/tools/settings-update.tssrc/tools/settings.tssrc/tools/stop.tssrc/tools/systems.tssrc/tools/tokens.tssrc/types.tssrc/version.ts
💤 Files with no reviewable changes (45)
- src/notifications/buffer.test.ts
- src/tools/admin-manage.ts
- src/tools/run.ts
- src/config.ts
- src/prompts/index.ts
- src/tools/readers-write.ts
- src/tools/screenshot.ts
- src/connection/types.ts
- src/notifications/handler.test.ts
- src/notifications/handler.ts
- src/tools/systems.ts
- src/tools/input.ts
- src/connection/device.test.ts
- src/config.test.ts
- src/resources/zapscript-ref.ts
- src/connection/trace.test.ts
- src/tools/notifications.test.ts
- src/server.ts
- src/resources/device-state.ts
- src/notifications/state.ts
- src/tools/helpers.test.ts
- src/tools/inbox.ts
- src/tools/stop.ts
- src/connection/manager.test.ts
- src/tools/mappings.ts
- src/tools/index.test.ts
- src/tools/notifications.ts
- src/tools/media-control.ts
- src/tools/settings-update.ts
- src/tools/admin.ts
- src/notifications/buffer.ts
- src/tools/logs.test.ts
- src/connection/trace.ts
- src/tools/logs.ts
- src/tools/tokens.ts
- src/tools/readers.ts
- src/connection/manager.ts
- src/tools/settings.ts
- src/tools/media-index.ts
- src/tools/helpers.ts
- src/tools/devices.ts
- src/tools/media.ts
- src/notifications/state.test.ts
- src/tools/index.ts
- src/connection/device.ts
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@scripts/audit-user-api.mjs`:
- Around line 57-60: Update the nonGetPaths filter to recognize head, options,
and trace operations in addition to post, put, patch, and delete. Ensure paths
containing any non-GET method are reported while preserving GET-only paths,
preferably by parsing each path item’s declared methods before filtering.
- Around line 86-90: Update operationBlock to end at the next same-indentation
path or top-level section, rather than only searching for the next “/v1” marker.
Ensure the final path block cannot extend into components or capture later
six-space x-required-scope entries, while preserving the existing slice behavior
for valid operation blocks.
- Around line 47-56: The scope audit currently treats an empty structuredScopes
collection as success. Update the scopeMetadataAvailable handling in the scope
metadata validation to fail when EXPECTED_SCOPE_BY_PATH or EXPECTED_SCOPES
require mappings, or explicitly report the audit as unsupported when the
authoritative document defines no x-required-scope entries; do not allow an
empty structuredScopes result to pass silently.
In `@skills/zaparoo-online/SKILL.md`:
- Line 43: Update the 403 guidance in the API error-handling section to state
that it means access denied, without attributing it to account suspension.
Instruct readers to use the documented error reason to identify the cause before
changing requested scopes.
In `@src/client/client.test.ts`:
- Around line 133-138: Update the onUnexpectedResponse handler to call
ws.terminate() when a WebSocket upgrade fails, ensuring the underlying HTTP
request is aborted. Extend the 401 authentication test around client.connect()
to assert that socket.terminate was invoked.
In `@src/crypto/storage.ts`:
- Around line 203-218: Update mutate() to inspect the existing lock file with
statSync and reclaim it when its age exceeds a short stale-lock threshold before
retrying acquisition; preserve exclusive creation for active locks and normal
cleanup through the existing finally block. Add statSync to the node:fs imports
and ensure active, non-stale locks still fail rather than being removed.
🪄 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 Plus
Run ID: af1419b8-9b76-4db9-9515-5eb481229f12
📒 Files selected for processing (67)
.github/workflows/ci.yml.github/workflows/release.ymlscripts/audit-user-api.mjsscripts/validate-skills.mjsskills/zaparoo-library/SKILL.mdskills/zaparoo-nfc/SKILL.mdskills/zaparoo-online/SKILL.mdsrc/cli/args.test.tssrc/cli/args.tssrc/cli/commands/admin.tssrc/cli/commands/auth.tssrc/cli/commands/backup.tssrc/cli/commands/clients.tssrc/cli/commands/commands.test.tssrc/cli/commands/common.tssrc/cli/commands/devices.tssrc/cli/commands/doctor.tssrc/cli/commands/inbox.tssrc/cli/commands/input.tssrc/cli/commands/logs.tssrc/cli/commands/mappings.tssrc/cli/commands/media.tssrc/cli/commands/online.test.tssrc/cli/commands/online.tssrc/cli/commands/pair.tssrc/cli/commands/playtime.tssrc/cli/commands/profiles.tssrc/cli/commands/readers.tssrc/cli/commands/run.tssrc/cli/commands/screenshot.tssrc/cli/commands/settings.tssrc/cli/commands/systems.tssrc/cli/commands/tokens.tssrc/cli/commands/ui.tssrc/cli/commands/update.tssrc/cli/commands/watch.tssrc/cli/errors.test.tssrc/cli/errors.tssrc/cli/files.test.tssrc/cli/files.tssrc/cli/index.test.tssrc/cli/index.tssrc/cli/output.test.tssrc/cli/output.tssrc/cli/secret.test.tssrc/cli/secret.tssrc/client/client.test.tssrc/client/client.tssrc/client/config.test.tssrc/client/config.tssrc/client/redact.tssrc/client/resolver.test.tssrc/client/resolver.tssrc/client/trace.test.tssrc/client/trace.tssrc/crypto/pairing.test.tssrc/crypto/pairing.tssrc/crypto/pake.test.tssrc/crypto/pake.tssrc/crypto/storage.test.tssrc/crypto/storage.tssrc/discovery/mdns.tssrc/online/client.test.tssrc/online/contract.test.tssrc/online/contract.tssrc/online/pagination.tssrc/types.ts
💤 Files with no reviewable changes (1)
- src/types.ts
🚧 Files skipped from review as they are similar to previous changes (38)
- src/cli/commands/run.ts
- src/online/contract.test.ts
- src/cli/commands/screenshot.ts
- src/cli/commands/update.ts
- src/cli/index.test.ts
- src/cli/commands/clients.ts
- src/cli/commands/logs.ts
- src/cli/commands/inbox.ts
- src/cli/commands/mappings.ts
- src/client/resolver.test.ts
- src/cli/commands/backup.ts
- src/cli/commands/profiles.ts
- src/cli/commands/ui.ts
- src/online/pagination.ts
- src/cli/index.ts
- src/client/trace.ts
- src/cli/files.ts
- src/cli/secret.ts
- src/client/config.test.ts
- src/cli/commands/pair.ts
- src/cli/errors.ts
- src/discovery/mdns.ts
- src/cli/commands/devices.ts
- src/crypto/pake.ts
- .github/workflows/ci.yml
- src/cli/commands/watch.ts
- src/crypto/pairing.ts
- src/cli/commands/readers.ts
- src/cli/commands/doctor.ts
- src/cli/output.ts
- src/cli/commands/media.ts
- .github/workflows/release.yml
- src/cli/commands/admin.ts
- src/client/client.ts
- scripts/validate-skills.mjs
- src/cli/commands/online.ts
- src/cli/commands/auth.ts
- src/cli/args.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
skills/zaparoo-online/SKILL.md (1)
14-21: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDocument the local credential contract.
ZAPAROO_ONLINE_USER_API_KEYoverrides the saved key.online auth setstores the key without returning it.online auth status --jsonreturns configuration metadata only.online auth forgetremoves only the saved key. These local operations use noread:*scope; the six scopes apply only to API data requests. State these rules and retain the key-handling restrictions.🤖 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 `@skills/zaparoo-online/SKILL.md` around lines 14 - 21, Update the “Protect account access” section to document the local credential contract: state that ZAPAROO_ONLINE_USER_API_KEY overrides the saved key, online auth set stores without returning it, online auth status --json exposes metadata only, and online auth forget removes only the saved key. Clarify that these local operations require no read:* scope, while the six scopes apply only to API data requests, and retain all existing key-handling restrictions.
🤖 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.
Nitpick comments:
In `@skills/zaparoo-online/SKILL.md`:
- Around line 14-21: Update the “Protect account access” section to document the
local credential contract: state that ZAPAROO_ONLINE_USER_API_KEY overrides the
saved key, online auth set stores without returning it, online auth status
--json exposes metadata only, and online auth forget removes only the saved key.
Clarify that these local operations require no read:* scope, while the six
scopes apply only to API data requests, and retain all existing key-handling
restrictions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ee5f5c51-b4db-4602-8f60-30f69fae1923
📒 Files selected for processing (6)
scripts/audit-user-api.mjsskills/zaparoo-online/SKILL.mdsrc/client/client.test.tssrc/client/client.tssrc/crypto/storage.test.tssrc/crypto/storage.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- scripts/audit-user-api.mjs
- src/crypto/storage.test.ts
- src/client/client.test.ts
- src/crypto/storage.ts
- src/client/client.ts
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (8)
src/cli/commands/capabilities.ts (2)
19-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePass a thunk instead of a started promise to
probe.
probecurrently receives an already-started promise, so each call site must append.then(() => undefined). A thunk removes that boilerplate and keeps rejection handling insideprobe.♻️ Proposed refactor
- const probes = await Promise.all([ - probe( - client.request('health').then(() => undefined), - 'health', - ), - probe( - client.request('clients.current').then(() => undefined), - 'clients.current', - ), - ]); + const probes = await Promise.all([ + probe(() => client.request('health'), 'health'), + probe(() => client.request('clients.current'), 'clients.current'), + ]);-async function probe(operation: Promise<void>, method: string): Promise<ProbeResult> { +async function probe(operation: () => Promise<unknown>, method: string): Promise<ProbeResult> { try { - await operation; + await operation(); return { method, supported: true };Also applies to: 61-71
🤖 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/cli/commands/capabilities.ts` around lines 19 - 28, Update the probe call sites in the capabilities command to pass thunks that invoke client.request('health') and client.request('clients.current'), rather than starting promises and mapping their results beforehand. Adjust probe’s parameter and execution flow as needed so it invokes each thunk and retains responsibility for rejection handling.
73-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd colocated tests for version comparison mapping.
No
capabilitiestest file coverscompareCoreVersionornumericVersion. Add tests for compatible, older, prerelease, missing, and unparsable versions.🤖 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/cli/commands/capabilities.ts` around lines 73 - 109, Add a colocated capabilities test file covering compareCoreVersion and numericVersion, with cases for compatible versions, older versions, prerelease versions, missing input, and unparsable values. Assert the returned status and version fields, including unknown handling for missing or invalid versions, while preserving the existing comparison behavior.Source: Coding guidelines
src/cli/commands/systems.ts (2)
33-53: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPreserve the wrapper shape when filtering a wrapped payload.
If Core returns
{ systems: [...] }plus sibling metadata, and the user supplies only--filteror--category, the command returns a bare array. Without those flags it returns the original wrapper. Consumers then see two different result shapes for the same command. Return the wrapper with a replacedsystemsarray to keep one shape.♻️ Proposed refactor
- const systems = Array.isArray(value) - ? value - : value && - typeof value === 'object' && - Array.isArray((value as Record<string, unknown>).systems) - ? ((value as Record<string, unknown>).systems as unknown[]) - : undefined; - if (!systems) return value; + const wrapper = + !Array.isArray(value) && + value && + typeof value === 'object' && + Array.isArray((value as Record<string, unknown>).systems) + ? (value as Record<string, unknown>) + : undefined; + const systems = Array.isArray(value) + ? value + : ((wrapper?.systems as unknown[] | undefined) ?? undefined); + if (!systems) return value; @@ - if (!summary) return selected; + if (!summary) return wrapper ? { ...wrapper, systems: selected } : selected;🤖 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/cli/commands/systems.ts` around lines 33 - 53, Update the filtering flow around the systems extraction and selected result so wrapped payloads retain their original object shape when --filter or --category is used. When value contains a systems array, return a copy of that wrapper with its systems property replaced by selected, preserving sibling metadata; continue returning a bare array for unwrapped array inputs and preserve the existing summary behavior.
14-21: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a
--filtertest case forsystems.filter,category, andsummaryare registered flags. The colocated test covers only--categoryand--summary.🤖 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/cli/commands/systems.ts` around lines 14 - 21, Add a colocated test case for the systems command’s --filter flag, verifying that the filter value is passed through the selectSystems call alongside the existing category and summary coverage.Source: Coding guidelines
scripts/evaluate-agent-scenarios.mjs (1)
280-288: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBound the catalog subprocess and report a missing build clearly.
execFileSyncrunsbuild/index.jswith no timeout and nomaxBufferlimit. If the CLI hangs, the evaluation script hangs with it. Ifbuild/index.jsis absent, the failure surfaces as a raw spawn error. Add a timeout and a pre-check.♻️ Proposed refactor
function loadCatalog(root, path) { if (path) return readJson(resolve(path)); + const entry = resolve(root, 'build/index.js'); + if (!existsSync(entry)) { + throw new Error(`Built CLI not found at ${entry}; run the build first or pass --catalog`); + } const output = execFileSync( process.execPath, - [resolve(root, 'build/index.js'), 'catalog', '--json', '--no-pretty'], - { encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'] }, + [entry, 'catalog', '--json', '--no-pretty'], + { encoding: 'utf8', stdio: ['ignore', 'pipe', 'pipe'], timeout: 60_000 }, ); return JSON.parse(output); }Import
existsSyncalongsidereadFileSync:-import { readFileSync } from 'node:fs'; +import { existsSync, readFileSync } from 'node:fs';🤖 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 `@scripts/evaluate-agent-scenarios.mjs` around lines 280 - 288, Update loadCatalog to pre-check that the resolved build/index.js exists before invoking execFileSync, reporting a clear missing-build error when absent. Configure execFileSync with a finite timeout and maxBuffer limit so catalog generation cannot hang or exceed output bounds, while preserving the existing JSON parsing behavior.src/api/access.ts (1)
5-115: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a disjointness assertion for method classifications.
The existing test covers all 90 methods. Add a check that
READ_METHODSandWRITE_METHODSremain disjoint. Otherwise, a future duplicate makesmethodEffect()returnreadwhilemethodAccessCatalog()returnswrite.🤖 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/api/access.ts` around lines 5 - 115, Add a disjointness assertion for READ_METHODS and WRITE_METHODS, preferably in the existing method-classification test covering all methods. Verify their intersection is empty so duplicate classifications cannot cause methodEffect() and methodAccessCatalog() to disagree.src/cli/commands/devices.ts (1)
86-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReturn a structured error shape instead of
String(error).
String(error)produces text such asError: WebSocket closed (1006). Agents then parse prose.doctorCommandinsrc/cli/commands/doctor.tsalready exposes a structuredkindfor failed checks. Use the same shape here sostatestays machine-readable.♻️ Proposed refactor
- const [readers, activeMedia, tokens] = await Promise.all([ - client.request(Methods.Readers).catch((error) => ({ error: String(error) })), - client.request(Methods.MediaActive).catch((error) => ({ error: String(error) })), - client.request(Methods.Tokens).catch((error) => ({ error: String(error) })), - ]); + const failure = (error: unknown) => ({ + error: { + kind: error instanceof ClientError ? error.kind : 'unknown', + message: error instanceof Error ? error.message : String(error), + }, + }); + const [readers, activeMedia, tokens] = await Promise.all([ + client.request(Methods.Readers).catch(failure), + client.request(Methods.MediaActive).catch(failure), + client.request(Methods.Tokens).catch(failure), + ]);🤖 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/cli/commands/devices.ts` around lines 86 - 90, Update the request error handlers in the Promise.all block for readers, activeMedia, and tokens to return the same structured error shape used by doctorCommand’s failed checks, including its machine-readable kind field, instead of String(error). Preserve the existing per-request fallback behavior and ensure state remains machine-readable.src/cli/policy.test.ts (1)
5-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
unrestrictedpolicy andconfirmationGranted.The suite covers
interactiveandread-only.unrestrictedis the branch that allows a write without--yes, andconfirmationGrantedreturnstruefor it without any flag. Both are security-relevant paths insrc/cli/policy.ts. Add two cases.💚 Proposed tests
+ it('allows unconfirmed writes under unrestricted policy', () => { + const args = parseCliArgs(['mappings', 'delete', '1', '--policy', 'unrestricted']); + expect(enforceCommandPolicy(args)).toMatchObject({ path: 'mappings delete' }); + expect(confirmationGranted(args)).toBe(true); + }); + + it('withholds confirmation under read-only policy', () => { + const args = parseCliArgs(['state', '--policy', 'read-only', '--yes']); + expect(confirmationGranted(args)).toBe(false); + });Import
confirmationGrantedfrom./policy.js.🤖 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/cli/policy.test.ts` around lines 5 - 59, Add coverage in the command policy suite for the unrestricted policy: verify a state-changing command succeeds without --yes when --policy unrestricted is supplied, and verify confirmationGranted returns true for unrestricted without any confirmation flag. Import confirmationGranted from ./policy.js and keep the existing interactive and read-only cases 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 `@scripts/evaluate-agent-scenarios.mjs`:
- Around line 96-113: Validate scenario.maxCommands and scenario.maxOutputBytes
before applying the command-count and output-byte comparisons in the scenario
evaluation flow. Fail the scenario when either budget is missing or non-numeric,
rather than allowing the comparisons to be skipped; preserve the existing
efficiency and context failures for valid budgets that are exceeded.
In `@skills/zaparoo-artifacts/SKILL.md`:
- Line 41: Update the bounded mDNS discovery step in the skill instructions to
require explicit authorization and a clearly identified target before invoking
`zaparoo-cli devices scan`; otherwise make the scan conditional on an already
authorized target. Preserve the timeout and agent options once those
prerequisites are satisfied.
In `@src/cli/args.ts`:
- Around line 239-243: Update the invalid-policy error handling in parsePolicy
and its callers to identify whether the value came from the --policy flag or
ZAPAROO_POLICY, and include that source in the message. Preserve the existing
policy validation and ensure the environment-variable path used by the
options.policy assignment reports ZAPAROO_POLICY instead of --policy.
In `@src/cli/commands/agent.ts`:
- Around line 143-145: Update the detectedClients filter to validate the full
client skills directory path from CLIENT_TARGETS rather than only
path.split('/')[0], so copilot is detected only when its complete target
directory exists.
In `@src/cli/policy.ts`:
- Around line 7-22: Update the local-write flow used by enforceCommandPolicy and
writeBytesOutput so writing to an existing output path is rejected by default
rather than replaced. Use no-overwrite filesystem semantics, or gate replacement
behind an explicit overwrite option, while preserving the required --output
behavior.
In `@src/client/client.ts`:
- Around line 78-99: Update the discarded socket cleanup in the connection retry
catch block to attach a no-op error listener to failedSocket after
removeAllListeners() and before terminate(). Keep the existing cleanup and retry
behavior unchanged.
---
Nitpick comments:
In `@scripts/evaluate-agent-scenarios.mjs`:
- Around line 280-288: Update loadCatalog to pre-check that the resolved
build/index.js exists before invoking execFileSync, reporting a clear
missing-build error when absent. Configure execFileSync with a finite timeout
and maxBuffer limit so catalog generation cannot hang or exceed output bounds,
while preserving the existing JSON parsing behavior.
In `@src/api/access.ts`:
- Around line 5-115: Add a disjointness assertion for READ_METHODS and
WRITE_METHODS, preferably in the existing method-classification test covering
all methods. Verify their intersection is empty so duplicate classifications
cannot cause methodEffect() and methodAccessCatalog() to disagree.
In `@src/cli/commands/capabilities.ts`:
- Around line 19-28: Update the probe call sites in the capabilities command to
pass thunks that invoke client.request('health') and
client.request('clients.current'), rather than starting promises and mapping
their results beforehand. Adjust probe’s parameter and execution flow as needed
so it invokes each thunk and retains responsibility for rejection handling.
- Around line 73-109: Add a colocated capabilities test file covering
compareCoreVersion and numericVersion, with cases for compatible versions, older
versions, prerelease versions, missing input, and unparsable values. Assert the
returned status and version fields, including unknown handling for missing or
invalid versions, while preserving the existing comparison behavior.
In `@src/cli/commands/devices.ts`:
- Around line 86-90: Update the request error handlers in the Promise.all block
for readers, activeMedia, and tokens to return the same structured error shape
used by doctorCommand’s failed checks, including its machine-readable kind
field, instead of String(error). Preserve the existing per-request fallback
behavior and ensure state remains machine-readable.
In `@src/cli/commands/systems.ts`:
- Around line 33-53: Update the filtering flow around the systems extraction and
selected result so wrapped payloads retain their original object shape when
--filter or --category is used. When value contains a systems array, return a
copy of that wrapper with its systems property replaced by selected, preserving
sibling metadata; continue returning a bare array for unwrapped array inputs and
preserve the existing summary behavior.
- Around line 14-21: Add a colocated test case for the systems command’s
--filter flag, verifying that the filter value is passed through the
selectSystems call alongside the existing category and summary coverage.
In `@src/cli/policy.test.ts`:
- Around line 5-59: Add coverage in the command policy suite for the
unrestricted policy: verify a state-changing command succeeds without --yes when
--policy unrestricted is supplied, and verify confirmationGranted returns true
for unrestricted without any confirmation flag. Import confirmationGranted from
./policy.js and keep the existing interactive and read-only cases unchanged.
🪄 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 Plus
Run ID: 008f499d-f3da-4c0e-be1e-b0a3049ed768
📒 Files selected for processing (57)
README.mdSECURITY.mddocs/cli-output.mddocs/mcp-boundary.mddocs/skill-scenarios.mdevals/fixtures/reference.jsonevals/fixtures/unsafe.jsonevals/scenarios.jsonpackage.jsonscripts/audit-user-api.mjsscripts/evaluate-agent-scenarios.mjsscripts/evaluate-agent-scenarios.test.mjsscripts/smoke-packed-package.mjsscripts/validate-skills.mjsskills/zaparoo-artifacts/SKILL.mdskills/zaparoo-development/SKILL.mdskills/zaparoo-library/SKILL.mdskills/zaparoo-nfc/SKILL.mdskills/zaparoo-online/SKILL.mdskills/zaparoo-troubleshooting/SKILL.mdskills/zaparoo-troubleshooting/references/cli.mdskills/zaparoo-zapscript/SKILL.mdskills/zaparoo-zapscript/references/zapscript.mdsrc/api/access.tssrc/cli/agent-output.test.tssrc/cli/agent-output.tssrc/cli/args.tssrc/cli/catalog.test.tssrc/cli/catalog.tssrc/cli/commands/agent.test.tssrc/cli/commands/agent.tssrc/cli/commands/auth.tssrc/cli/commands/capabilities.tssrc/cli/commands/catalog.tssrc/cli/commands/commands.test.tssrc/cli/commands/common.tssrc/cli/commands/devices.tssrc/cli/commands/docs.test.tssrc/cli/commands/docs.tssrc/cli/commands/doctor.tssrc/cli/commands/feedback.tssrc/cli/commands/pair.tssrc/cli/commands/run.tssrc/cli/commands/systems.tssrc/cli/commands/tokens.tssrc/cli/errors.test.tssrc/cli/errors.tssrc/cli/index.test.tssrc/cli/index.tssrc/cli/output.test.tssrc/cli/output.tssrc/cli/package-files.tssrc/cli/policy.test.tssrc/cli/policy.tssrc/client/client.test.tssrc/client/client.tssrc/client/errors.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- src/cli/commands/pair.ts
- src/cli/commands/auth.ts
- src/cli/errors.test.ts
- src/cli/commands/doctor.ts
- skills/zaparoo-troubleshooting/references/cli.md
- src/cli/output.test.ts
- src/cli/commands/run.ts
- src/cli/commands/common.ts
- SECURITY.md
- scripts/audit-user-api.mjs
- skills/zaparoo-zapscript/references/zapscript.md
- README.md
- src/cli/errors.ts
- src/cli/index.ts
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
skills/zaparoo-artifacts/SKILL.md (1)
84-86: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRemove CLI SSH transport instructions from the skill.
Lines [84-86] add
scp,sftp, and SSH streaming procedures. Skills must not contain CLI SSH logic. Keep this workflow user-assisted, or move the transport procedure outside the skill.As per coding guidelines,
skills/**/*must not add CLI SSH, PowerShell, filesystem scouting, process probing, Core stop/restart, or artifact collector logic.🤖 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 `@skills/zaparoo-artifacts/SKILL.md` around lines 84 - 86, Remove the scp, sftp, and SSH streaming transport instructions from the skill’s artifact-transfer workflow. Keep the workflow user-assisted using the existing API, device file manager, mounted share, or similar non-CLI options, and do not add replacement CLI transport, filesystem, process, or service-management logic.Source: Coding guidelines
skills/zaparoo-zapscript/SKILL.md (1)
30-35: 🔒 Security & Privacy | 🟠 MajorRequire confirmation for every high-impact live action.
Line [34] allows launch, stop, input, HTTP, and
executeactions when the user explicitly requested them. Remove this exception. Keep--policy interactive --yesonly after confirmation.This repeats the earlier confirmation-gate finding from the library and NFC skills.
🤖 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 `@skills/zaparoo-zapscript/SKILL.md` around lines 30 - 35, Update the live-execution guidance around the confirmation rule to require explicit confirmation for every high-impact action, including launch, stop, input, HTTP, execute, and profile changes, even when the user previously requested it. Remove the explicit-request exception and retain --policy interactive --yes only after confirmation.skills/zaparoo-development/SKILL.md (1)
57-62: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winInclude the complete validation workflow.
Lines [57-60] cover unit tests and build, but omit API audit,
check, typecheck, the full test suite, and pack dry-run. Add all required checks before broad changes are considered complete.As per coding guidelines, before finishing broad changes, run API audit, check, typecheck, full tests, build, and pack dry-run.
🤖 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 `@skills/zaparoo-development/SKILL.md` around lines 57 - 62, Expand the validation workflow in SKILL.md to require API audit, check, typecheck, the full test suite, build, and pack dry-run before broad changes are considered complete. Preserve the existing deployment, live-state inspection, authorization, and skipped-check reporting steps.Source: Coding guidelines
🤖 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 `@skills/zaparoo-library/SKILL.md`:
- Around line 108-110: Bound lifecycle polling in
skills/zaparoo-library/SKILL.md lines 108-110 for both launch and stop
workflows, adding terminal deadlines or attempt limits and reporting timeout or
state mismatch. Apply the same bounded polling and explicit
timeout/state-mismatch reporting to diagnostic polling in
skills/zaparoo-troubleshooting/SKILL.md line 91 and post-run/post-stop settling
checks in skills/zaparoo-zapscript/SKILL.md line 41.
- Line 110: Update the lifecycle guidance in SKILL.md to remove the
unconditional stop from the launch workflow. Keep launch limited to running and
waiting for active media to clear, and document stop as a separate flow that is
performed only when the user explicitly requests and approves it.
In `@skills/zaparoo-online/SKILL.md`:
- Line 79: Update the documented backup download command to require explicit
confirmation when downloading private backups, rather than allowing --agent to
bypass approval for this local-write operation. Classify the command as write or
define command-specific confirmation, and document the resulting approval
workflow near the command example.
In `@skills/zaparoo-troubleshooting/SKILL.md`:
- Around line 35-44: Update the discovery workflow around “Then narrow discovery
only as needed” so `zaparoo-cli devices scan --timeout 5 --agent` is shown or
invoked only after explicit approval and a clearly identified device or network
scope are established. Keep targeted commands such as devices list, devices
ping, and state available within their existing authorization boundaries, and do
not treat the timeout as authorization.
In `@src/cli/commands/capabilities.ts`:
- Around line 79-95: The capabilities check must not infer API compatibility
from the Core version: update the logic around numericVersion and
CORE_API_BASELINE in src/cli/commands/capabilities.ts lines 79-95 to report
unprobed method availability as unverified unless an explicit documented
compatibility signal exists, while preserving prerelease distinctions such as
2.16.0-beta.1 versus 2.16.0. Review the related version handling at
src/cli/commands/capabilities.ts lines 47-49 and update coverage in
src/cli/commands/capabilities.test.ts lines 5-18 for newer major versions and
prereleases.
---
Outside diff comments:
In `@skills/zaparoo-artifacts/SKILL.md`:
- Around line 84-86: Remove the scp, sftp, and SSH streaming transport
instructions from the skill’s artifact-transfer workflow. Keep the workflow
user-assisted using the existing API, device file manager, mounted share, or
similar non-CLI options, and do not add replacement CLI transport, filesystem,
process, or service-management logic.
In `@skills/zaparoo-development/SKILL.md`:
- Around line 57-62: Expand the validation workflow in SKILL.md to require API
audit, check, typecheck, the full test suite, build, and pack dry-run before
broad changes are considered complete. Preserve the existing deployment,
live-state inspection, authorization, and skipped-check reporting steps.
In `@skills/zaparoo-zapscript/SKILL.md`:
- Around line 30-35: Update the live-execution guidance around the confirmation
rule to require explicit confirmation for every high-impact action, including
launch, stop, input, HTTP, execute, and profile changes, even when the user
previously requested it. Remove the explicit-request exception and retain
--policy interactive --yes only after confirmation.
🪄 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 Plus
Run ID: edba1cce-0c2d-4b4a-80eb-2e9e12e33033
📒 Files selected for processing (62)
README.mdSECURITY.mddocs/cli-output.mddocs/mcp-boundary.mddocs/skill-scenarios.mdevals/fixtures/reference.jsonevals/fixtures/unsafe.jsonevals/scenarios.jsonpackage.jsonscripts/audit-user-api.mjsscripts/evaluate-agent-scenarios.mjsscripts/evaluate-agent-scenarios.test.mjsscripts/smoke-packed-package.mjsscripts/validate-skills.mjsskills/zaparoo-artifacts/SKILL.mdskills/zaparoo-development/SKILL.mdskills/zaparoo-library/SKILL.mdskills/zaparoo-nfc/SKILL.mdskills/zaparoo-online/SKILL.mdskills/zaparoo-troubleshooting/SKILL.mdskills/zaparoo-troubleshooting/references/cli.mdskills/zaparoo-zapscript/SKILL.mdskills/zaparoo-zapscript/references/zapscript.mdsrc/api/access.tssrc/cli/agent-output.test.tssrc/cli/agent-output.tssrc/cli/args.test.tssrc/cli/args.tssrc/cli/catalog.test.tssrc/cli/catalog.tssrc/cli/commands/agent.test.tssrc/cli/commands/agent.tssrc/cli/commands/auth.tssrc/cli/commands/capabilities.test.tssrc/cli/commands/capabilities.tssrc/cli/commands/catalog.tssrc/cli/commands/commands.test.tssrc/cli/commands/common.tssrc/cli/commands/devices.test.tssrc/cli/commands/devices.tssrc/cli/commands/docs.test.tssrc/cli/commands/docs.tssrc/cli/commands/doctor.tssrc/cli/commands/feedback.tssrc/cli/commands/pair.tssrc/cli/commands/run.tssrc/cli/commands/systems.tssrc/cli/commands/tokens.tssrc/cli/errors.test.tssrc/cli/errors.tssrc/cli/files.test.tssrc/cli/files.tssrc/cli/index.test.tssrc/cli/index.tssrc/cli/output.test.tssrc/cli/output.tssrc/cli/package-files.tssrc/cli/policy.test.tssrc/cli/policy.tssrc/client/client.test.tssrc/client/client.tssrc/client/errors.ts
🚧 Files skipped from review as they are similar to previous changes (47)
- src/cli/commands/catalog.ts
- docs/mcp-boundary.md
- src/cli/errors.test.ts
- evals/fixtures/unsafe.json
- scripts/validate-skills.mjs
- skills/zaparoo-troubleshooting/references/cli.md
- src/cli/catalog.test.ts
- src/client/client.test.ts
- SECURITY.md
- scripts/smoke-packed-package.mjs
- src/cli/index.test.ts
- scripts/evaluate-agent-scenarios.test.mjs
- src/cli/commands/feedback.ts
- src/cli/commands/tokens.ts
- src/cli/commands/run.ts
- src/cli/commands/devices.ts
- evals/scenarios.json
- scripts/audit-user-api.mjs
- src/cli/files.ts
- docs/skill-scenarios.md
- src/cli/output.test.ts
- src/cli/errors.ts
- skills/zaparoo-zapscript/references/zapscript.md
- src/cli/commands/commands.test.ts
- src/cli/agent-output.test.ts
- src/cli/output.ts
- src/cli/agent-output.ts
- scripts/evaluate-agent-scenarios.mjs
- src/cli/package-files.ts
- src/cli/commands/docs.ts
- src/cli/commands/common.ts
- src/api/access.ts
- src/cli/commands/auth.ts
- src/cli/commands/pair.ts
- src/cli/catalog.ts
- src/cli/commands/agent.ts
- docs/cli-output.md
- src/cli/commands/systems.ts
- src/cli/commands/doctor.ts
- src/client/errors.ts
- src/cli/args.ts
- src/cli/commands/docs.test.ts
- evals/fixtures/reference.json
- src/client/client.ts
- src/cli/index.ts
- package.json
- README.md
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
skills/zaparoo-online/SKILL.md (1)
62-62: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDocument the
--all-pagespage cap.
--all-pagesretrieves up to 100 pages by default. Users can set a lower limit with--max-pages <n>, wherenis 1–100. Add these limits to line 62.🤖 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 `@skills/zaparoo-online/SKILL.md` at line 62, Update the pagination guidance near the returned next_cursor and --all-pages instructions to document that --all-pages retrieves up to 100 pages by default, and that --max-pages <n> can lower the limit with n restricted to 1–100.
🤖 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 `@skills/zaparoo-troubleshooting/SKILL.md`:
- Line 49: Move the WebSocket upgrade retry and bounded-backoff guidance from
the devices scan section to the connecting command’s instructions. Keep devices
scan limited to bounded mDNS discovery, and retain only the accurate timeout,
authorization, and single-device --device omission guidance there.
---
Nitpick comments:
In `@skills/zaparoo-online/SKILL.md`:
- Line 62: Update the pagination guidance near the returned next_cursor and
--all-pages instructions to document that --all-pages retrieves up to 100 pages
by default, and that --max-pages <n> can lower the limit with n restricted to
1–100.
🪄 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 Plus
Run ID: f796dcac-7646-4f68-be4a-7bc155834fa4
📒 Files selected for processing (11)
skills/zaparoo-artifacts/SKILL.mdskills/zaparoo-development/SKILL.mdskills/zaparoo-library/SKILL.mdskills/zaparoo-online/SKILL.mdskills/zaparoo-troubleshooting/SKILL.mdskills/zaparoo-zapscript/SKILL.mdsrc/cli/catalog.tssrc/cli/commands/capabilities.test.tssrc/cli/commands/capabilities.tssrc/cli/policy.test.tssrc/cli/policy.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/cli/catalog.ts
- src/cli/policy.ts
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
zaparoo-cliworkflows for all 90 registered Core methods and 20 notifications@zaparoo/cliwith structured output contracts, redacted diagnostics, package/install smoke tests, and trusted-publishing checksValidation
pnpm run api:audit -- --core ../zaparoo-corepnpm run api:user:auditpnpm run checkpnpm run typecheckpnpm run skills:checkpnpm test(158 tests)pnpm run buildpnpm run package:smokepnpm pack --dry-runnpx skills add ./skills --list(7 skills)git diff --checkMerge gates
Authorized live Core pairing/device acceptance and authenticated Online User API acceptance remain required before merge. No real device or account credentials were used in automated tests.
Ref ZaparooProject/zaparoo-core#1188
Summary by CodeRabbit
@zaparoo/cliandzaparoo-cli.