This repository was archived by the owner on Mar 30, 2026. It is now read-only.
feat: add onboard hook type to plugin manifest schema (AGE-208)#152
Merged
feat: add onboard hook type to plugin manifest schema (AGE-208)#152
Conversation
Contributor
📝 WalkthroughWalkthroughAdds an onboarding feature: schema and manifest support for onboard hooks, runtime execution (runOnboardHook), CLI commands and setup wiring to run hooks, an orchestrator to run per-agent/plugin onboard flows, a redaction utility, and unit + E2E tests and fixtures. Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as CLI (onboard/setup)
participant PluginMgr as Plugin Manager
participant Onboard as Onboard Orchestrator
participant HookExec as runOnboardHook (Shell)
participant Env as Env/.env & Secrets
User->>CLI: run setup or onboard (flags)
alt skipOnboard true
CLI->>User: Log "Onboarding skipped"
else
CLI->>PluginMgr: load manifest & identities
PluginMgr->>PluginMgr: build agent→plugins map
CLI->>Env: load .env and resolved secrets
PluginMgr->>Onboard: for each plugin with onboard hook
Onboard->>Env: check env vars / autoResolvedSecrets
alt runOnce already satisfied
Onboard->>User: Log "skipped (already configured)"
else
alt inputs from env
Env->>Onboard: provide input values
else prompt needed
Onboard->>User: Prompt for input
User->>Onboard: Provide input / cancel
end
Onboard->>HookExec: spawn script with assembled env
HookExec->>Onboard: return success {instructions} or error
alt success
Onboard->>CLI: redact & print instructions
else failure
Onboard->>CLI: exitWithError (message)
end
end
end
CLI->>User: Setup complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Owner
Author
🔍 QA ApprovedBuild: ✅ Clean Verified against AGE-208 Definition of Done:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
packages/cli/commands/setup.ts (1)
382-479: Extract onboard orchestration into a dedicated module/helper.This block is substantial and pushes an already large file further past the size guideline, making maintenance harder.
As per coding guidelines: "Keep files under 300 lines; split large files into smaller modules".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/commands/setup.ts` around lines 382 - 479, Extract the large onboard orchestration block into a new helper function (e.g., runOnboardHooks or onboardOrchestrator) and move it to its own module; the function should accept the required inputs (fetchedIdentities, agentPlugins, resolvePlugin, autoResolvedSecrets, envDict, resolvedSecrets, p, runOnboardHook, exitWithError, and opts.skipOnboard) and return/throw errors as currently handled. Replace the in-file loop with a single awaited call to runOnboardHooks(...) and keep all original behavior (runOnce logic, env merging, interactive prompts, logging via p.log, and handling of runOnboardHook results) so callers and tests need only wiring changes. Ensure you export the new function and import it where used, and add small unit tests around runOnce skipping, env-derived secret population (envDerivedKey logic), and error path to preserve coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/__e2e__/onboard-hooks.e2e.test.ts`:
- Around line 310-314: The test mutates a shared fixture by writing into
PLUGIN_IDENTITY_DIR/plugins/test-linear.yaml (variables manifestPath and
originalManifest); instead, create a per-test copy of the identity fixture
(e.g., copy PLUGIN_IDENTITY_DIR to a temp directory or copy the single plugin
file to a tmp folder), update manifestPath to point to that per-test copy, then
modify the copied file (replace "runOnce: false" -> "runOnce: true") so the
original fixture is never mutated; apply the same change for the similar patch
at the other occurrence (lines referencing the same manifest modification).
In `@packages/cli/commands/setup.ts`:
- Around line 463-467: The code prints raw hook output via result.instructions
(in the setup command handler in packages/cli/commands/setup.ts) which may
contain secrets; update the handler so that before calling p.log.info and
console.log you sanitize/redact sensitive values from result.instructions (e.g.,
redact values that match known env var names like API_KEY, TOKEN, SECRET, or any
patterns for keys/credentials) by applying a helper such as
redactSecretsFromString(result.instructions) and then log/display the redacted
string; ensure you reuse the same redaction helper for any other hook outputs
and keep the original secret-containing string out of logs or console output.
- Around line 396-410: The current runOnce check treats an empty set of required
secrets as "all present" because .every returns true for empty arrays; change
the logic around onboard.runOnce so you first collect required secrets from
pluginManifest.secrets (e.g., const requiredSecrets =
Object.entries(pluginManifest.secrets).filter(([, s]) => s.required)), then
compute allSecretsPresent only if requiredSecrets.length > 0 (e.g.,
requiredSecrets.length > 0 && requiredSecrets.every(...)); use the same keys
(key, secret) and existing checks against
autoResolvedSecrets[fi.agent.role]?.[key] and
envDict[`${roleUpper}_${secret.envVar}`] so onboarding is not skipped when there
are no required secrets.
---
Nitpick comments:
In `@packages/cli/commands/setup.ts`:
- Around line 382-479: Extract the large onboard orchestration block into a new
helper function (e.g., runOnboardHooks or onboardOrchestrator) and move it to
its own module; the function should accept the required inputs
(fetchedIdentities, agentPlugins, resolvePlugin, autoResolvedSecrets, envDict,
resolvedSecrets, p, runOnboardHook, exitWithError, and opts.skipOnboard) and
return/throw errors as currently handled. Replace the in-file loop with a single
awaited call to runOnboardHooks(...) and keep all original behavior (runOnce
logic, env merging, interactive prompts, logging via p.log, and handling of
runOnboardHook results) so callers and tests need only wiring changes. Ensure
you export the new function and import it where used, and add small unit tests
around runOnce skipping, env-derived secret population (envDerivedKey logic),
and error path to preserve coverage.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
packages/cli/__e2e__/helpers/fixtures/plugin-identity/plugins/test-linear.yamlpackages/cli/__e2e__/onboard-hooks.e2e.test.tspackages/cli/__e2e__/plugins.e2e.test.tspackages/cli/bin.tspackages/cli/commands/setup.tspackages/core/src/__tests__/manifest-hooks.test.tspackages/core/src/__tests__/plugin-manifest.test.tspackages/core/src/index.tspackages/core/src/manifest-hooks.tspackages/core/src/schemas/index.tspackages/core/src/schemas/plugin-manifest.ts
Owner
Author
🔍 Re-test after 9a43ca0 (merged #111 + new E2E tests)Unit tests: ✅ 242/242 (10 more from merged #111 provider tests) Unit test coverage for onboard hooks remains solid (5 execution + 4 schema tests). QA approval stands. |
Owner
Author
|
QA update (head 9a43ca0): Local verification:
E2E: failing due to Pulumi binary not available in test env / suite assumptions (known issue; not a regression from this PR). |
Owner
Author
|
Pushed fixup commit c5c97b3 addressing CodeRabbit items:
Verification:
Diff: adds packages/cli/lib/redact.ts and wires into setup.ts. |
Owner
Author
|
Pushed refactor commit c878878:
Verification:
Behavior preserved (runOnce logic, env merging, prompting, redaction, error handling). |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/cli/__e2e__/onboard-hooks.e2e.test.ts (1)
309-313:⚠️ Potential issue | 🟠 MajorAvoid mutating the shared plugin fixture in-place.
At Line 310, this test edits
packages/cli/__e2e__/helpers/fixtures/plugin-identity/plugins/test-linear.yamldirectly. That can leak state across tests/workers.🔧 Proposed fix
- const manifestPath = path.join(PLUGIN_IDENTITY_DIR, "plugins", "test-linear.yaml"); - const originalManifest = fs.readFileSync(manifestPath, "utf-8"); - fs.writeFileSync(manifestPath, originalManifest.replace("runOnce: false", "runOnce: true"), "utf-8"); + const identityDirForRunOnce = path.join(tempDir, `plugin-identity-runonce-${Date.now()}`); + fs.cpSync(PLUGIN_IDENTITY_DIR, identityDirForRunOnce, { recursive: true }); + const manifestPath = path.join(identityDirForRunOnce, "plugins", "test-linear.yaml"); + const originalManifest = fs.readFileSync(manifestPath, "utf-8"); + fs.writeFileSync(manifestPath, originalManifest.replace("runOnce: false", "runOnce: true"), "utf-8"); @@ - identityDir: PLUGIN_IDENTITY_DIR, + identityDir: identityDirForRunOnce, @@ - } finally { - // Restore original manifest - fs.writeFileSync(manifestPath, originalManifest, "utf-8"); - } + } finally { + fs.rmSync(identityDirForRunOnce, { recursive: true, force: true }); + }Also applies to: 340-343
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/__e2e__/onboard-hooks.e2e.test.ts` around lines 309 - 313, The test currently mutates the shared fixture file by writing to manifestPath (constructed from PLUGIN_IDENTITY_DIR + "plugins/test-linear.yaml"); instead, avoid in-place edits by creating a temporary copy of the fixture, modify the copy to set runOnce: true, and point the code under test at that temp manifest (or restore the original in a finally/after hook). Update the sections around manifestPath/originalManifest so they copy the file to a temp directory (or use a tmp file), perform the replace on the copy, and ensure cleanup to prevent state leaking across tests/workers.
🧹 Nitpick comments (3)
packages/cli/lib/onboard-hooks.ts (1)
8-20: Use interfaces for object-shape definitions in this module.At Line 8 and Line 30, object-shape
typealiases are used where repo standards preferinterface.♻️ Proposed refactor
-type PromptLike = { +interface PromptLike { log: { info: (msg: string) => void; warn: (msg: string) => void; error: (msg: string) => void; }; spinner: () => { start: (msg: string) => void; stop: (msg?: string) => void }; text: (opts: { message: string; validate?: (val: string) => string | undefined; }) => Promise<string | symbol>; isCancel: (val: unknown) => boolean; -}; +} + +interface RunOnboardHooksArgs { + fetchedIdentities: Array<{ + agent: { name: string; role: string; displayName: string }; + identityResult: IdentityResult; + }>; + agentPlugins: Map<string, Set<string>>; + resolvePlugin: (pluginName: string, identityResult: IdentityResult) => PluginManifest; + autoResolvedSecrets: Record<string, Record<string, string>>; + envDict: Record<string, string>; + resolvedSecrets: { perAgent: Record<string, Record<string, string>> }; + p: PromptLike; + runOnboardHook: (opts: { script: string; env: Record<string, string> }) => Promise< + | { ok: true; instructions?: string } + | { ok: false; error: string } + >; + exitWithError: (message: string) => never; + skipOnboard: boolean; +} -export async function runOnboardHooks(args: { - ... -}): Promise<void> { +export async function runOnboardHooks(args: RunOnboardHooksArgs): Promise<void> {As per coding guidelines: "Prefer interfaces over type aliases for object shapes in TypeScript".
Also applies to: 30-47
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/lib/onboard-hooks.ts` around lines 8 - 20, Replace the object-shape type aliases with interfaces: change the PromptLike type alias to an interface named PromptLike (keeping members log, spinner, text, isCancel and their signatures intact) and likewise convert the other object-shaped type alias at the later block (lines 30-47) to an interface with the same property names and method signatures; ensure exported names and usages remain unchanged so code referencing PromptLike, log.info/warn/error, spinner.start/stop, text(...), and isCancel(...) continue to type-check.packages/cli/__e2e__/onboard-hooks.e2e.test.ts (1)
1-345: Consider splitting this E2E file into smaller scenario-focused suites.This new file is over 300 lines; splitting by flow (
env input,interactive input,runOnce,skip) will make maintenance easier.As per coding guidelines: "
packages/**/*.{ts,tsx}: Keep files under 300 lines; split large files into smaller, focused modules".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/__e2e__/onboard-hooks.e2e.test.ts` around lines 1 - 345, The test file is too large and should be split into smaller scenario-focused suites; create separate test files (or describe blocks in their own files) for each flow referenced by the current describe: "setup runs onboard hook and captures instructions from stdout", "setup prompts for onboard input when not in env", "setup skips onboard hooks by default", and "runOnce skips onboard when all required secrets are present"; move related setup/teardown, mocks (the `@clack/prompts`, ../lib/project, ../lib/workspace mocks and shared helpers like createTestProject/forceCleanup), and constants (PLUGIN_IDENTITY_DIR, E2E_ENV_KEYS) into a shared test helper module so each new file imports common fixtures and only contains the specific it(...) test and any small per-test setup (stackName/tempDir creation and unique extraEnvLines), leaving beforeAll/afterAll minimal in each file and ensuring the ProcessExitError spy and tempDir cleanup are handled via the shared helper.packages/cli/commands/onboard.ts (1)
45-47: Use the CLI config module for manifest YAML handling in command code.At Line 45,
onboard.tsmanually reads/parses YAML. This command should delegate manifest I/O topackages/cli/lib/config.tsfor consistency with command-layer conventions.As per coding guidelines: "
packages/cli/commands/**/*.ts: Load and save YAML manifests using the config module in packages/cli/lib/config.ts".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/commands/onboard.ts` around lines 45 - 47, Replace the manual fs/YAML parsing in the onboard command (the raw/parsed variables and fs.readFileSync + YAML.parse usage around manifestPath) with the manifest load helper from packages/cli/lib/config.ts (use the config module's exported manifest loader function instead of reading/parsing directly); call that loader with manifestPath to obtain the parsed manifest object, then continue to validate with ClawupManifestSchema.safeParse on the returned object and handle errors consistently with other CLI commands.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/__e2e__/onboard-hooks.e2e.test.ts`:
- Around line 202-205: The execSync call assigning result for "pulumi stack ls
--json" is unguarded and should be wrapped in a try-catch to match the existing
pattern used for other Pulumi shell-outs (the other calls that wrap in try-catch
at the same test file); update the block around the execSync invocation that
produces result so any thrown error is caught and ignored (or handled the same
way as the other three shell-outs) to prevent the test from hard-failing when
Pulumi is unavailable.
In `@packages/cli/commands/onboard.ts`:
- Around line 122-137: In the agents mapping passed to buildManifestSecrets, the
requiredSecrets field can be undefined due to optional chaining on
fetchedIdentities, violating the SecretsBuilderOpts type; update the mapping
(inside the agents.map callback that builds objects for buildManifestSecrets) to
coerce requiredSecrets to an array (e.g., use the nullish coalescing operator to
set requiredSecrets to [] when fi?.identityResult.manifest.requiredSecrets is
undefined) so buildManifestSecrets always receives a string[].
In `@packages/cli/lib/redact.ts`:
- Around line 11-12: The regex keyValue used to redact secrets only matches
uppercase names so lowercase/mixed keys like "token=" or "api_key=" slip
through; update the keyValue pattern in redact.ts (the const keyValue and the
subsequent input.replace call) to be case-insensitive (e.g., allow a-z or add
the /i flag) so it matches keys regardless of case, ensuring the replace still
returns `${key}=[REDACTED]` for the captured key and that out =
input.replace(keyValue, ...) continues to redact all token/secret-like keys.
---
Duplicate comments:
In `@packages/cli/__e2e__/onboard-hooks.e2e.test.ts`:
- Around line 309-313: The test currently mutates the shared fixture file by
writing to manifestPath (constructed from PLUGIN_IDENTITY_DIR +
"plugins/test-linear.yaml"); instead, avoid in-place edits by creating a
temporary copy of the fixture, modify the copy to set runOnce: true, and point
the code under test at that temp manifest (or restore the original in a
finally/after hook). Update the sections around manifestPath/originalManifest so
they copy the file to a temp directory (or use a tmp file), perform the replace
on the copy, and ensure cleanup to prevent state leaking across tests/workers.
---
Nitpick comments:
In `@packages/cli/__e2e__/onboard-hooks.e2e.test.ts`:
- Around line 1-345: The test file is too large and should be split into smaller
scenario-focused suites; create separate test files (or describe blocks in their
own files) for each flow referenced by the current describe: "setup runs onboard
hook and captures instructions from stdout", "setup prompts for onboard input
when not in env", "setup skips onboard hooks by default", and "runOnce skips
onboard when all required secrets are present"; move related setup/teardown,
mocks (the `@clack/prompts`, ../lib/project, ../lib/workspace mocks and shared
helpers like createTestProject/forceCleanup), and constants
(PLUGIN_IDENTITY_DIR, E2E_ENV_KEYS) into a shared test helper module so each new
file imports common fixtures and only contains the specific it(...) test and any
small per-test setup (stackName/tempDir creation and unique extraEnvLines),
leaving beforeAll/afterAll minimal in each file and ensuring the
ProcessExitError spy and tempDir cleanup are handled via the shared helper.
In `@packages/cli/commands/onboard.ts`:
- Around line 45-47: Replace the manual fs/YAML parsing in the onboard command
(the raw/parsed variables and fs.readFileSync + YAML.parse usage around
manifestPath) with the manifest load helper from packages/cli/lib/config.ts (use
the config module's exported manifest loader function instead of reading/parsing
directly); call that loader with manifestPath to obtain the parsed manifest
object, then continue to validate with ClawupManifestSchema.safeParse on the
returned object and handle errors consistently with other CLI commands.
In `@packages/cli/lib/onboard-hooks.ts`:
- Around line 8-20: Replace the object-shape type aliases with interfaces:
change the PromptLike type alias to an interface named PromptLike (keeping
members log, spinner, text, isCancel and their signatures intact) and likewise
convert the other object-shaped type alias at the later block (lines 30-47) to
an interface with the same property names and method signatures; ensure exported
names and usages remain unchanged so code referencing PromptLike,
log.info/warn/error, spinner.start/stop, text(...), and isCancel(...) continue
to type-check.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/cli/__e2e__/onboard-hooks.e2e.test.tspackages/cli/bin.tspackages/cli/commands/onboard.tspackages/cli/commands/setup.tspackages/cli/lib/onboard-hooks.tspackages/cli/lib/redact.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/__e2e__/onboard-command.e2e.test.ts`:
- Around line 186-219: The test mutates the shared fixture file via
manifestPath/originalManifest; instead, create and modify an isolated copy of
the fixture and point createTestProject at that copy so other tests aren't
affected. Concretely: copy the plugin identity fixture (or at minimum the
test-linear.yaml file) into a temporary directory, update the copied file to set
"runOnce: true" (keeping originalManifest for reference if needed), and pass the
temp identityDir into createTestProject; keep the finally block to clean up the
temp copy rather than rewriting the shared fixture. This change touches the
manifestPath/originalManifest handling and the createTestProject(identityDir)
call used before invoking onboardCommand.
Owner
Author
Review comments addressed (074f479)All actionable review comments have been addressed:
Not addressed (intentionally):
CI is green ✅ |
- Add OnboardHookSchema with description, inputs, script, runOnce fields - Add OnboardHookInputSchema with envVar, prompt, instructions, validator - Add runOnboardHook() to manifest-hooks.ts (captures stdout for instructions) - Wire onboard hooks into setup.ts between secret validation and Pulumi config - Add --skip-onboard CLI flag - runOnce support: skip if all required secrets are already present - Interactive input prompting with env var override and prefix validation - 10 new tests (4 schema + 5 execution + 1 resolve edge case) - All 206 tests passing, build green
The runOnce check was comparing raw plugin secret keys against resolvedSecrets.perAgent which uses agent-scoped keys. Now checks the env dict using the plugin secret's envVar (prefixed with role), which correctly detects already-configured secrets. Fixes bug reported by Scout on AGE-208.
4 new E2E tests in onboard-hooks.e2e.test.ts: 1. Onboard hook runs and captures stdout instructions (input from env) 2. Onboard hook prompts interactively when input not in env (p.text mock) 3. --skip-onboard flag bypasses onboard hooks entirely 4. runOnce: true skips onboard when all required secrets are present Also: - Added echo-based onboard hook to test-linear fixture manifest - Added TEST_LINEAR_CONFIG_TOKEN to plugin test env lines - Merged main (PR #111 multi-provider changes) All 242 unit tests + 28 E2E tests passing
…ommand Flip the default so setup skips onboard hooks unless --onboard is passed, and add `clawup onboard` as a standalone command for first-time interactive plugin setup. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Cover the onboardCommand bootstrapping and hook execution without Pulumi: env-var resolution, interactive prompting, runOnce skip, missing .env error, and missing manifest error. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The SecretsBuilderOpts interface on main requires allModels for multi-provider API key resolution. Without it the merge build fails. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…safety - Use isolated copies of identity fixture in runOnce tests instead of mutating shared fixture in-place (both test files) - Add case-insensitive flag to redact regex and handle quoted values - Default requiredSecrets to [] when undefined in onboard command - Wrap unguarded pulumi execSync in try-catch - Convert type aliases to interfaces in onboard-hooks Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
After rebase onto main, agent schema fields (name, role, displayName) became optional. Use resolveAgentSync to get ResolvedAgent with required fields, matching setup.ts pattern.
stepandel
force-pushed
the
feat/AGE-208-onboard-hook
branch
from
074f479 to
2b23dd2
Compare
Owner
Author
🔍 QA Re-verified (2b23dd2)Build: ✅ Clean Re-tested after new commits (refactoring, fixture isolation, type fixes). All AGE-208 acceptance criteria remain satisfied. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/cli/__e2e__/onboard-command.e2e.test.ts (1)
189-190:⚠️ Potential issue | 🟡 MinorAssert the
runOncefixture patch was applied.This replace can silently no-op if the fixture changes shape, weakening the test intent.
Proposed reliability fix
- const originalManifest = fs.readFileSync(manifestPath, "utf-8"); - fs.writeFileSync(manifestPath, originalManifest.replace("runOnce: false", "runOnce: true"), "utf-8"); + const originalManifest = fs.readFileSync(manifestPath, "utf-8"); + const patchedManifest = originalManifest.replace("runOnce: false", "runOnce: true"); + if (patchedManifest === originalManifest) { + throw new Error("Expected runOnce flag in test-linear fixture"); + } + fs.writeFileSync(manifestPath, patchedManifest, "utf-8");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/__e2e__/onboard-command.e2e.test.ts` around lines 189 - 190, The test currently overwrites the manifest using originalManifest.replace(...) which may silently no-op; after writing, read the file back (or check the replaced string) and assert that the fixture now contains "runOnce: true" (e.g., verify fs.readFileSync(manifestPath, "utf-8") includes "runOnce: true" or assert that originalManifest !== patched and that patched.includes("runOnce: true")), and fail the test if the replacement did not occur so the test cannot silently proceed when the fixture shape changes; reference the variables manifestPath and originalManifest (and the write operation) to locate where to add this assertion.
🧹 Nitpick comments (5)
packages/core/src/manifest-hooks.ts (1)
270-274: Consider redacting stderr output for onboard hooks.Stderr is streamed directly to
process.stderrwithout redaction. If an onboard script inadvertently echoes sensitive values to stderr (e.g., debug output), they would be visible in the terminal. The stdout instructions are redacted by the caller, but stderr is not.This mirrors the existing
runLifecycleHookbehavior, so it may be intentional for real-time progress visibility. However, given the onboarding context where users may input tokens, consider whether stderr should pass throughredactSecretsFromString.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/manifest-hooks.ts` around lines 270 - 274, The stderr handler in child.stderr.on is writing raw stderr to process.stderr (accumulating into the stderr variable) without redaction; update the handler in manifest-hooks.ts to pass the chunk string through redactSecretsFromString before appending to stderr and writing to process.stderr (mirroring how stdout is handled), i.e., use redactSecretsFromString(msg) when updating the stderr accumulator and when calling process.stderr.write; reference the child.stderr.on listener, the stderr accumulator, and redactSecretsFromString to locate and patch the logic (keeping behavior consistent with runLifecycleHook).packages/cli/commands/setup.ts (2)
32-32: Unused import:redactSecretsFromString.The
redactSecretsFromStringimport is not used insetup.ts- it's only used inonboard-hooks.ts. This import can be removed.Proposed fix
import { runOnboardHooks } from "../lib/onboard-hooks"; -import { redactSecretsFromString } from "../lib/redact";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/commands/setup.ts` at line 32, Remove the unused import `redactSecretsFromString` from the top of the file (the import statement in setup.ts: `import { redactSecretsFromString } from "../lib/redact";`) since it is not referenced in this file (it’s only used in onboard-hooks.ts); update the imports accordingly and run the linter/TypeScript build to ensure no other unused imports remain.
1-599: File exceeds 300-line guideline.At 599 lines, this file significantly exceeds the 300-line guideline. The extraction of onboard hook orchestration to
onboard-hooks.tswas a good step. Consider further refactoring in a follow-up PR to split remaining concerns (e.g., Pulumi configuration, secret resolution) into separate modules.As per coding guidelines: "Keep files under 300 lines; split large files into smaller, focused modules."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/commands/setup.ts` around lines 1 - 599, This file is over the 300-line guideline; split it into focused modules by extracting major responsibilities: (1) identity fetching & plugin/manifest resolution — move the loop that builds fetchedIdentities, agentPlugins/agentDeps, allModels and related helpers into a new module (e.g., functions like resolveAgentSync/fetchIdentity orchestration exported as fetchAllIdentities or buildIdentityContext), (2) secret resolution and validation — extract the expectedSecrets construction, loadEnvSecrets usage, validators building, requiredMissing logic and helper functions like resolveIsSecret/getValidatorHint into a secrets module (e.g., secrets/resolve.ts) that exports resolveSecrets and validateSecrets, and (3) Pulumi configuration — move configuration logic that calls ensureWorkspace, selectOrCreateStack, setConfig and the per-agent/autoresolved config loops into a pulumi/configure.ts module that exports configurePulumi; update setupCommand to import and call these new functions and keep only high-level orchestration, preserving existing function names (resolveIsSecret, getValidatorHint) while making them internal to the new modules or re-exporting as needed so tests/consumers keep working.packages/cli/lib/onboard-hooks.ts (1)
41-44: Consider using the fullOnboardHookResulttype from core.The inline type definition for
runOnboardHook's return type differs slightly from the actualOnboardHookResulttype in@clawup/core/manifest-hooks(which usesinstructions: stringrather thaninstructions?: string). While functionally compatible, importing and using the actual type would ensure type safety and consistency.Proposed change
+import type { OnboardHookResult } from "@clawup/core/manifest-hooks"; + interface RunOnboardHooksArgs { // ... - runOnboardHook: (opts: { script: string; env: Record<string, string> }) => Promise< - | { ok: true; instructions?: string } - | { ok: false; error: string } - >; + runOnboardHook: (opts: { script: string; env: Record<string, string> }) => Promise<OnboardHookResult>; // ... }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/lib/onboard-hooks.ts` around lines 41 - 44, Replace the inline return union for runOnboardHook with the canonical OnboardHookResult from `@clawup/core/manifest-hooks`: import OnboardHookResult and change the method signature runOnboardHook: (opts: { script: string; env: Record<string, string> }) => Promise<OnboardHookResult>. Ensure the imported type is used so the instructions field matches the core definition (instructions: string) and update any callers or tests that relied on the previous optionality if needed.packages/cli/__e2e__/onboard-hooks.e2e.test.ts (1)
1-348: Consider splitting this E2E suite into smaller focused files.This file is above the project’s size guideline; extracting shared setup/helpers and splitting scenarios would improve maintainability.
As per coding guidelines:
packages/**/*.{ts,tsx}: Keep files under 300 lines; split large files into smaller, focused modules.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/__e2e__/onboard-hooks.e2e.test.ts` around lines 1 - 348, This test file is too large and should be split into smaller focused test files; extract the shared setup/teardown and fixtures (tempDir creation, E2E_ENV_KEYS handling, beforeAll/afterAll logic, PLUGIN_IDENTITY_DIR, and helper calls to createTestProject/forceCleanup) into a reusable helper module (e.g., export functions like createTempWorkspace, prepareEnv, cleanupWorkspace) and then move each scenario (the three it blocks: "setup runs onboard hook...", "setup prompts...", "setup skips...", "runOnce skips...") into its own test file that imports the shared helpers and calls setupCommand and assertions; ensure each new file remains under the 300-line guideline and keep references to the mocked modules (vi.mock of `@clack/prompts`, ../lib/project, ../lib/workspace) in a common setup helper or a per-file top-level mock so behavior is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/__e2e__/onboard-hooks.e2e.test.ts`:
- Around line 315-317: The test currently does a blind string replace on
originalManifest (using originalManifest.replace("runOnce: false", "runOnce:
true")) which can silently no-op if formatting changes; modify the patch step in
the test (around variables manifestPath and originalManifest) to perform a
replacement that asserts it succeeded—e.g., use a regex or search to find the
runOnce key, produce the modified content, and throw or assert/fail the test if
no substitution occurred before calling fs.writeFileSync; ensure the assertion
references manifestPath/originalManifest so failures point to the fixture.
In `@packages/cli/commands/onboard.ts`:
- Around line 43-49: The onboard command currently reads and parses clawup.yaml
directly (manifestPath, fs.readFileSync, YAML.parse and
ClawupManifestSchema.safeParse); replace that direct I/O with the centralized
manifest helpers exported from packages/cli/lib/config.ts (use the module's
load/read manifest function and its validated return instead of manual fs/YAML
parsing), then feed the returned object into ClawupManifestSchema.safeParse or
use the module's already-validated result; ensure any thrown errors from the
config module are handled the same way the current try/catch does so behavior
remains consistent.
---
Duplicate comments:
In `@packages/cli/__e2e__/onboard-command.e2e.test.ts`:
- Around line 189-190: The test currently overwrites the manifest using
originalManifest.replace(...) which may silently no-op; after writing, read the
file back (or check the replaced string) and assert that the fixture now
contains "runOnce: true" (e.g., verify fs.readFileSync(manifestPath, "utf-8")
includes "runOnce: true" or assert that originalManifest !== patched and that
patched.includes("runOnce: true")), and fail the test if the replacement did not
occur so the test cannot silently proceed when the fixture shape changes;
reference the variables manifestPath and originalManifest (and the write
operation) to locate where to add this assertion.
---
Nitpick comments:
In `@packages/cli/__e2e__/onboard-hooks.e2e.test.ts`:
- Around line 1-348: This test file is too large and should be split into
smaller focused test files; extract the shared setup/teardown and fixtures
(tempDir creation, E2E_ENV_KEYS handling, beforeAll/afterAll logic,
PLUGIN_IDENTITY_DIR, and helper calls to createTestProject/forceCleanup) into a
reusable helper module (e.g., export functions like createTempWorkspace,
prepareEnv, cleanupWorkspace) and then move each scenario (the three it blocks:
"setup runs onboard hook...", "setup prompts...", "setup skips...", "runOnce
skips...") into its own test file that imports the shared helpers and calls
setupCommand and assertions; ensure each new file remains under the 300-line
guideline and keep references to the mocked modules (vi.mock of `@clack/prompts`,
../lib/project, ../lib/workspace) in a common setup helper or a per-file
top-level mock so behavior is preserved.
In `@packages/cli/commands/setup.ts`:
- Line 32: Remove the unused import `redactSecretsFromString` from the top of
the file (the import statement in setup.ts: `import { redactSecretsFromString }
from "../lib/redact";`) since it is not referenced in this file (it’s only used
in onboard-hooks.ts); update the imports accordingly and run the
linter/TypeScript build to ensure no other unused imports remain.
- Around line 1-599: This file is over the 300-line guideline; split it into
focused modules by extracting major responsibilities: (1) identity fetching &
plugin/manifest resolution — move the loop that builds fetchedIdentities,
agentPlugins/agentDeps, allModels and related helpers into a new module (e.g.,
functions like resolveAgentSync/fetchIdentity orchestration exported as
fetchAllIdentities or buildIdentityContext), (2) secret resolution and
validation — extract the expectedSecrets construction, loadEnvSecrets usage,
validators building, requiredMissing logic and helper functions like
resolveIsSecret/getValidatorHint into a secrets module (e.g.,
secrets/resolve.ts) that exports resolveSecrets and validateSecrets, and (3)
Pulumi configuration — move configuration logic that calls ensureWorkspace,
selectOrCreateStack, setConfig and the per-agent/autoresolved config loops into
a pulumi/configure.ts module that exports configurePulumi; update setupCommand
to import and call these new functions and keep only high-level orchestration,
preserving existing function names (resolveIsSecret, getValidatorHint) while
making them internal to the new modules or re-exporting as needed so
tests/consumers keep working.
In `@packages/cli/lib/onboard-hooks.ts`:
- Around line 41-44: Replace the inline return union for runOnboardHook with the
canonical OnboardHookResult from `@clawup/core/manifest-hooks`: import
OnboardHookResult and change the method signature runOnboardHook: (opts: {
script: string; env: Record<string, string> }) => Promise<OnboardHookResult>.
Ensure the imported type is used so the instructions field matches the core
definition (instructions: string) and update any callers or tests that relied on
the previous optionality if needed.
In `@packages/core/src/manifest-hooks.ts`:
- Around line 270-274: The stderr handler in child.stderr.on is writing raw
stderr to process.stderr (accumulating into the stderr variable) without
redaction; update the handler in manifest-hooks.ts to pass the chunk string
through redactSecretsFromString before appending to stderr and writing to
process.stderr (mirroring how stdout is handled), i.e., use
redactSecretsFromString(msg) when updating the stderr accumulator and when
calling process.stderr.write; reference the child.stderr.on listener, the stderr
accumulator, and redactSecretsFromString to locate and patch the logic (keeping
behavior consistent with runLifecycleHook).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
packages/cli/__e2e__/helpers/fixtures/plugin-identity/plugins/test-linear.yamlpackages/cli/__e2e__/onboard-command.e2e.test.tspackages/cli/__e2e__/onboard-hooks.e2e.test.tspackages/cli/__e2e__/plugins.e2e.test.tspackages/cli/bin.tspackages/cli/commands/onboard.tspackages/cli/commands/setup.tspackages/cli/lib/onboard-hooks.tspackages/cli/lib/redact.tspackages/core/src/__tests__/manifest-hooks.test.tspackages/core/src/__tests__/plugin-manifest.test.tspackages/core/src/index.tspackages/core/src/manifest-hooks.tspackages/core/src/schemas/index.tspackages/core/src/schemas/plugin-manifest.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/core/src/index.ts
- packages/core/src/schemas/plugin-manifest.ts
- packages/cli/bin.ts
Comment on lines
+315
to
+317
| const originalManifest = fs.readFileSync(manifestPath, "utf-8"); | ||
| fs.writeFileSync(manifestPath, originalManifest.replace("runOnce: false", "runOnce: true"), "utf-8"); | ||
|
|
Contributor
There was a problem hiding this comment.
Guard against silent no-op when patching runOnce in the fixture.
If fixture formatting changes, replace("runOnce: false", "runOnce: true") can do nothing and still let the test proceed.
Proposed reliability fix
- const originalManifest = fs.readFileSync(manifestPath, "utf-8");
- fs.writeFileSync(manifestPath, originalManifest.replace("runOnce: false", "runOnce: true"), "utf-8");
+ const originalManifest = fs.readFileSync(manifestPath, "utf-8");
+ const patchedManifest = originalManifest.replace("runOnce: false", "runOnce: true");
+ if (patchedManifest === originalManifest) {
+ throw new Error("Expected runOnce flag in test-linear fixture");
+ }
+ fs.writeFileSync(manifestPath, patchedManifest, "utf-8");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/__e2e__/onboard-hooks.e2e.test.ts` around lines 315 - 317, The
test currently does a blind string replace on originalManifest (using
originalManifest.replace("runOnce: false", "runOnce: true")) which can silently
no-op if formatting changes; modify the patch step in the test (around variables
manifestPath and originalManifest) to perform a replacement that asserts it
succeeded—e.g., use a regex or search to find the runOnce key, produce the
modified content, and throw or assert/fail the test if no substitution occurred
before calling fs.writeFileSync; ensure the assertion references
manifestPath/originalManifest so failures point to the fixture.
Comment on lines
+43
to
+49
| const manifestPath = path.join(projectRoot, MANIFEST_FILE); | ||
| let validation: ReturnType<typeof ClawupManifestSchema.safeParse>; | ||
| try { | ||
| const raw = fs.readFileSync(manifestPath, "utf-8"); | ||
| const parsed = YAML.parse(raw); | ||
| validation = ClawupManifestSchema.safeParse(parsed); | ||
| } catch (err) { |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use the CLI config module for manifest loading here.
This command reads/parses clawup.yaml directly via fs + yaml; please route manifest I/O through packages/cli/lib/config.ts to keep command behavior consistent and centralized.
As per coding guidelines: packages/cli/commands/**/*.ts: Load and save YAML manifests using the config module in packages/cli/lib/config.ts.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/commands/onboard.ts` around lines 43 - 49, The onboard command
currently reads and parses clawup.yaml directly (manifestPath, fs.readFileSync,
YAML.parse and ClawupManifestSchema.safeParse); replace that direct I/O with the
centralized manifest helpers exported from packages/cli/lib/config.ts (use the
module's load/read manifest function and its validated return instead of manual
fs/YAML parsing), then feed the returned object into
ClawupManifestSchema.safeParse or use the module's already-validated result;
ensure any thrown errors from the config module are handled the same way the
current try/catch does so behavior remains consistent.
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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
Adds an
onboardhook type to the plugin manifest schema for first-time interactive plugin setup flows (e.g., creating a Slack app).Cherry-picked from the merged
feat/manifest-hooks-schemabranch onto a fresh branch rebased against latestmain.Changes
OnboardHookSchemawith description, inputs (envVar/prompt/instructions/validator), script, runOncerunOnboardHook()in manifest-hooks.ts — captures stdout for instructions, streams stderr, timeout protectionrunOncesupport: skips if all required secrets already present--skip-onboardCLI flagBug fix included
Fixed the
runOncesecret check that Scout flagged — was comparing raw plugin keys against agent-scoped resolved secrets. Now uses envVar-based env dict lookup which correctly detects already-configured secrets.Tests: 206 passing | Build: ✅ green
Closes AGE-208
Summary by CodeRabbit
New Features
Tests