Open
Conversation
Phase 1: workspace:* protocol, root scripts, eslint-plugin-oxlint, workspace scripts Phase 2: CI bunx→bun run, husky bunx→bun run Phase 3: process.env→env.ts (valibot, getter pattern) Phase 4: error swallowing→specific catches Phase 5: CLI positional→option-style, interface→type Phase 6: test.extends fixtures, specific matchers, vi.mock TODO Phase 7: docs cleanup, devkit hooks wiring Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…nt, remove typecheck (pre-existing type errors) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…, ops-harbor These files were created by subagents but missed from the initial commit-tree due to worktree index corruption. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR introduces environment variable validation infrastructure across the codebase using Valibot, refactors Vitest tests from lifecycle hooks to fixture-based patterns, updates plugin versions, converts CLI scripts to use Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
plugins/github-workflow/package.json (1)
11-14:⚠️ Potential issue | 🟠 MajorAdd required
mutilsdependency todependencies.The
mutilspackage is a mandatory prerequisite dependency for all plugins in the marketplace repository and is missing from this package.json. Add it to thedependenciessection.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/github-workflow/package.json` around lines 11 - 14, Add the missing required dependency "mutils" to the package.json dependencies: update the "dependencies" object (currently listing "@r_masseater/cc-plugin-lib" and "cc-hooks-ts") to include "mutils" with the appropriate version (e.g., a marketplace-required version or caret/latest as your repo policy dictates) so the plugin (plugins/github-workflow) declares the mandatory mutils dependency.plugins/sdd/package.json (1)
10-13:⚠️ Potential issue | 🟠 MajorAdd missing prerequisite dependency
mutils.This plugin manifest does not include
mutilsunderdependencies, which violates repository plugin requirements.🔧 Proposed fix
"dependencies": { "@r_masseater/cc-plugin-lib": "0.0.3", - "cc-hooks-ts": "2.1.91" + "cc-hooks-ts": "2.1.91", + "mutils": "workspace:*" },As per coding guidelines, "
plugins/*/package.json: All plugins in the marketplace repository must have mutils as a prerequisite dependency".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/sdd/package.json` around lines 10 - 13, The package.json for this plugin is missing the required prerequisite dependency "mutils"; update the manifest by adding "mutils" to the "dependencies" object in package.json (next to "@r_masseater/cc-plugin-lib" and "cc-hooks-ts") so the plugin meets the repository guideline requiring mutils as a prerequisite dependency.plugins/eslint-lsp/package.json (1)
11-14:⚠️ Potential issue | 🟠 MajorAdd
mutilsas a dependency for standards compliance.The
dependenciessection is missing the mandatorymutilsprerequisite required for all marketplace plugins per coding guidelines.Current dependencies
"dependencies": { "@r_masseater/cc-plugin-lib": "0.0.3", "cc-hooks-ts": "2.1.91" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/eslint-lsp/package.json` around lines 11 - 14, The package.json dependencies block for the eslint-lsp plugin is missing the required marketplace dependency "mutils"; update the "dependencies" object in package.json to include "mutils" with the appropriate version (e.g., the marketplace-mandated version or a compatible semver) so that the module is installed alongside "@r_masseater/cc-plugin-lib" and "cc-hooks-ts"—ensure the entry is added to the same dependencies map and package is published/lockfile updated.plugins/context/package.json (1)
11-14:⚠️ Potential issue | 🟠 MajorAdd
mutilsto dependencies inplugins/context/package.json.The
mutilsdependency is missing. All marketplace plugins must includemutilsas a prerequisite dependency per coding guidelines."dependencies": { "@r_masseater/cc-plugin-lib": "0.0.3", "cc-hooks-ts": "2.1.91", "mutils": "latest" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/context/package.json` around lines 11 - 14, The package.json dependencies block is missing the required marketplace prerequisite "mutils"; update the "dependencies" object in plugins' package.json (the existing "@r_masseater/cc-plugin-lib" and "cc-hooks-ts" entries) to include a "mutils" entry (e.g., "mutils": "latest") so that the module is declared and installed for the plugin.plugins/discord-notify/hooks/lib/discord-api.test.ts (1)
55-74:⚠️ Potential issue | 🟠 MajorMove global stub cleanup to
afterEachhook for failure-safe isolation.Tests that stub
fetchwithvi.unstubAllGlobals()at the end risk leaking into subsequent tests if an assertion fails first. Test isolation breaks because the cleanup call never executes.Use `afterEach` hook for guaranteed cleanup
-import { describe, expect, it, vi } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; @@ describe("sendMessage", () => { + afterEach(() => { + vi.unstubAllGlobals(); + }); + it("sends POST request to Discord API", async () => { @@ - vi.unstubAllGlobals(); }); @@ - vi.unstubAllGlobals(); }); }); @@ describe("createThread", () => { + afterEach(() => { + vi.unstubAllGlobals(); + }); + it("creates thread from message", async () => { @@ - vi.unstubAllGlobals(); }); @@ - vi.unstubAllGlobals(); }); });Applies to sendMessage (lines 55–91) and createThread (lines 95–133) suites.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/discord-notify/hooks/lib/discord-api.test.ts` around lines 55 - 74, The tests stub global fetch inside the sendMessage and createThread specs but call vi.unstubAllGlobals() only at the end of each test, which can leak if assertions fail; move the global cleanup into an afterEach hook in discord-api.test.ts so vi.unstubAllGlobals() always runs after every test, remove the per-test vi.unstubAllGlobals() calls, and ensure both the sendMessage and createThread describe blocks (or the file-level scope) use the afterEach to restore globals for failure-safe isolation.
🧹 Nitpick comments (10)
docs/ops-harbor-local-github-app.md (1)
1-1: Consider keeping the top NOTE in Japanese for consistency.The warning is helpful, but this file is otherwise Japanese; localizing this line will improve readability consistency for operators.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/ops-harbor-local-github-app.md` at line 1, The top HTML comment note in docs/ops-harbor-local-github-app.md is English while the rest of the file is Japanese; replace the existing comment "<!-- NOTE: This document describes operational setup and may become stale. Consider converting to executable setup scripts. -->" with an equivalent Japanese sentence to match the document language and maintain consistency for operators.plugins/agnix/hooks/lint-config-file.test.ts (1)
12-12: Track the DI refactor TODOs with an issue link and target seam.These TODOs are clear, but they’ll be easier to complete if each includes a tracking reference (issue/PR) and the intended DI boundary (e.g., logger, hook factory, agnix runner adapter).
I can draft a small DI-oriented test harness pattern for this file (factory + injected deps) if you want.
Also applies to: 24-24
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/agnix/hooks/lint-config-file.test.ts` at line 12, Add tracking metadata to the "TODO: refactor to dependency injection" comment in plugins/agnix/hooks/lint-config-file.test.ts by linking it to an issue/PR and specifying the intended DI seam(s) to be refactored (for example: logger, hook factory, agnix runner adapter); update the TODO comment text to include the issue URL/ID and the chosen seam(s) and repeat the same change for the other TODO noted at line 24 so future work is triaged and the target boundary for refactoring is explicit.plugins/code-review/skills/github-pr/env.test.ts (1)
55-65: Consider extracting a helper to reduce cleanup duplication.The try/finally pattern is solid for ensuring cleanup even when assertions fail. However, the cleanup logic is duplicated verbatim across both tests. A local helper within the describe block would maintain the per-test cleanup intent while reducing duplication.
♻️ Optional refactor using a helper function
describe("env.ts module import", () => { const originalToken = process.env.GITHUB_TOKEN; + const withCleanup = async <T>(fn: () => Promise<T>): Promise<T> => { + try { + return await fn(); + } finally { + if (originalToken !== undefined) { + process.env.GITHUB_TOKEN = originalToken; + } else { + delete process.env.GITHUB_TOKEN; + } + vi.resetModules(); + } + }; + test("exports env with GITHUB_TOKEN when set", async () => { process.env.GITHUB_TOKEN = "ghp_test_token"; vi.resetModules(); - try { + await withCleanup(async () => { const mod = await import("./env.js"); expect(mod.env.GITHUB_TOKEN).toBe("ghp_test_token"); - } finally { - if (originalToken !== undefined) { - process.env.GITHUB_TOKEN = originalToken; - } else { - delete process.env.GITHUB_TOKEN; - } - vi.resetModules(); - } + }); }); test("throws when GITHUB_TOKEN is missing", async () => { delete process.env.GITHUB_TOKEN; vi.resetModules(); - try { + await withCleanup(async () => { await expect(() => import("./env.js")).rejects.toThrow(); - } finally { - if (originalToken !== undefined) { - process.env.GITHUB_TOKEN = originalToken; - } else { - delete process.env.GITHUB_TOKEN; - } - vi.resetModules(); - } + }); }); });Also applies to: 71-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/code-review/skills/github-pr/env.test.ts` around lines 55 - 65, Extract a small helper inside the describe block to centralize the try/finally cleanup used in both tests: create a helper (e.g., withEnvRestore or runTestWithEnv) that accepts an async callback, saves originalToken = process.env.GITHUB_TOKEN, runs the callback (where you import("./env.js") and assert mod.env.GITHUB_TOKEN), and in a finally block restores process.env.GITHUB_TOKEN (or deletes it) and calls vi.resetModules(); then replace the duplicated try/finally in both tests with calls to this helper to remove the duplicated cleanup logic.plugins/sdd/skills/spec-template/scripts/copy-templates.test.ts (1)
65-81: Consider centralizing duplicated fixture setup across both copy-template test files.The temp-dir fixture lifecycle is good, but this block is effectively duplicated in the steering test file. A shared helper would reduce drift and future maintenance cost.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/sdd/skills/spec-template/scripts/copy-templates.test.ts` around lines 65 - 81, The duplicated temp-dir fixture logic (the test.extend<TemplateFixture> block that defines templatesDir and targetDir) should be extracted into a shared helper so both copy-template test files reuse it; create a utility (e.g., a function or exported Playwright fixture factory like createTemplateFixtures) that encapsulates the mkdirSync/rmSync lifecycle and returns templatesDir and targetDir fixtures, then replace the inline test.extend usage in this file (symbols: test, TemplateFixture, templatesDir, targetDir) and the steering test with imports from that shared helper so both tests call the same factory and avoid drift.plugins/devkit/skills/standards-audit/scripts/checks/scripts/index.test.ts (1)
47-49: Tautological assertion mirrors the find predicate.
fis found usingf.rule === "scripts", so assertingf?.rule === "scripts"is redundant. Thef?.severityassertion on line 49 is meaningful since severity isn't part of the filter.Consider either
expect(f).toBeDefined()or removing line 48 entirely, since line 49 will already fail with a clear message iffis undefined.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/devkit/skills/standards-audit/scripts/checks/scripts/index.test.ts` around lines 47 - 49, The test redundantly asserts f?.rule === "scripts" even though f was found using findings.find with that predicate; update the assertion to either check that the found object exists (e.g., replace the tautological expect(f?.rule).toBe("scripts") with expect(f).toBeDefined()) or remove that line entirely so only the meaningful assertion on f?.severity remains; locate the variable f defined by findings.find in index.test.ts and make the change there.plugins/devkit/skills/standards-audit/scripts/audit.test.ts (1)
95-96: Tautological assertion: the check re-asserts the find predicate.Line 95 finds
webScopeusingsr.scope.label === "apps/web", so ifwebScopeis defined, itsscope.labelis already guaranteed to be"apps/web". The previoustoBeDefined()was clearer about the test intent: verifying the workspace exists in the results.If the goal is to assert a specific value (for consistency with other tests), consider asserting a different property not used in the find predicate, or keep
toBeDefined()since the find already encodes the label constraint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/devkit/skills/standards-audit/scripts/audit.test.ts` around lines 95 - 96, The test contains a tautological assertion: it finds webScope via result.scopeResults.find(sr => sr.scope.label === "apps/web") and then asserts webScope?.scope.label is "apps/web", which merely rechecks the find predicate; change the assertion to verify presence or a different property instead—replace the redundant expect(webScope?.scope.label).toBe("apps/web") with expect(webScope).toBeDefined() to assert the workspace exists, or assert another property on webScope (e.g., webScope.someOtherProperty) if you need to validate a value distinct from the find predicate; update the assertion referencing the webScope variable accordingly.plugins/context/hooks/redirect-claude-md-edit.test.ts (1)
11-11: Good documentation of technical debt.The TODO comments appropriately flag that the module-level
vi.mock()calls should be refactored to dependency injection. This would improve test clarity and align with the learning about making tests readable to junior engineers, as dependency injection makes dependencies explicit rather than relying on Vitest's implicit hoisting behavior.Since this pattern appears across multiple test files in this PR, would you like me to:
- Create a tracking issue to coordinate this refactoring across all affected files?
- Generate a concrete example of how one of these tests could be refactored to use dependency injection?
Also applies to: 16-16
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/context/hooks/redirect-claude-md-edit.test.ts` at line 11, The module-level vi.mock() calls in redirect-claude-md-edit.test.ts should be removed and replaced with explicit dependency injection: refactor the module under test to export an initializer/factory (e.g., createRedirectClaudeMdEdit or initRedirectHandler) that accepts its external dependencies (the things currently mocked via vi.mock), then in the test create and pass explicit mock implementations from within each test scope (use vi.fn() inside beforeEach/it). Update tests to import the factory function and supply those mocks instead of relying on module-level vi.mock() hoisting to make dependencies explicit and easily testable.plugins/mutils/skills/skill-create/scripts/validate.ts (1)
28-28: Usage text is narrower than actual accepted input.The parser still accepts a positional fallback, but the error help text only documents the flag form. Please include both forms to avoid confusion.
Suggested tweak
- process.stderr.write("Usage: validate.ts --skill-dir <skill-dir>\n"); + process.stderr.write("Usage: validate.ts --skill-dir <skill-dir> | validate.ts <skill-dir>\n");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/mutils/skills/skill-create/scripts/validate.ts` at line 28, Update the usage message emitted by the script so it documents both the flagged and positional forms accepted by the parser: modify the process.stderr.write call in validate.ts to include the flag form "--skill-dir <skill-dir>" and the positional fallback (e.g., "<skill-dir>") so users see both acceptable invocations; ensure the message remains clear and concise while referencing the same process.stderr.write location in the file.plugins/debug/hooks/session-start.test.ts (1)
6-8: Avoid coupling integration tests tocapturedRuns[0].Using a fixed index is brittle if another
defineHookregistration is added later. Prefer taking the latest captured callback (or resetting the array per test).♻️ Suggested refactor
const { capturedRuns } = vi.hoisted(() => ({ capturedRuns: [] as Array<(context: unknown) => Promise<unknown>>, })); + +function getCapturedRun() { + const run = capturedRuns.at(-1); + expect(typeof run).toBe("function"); + return run as (context: unknown) => Promise<unknown>; +} @@ - const capturedRun = capturedRuns[0]; - expect(typeof capturedRun).toBe("function"); + const capturedRun = getCapturedRun(); @@ - await capturedRun!(fakeContext); + await capturedRun(fakeContext); @@ - const capturedRun = capturedRuns[0]; - expect(typeof capturedRun).toBe("function"); + const capturedRun = getCapturedRun(); @@ - await capturedRun!(fakeContext); + await capturedRun(fakeContext);Also applies to: 24-25, 149-150, 194-195
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/debug/hooks/session-start.test.ts` around lines 6 - 8, Tests currently access capturedRuns[0] (and other fixed indices) which is brittle; change the tests to either reset capturedRuns before each test (e.g., clear capturedRuns inside a beforeEach) or always select the most recently pushed callback (use capturedRuns[capturedRuns.length - 1]) when asserting hooks. Update all references to capturedRuns[0] (and the other fixed-index usages in the file) to use the latest element or clear the array per-test so defineHook registrations don't collide; keep the vi.hoisted declaration and ensure any assertion uses the same symbol capturedRuns to locate the callback.plugins/discord-notify/hooks/lib/env.ts (1)
22-32: Consider usingenvinstead ofprocess.envinreadDiscordEnv()for consistency.The module exports a validated
envobject butreadDiscordEnv()still reads directly fromprocess.env. While this works (validation happens at import time), usingenv.DISCORD_NOTIFY_CHANNEL_IDandenv.DISCORD_BOT_TOKENwould be more consistent with the new pattern.♻️ Optional refactor for consistency
function readDiscordEnv(): DiscordEnv | null { - const channelId = process.env.DISCORD_NOTIFY_CHANNEL_ID; + const channelId = env.DISCORD_NOTIFY_CHANNEL_ID; if (!channelId) return null; - const botToken = process.env.DISCORD_BOT_TOKEN; + const botToken = env.DISCORD_BOT_TOKEN; if (!botToken) { throw new Error("DISCORD_NOTIFY_CHANNEL_ID is set but DISCORD_BOT_TOKEN is missing"); } return { channelId, botToken }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/discord-notify/hooks/lib/env.ts` around lines 22 - 32, The readDiscordEnv function currently reads directly from process.env; update it to use the validated exported env object instead by replacing process.env.DISCORD_NOTIFY_CHANNEL_ID and process.env.DISCORD_BOT_TOKEN with env.DISCORD_NOTIFY_CHANNEL_ID and env.DISCORD_BOT_TOKEN (preserve the existing control flow: return null if channel ID is absent, throw the same error message if bot token is missing, and return the { channelId, botToken } DiscordEnv object).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/ops-harbor-control-plane/src/lib/config.ts`:
- Around line 97-102: The catch block in loadStoredConfig currently logs the
full error object which may contain secret-bearing config values; change the
catch to log a sanitized summary (e.g. a short message plus non-sensitive error
type or e.message only) instead of the entire error object and avoid printing
full stack traces—replace console.error("[ops-harbor-control-plane]
loadStoredConfig: failed to parse config:", e) with a safe log like a
single-line summary indicating parse/validation failure and the error name or
message only (or a hardcoded hint) so that loadStoredConfig still returns {} but
no sensitive data is emitted.
In `@apps/ops-harbor/src/lib/config.ts`:
- Around line 120-121: In loadConfig, avoid logging the raw caught object
(currently console.error("[ops-harbor] loadConfig: failed to load or parse
config:", e)); instead log a sanitized message and minimal non-sensitive detail
(e.g., console.error("[ops-harbor] loadConfig: failed to load or parse config:",
e?.message ?? "unknown error")) and then rethrow the original error (throw e) so
callers still get the full exception; update the logging in the catch block
around loadConfig to output only the safe message/fields rather than the entire
error object.
In `@apps/ops-harbor/src/lib/sync.ts`:
- Around line 16-19: The catch in syncFromControlPlane currently logs the raw
thrown object (variable e) which can leak sensitive request data; replace the
console.error("[ops-harbor] syncFromControlPlane: triggerSync failed:", e) with
a sanitized log that only includes non-sensitive fields (e.g., e.name,
e.message, e.code or e.status) or a small sanitizedError object produced by a
new helper (e.g., sanitizeError(e)) and avoid logging stacks, headers, bodies or
the full error object; keep the existing synchronized = undefined behavior. Use
the symbols syncFromControlPlane, triggerSync, and the local variable e to
locate and change the logging.
In `@apps/sdd-webapp/src/lib/status-parser.test.ts`:
- Line 4: Remove the unused import randomUUID from the top of the test file;
locate the import statement that reads import { randomUUID } from "node:crypto";
in apps/sdd-webapp/src/lib/status-parser.test.ts and delete randomUUID (or the
entire import line if it's the only specifier) so no unused imports remain.
In `@docs/api/cc-plugin-lib.md`:
- Around line 74-79: The code sample showing HookLogger is using malformed
nested backtick fences; replace the incorrect opening/closing sequence with a
single proper TypeScript code fence so the example reads: start with
```typescript, include the HookLogger.fromFile(import.meta.filename) and
logger.debug("Hook started"); lines, and end with ```; ensure the example
references HookLogger.fromFile and logger.debug exactly as shown and remove the
extra backticks.
- Around line 100-114: The markdown example contains malformed fenced code
blocks and stray backticks around the wrapRun/defineHook example which breaks
rendering; fix by replacing the nested/back-to-back fences with a single proper
TypeScript fence so the snippet starting with "const hook = defineHook({ ...
run: wrapRun(logger, (context) => { ... }) });" is enclosed between one opening
"```typescript" and one closing "```", ensuring no extra backticks or stray
lines remain after the closing fence and preserving the references to wrapRun,
defineHook, logger, and the hook variable.
In `@docs/api/ui-dev.md`:
- Line 5: The documented signature for findConfigPath is incorrect because
flowName is optional; update the API doc to reflect that by changing the
signature shown for findConfigPath to indicate an optional parameter (e.g., use
flowName?: string or equivalent), and add a brief note that calling
findConfigPath() with no arguments returns the first available config while
findConfigPath("registration") locates a specific flow config to match test
usage.
In `@plugins/code-review/package.json`:
- Around line 18-19: The package manifest for the code-review plugin is missing
the required prerequisite dependency "mutils"; update the plugins/code-review
package.json dependencies section to include "mutils" (use the same workspace
versioning convention as other internal deps, e.g., add "mutils": "workspace:*"
alongside "@repo/ts-config" and "@repo/vitest-config") so the plugin lists
mutils as a dependency.
In `@plugins/code-review/skills/github-pr/scripts/get-ci-logs.test.ts`:
- Around line 55-61: The fixture installs a console.log spy via
vi.spyOn(console, "log") but never restores it, causing test-state leakage;
update the fixture to capture the spy (e.g., const consoleLogSpy =
vi.spyOn(console, "log") ) before await use() and then restore it in teardown
after await use() by calling consoleLogSpy.mockRestore() (or
vi.restoreAllMocks()) so the spy on console.log is removed between tests; keep
the existing mockMkdir/mockWriteFile setup as-is.
In `@plugins/devkit/package.json`:
- Around line 11-15: Add the missing "mutils" dependency to this package's
package.json under the "dependencies" object (the same section containing
"@r_masseater/cc-plugin-lib"/"cc-hooks-ts"/"valibot"); pick the project-standard
version (or caret latest compatible version) and save it as "mutils":
"<version>"; then run the package manager to update the lockfile. Ensure the
dependency is listed exactly under "dependencies" so build and CI tooling pick
it up.
In `@plugins/discord-notify/package.json`:
- Around line 11-16: Add the missing prerequisite dependency "mutils" to the
package.json dependencies section used by this plugin so it follows guidelines;
update the "dependencies" object (where "@r_masseater/cc-plugin-lib",
"better-sqlite3", "cc-hooks-ts", and "valibot" are declared) to include an entry
for "mutils" with the appropriate version specifier consistent with other
plugins.
In `@plugins/mutils/hooks/lib/env.ts`:
- Around line 3-7: The module currently calls v.parse at import time (EnvSchema
and env), causing hard failure if HOME is missing; change to validate lazily
instead: stop exporting a pre-parsed env value and either (a) replace
v.parse(process.env) with v.safeParse(process.env) and export a non-throwing
accessor that returns a safeParse result or defaults, or (b) move the v.parse
call into a function like getEnv() that callers invoke when they need validated
values so missing HOME can be handled gracefully; update any consumers of env to
call the new accessor/getEnv and ensure errors are handled where used rather
than during module initialization.
In `@plugins/mutils/skills/skill-create/scripts/inject-references.ts`:
- Around line 19-23: The usage message is inconsistent with the code's
acceptance of a positional argument; update the error output that currently runs
when skillDirArg is falsy to document both accepted forms (e.g. "--skill-dir
<skill-dir> or <skill-dir> (positional)") instead of only "--skill-dir", by
changing the string passed to process.stderr.write where skillDirArg is checked
(variables: skillDirArg, values, positionals[0]); alternatively, if you prefer
to remove positional support, drop the positionals[0] fallback and only accept
values["skill-dir"] so the current usage text remains accurate.
In `@plugins/research/package.json`:
- Around line 21-22: The package.json for the plugin is missing the required
prerequisite dependency "mutils"; update the plugin manifest (plugins/research
package.json) to include "mutils" as a prerequisite dependency alongside the
existing workspace entries (e.g., next to "@repo/ts-config" and
"@repo/vitest-config") by adding the "mutils" entry with the appropriate
workspace version spec (e.g., "workspace:*") under the same
dependencies/prerequisites section so the plugin satisfies the repository
guideline requiring mutils as a prerequisite.
In
`@plugins/research/skills/library-research/scripts/lib/resolve-knowledge-dir.test.ts`:
- Line 4: The file uses the imported "test" alias before it's redefined by the
fixture, causing a temporal-dead-zone ReferenceError; rename the const fixture
from test to testWithProjectDir (the declaration currently: const test =
base.extend<ProjectDirFixture>(...)) and update all calls that invoke test(...)
in the file (the suites at the top that currently call test(...) — lines
referencing test in the describe/test blocks) to call testWithProjectDir(...),
ensuring the import remains "import { test as base, ... }" and that
ProjectDirFixture type usage is unchanged.
In `@plugins/ui-dev/package.json`:
- Around line 17-18: The plugin manifest (package.json) is missing the required
marketplace prerequisite "mutils"; add an entry for "mutils" with the workspace
version spec (e.g., "mutils": "workspace:*") to the plugin's
prerequisites/prerequisite section in package.json so it matches other
marketplace plugins' manifests and the repo guideline requiring mutils as a
prerequisite.
---
Outside diff comments:
In `@plugins/context/package.json`:
- Around line 11-14: The package.json dependencies block is missing the required
marketplace prerequisite "mutils"; update the "dependencies" object in plugins'
package.json (the existing "@r_masseater/cc-plugin-lib" and "cc-hooks-ts"
entries) to include a "mutils" entry (e.g., "mutils": "latest") so that the
module is declared and installed for the plugin.
In `@plugins/discord-notify/hooks/lib/discord-api.test.ts`:
- Around line 55-74: The tests stub global fetch inside the sendMessage and
createThread specs but call vi.unstubAllGlobals() only at the end of each test,
which can leak if assertions fail; move the global cleanup into an afterEach
hook in discord-api.test.ts so vi.unstubAllGlobals() always runs after every
test, remove the per-test vi.unstubAllGlobals() calls, and ensure both the
sendMessage and createThread describe blocks (or the file-level scope) use the
afterEach to restore globals for failure-safe isolation.
In `@plugins/eslint-lsp/package.json`:
- Around line 11-14: The package.json dependencies block for the eslint-lsp
plugin is missing the required marketplace dependency "mutils"; update the
"dependencies" object in package.json to include "mutils" with the appropriate
version (e.g., the marketplace-mandated version or a compatible semver) so that
the module is installed alongside "@r_masseater/cc-plugin-lib" and
"cc-hooks-ts"—ensure the entry is added to the same dependencies map and package
is published/lockfile updated.
In `@plugins/github-workflow/package.json`:
- Around line 11-14: Add the missing required dependency "mutils" to the
package.json dependencies: update the "dependencies" object (currently listing
"@r_masseater/cc-plugin-lib" and "cc-hooks-ts") to include "mutils" with the
appropriate version (e.g., a marketplace-required version or caret/latest as
your repo policy dictates) so the plugin (plugins/github-workflow) declares the
mandatory mutils dependency.
In `@plugins/sdd/package.json`:
- Around line 10-13: The package.json for this plugin is missing the required
prerequisite dependency "mutils"; update the manifest by adding "mutils" to the
"dependencies" object in package.json (next to "@r_masseater/cc-plugin-lib" and
"cc-hooks-ts") so the plugin meets the repository guideline requiring mutils as
a prerequisite dependency.
---
Nitpick comments:
In `@docs/ops-harbor-local-github-app.md`:
- Line 1: The top HTML comment note in docs/ops-harbor-local-github-app.md is
English while the rest of the file is Japanese; replace the existing comment
"<!-- NOTE: This document describes operational setup and may become stale.
Consider converting to executable setup scripts. -->" with an equivalent
Japanese sentence to match the document language and maintain consistency for
operators.
In `@plugins/agnix/hooks/lint-config-file.test.ts`:
- Line 12: Add tracking metadata to the "TODO: refactor to dependency injection"
comment in plugins/agnix/hooks/lint-config-file.test.ts by linking it to an
issue/PR and specifying the intended DI seam(s) to be refactored (for example:
logger, hook factory, agnix runner adapter); update the TODO comment text to
include the issue URL/ID and the chosen seam(s) and repeat the same change for
the other TODO noted at line 24 so future work is triaged and the target
boundary for refactoring is explicit.
In `@plugins/code-review/skills/github-pr/env.test.ts`:
- Around line 55-65: Extract a small helper inside the describe block to
centralize the try/finally cleanup used in both tests: create a helper (e.g.,
withEnvRestore or runTestWithEnv) that accepts an async callback, saves
originalToken = process.env.GITHUB_TOKEN, runs the callback (where you
import("./env.js") and assert mod.env.GITHUB_TOKEN), and in a finally block
restores process.env.GITHUB_TOKEN (or deletes it) and calls vi.resetModules();
then replace the duplicated try/finally in both tests with calls to this helper
to remove the duplicated cleanup logic.
In `@plugins/context/hooks/redirect-claude-md-edit.test.ts`:
- Line 11: The module-level vi.mock() calls in redirect-claude-md-edit.test.ts
should be removed and replaced with explicit dependency injection: refactor the
module under test to export an initializer/factory (e.g.,
createRedirectClaudeMdEdit or initRedirectHandler) that accepts its external
dependencies (the things currently mocked via vi.mock), then in the test create
and pass explicit mock implementations from within each test scope (use vi.fn()
inside beforeEach/it). Update tests to import the factory function and supply
those mocks instead of relying on module-level vi.mock() hoisting to make
dependencies explicit and easily testable.
In `@plugins/debug/hooks/session-start.test.ts`:
- Around line 6-8: Tests currently access capturedRuns[0] (and other fixed
indices) which is brittle; change the tests to either reset capturedRuns before
each test (e.g., clear capturedRuns inside a beforeEach) or always select the
most recently pushed callback (use capturedRuns[capturedRuns.length - 1]) when
asserting hooks. Update all references to capturedRuns[0] (and the other
fixed-index usages in the file) to use the latest element or clear the array
per-test so defineHook registrations don't collide; keep the vi.hoisted
declaration and ensure any assertion uses the same symbol capturedRuns to locate
the callback.
In `@plugins/devkit/skills/standards-audit/scripts/audit.test.ts`:
- Around line 95-96: The test contains a tautological assertion: it finds
webScope via result.scopeResults.find(sr => sr.scope.label === "apps/web") and
then asserts webScope?.scope.label is "apps/web", which merely rechecks the find
predicate; change the assertion to verify presence or a different property
instead—replace the redundant expect(webScope?.scope.label).toBe("apps/web")
with expect(webScope).toBeDefined() to assert the workspace exists, or assert
another property on webScope (e.g., webScope.someOtherProperty) if you need to
validate a value distinct from the find predicate; update the assertion
referencing the webScope variable accordingly.
In `@plugins/devkit/skills/standards-audit/scripts/checks/scripts/index.test.ts`:
- Around line 47-49: The test redundantly asserts f?.rule === "scripts" even
though f was found using findings.find with that predicate; update the assertion
to either check that the found object exists (e.g., replace the tautological
expect(f?.rule).toBe("scripts") with expect(f).toBeDefined()) or remove that
line entirely so only the meaningful assertion on f?.severity remains; locate
the variable f defined by findings.find in index.test.ts and make the change
there.
In `@plugins/discord-notify/hooks/lib/env.ts`:
- Around line 22-32: The readDiscordEnv function currently reads directly from
process.env; update it to use the validated exported env object instead by
replacing process.env.DISCORD_NOTIFY_CHANNEL_ID and
process.env.DISCORD_BOT_TOKEN with env.DISCORD_NOTIFY_CHANNEL_ID and
env.DISCORD_BOT_TOKEN (preserve the existing control flow: return null if
channel ID is absent, throw the same error message if bot token is missing, and
return the { channelId, botToken } DiscordEnv object).
In `@plugins/mutils/skills/skill-create/scripts/validate.ts`:
- Line 28: Update the usage message emitted by the script so it documents both
the flagged and positional forms accepted by the parser: modify the
process.stderr.write call in validate.ts to include the flag form "--skill-dir
<skill-dir>" and the positional fallback (e.g., "<skill-dir>") so users see both
acceptable invocations; ensure the message remains clear and concise while
referencing the same process.stderr.write location in the file.
In `@plugins/sdd/skills/spec-template/scripts/copy-templates.test.ts`:
- Around line 65-81: The duplicated temp-dir fixture logic (the
test.extend<TemplateFixture> block that defines templatesDir and targetDir)
should be extracted into a shared helper so both copy-template test files reuse
it; create a utility (e.g., a function or exported Playwright fixture factory
like createTemplateFixtures) that encapsulates the mkdirSync/rmSync lifecycle
and returns templatesDir and targetDir fixtures, then replace the inline
test.extend usage in this file (symbols: test, TemplateFixture, templatesDir,
targetDir) and the steering test with imports from that shared helper so both
tests call the same factory and avoid drift.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d0258974-d437-4519-9c7a-2c8cb9a8bddd
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (124)
.claude-plugin/marketplace.json.github/workflows/ci.yml.husky/pre-commit.husky/pre-pushapps/ops-harbor-control-plane/src/lib/config.tsapps/ops-harbor-control-plane/src/lib/github.test.tsapps/ops-harbor-control-plane/src/server.test.tsapps/ops-harbor/src/env.tsapps/ops-harbor/src/lib/automation.tsapps/ops-harbor/src/lib/config.tsapps/ops-harbor/src/lib/sync.tsapps/sdd-webapp/package.jsonapps/sdd-webapp/src/env.tsapps/sdd-webapp/src/features/file-system/index.test.tsapps/sdd-webapp/src/lib/config.test.tsapps/sdd-webapp/src/lib/config.tsapps/sdd-webapp/src/lib/project-path.test.tsapps/sdd-webapp/src/lib/specs-scanner.test.tsapps/sdd-webapp/src/lib/status-parser.test.tsapps/sdd-webapp/src/lib/status-store.test.tsapps/sdd-webapp/src/lib/status-store.tsapps/sdd-webapp/src/server.test.tsdocs/api/cc-plugin-lib.mddocs/api/doc-engine.mddocs/api/mutils.mddocs/api/research.mddocs/api/sdd-webapp.mddocs/api/ui-dev.mddocs/dev-commands.mddocs/ops-harbor-local-github-app.mdknip.jsonpackage.jsonpackages/cc-plugin-lib/README.mdpackages/doc-engine/package.jsonpackages/ops-harbor-core/src/port-finder.test.tspackages/ops-harbor-core/src/sqlite.tspackages/repository-lint/package.jsonpackages/repository-lint/src/rules/hook-file-structure.tspackages/repository-lint/src/rules/require-version-bump.tsplugins/agnix/hooks/lib/agnix-runner.test.tsplugins/agnix/hooks/lint-config-file.test.tsplugins/agnix/package.jsonplugins/agnix/plugin.jsonplugins/code-review/package.jsonplugins/code-review/plugin.jsonplugins/code-review/skills/github-pr/env.test.tsplugins/code-review/skills/github-pr/lib/pr-info.test.tsplugins/code-review/skills/github-pr/scripts/get-ci-logs.test.tsplugins/code-review/skills/github-pr/scripts/get-ci-status.test.tsplugins/code-review/skills/github-pr/scripts/get-comments-by-thread.test.tsplugins/code-review/skills/github-pr/scripts/get-unresolved-threads.test.tsplugins/context/hooks/redirect-claude-md-edit.test.tsplugins/context/package.jsonplugins/context/plugin.jsonplugins/debug/hooks/entry/session-start.tsplugins/debug/hooks/lib/debug-file.tsplugins/debug/hooks/lib/env.tsplugins/debug/hooks/session-start.test.tsplugins/debug/package.jsonplugins/debug/plugin.jsonplugins/devkit/AGENTS.mdplugins/devkit/hooks/entry/block-deny-command.tsplugins/devkit/hooks/entry/standards-audit.tsplugins/devkit/hooks/hooks.jsonplugins/devkit/hooks/lib/env.tsplugins/devkit/package.jsonplugins/devkit/plugin.jsonplugins/devkit/skills/standards-audit/scripts/audit.test.tsplugins/devkit/skills/standards-audit/scripts/audit.tsplugins/devkit/skills/standards-audit/scripts/checks/code-patterns/index.test.tsplugins/devkit/skills/standards-audit/scripts/checks/config-files/index.test.tsplugins/devkit/skills/standards-audit/scripts/checks/documentation/index.test.tsplugins/devkit/skills/standards-audit/scripts/checks/quality-libs/index.test.tsplugins/devkit/skills/standards-audit/scripts/checks/scripts/index.test.tsplugins/devkit/skills/standards-audit/scripts/checks/versions/index.test.tsplugins/devkit/skills/standards-audit/scripts/core/context.test.tsplugins/devkit/skills/standards/scripts/expand-intentional-violations.tsplugins/discord-notify/hooks/entry/create-thread.test.tsplugins/discord-notify/hooks/entry/send-discord.test.tsplugins/discord-notify/hooks/lib/db.test.tsplugins/discord-notify/hooks/lib/db.tsplugins/discord-notify/hooks/lib/discord-api.test.tsplugins/discord-notify/hooks/lib/env.test.tsplugins/discord-notify/hooks/lib/env.tsplugins/discord-notify/package.jsonplugins/discord-notify/plugin.jsonplugins/eslint-lsp/hooks/check-eslint-server.test.tsplugins/eslint-lsp/package.jsonplugins/eslint-lsp/plugin.jsonplugins/github-workflow/hooks/auto-ci-watch.test.tsplugins/github-workflow/hooks/check-branch-status.test.tsplugins/github-workflow/hooks/check-push-pr-conflicts.test.tsplugins/github-workflow/hooks/log-git-status.test.tsplugins/github-workflow/package.jsonplugins/github-workflow/plugin.jsonplugins/mutils/README.mdplugins/mutils/hooks/entry/check-context-version.tsplugins/mutils/hooks/lib/env.tsplugins/mutils/package.jsonplugins/mutils/plugin.jsonplugins/mutils/skills/ccs-handoff/ccs-handoff.tsplugins/mutils/skills/skill-create/scripts/detect-context.test.tsplugins/mutils/skills/skill-create/scripts/detect-context.tsplugins/mutils/skills/skill-create/scripts/inject-references.test.tsplugins/mutils/skills/skill-create/scripts/inject-references.tsplugins/mutils/skills/skill-create/scripts/scaffold.test.tsplugins/mutils/skills/skill-create/scripts/validate.test.tsplugins/mutils/skills/skill-create/scripts/validate.tsplugins/mutils/skills/workspace-id/generate.tsplugins/progress-tracker/package.jsonplugins/progress-tracker/plugin.jsonplugins/research/package.jsonplugins/research/plugin.jsonplugins/research/skills/library-research/scripts/lib/resolve-knowledge-dir.test.tsplugins/sdd/hooks/block-archived-edit.test.tsplugins/sdd/hooks/require-phase-status.test.tsplugins/sdd/package.jsonplugins/sdd/plugin.jsonplugins/sdd/skills/spec-template/scripts/copy-templates.test.tsplugins/sdd/skills/steering/scripts/copy-templates.test.tsplugins/ui-dev/hooks/notify-phase-status.test.tsplugins/ui-dev/lib/config.test.tsplugins/ui-dev/package.jsonplugins/ui-dev/plugin.json
Comment on lines
+97
to
102
| } catch (e) { | ||
| // ENOENT is guarded by existsSync above. Remaining errors are JSON parse | ||
| // or schema failures from a corrupt config file. Log and return defaults | ||
| // so the control plane can still start and write a fresh config. | ||
| console.error("[ops-harbor-control-plane] loadStoredConfig: failed to parse config:", e); | ||
| return {}; |
There was a problem hiding this comment.
Avoid logging raw parse/validation errors for secret-bearing config.
Line 101 logs the full caught error object, which can expose sensitive config values in logs (especially on schema-validation failures). Log a sanitized error summary instead.
🔧 Suggested safe logging change
- } catch (e) {
+ } catch (e) {
// ENOENT is guarded by existsSync above. Remaining errors are JSON parse
// or schema failures from a corrupt config file. Log and return defaults
// so the control plane can still start and write a fresh config.
- console.error("[ops-harbor-control-plane] loadStoredConfig: failed to parse config:", e);
+ const errorType = e instanceof Error ? e.name : typeof e;
+ console.error(
+ `[ops-harbor-control-plane] loadStoredConfig: failed to parse config (${errorType})`,
+ );
return {};
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ops-harbor-control-plane/src/lib/config.ts` around lines 97 - 102, The
catch block in loadStoredConfig currently logs the full error object which may
contain secret-bearing config values; change the catch to log a sanitized
summary (e.g. a short message plus non-sensitive error type or e.message only)
instead of the entire error object and avoid printing full stack traces—replace
console.error("[ops-harbor-control-plane] loadStoredConfig: failed to parse
config:", e) with a safe log like a single-line summary indicating
parse/validation failure and the error name or message only (or a hardcoded
hint) so that loadStoredConfig still returns {} but no sensitive data is
emitted.
Comment on lines
+120
to
+121
| console.error("[ops-harbor] loadConfig: failed to load or parse config:", e); | ||
| throw e; |
There was a problem hiding this comment.
Avoid logging raw thrown objects from config load failures.
Line [120] logs the full caught value. In failure cases, raw error objects can include sensitive config context (for example token-bearing content). Log a sanitized message and still rethrow.
🔒 Suggested safe logging adjustment
- console.error("[ops-harbor] loadConfig: failed to load or parse config:", e);
+ const errorSummary =
+ e instanceof Error ? { name: e.name } : { type: typeof e };
+ console.error("[ops-harbor] loadConfig: failed to load or parse config", errorSummary);
throw e;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ops-harbor/src/lib/config.ts` around lines 120 - 121, In loadConfig,
avoid logging the raw caught object (currently console.error("[ops-harbor]
loadConfig: failed to load or parse config:", e)); instead log a sanitized
message and minimal non-sensitive detail (e.g., console.error("[ops-harbor]
loadConfig: failed to load or parse config:", e?.message ?? "unknown error"))
and then rethrow the original error (throw e) so callers still get the full
exception; update the logging in the catch block around loadConfig to output
only the safe message/fields rather than the entire error object.
Comment on lines
+16
to
19
| } catch (e) { | ||
| // triggerSync is best-effort; network/control-plane errors should not abort the sync | ||
| console.error("[ops-harbor] syncFromControlPlane: triggerSync failed:", e); | ||
| synchronized = undefined; |
There was a problem hiding this comment.
Sanitize triggerSync error logging to avoid accidental secret exposure.
Line [18] logs the raw thrown object. Client/network errors can contain sensitive request metadata; prefer sanitized fields.
🔒 Suggested safe logging adjustment
- } catch (e) {
+ } catch (e) {
// triggerSync is best-effort; network/control-plane errors should not abort the sync
- console.error("[ops-harbor] syncFromControlPlane: triggerSync failed:", e);
+ const errorSummary =
+ e instanceof Error ? { name: e.name } : { type: typeof e };
+ console.error("[ops-harbor] syncFromControlPlane: triggerSync failed", errorSummary);
synchronized = undefined;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } catch (e) { | |
| // triggerSync is best-effort; network/control-plane errors should not abort the sync | |
| console.error("[ops-harbor] syncFromControlPlane: triggerSync failed:", e); | |
| synchronized = undefined; | |
| } catch (e) { | |
| // triggerSync is best-effort; network/control-plane errors should not abort the sync | |
| const errorSummary = | |
| e instanceof Error ? { name: e.name } : { type: typeof e }; | |
| console.error("[ops-harbor] syncFromControlPlane: triggerSync failed", errorSummary); | |
| synchronized = undefined; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/ops-harbor/src/lib/sync.ts` around lines 16 - 19, The catch in
syncFromControlPlane currently logs the raw thrown object (variable e) which can
leak sensitive request data; replace the console.error("[ops-harbor]
syncFromControlPlane: triggerSync failed:", e) with a sanitized log that only
includes non-sensitive fields (e.g., e.name, e.message, e.code or e.status) or a
small sanitizedError object produced by a new helper (e.g., sanitizeError(e))
and avoid logging stacks, headers, bodies or the full error object; keep the
existing synchronized = undefined behavior. Use the symbols
syncFromControlPlane, triggerSync, and the local variable e to locate and change
the logging.
| @@ -2,28 +2,29 @@ import { formatForApi, parseStatusFromSpecs } from "./status-parser"; | |||
| import { deleteStatusStore, setPhase } from "./status-store"; | |||
| import type { StatusSummary } from "../types"; | |||
| import { randomUUID } from "node:crypto"; | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if randomUUID is used in this file
rg -n 'randomUUID' apps/sdd-webapp/src/lib/status-parser.test.tsRepository: masseater/claude-code-plugin
Length of output: 115
Remove unused randomUUID import.
The randomUUID import is not used anywhere in the file and should be removed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/sdd-webapp/src/lib/status-parser.test.ts` at line 4, Remove the unused
import randomUUID from the top of the test file; locate the import statement
that reads import { randomUUID } from "node:crypto"; in
apps/sdd-webapp/src/lib/status-parser.test.ts and delete randomUUID (or the
entire import line if it's the only specifier) so no unused imports remain.
Comment on lines
+74
to
+79
| ````typescript | ||
| ```typescript | ||
| using logger = HookLogger.fromFile(import.meta.filename); | ||
| logger.debug("Hook started"); | ||
| ```` | ||
|
|
There was a problem hiding this comment.
Malformed markdown code fence in HookLogger example.
The code block has nested/improper fence markers that will render incorrectly:
### Examples
-````typescript
-```typescript
+```typescript
using logger = HookLogger.fromFile(import.meta.filename);
logger.debug("Hook started");
-````
-
-````
+```🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/api/cc-plugin-lib.md` around lines 74 - 79, The code sample showing
HookLogger is using malformed nested backtick fences; replace the incorrect
opening/closing sequence with a single proper TypeScript code fence so the
example reads: start with ```typescript, include the
HookLogger.fromFile(import.meta.filename) and logger.debug("Hook started");
lines, and end with ```; ensure the example references HookLogger.fromFile and
logger.debug exactly as shown and remove the extra backticks.
Comment on lines
+3
to
+7
| const EnvSchema = v.object({ | ||
| HOME: v.pipe(v.string(), v.minLength(1)), | ||
| }); | ||
|
|
||
| export const env = v.parse(EnvSchema, process.env); |
There was a problem hiding this comment.
Avoid import-time hard failure on missing HOME.
This currently throws during module initialization if HOME is absent, which can break hook startup before any graceful handling.
💡 Proposed hardening
import * as v from "valibot";
+import { homedir } from "node:os";
const EnvSchema = v.object({
- HOME: v.pipe(v.string(), v.minLength(1)),
+ HOME: v.optional(v.string()),
+ USERPROFILE: v.optional(v.string()),
});
-export const env = v.parse(EnvSchema, process.env);
+const parsed = v.parse(EnvSchema, process.env);
+const resolvedHome = (parsed.HOME ?? parsed.USERPROFILE ?? homedir()).trim();
+
+if (!resolvedHome) {
+ throw new Error("Unable to resolve home directory from environment");
+}
+
+export const env = { HOME: resolvedHome };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const EnvSchema = v.object({ | |
| HOME: v.pipe(v.string(), v.minLength(1)), | |
| }); | |
| export const env = v.parse(EnvSchema, process.env); | |
| import * as v from "valibot"; | |
| import { homedir } from "node:os"; | |
| const EnvSchema = v.object({ | |
| HOME: v.optional(v.string()), | |
| USERPROFILE: v.optional(v.string()), | |
| }); | |
| const parsed = v.parse(EnvSchema, process.env); | |
| const resolvedHome = (parsed.HOME ?? parsed.USERPROFILE ?? homedir()).trim(); | |
| if (!resolvedHome) { | |
| throw new Error("Unable to resolve home directory from environment"); | |
| } | |
| export const env = { HOME: resolvedHome }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/mutils/hooks/lib/env.ts` around lines 3 - 7, The module currently
calls v.parse at import time (EnvSchema and env), causing hard failure if HOME
is missing; change to validate lazily instead: stop exporting a pre-parsed env
value and either (a) replace v.parse(process.env) with v.safeParse(process.env)
and export a non-throwing accessor that returns a safeParse result or defaults,
or (b) move the v.parse call into a function like getEnv() that callers invoke
when they need validated values so missing HOME can be handled gracefully;
update any consumers of env to call the new accessor/getEnv and ensure errors
are handled where used rather than during module initialization.
Comment on lines
+19
to
23
| const skillDirArg = values["skill-dir"] ?? positionals[0]; | ||
|
|
||
| if (!skillDirArg) { | ||
| process.stderr.write("Usage: ./inject-references.ts <skill-dir>\n"); | ||
| process.stderr.write("Usage: ./inject-references.ts --skill-dir <skill-dir>\n"); | ||
| process.exit(1); |
There was a problem hiding this comment.
Usage output is inconsistent with accepted CLI inputs.
The parser accepts positional input (positionals[0]), but the error message only documents --skill-dir. Please document both forms (or remove positional fallback).
✏️ Suggested usage text update
-if (!skillDirArg) {
- process.stderr.write("Usage: ./inject-references.ts --skill-dir <skill-dir>\n");
+if (!skillDirArg) {
+ process.stderr.write(
+ "Usage: ./inject-references.ts --skill-dir <skill-dir> (or ./inject-references.ts <skill-dir>)\n",
+ );
process.exit(1);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/mutils/skills/skill-create/scripts/inject-references.ts` around lines
19 - 23, The usage message is inconsistent with the code's acceptance of a
positional argument; update the error output that currently runs when
skillDirArg is falsy to document both accepted forms (e.g. "--skill-dir
<skill-dir> or <skill-dir> (positional)") instead of only "--skill-dir", by
changing the string passed to process.stderr.write where skillDirArg is checked
(variables: skillDirArg, values, positionals[0]); alternatively, if you prefer
to remove positional support, drop the positionals[0] fallback and only accept
values["skill-dir"] so the current usage text remains accurate.
Comment on lines
+21
to
+22
| "@repo/ts-config": "workspace:*", | ||
| "@repo/vitest-config": "workspace:*", |
There was a problem hiding this comment.
Add the required mutils prerequisite dependency.
plugin-research still appears to be missing the mandatory mutils prerequisite dependency in its plugin manifest metadata.
As per coding guidelines plugins/*/package.json: “All plugins in the marketplace repository must have mutils as a prerequisite dependency”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/research/package.json` around lines 21 - 22, The package.json for the
plugin is missing the required prerequisite dependency "mutils"; update the
plugin manifest (plugins/research package.json) to include "mutils" as a
prerequisite dependency alongside the existing workspace entries (e.g., next to
"@repo/ts-config" and "@repo/vitest-config") by adding the "mutils" entry with
the appropriate workspace version spec (e.g., "workspace:*") under the same
dependencies/prerequisites section so the plugin satisfies the repository
guideline requiring mutils as a prerequisite.
| import { tmpdir } from "node:os"; | ||
| import { join } from "node:path"; | ||
| import { afterEach, beforeEach, describe, expect, test, vi } from "vitest"; | ||
| import { test as base, describe, expect, vi } from "vitest"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="plugins/research/skills/library-research/scripts/lib/resolve-knowledge-dir.test.ts"
python - <<'PY'
from pathlib import Path
import re
p = Path("plugins/research/skills/library-research/scripts/lib/resolve-knowledge-dir.test.ts")
lines = p.read_text().splitlines()
def_line = None
test_calls = []
for i, line in enumerate(lines, start=1):
if re.search(r'^\s*const\s+test\s*=\s*base\.extend', line):
def_line = i
if re.search(r'^\s*test\s*\(', line):
test_calls.append(i)
print(f"fixture definition line: {def_line}")
print(f"test() call lines: {test_calls}")
if def_line is not None and any(l < def_line for l in test_calls):
print("TDZ risk confirmed: test() is called before const test is initialized.")
else:
print("No TDZ ordering issue detected.")
PYRepository: masseater/claude-code-plugin
Length of output: 241
🏁 Script executed:
cat -n plugins/research/skills/library-research/scripts/lib/resolve-knowledge-dir.test.ts | head -110Repository: masseater/claude-code-plugin
Length of output: 4502
test is referenced before initialization (Temporal Dead Zone) — this will cause a ReferenceError at runtime.
The test fixture at line 51 defines const test = base.extend<ProjectDirFixture>(...), but test suites at lines 9, 13, 17, 21, 25, 29, 35, and 40 call test(...) before that declaration. Since const declarations are hoisted but not initialized until their definition line is reached, accessing test() before line 51 throws a ReferenceError.
Rename the fixture to testWithProjectDir and update all references:
Suggested fix
-import { test as base, describe, expect, vi } from "vitest";
+import { test, describe, expect, vi } from "vitest";
interface ProjectDirFixture {
projectDir: string;
}
-const test = base.extend<ProjectDirFixture>({
+const testWithProjectDir = test.extend<ProjectDirFixture>({
projectDir: async ({}, use) => {
const dir = join(tmpdir(), `resolve-knowledge-dir-test-${Date.now()}`);
mkdirSync(dir, { recursive: true });
vi.spyOn(process, "cwd").mockReturnValue(dir);
await use(dir);
vi.restoreAllMocks();
rmSync(dir, { recursive: true, force: true });
},
});
describe("resolveKnowledgeDir", () => {
- test("creates library directory if it does not exist", ({ projectDir }) => {
+ testWithProjectDir("creates library directory if it does not exist", ({ projectDir }) => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@plugins/research/skills/library-research/scripts/lib/resolve-knowledge-dir.test.ts`
at line 4, The file uses the imported "test" alias before it's redefined by the
fixture, causing a temporal-dead-zone ReferenceError; rename the const fixture
from test to testWithProjectDir (the declaration currently: const test =
base.extend<ProjectDirFixture>(...)) and update all calls that invoke test(...)
in the file (the suites at the top that currently call test(...) — lines
referencing test in the describe/test blocks) to call testWithProjectDir(...),
ensuring the import remains "import { test as base, ... }" and that
ProjectDirFixture type usage is unchanged.
Comment on lines
+17
to
+18
| "@repo/ts-config": "workspace:*", | ||
| "@repo/vitest-config": "workspace:*", |
There was a problem hiding this comment.
Add the required mutils prerequisite dependency.
This plugin manifest still doesn’t declare the required mutils prerequisite dependency for marketplace plugins.
As per coding guidelines plugins/*/package.json: “All plugins in the marketplace repository must have mutils as a prerequisite dependency”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@plugins/ui-dev/package.json` around lines 17 - 18, The plugin manifest
(package.json) is missing the required marketplace prerequisite "mutils"; add an
entry for "mutils" with the workspace version spec (e.g., "mutils":
"workspace:*") to the plugin's prerequisites/prerequisite section in
package.json so it matches other marketplace plugins' manifests and the repo
guideline requiring mutils as a prerequisite.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
cc-plugin-lib,doc-engine,mutils,research,sdd-webapp, andui-dev.--skill-dir,--target-dir,--profile).Bug Fixes
Chores