Feat/proposal provenance and skill packages#10
Conversation
…t, goal-loop, cron-job-workflows patch)
…op, cron-job-workflows)
📝 WalkthroughWalkthroughAdds SQLite-backed ChangesController Run Persistence, CLI Commands, and Loop Engineering Docs
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant SkillLoopStore
participant controller_tick
rect rgba(70, 130, 180, 0.5)
Note over User,SkillLoopStore: skillloop setup
User->>CLI: setup --connect hermes --start
CLI->>SkillLoopStore: init()
CLI->>CLI: validate bounds, build SkillLoopPolicy
CLI->>SkillLoopStore: write policy.json
CLI->>controller_tick: controller_tick(store, policy)
controller_tick->>SkillLoopStore: save_controller_run(payload)
SkillLoopStore-->>controller_tick: run id
controller_tick-->>CLI: ControllerRunReport
CLI-->>User: policy written + tick summary
end
rect rgba(60, 179, 113, 0.5)
Note over User,SkillLoopStore: skillloop controller show
User->>CLI: controller show abc123
CLI->>SkillLoopStore: get_controller_run("abc123")
SkillLoopStore->>SkillLoopStore: exact match → LIKE prefix with escaped wildcards
SkillLoopStore-->>CLI: decoded JSON payload
CLI-->>User: JSON run report
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 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)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
![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 (2)
tests/test_cli.py (1)
127-136: ⚡ Quick winAdd a corrupt-policy CLI regression test.
Given
statusnow reports manifest parse errors, add a sibling test for malformed.skillloop/policy.jsonto ensure policy load failures return a clean CLI error path instead of a traceback.🤖 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 `@tests/test_cli.py` around lines 127 - 136, Add a new test function similar to test_cli_status_handles_corrupt_dataset_manifest to verify policy load error handling. Create a corrupt .skillloop/policy.json file by creating the directory structure and writing invalid JSON content, then call main with the status command and verify it returns exit code 0 and outputs a clean error message about the policy load failure (using capsys to capture output), ensuring the CLI handles policy parsing errors gracefully without raising a traceback.tests/test_store.py (1)
65-80: ⚡ Quick winAdd store-contract tests for invalid
limitand emptyrun_id.These controller-run tests are solid; add two small assertions to lock behavior for
SkillLoopStore.list_controller_runs(limit=0/-1)andSkillLoopStore.get_controller_run("")once the API validation is added.🤖 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 `@tests/test_store.py` around lines 65 - 80, Add validation assertions to the test_store.py file to lock in expected behavior for SkillLoopStore API contract validation. Following the pattern of test_store_validates_controller_run_report_contract, add test assertions that verify list_controller_runs raises ValueError when called with limit=0 or limit=-1, and that get_controller_run raises ValueError when called with an empty string run_id. These assertions should document the contract validation behavior for invalid limit values and empty identifiers.
🤖 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/analysis/loop-engineering-analysis.md`:
- Around line 1-6: Verify the Addy Osmani source attribution in the opening
section. Confirm whether you are referencing the June 7, 2026 blog post on
addyosmani.com or a separate X post from June 8, 2026, and update the source
date and platform accordingly if needed. Replace or remove the stale engagement
metrics (836K views, 11.5K bookmarks, 4.8K likes) that are outdated as of June
16, 2026—either use relative descriptors like "widely shared" or add an explicit
timestamp indicating when the metrics were captured. Finally, verify the core
quote "Loop engineering is replacing yourself as the person who prompts the
agent. You design the system that does it instead." matches the exact wording
from the original source.
In `@skillloop/cli.py`:
- Around line 79-81: The current validation in cmd_setup only checks if
hermes_db exists using exists(), but this returns true for directories as well.
Replace the exists() check with is_file() on the hermes_db path object to ensure
the provided path is actually a file and not a directory, which will prevent
unclear runtime errors later.
- Around line 35-39: The SkillLoopPolicy.load(path) call in the _load_policy
function can throw exceptions when the policy file contains invalid JSON or
violates the schema, causing commands like status and controller run to crash.
Wrap the SkillLoopPolicy.load(path) call in a try-except block to catch any
exceptions thrown during loading, then raise a SystemExit with a descriptive
error message that includes the file path to provide clear context about which
policy file is malformed.
In `@skillloop/store.py`:
- Around line 202-208: Add validation in the list_controller_runs method to
reject non-positive limit values before they are used in the SQL query. When
limit is not None, check if it is less than or equal to zero and raise an
appropriate ValueError or similar exception with a clear error message. This
validation should occur immediately after the init() call and before
constructing the SQL query, ensuring that SQLite never receives a non-positive
LIMIT value.
- Around line 213-221: The get_controller_run method accepts an empty run_id
which results in a broad LIKE '%' database scan when no exact match is found.
Add validation at the start of the get_controller_run method to reject empty or
whitespace-only run_id values before any database operations, either by raising
an appropriate exception or returning early. This ensures strict lookup
semantics and prevents unintended broad prefix scans.
---
Nitpick comments:
In `@tests/test_cli.py`:
- Around line 127-136: Add a new test function similar to
test_cli_status_handles_corrupt_dataset_manifest to verify policy load error
handling. Create a corrupt .skillloop/policy.json file by creating the directory
structure and writing invalid JSON content, then call main with the status
command and verify it returns exit code 0 and outputs a clean error message
about the policy load failure (using capsys to capture output), ensuring the CLI
handles policy parsing errors gracefully without raising a traceback.
In `@tests/test_store.py`:
- Around line 65-80: Add validation assertions to the test_store.py file to lock
in expected behavior for SkillLoopStore API contract validation. Following the
pattern of test_store_validates_controller_run_report_contract, add test
assertions that verify list_controller_runs raises ValueError when called with
limit=0 or limit=-1, and that get_controller_run raises ValueError when called
with an empty string run_id. These assertions should document the contract
validation behavior for invalid limit values and empty identifiers.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
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: cb3101b7-0091-4a73-8706-ca7440833b41
📒 Files selected for processing (16)
.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.pytests/test_store.py
💤 Files with no reviewable changes (2)
- docs/next-steps.md
- docs/training-integrations.md
| # Deep Analysis: Addy Osmani's Loop Engineering vs SkillLoop/Hermes | ||
|
|
||
| **Source:** Addy Osmani (@addyosmani) on X, June 8, 2026. 836K views, 11.5K bookmarks, 4.8K likes. | ||
| **Core quote:** "Loop engineering is replacing yourself as the person who prompts the agent. You design the system that does it instead." | ||
|
|
||
| --- |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Addy Osmani loop engineering post June 2026
💡 Result:
In a blog post published on June 7, 2026, Addy Osmani introduced the term "Loop Engineering" to describe an emerging paradigm in working with AI coding agents [1][2]. The core concept is a shift from manual, one-time prompt engineering to designing automated, recursive systems—or "loops"—where an AI agent iterates toward a goal without continuous human intervention [1][3]. Osmani identified five key building blocks that constitute these loops [2][4]: 1. Automations: The "heartbeat" of the loop, consisting of scheduled tasks (like discovery or triage) and run-until-done primitives [1][2][4]. 2. Worktrees: Mechanisms (often based on git worktrees) that allow for isolated, parallel execution environments [2][4]. 3. Skills: Codified project knowledge (e.g., using a SKILL.md format) that prevents agents from re-deriving project conventions every cycle [2][4]. 4. Plugins/Connectors: Integrations (frequently based on MCP) that connect the loop to external systems, such as opening pull requests or updating tickets [2][4]. 5. Sub-agents: Specialized agents that separate execution from verification, addressing the challenge of agents potentially being too lenient when grading their own work [2][4]. Osmani noted that the highest-leverage skill for engineers is moving from "directly operating the agent" to designing the systems that govern the agent's behavior [2][3]. He observed that these capabilities are increasingly being integrated directly into tools like the Codex app and Claude Code [1][4]. Osmani also cautioned that while this approach offers significant automation potential, it is not inherently easier than prompt engineering and requires careful management of token costs and system observability [1][3].
Citations:
- 1: https://addyosmani.com/blog/loop-engineering/
- 2: https://www.gate.com/news/detail/google-engineer-loop-engineerings-five-building-blocks-let-ai-automatically-21751012
- 3: https://loomstack.co/blog/loop-engineering/
- 4: https://www.truefoundry.com/blog/loop-engineering-enterprise-agent-runtime
Verify the source and date for the Addy Osmani reference.
The document attributes analysis to Addy Osmani on X from June 8, 2026, but web search confirms a blog post was published June 7, 2026 (not June 8) on addyosmani.com. Clarify whether this analysis references the June 7 blog post or a separate X post from June 8, and update the source attribution accordingly.
The specific metrics (836K views, 11.5K bookmarks, 4.8K likes) are stale as of June 16, 2026. Either remove these numbers, replace with relative descriptors like "widely shared," or explicitly mark when the metrics were captured.
The core concept in your quote aligns with Osmani's framework (moving from directly operating the agent to designing systems that govern agent behavior), but verify the exact wording is accurate to the source.
🤖 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/analysis/loop-engineering-analysis.md` around lines 1 - 6, Verify the
Addy Osmani source attribution in the opening section. Confirm whether you are
referencing the June 7, 2026 blog post on addyosmani.com or a separate X post
from June 8, 2026, and update the source date and platform accordingly if
needed. Replace or remove the stale engagement metrics (836K views, 11.5K
bookmarks, 4.8K likes) that are outdated as of June 16, 2026—either use relative
descriptors like "widely shared" or add an explicit timestamp indicating when
the metrics were captured. Finally, verify the core quote "Loop engineering is
replacing yourself as the person who prompts the agent. You design the system
that does it instead." matches the exact wording from the original source.
| def _load_policy(store: SkillLoopStore) -> SkillLoopPolicy: | ||
| path = _policy_path(store) | ||
| if not path.exists(): | ||
| return SkillLoopPolicy.default() | ||
| return SkillLoopPolicy.load(path) |
There was a problem hiding this comment.
Gracefully handle malformed policy files.
Line 39 can throw on invalid JSON/schema and currently crashes commands like status and controller run. Convert this into a clean SystemExit with file context.
Proposed fix
def _load_policy(store: SkillLoopStore) -> SkillLoopPolicy:
path = _policy_path(store)
if not path.exists():
return SkillLoopPolicy.default()
- return SkillLoopPolicy.load(path)
+ try:
+ return SkillLoopPolicy.load(path)
+ except (OSError, ValueError, json.JSONDecodeError) as exc:
+ raise SystemExit(f"Failed to load policy at {path}: {exc}") from 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 35 - 39, The SkillLoopPolicy.load(path) call
in the _load_policy function can throw exceptions when the policy file contains
invalid JSON or violates the schema, causing commands like status and controller
run to crash. Wrap the SkillLoopPolicy.load(path) call in a try-except block to
catch any exceptions thrown during loading, then raise a SystemExit with a
descriptive error message that includes the file path to provide clear context
about which policy file is malformed.
| hermes_db = Path(args.db_path).expanduser().resolve() | ||
| if not hermes_db.exists(): | ||
| raise SystemExit(f"Hermes state database not found: {hermes_db}") |
There was a problem hiding this comment.
Validate --db-path is a file.
Line 80 only checks existence; directories pass and fail later with a less clear runtime error. Require is_file() in cmd_setup.
Proposed fix
hermes_db = Path(args.db_path).expanduser().resolve()
- if not hermes_db.exists():
- raise SystemExit(f"Hermes state database not found: {hermes_db}")
+ if not hermes_db.exists() or not hermes_db.is_file():
+ raise SystemExit(f"Hermes state database file not found: {hermes_db}")📝 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.
| hermes_db = Path(args.db_path).expanduser().resolve() | |
| if not hermes_db.exists(): | |
| raise SystemExit(f"Hermes state database not found: {hermes_db}") | |
| hermes_db = Path(args.db_path).expanduser().resolve() | |
| if not hermes_db.exists() or not hermes_db.is_file(): | |
| raise SystemExit(f"Hermes state database file not found: {hermes_db}") |
🤖 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 79 - 81, The current validation in cmd_setup
only checks if hermes_db exists using exists(), but this returns true for
directories as well. Replace the exists() check with is_file() on the hermes_db
path object to ensure the provided path is actually a file and not a directory,
which will prevent unclear runtime errors later.
| def list_controller_runs(self, limit: int | None = None) -> list[dict]: | ||
| self.init() | ||
| query = "SELECT payload FROM controller_runs ORDER BY started_at DESC" | ||
| args: tuple[int, ...] = () | ||
| if limit is not None: | ||
| query += " LIMIT ?" | ||
| args = (int(limit),) |
There was a problem hiding this comment.
Validate limit at the store boundary.
Line 207 currently applies SQL LIMIT to any integer. With SQLite, non-positive values can yield surprising results (including effectively unbounded reads), which bypasses caller intent. Reject limit <= 0 in SkillLoopStore.list_controller_runs.
Proposed fix
def list_controller_runs(self, limit: int | None = None) -> list[dict]:
self.init()
query = "SELECT payload FROM controller_runs ORDER BY started_at DESC"
args: tuple[int, ...] = ()
if limit is not None:
+ limit = int(limit)
+ if limit <= 0:
+ raise ValueError("limit must be positive")
query += " LIMIT ?"
- args = (int(limit),)
+ args = (limit,)📝 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.
| def list_controller_runs(self, limit: int | None = None) -> list[dict]: | |
| self.init() | |
| query = "SELECT payload FROM controller_runs ORDER BY started_at DESC" | |
| args: tuple[int, ...] = () | |
| if limit is not None: | |
| query += " LIMIT ?" | |
| args = (int(limit),) | |
| def list_controller_runs(self, limit: int | None = None) -> list[dict]: | |
| self.init() | |
| query = "SELECT payload FROM controller_runs ORDER BY started_at DESC" | |
| args: tuple[int, ...] = () | |
| if limit is not None: | |
| limit = int(limit) | |
| if limit <= 0: | |
| raise ValueError("limit must be positive") | |
| query += " LIMIT ?" | |
| args = (limit,) |
🤖 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 202 - 208, Add validation in the
list_controller_runs method to reject non-positive limit values before they are
used in the SQL query. When limit is not None, check if it is less than or equal
to zero and raise an appropriate ValueError or similar exception with a clear
error message. This validation should occur immediately after the init() call
and before constructing the SQL query, ensuring that SQLite never receives a
non-positive LIMIT value.
| def get_controller_run(self, run_id: str) -> dict: | ||
| self.init() | ||
| with self._connect() as conn: | ||
| row = conn.execute("SELECT payload FROM controller_runs WHERE id = ?", (run_id,)).fetchone() | ||
| if row is None: | ||
| escaped_id = run_id.replace("\\", "\\\\").replace("%", "\\%").replace("_", "\\_") | ||
| rows = conn.execute( | ||
| "SELECT payload FROM controller_runs WHERE id LIKE ? ESCAPE '\\' ORDER BY started_at DESC", | ||
| (f"{escaped_id}%",), |
There was a problem hiding this comment.
Reject empty run IDs before prefix lookup.
Line 218 turns an empty run_id into a broad LIKE '%' scan. Fail fast on empty IDs in SkillLoopStore.get_controller_run to keep lookup semantics strict and predictable.
Proposed fix
def get_controller_run(self, run_id: str) -> dict:
self.init()
+ if not str(run_id).strip():
+ raise ValueError("run_id must be non-empty")
with self._connect() as conn:
row = conn.execute("SELECT payload FROM controller_runs WHERE id = ?", (run_id,)).fetchone()🤖 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 213 - 221, The get_controller_run method
accepts an empty run_id which results in a broad LIKE '%' database scan when no
exact match is found. Add validation at the start of the get_controller_run
method to reject empty or whitespace-only run_id values before any database
operations, either by raising an appropriate exception or returning early. This
ensures strict lookup semantics and prevents unintended broad prefix scans.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1e5bfc9db1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| store = _store(args) | ||
| if args.limit is not None and args.limit <= 0: | ||
| raise SystemExit("--limit must be positive") | ||
| runs = store.list_controller_runs(limit=args.limit) |
There was a problem hiding this comment.
On projects upgraded from the previous controller implementation, prior reports exist only as .skillloop/controller_runs/*.json because save_controller_report used to write just those files. This new history path reads only the new SQLite table, so controller history (and similarly status/show) reports no prior runs until another tick occurs, even though the report files are still present. Consider migrating or falling back to the JSON reports when the table is empty.
Useful? React with 👍 / 👎.
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. ❌ Cannot run autofix: This PR has merge conflicts. Please resolve the conflicts with the base branch and try again. Alternatively, use |
1 participant
Summary by CodeRabbit
New Features
setupcommand to configure SkillLoop with policy managementstatuscommand to report current state and statisticscontrollercommand group withrun,history, andshowsubcommands for managing autonomous controller operationsDocumentation