Add SkillLoop setup/status controller UX#9
Merged
lamenting-hawthorn merged 6 commits intoJun 15, 2026
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds controller-run persistence and store APIs, extends the skillloop CLI with setup/status/controller commands and policy helpers, adds tests asserting persistence and prefix lookup behavior, and introduces extensive loop-engineering documentation and three Hermes skills (pre-loop-checklist, cron-job-workflows, goal-loop). ChangesController CLI and Persistence Implementation
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
…t, goal-loop, cron-job-workflows patch)
…op, cron-job-workflows)
lamenting-hawthorn
force-pushed
the
feat/proposal-provenance-and-skill-packages
branch
from
June 13, 2026 21:59
a4fc05c to
f7bf18c
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: 5
🧹 Nitpick comments (3)
skillloop/cli.py (2)
456-456: 💤 Low valueValidate that
limitis positive when provided.Line 456 uses
args.limitwithout validation. In SQLite, a negativeLIMITis treated as "no limit," and zero returns no rows. This may confuse users. Consider validatinglimit > 0or documenting the behavior.🤖 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 `@skillloop/cli.py` at line 456, Validate args.limit before calling store.list_controller_runs: ensure that if args.limit is not None it is an integer > 0 and reject or normalize invalid values (e.g., raise a user-facing error or ignore/convert zero/negative to None) so you never pass zero or negative to store.list_controller_runs; update the CLI handling where runs = store.list_controller_runs(limit=args.limit) is invoked to perform this check and return a clear error message or fallback behavior when args.limit <= 0.
112-118: ⚡ Quick winHandle JSON decode errors when loading the dataset manifest.
Line 113 loads the manifest JSON without error handling. If the file is corrupt, the user will see a raw
json.JSONDecodeErrortraceback. Consider wrapping in a try/except for a friendlier message.🛡️ Proposed fix: add error handling
dataset_stats = None if dataset_manifest.exists(): - manifest = json.loads(dataset_manifest.read_text(encoding="utf-8")) - dataset_stats = { - "manifest": str(dataset_manifest), - "records": manifest.get("records", 0), - "estimated_tokens": manifest.get("estimated_tokens", 0), - } + try: + manifest = json.loads(dataset_manifest.read_text(encoding="utf-8")) + dataset_stats = { + "manifest": str(dataset_manifest), + "records": manifest.get("records", 0), + "estimated_tokens": manifest.get("estimated_tokens", 0), + } + except (json.JSONDecodeError, OSError) as exc: + dataset_stats = {"error": f"Failed to load manifest: {exc}"}🤖 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 `@skillloop/cli.py` around lines 112 - 118, Wrap the JSON load of dataset_manifest (the line manifest = json.loads(dataset_manifest.read_text(...)) and the subsequent population of dataset_stats) in a try/except that catches json.JSONDecodeError; on error, emit a clear, user-friendly message (e.g., using the existing logger or a click.echo) indicating the manifest is corrupt and include the filename, then set dataset_stats to a safe default (manifest path plus records and estimated_tokens = 0) or bail out gracefully as appropriate. Ensure you import json.JSONDecodeError if needed and do not let the raw traceback propagate to the user.skillloop/store.py (1)
187-196: ⚡ Quick winConsider validating required keys or documenting the report dict contract.
save_controller_runis a public method accepting a dict. Line 189 assumesreport["id"]exists (will raiseKeyErrorif missing), while Line 194 uses.get()with an empty-string fallback forstarted_at. Ifstarted_atis missing, inserting""may mask a caller bug rather than fail fast.🛡️ Proposed fix: validate required keys
def save_controller_run(self, report: dict) -> str: self.init() + if "id" not in report or not report["id"]: + raise ValueError("report must have non-empty 'id'") + if "started_at" not in report: + raise ValueError("report must have 'started_at'") run_id = str(report["id"]) payload = json.dumps(report, ensure_ascii=False) with self._connect() as conn: conn.execute( "INSERT OR REPLACE INTO controller_runs (id, started_at, finished_at, payload) VALUES (?, ?, ?, ?)", - (run_id, str(report.get("started_at") or ""), report.get("finished_at"), payload), + (run_id, str(report["started_at"]), report.get("finished_at"), payload), ) return run_id🤖 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 `@skillloop/store.py` around lines 187 - 196, The save_controller_run method assumes required keys in the report dict (uses report["id"] and falls back to "" for started_at), so add explicit validation at the top of save_controller_run: verify 'id' exists and is non-empty (raise ValueError with a clear message if missing) and decide whether 'started_at' is required—if it is, validate it similarly instead of defaulting to "" (or if optional, keep using report.get('started_at') but persist None rather than an empty string); keep the rest of the logic (self.init(), payload = json.dumps(...), and the DB insert via self._connect()) unchanged and convert id/started_at to strings only after validation.
🤖 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 `@references/hermes-skills/goal-loop/SKILL.md`:
- Around line 241-244: The example in SKILL.md sets workdir to a host-specific
path (workdir='/Users/raghav/myproject'); change that to a neutral, non-personal
path such as a repo-relative or placeholder (for example workdir='./myproject'
or workdir='${REPO_ROOT}/myproject' or workdir='<your-project-path>') so readers
can copy/paste without editing a personal machine path; update the line that
assigns workdir in the given example block to use the chosen neutral
placeholder.
- Around line 176-178: The current guidance tells the skill to find its own cron
entry by name/prompt text and then call cronjob(action='remove') which can
delete the wrong job; instead, capture and persist the exact job_id returned
when scheduling the job (the response from cronjob(action='create' or schedule
API)), then after reporting success call cronjob(action='remove',
job_id=THE_JOB_ID) with that stored job_id to deterministically self-cancel;
update any places that instruct searching by name/prompt (the lines using
cronjob(action='list') and fuzzy matching) to use the persisted job_id approach
instead.
In `@skillloop/cli.py`:
- Line 88: Validate args.max_sessions is a positive integer before passing it
into the constructor: check args.max_sessions > 0 (or use an argparse
type/validator) and if not, raise a clear error (e.g.,
argparse.ArgumentTypeError or print an error and exit) so the assignment
max_sessions=args.max_sessions never receives zero or negative values; update
the validation near where max_sessions=args.max_sessions is set (referencing
args.max_sessions and the code that constructs the object receiving
max_sessions).
- Around line 90-92: Validate args.min_score before assigning it to
policy.dataset, policy.evaluation.min_score, and when constructing
LoopCondition; ensure it falls within the expected range (e.g., 0–100) and raise
or exit with a clear error message if out of range. Specifically, add a guard
near the top of the CLI handling that checks args.min_score and normalizes or
rejects invalid values, then only set policy.dataset = DatasetPolicy(...),
policy.evaluation.min_score = args.min_score, and policy.evaluation.condition =
LoopCondition(score_gte=args.min_score) after the value passes validation.
In `@skillloop/store.py`:
- Around line 209-224: In get_controller_run, escape '%' '_' and backslash in
the run_id before using it in the LIKE pattern to avoid unintended wildcard
matches: replace '\' with '\\', '%' with '\%', and '_' with '\_' to produce
escaped_id, then use conn.execute with the pattern f"{escaped_id}%" and include
an ESCAPE '\' clause in the SQL (e.g., "WHERE id LIKE ? ESCAPE '\'") so the
prefix search treats run_id literally; keep the exact error handling and JSON
loading unchanged.
---
Nitpick comments:
In `@skillloop/cli.py`:
- Line 456: Validate args.limit before calling store.list_controller_runs:
ensure that if args.limit is not None it is an integer > 0 and reject or
normalize invalid values (e.g., raise a user-facing error or ignore/convert
zero/negative to None) so you never pass zero or negative to
store.list_controller_runs; update the CLI handling where runs =
store.list_controller_runs(limit=args.limit) is invoked to perform this check
and return a clear error message or fallback behavior when args.limit <= 0.
- Around line 112-118: Wrap the JSON load of dataset_manifest (the line manifest
= json.loads(dataset_manifest.read_text(...)) and the subsequent population of
dataset_stats) in a try/except that catches json.JSONDecodeError; on error, emit
a clear, user-friendly message (e.g., using the existing logger or a click.echo)
indicating the manifest is corrupt and include the filename, then set
dataset_stats to a safe default (manifest path plus records and estimated_tokens
= 0) or bail out gracefully as appropriate. Ensure you import
json.JSONDecodeError if needed and do not let the raw traceback propagate to the
user.
In `@skillloop/store.py`:
- Around line 187-196: The save_controller_run method assumes required keys in
the report dict (uses report["id"] and falls back to "" for started_at), so add
explicit validation at the top of save_controller_run: verify 'id' exists and is
non-empty (raise ValueError with a clear message if missing) and decide whether
'started_at' is required—if it is, validate it similarly instead of defaulting
to "" (or if optional, keep using report.get('started_at') but persist None
rather than an empty string); keep the rest of the logic (self.init(), payload =
json.dumps(...), and the DB insert via self._connect()) unchanged and convert
id/started_at to strings only after validation.
🪄 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 Plus
Run ID: 5a27e520-13f7-4581-bedb-70ae8bac30df
📒 Files selected for processing (15)
.gitignoreREADME.mddocs/analysis/loop-engineering-analysis.mddocs/cli.mddocs/next-steps.mddocs/training-integrations.mdreferences/hermes-skills/README.mdreferences/hermes-skills/cron-job-workflows/SKILL.mdreferences/hermes-skills/goal-loop/SKILL.mdreferences/hermes-skills/pre-loop-checklist/SKILL.mdskillloop/cli.pyskillloop/controller.pyskillloop/store.pytests/test_cli.pytests/test_controller.py
💤 Files with no reviewable changes (2)
- docs/next-steps.md
- docs/training-integrations.md
lamenting-hawthorn
deleted the
feat/proposal-provenance-and-skill-packages
branch
June 15, 2026 07:53
lamenting-hawthorn
restored the
feat/proposal-provenance-and-skill-packages
branch
June 15, 2026 07:54
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
Verification
Summary by CodeRabbit
New Features
Documentation
Tests