fix(onboard): clarify skill credential prompts#74417
fix(onboard): clarify skill credential prompts#74417vyctorbrzezowski wants to merge 0 commit intoopenclaw:mainfrom
Conversation
Greptile SummaryAdds a "Skill API credentials" explanatory note before the credential-prompt loop in Confidence Score: 5/5Safe to merge — changes are UI-only, well-tested, and correctly scoped to the credential prompt phase. The logic change is small and correct: the filter that populates No files require special attention. Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/commands/onboard-skills.ts
Line: 33-35
Comment:
**`primaryEnv` typed as optional but used without guard**
The parameter type allows `primaryEnv` to be `undefined`, so the interpolated string would silently render `"- skillName: undefined"` if the function were ever called outside the current guarded path. Narrowing the type to `string` here makes the contract explicit and prevents accidental misuse.
```suggestion
function formatCredentialSkillLine(skill: { name: string; primaryEnv: string }): string {
return `- ${skill.name}: ${skill.primaryEnv}`;
}
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(onboard): clarify skill credential p..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function formatCredentialSkillLine(skill: { name: string; primaryEnv?: string }): string { | ||
| return `- ${skill.name}: ${skill.primaryEnv}`; | ||
| } |
There was a problem hiding this comment.
primaryEnv typed as optional but used without guard
The parameter type allows primaryEnv to be undefined, so the interpolated string would silently render "- skillName: undefined" if the function were ever called outside the current guarded path. Narrowing the type to string here makes the contract explicit and prevents accidental misuse.
| function formatCredentialSkillLine(skill: { name: string; primaryEnv?: string }): string { | |
| return `- ${skill.name}: ${skill.primaryEnv}`; | |
| } | |
| function formatCredentialSkillLine(skill: { name: string; primaryEnv: string }): string { | |
| return `- ${skill.name}: ${skill.primaryEnv}`; | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/commands/onboard-skills.ts
Line: 33-35
Comment:
**`primaryEnv` typed as optional but used without guard**
The parameter type allows `primaryEnv` to be `undefined`, so the interpolated string would silently render `"- skillName: undefined"` if the function were ever called outside the current guarded path. Narrowing the type to `string` here makes the contract explicit and prevents accidental misuse.
```suggestion
function formatCredentialSkillLine(skill: { name: string; primaryEnv: string }): string {
return `- ${skill.name}: ${skill.primaryEnv}`;
}
```
How can I resolve this? If you propose a fix, please make it concise.
|
Codex review: needs maintainer review before merge. What this changes: The PR adds a Maintainer follow-up before merge: This is already an open implementation PR and it overlaps with open PR #74407 for the same regression, so the next action is maintainer review and canonical branch selection rather than an automated replacement or repair PR. Security review: Security review cleared: Security review cleared: the diff changes onboarding prompt text, a unit test helper/test, and changelog text only, with no new secret handling, network calls, dependency sources, CI permissions, or code execution paths. Review detailsBest possible solution: Land one narrow onboarding fix that clearly separates API-backed skill credential setup from local dependency installation, keeps installer behavior and credential storage unchanged, includes regression coverage for the mixed Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 2a7d83b6ad30. |
vyctorbrzezowski
force-pushed
the
contrib/clarify-skill-env-prompts
branch
from
d0e3e66 to
dd9e675
Compare
vyctorbrzezowski
force-pushed
the
contrib/clarify-skill-env-prompts
branch
2 times, most recently
from
8e548b4 to
26172c8
Compare
vyctorbrzezowski
force-pushed
the
contrib/clarify-skill-env-prompts
branch
from
26172c8 to
6470a23
Compare
1 participant
Summary
Describe the problem and fix in 2–5 bullets:
openai-whisper, and then immediately ask for API keys required by separate API-backed skills.Skill API credentialsnote before env-gated skill prompts and covered the mixedopenai-whisper/openai-whisper-api/sagscenario.AI-assisted: yes.
Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Root Cause (if applicable)
openai-whisperskill has noprimaryEnv, whileopenai-whisper-apiandsagrequire API key env vars.Regression Test Plan (if applicable)
src/commands/onboard-skills.test.tsopenai-whisper-apiandsag.setupSkillsowns both the install selection prompt and the credential prompt loop.User-visible / Behavior Changes
Onboarding now shows a short note before skill API credential prompts, clarifying that those prompts are separate from local dependency installation.
Diagram (if applicable)
Security Impact (required)
Yes/No) NoYes/No) NoYes/No) NoYes/No) NoYes/No) NoYes, explain risk + mitigation: N/ARepro + Verification
Environment
Steps
openai-whisper,openai-whisper-api, andsagrequirements.Expected
Actual
Evidence
Human Verification (required)
What you personally verified (not just CI), and how:
setupSkillsunit coverage.primaryEnvand missing env requirements; install prompt still shows the keyless local skill hint.Review Conversations
If a bot review conversation is addressed by this PR, resolve that conversation yourself. Do not leave bot review conversation cleanup for maintainers.
Compatibility / Migration
Yes/No) YesYes/No) NoYes/No) NoRisks and Mitigations
Validation