Make SkillOpt the core optimization workflow#38
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (33)
📝 WalkthroughWalkthroughThe PR replaces GEPA-based routing with bounded SkillOpt optimization, adds baked embedding-cache detection, expands evidence-gated review and rejection workflows, updates authentication behavior, and revises OpenRouter, deployment, security, and SkillOpt documentation. ChangesSkillOpt routing and provider configuration
Evidence-gated review lifecycle
Documentation and repository guidance
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
07b2acf to
983a99e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
optimize/__init__.py (1)
76-85: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake Groq the actual default provider.
When
OPENROUTER_PROVIDERSis unset, this emits noprovider.order, so the default Qwen request uses the generic ZDR pool rather than preferring Groq. Set Groq in code as the default while preserving an explicit empty value as an opt-out; update the corresponding default assertion intests/test_env_check.py.Proposed fix
- order = [p.strip() for p in os.environ.get("OPENROUTER_PROVIDERS", "").split(",") if p.strip()] + configured = os.environ.get("OPENROUTER_PROVIDERS") + order = ([p.strip() for p in configured.split(",") if p.strip()] + if configured is not None else ["groq"])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@optimize/__init__.py` around lines 76 - 85, Update openrouter_extra_body to default provider.order to Groq when OPENROUTER_PROVIDERS is unset, while treating an explicitly empty environment value as an opt-out that omits the order. Update the corresponding default assertion in tests/test_env_check.py to verify Groq is preferred by default.
🧹 Nitpick comments (2)
ui/app.py (1)
202-208: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing exception chaining (ruff B904).
raise HTTPException(404, str(exc))drops the original traceback context. Minor, but flagged by static analysis on this newly-added code path.🔧 Proposed fix
try: components = load_snapshot_components(skill, version) except ValueError as exc: - raise HTTPException(404, str(exc)) + raise HTTPException(404, str(exc)) from excAs per static analysis hints, ruff flags "Within an
exceptclause, raise exceptions withraise ... from errorraise ... from None" (B904) at this line.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/app.py` around lines 202 - 208, Update the exception handling around load_snapshot_components to explicitly chain the HTTPException from the caught ValueError, preserving the original exception context while retaining the existing 404 response and message.Source: Linters/SAST tools
ui/static/index.html (1)
677-680: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStatus pill still says "generating"; inconsistent with the new SkillOpt wording elsewhere.
The per-skill chip now reads "SkillOpt running" (line 634) and the optimize button reads "optimizing…" (line 795), but the top-of-board status pill for the same running state still says just "generating" — a leftover from the pre-SkillOpt wording.
✏️ Proposed fix
const pill = $("`#status-pill`"); if (pending) { pill.className = "pill act"; pill.textContent = `${pending} to review`; } - else if (running) { pill.className = "pill run"; pill.textContent = "generating"; } + else if (running) { pill.className = "pill run"; pill.textContent = "optimizing"; } else { pill.className = "pill idle"; pill.textContent = "idle"; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/static/index.html` around lines 677 - 680, Update the running-state branch of the status-pill logic to display “SkillOpt running” instead of “generating,” keeping the existing class assignment and pending/idle behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/configuration.md`:
- Line 55: Update the “SkillOpt optimization” heading in the documentation to
use level-two Markdown heading syntax, restoring the hierarchy directly beneath
the document H1.
In `@docs/tutorial.md`:
- Around line 341-345: Declare the fenced output block as text by changing the
fence preceding the loop output in the tutorial to use the text language
identifier, while preserving the block contents unchanged.
---
Outside diff comments:
In `@optimize/__init__.py`:
- Around line 76-85: Update openrouter_extra_body to default provider.order to
Groq when OPENROUTER_PROVIDERS is unset, while treating an explicitly empty
environment value as an opt-out that omits the order. Update the corresponding
default assertion in tests/test_env_check.py to verify Groq is preferred by
default.
---
Nitpick comments:
In `@ui/app.py`:
- Around line 202-208: Update the exception handling around
load_snapshot_components to explicitly chain the HTTPException from the caught
ValueError, preserving the original exception context while retaining the
existing 404 response and message.
In `@ui/static/index.html`:
- Around line 677-680: Update the running-state branch of the status-pill logic
to display “SkillOpt running” instead of “generating,” keeping the existing
class assignment and pending/idle behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4be95044-2f45-43ef-aedb-8ff7c79bf2a4
📒 Files selected for processing (38)
.env.exampleAGENTS.mdARCHITECTURE.mdCONTRIBUTING.mdDockerfilePRODUCTION_SETUP.mdREADME.mddocs/configuration.mddocs/evidence-gate.mddocs/how-it-works.mddocs/security.mddocs/sso.mddocs/superpowers/plans/2026-07-15-skills-only-scope.mddocs/superpowers/plans/2026-07-17-human-gated-skill-lifecycle.mddocs/superpowers/specs/2026-07-15-component-pass-optimization.mddocs/superpowers/specs/2026-07-15-skills-only-scope-design.mddocs/superpowers/specs/2026-07-17-human-gated-skill-lifecycle-design.mddocs/superpowers/specs/2026-07-19-sso-rbac-design.mddocs/tutorial.mdmcp_server/embedding.pyoptimize/__init__.pyoptimize/ab.pyoptimize/promote.pyoptimize/rollout.pyoptimize/routing.pyrequirements.txttests/conftest.pytests/test_embedding.pytests/test_env_check.pytests/test_optimize.pytests/test_routing.pytests/test_ui.pyui/app.pyui/auth.pyui/oidc.pyui/oidc_flow.pyui/rbac.pyui/static/index.html
💤 Files with no reviewable changes (8)
- docs/superpowers/specs/2026-07-19-sso-rbac-design.md
- docs/superpowers/specs/2026-07-15-component-pass-optimization.md
- docs/superpowers/plans/2026-07-17-human-gated-skill-lifecycle.md
- docs/superpowers/specs/2026-07-15-skills-only-scope-design.md
- AGENTS.md
- docs/superpowers/plans/2026-07-15-skills-only-scope.md
- requirements.txt
- docs/superpowers/specs/2026-07-17-human-gated-skill-lifecycle-design.md
| `JUDGE_MODELS`) are covered in [The evidence gate](evidence-gate.md). | ||
|
|
||
| ### Candidate generation | ||
| ### SkillOpt optimization |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Restore the heading hierarchy.
### SkillOpt optimization skips directly from the document H1. Change it to ## so markdown linting and document navigation remain valid.
Proposed fix
-### SkillOpt optimization
+## SkillOpt optimization📝 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.
| ### SkillOpt optimization | |
| ## SkillOpt optimization |
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 55-55: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/configuration.md` at line 55, Update the “SkillOpt optimization” heading
in the documentation to use level-two Markdown heading syntax, restoring the
hierarchy directly beneath the document H1.
Source: Linters/SAST tools
| ``` | ||
| [loop] ===== pdf ===== | ||
| [mine] analyzed 23 real traces · mean judge score 0.52 · 11 bad cases | ||
| [loop] pdf: below health bar (mean 0.52), optimizing… | ||
| [loop] done. 1 challenger(s) queued for review: ['pdf'] | ||
| [mine] ... unjudged cluster(s) remain; the next run continues from the cache | ||
| [loop] pdf: mining coverage is ...; deferring the health decision until cached representative judging covers every trace cluster. | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Declare the output block language.
Add text to this fence to satisfy markdownlint.
Proposed fix
-```
+```text
[loop] ===== pdf =====📝 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.
| ``` | |
| [loop] ===== pdf ===== | |
| [mine] analyzed 23 real traces · mean judge score 0.52 · 11 bad cases | |
| [loop] pdf: below health bar (mean 0.52), optimizing… | |
| [loop] done. 1 challenger(s) queued for review: ['pdf'] | |
| [mine] ... unjudged cluster(s) remain; the next run continues from the cache | |
| [loop] pdf: mining coverage is ...; deferring the health decision until cached representative judging covers every trace cluster. | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 341-341: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/tutorial.md` around lines 341 - 345, Declare the fenced output block as
text by changing the fence preceding the loop output in the tutorial to use the
text language identifier, while preserving the block contents unchanged.
Source: Linters/SAST tools
1 participant
What changed
Why
The merged optimization path and its documentation had drifted apart. Hosted provider examples also exposed configurations the project no longer supports, and the approval surface did not make destructive candidate rewrites obvious enough. This change aligns the implementation, defaults, review UX, tests, and operator documentation around the current SkillOpt and OpenRouter architecture.
User and operator impact
Operators get clearer model names, risk signals, version history, audit reasons, reviewer attribution, and remote-agent deployment guidance. Agent setup remains idempotent and repairable. The local router starts without a Hugging Face download because its default embedding artifact is already in the image.
Validation
docker compose config --quietdocker compose build mcp uiuiandmcpservices, both healthy/auth/me,/api/skills, and/api/history, all returned 200Summary by CodeRabbit