fix: re-verify security findings + doctor API-key logic in skill mode#19
Conversation
… generate In skill mode (Claude Code, Codex, Gemini CLI, etc.) the host agent IS the model — calling forerunner generate should output the assembled prompt bundle for the agent to process, not make a provider API call. Changes: - `forerunner generate --prompt-only <task>`: outputs the assembled prompt bundle to stdout and exits; no API key or provider needed. This is the correct command for skill-mode use in all SKILL.md Execute sections. - Auto-detect skill context: when no explicit provider is configured, no API key is set, and Ollama is not running, cmd_generate now emits the prompt bundle directly (exit 0) instead of erroring. On a TTY, adds a brief stderr hint; on piped stdout, emits cleanly with no extra output. - `forerunner doctor`: "no API key" is now [ok] when a managed skill destination is installed (skill mode active) rather than [warn]. - All 26 per-task SKILL.md Execute sections updated: `forerunner doc <task>` → `forerunner generate --prompt-only <task>` - Canonical skill body (agent/codeforerunner.skill.md) updated with skill-mode explanation and synced to both distribution copies. - 194 tests pass (7 updated to reflect new behavior). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR implements "skill mode," enabling the host agent to assemble and emit prompt bundles via ChangesSkill Mode Feature
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/codeforerunner/doctor.py`:
- Around line 299-306: The current early return that emits Finding("ok",
"provider-api-key", ...) when _skill_mode_active() is true hides missing API-key
failures for explicitly configured providers; instead, change the logic in the
doctor check so that _skill_mode_active() only suppresses the missing-key
warning for providers that are not explicitly/configured, and if the provider is
configured (the same provider used by generate in cli.py), emit a failing
Finding indicating the API key is missing. In practice, remove or adjust the
unconditional return under _skill_mode_active(), add a check for whether the
provider is explicitly configured (the same condition used by generate in
cli.py), and if configured create a non-"ok" Finding reporting the missing
env_var; otherwise keep the skill-mode "ok" behavior.
🪄 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: 742b7136-d1bd-4888-a81f-cf69c40f8df3
📒 Files selected for processing (31)
agent/codeforerunner.skill.mdplugins/codeforerunner/skills/codeforerunner/SKILL.mdplugins/codeforerunner/skills/forerunner-api-docs/SKILL.mdplugins/codeforerunner/skills/forerunner-audit/SKILL.mdplugins/codeforerunner/skills/forerunner-changelog/SKILL.mdplugins/codeforerunner/skills/forerunner-check/SKILL.mdplugins/codeforerunner/skills/forerunner-diagrams/SKILL.mdplugins/codeforerunner/skills/forerunner-flows/SKILL.mdplugins/codeforerunner/skills/forerunner-init/SKILL.mdplugins/codeforerunner/skills/forerunner-readme/SKILL.mdplugins/codeforerunner/skills/forerunner-review/SKILL.mdplugins/codeforerunner/skills/forerunner-scan/SKILL.mdplugins/codeforerunner/skills/forerunner-stack-docs/SKILL.mdplugins/codeforerunner/skills/forerunner-version-audit/SKILL.mdskills/codeforerunner/SKILL.mdskills/forerunner-api-docs/SKILL.mdskills/forerunner-audit/SKILL.mdskills/forerunner-changelog/SKILL.mdskills/forerunner-check/SKILL.mdskills/forerunner-diagrams/SKILL.mdskills/forerunner-flows/SKILL.mdskills/forerunner-init/SKILL.mdskills/forerunner-readme/SKILL.mdskills/forerunner-review/SKILL.mdskills/forerunner-scan/SKILL.mdskills/forerunner-stack-docs/SKILL.mdskills/forerunner-version-audit/SKILL.mdsrc/codeforerunner/cli.pysrc/codeforerunner/doctor.pytests/test_cli.pytests/test_doctor.py
…in skill mode doctor was returning [ok] when skill mode active + provider configured + key missing. forerunner generate (without --prompt-only) still refuses to run in that case, so the ok was misleading. Now always emits warn; appends a --prompt-only hint when skill mode is detected so the user knows the key-free path exists. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Kept origin/main's _get_bundle(ns) → bundle structure in cmd_generate and the stream test added by the model-default PR. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…curity findings - Remove skill-mode conditional hint from _check_provider_api_key warn path. When a provider is explicitly configured in forerunner.config.yaml, a missing API key is always a real problem — skill mode does not excuse it. No-config + skill-mode-active path correctly returns ok (unchanged). - Fix L5: replace redundant counts.get() with direct += on pre-initialized dict. - Add test: explicit provider + skill mode active + missing key → warn (not ok). Security finding verification (all H/M/L from review): H1 fixed (--run-scripts gate), H2 fixed (npx pinned @0.4.1), H3 fixed (comment), M1-M7 fixed, L1-L9 fixed (L5 fixed in this commit; all others verified present). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Summary
counts.get()was redundant on a pre-initialized dict informat_report; replaced withcounts[f.severity] += 1._check_provider_api_keywas adding a--prompt-onlyhint conditionally when_skill_mode_active()was true, making the Finding differ from outside-skill-mode. When a provider is explicitly configured inforerunner.config.yaml, a missing API key is always a real failure — skill mode should not change the severity or message. Removed the conditional; warn path now always emits the sameFindingregardless of skill mode. No-config + skill-mode path correctly returnsok(unchanged — no explicit provider means no API key needed).Security finding status table
--run-scriptsgate in placecodeforerunner@0.4.1/v0.4.1pinned in install.sh + install.ps1tasks.check.enabled_rulescorrect in _STARTER_CONFIG and config.py/,\\,..blocked +is_relative_tochecktimeout=120on allurlopencallsany_errorflag_to_intwraps into ConfigError_validate_ollama_basein ollama.pycounts[f.severity] += 1_initializedflag + error response.includes()usedtest_generate_stream_flag_yields_chunksexistsparents[:10]depth limitTest plan
uv run python -m pytest tests/ -q→ 196 passed, 1 skippedtest_provider_api_key_warn_even_in_skill_mode_when_provider_explicitverifies warn is emitted when skill mode active + explicit provider + missing key🤖 Generated with Claude Code