Add in-app rule authoring backed by a draft rules table - #402
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds experimental in-app SML rule authoring with persistent drafts, AST validation, Rule Builder parsing, ACL protection, local deployment, optional ChangesRule Drafts Feature
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant UI as Rule Draft UI
participant API as rule_drafts blueprint
participant Engine as OspreyEngine
participant DB as rule_drafts table
participant FS as rules directory
UI->>API: Create or validate draft
API->>Engine: Validate SML sources
API->>DB: Save draft
UI->>API: Deploy draft
API->>FS: Write SML and optional Require
API->>DB: Mark draft deployed
API-->>UI: Return draft or deployment status
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
New ui-api blueprint for authoring SML rule drafts from the UI, with submission routed through a pluggable backend so each deployment picks where drafts go for review. Endpoints (all gated by a new CAN_EDIT_RULE_DRAFTS ability, granted to super_user): get-source, validate, vocabulary, submit, pending, and parse-into-builder. Validate and submit splice the draft into the engine's loaded sources and re-run the same AST validation the engine uses; submit re-validates server-side before touching any backend. Backends implement the RuleSubmissionBackend Protocol and are selected by OSPREY_RULES_SUBMISSION_BACKEND: - null (default): fails fast with 503 so an unconfigured install never writes anything - github: opens a PR via the REST API; supports GitHub Enterprise - local: writes into a mounted rules directory Contract and safety details: - SubmissionResult/PendingDraft.to_json spread extras first so a backend-specific extra can't shadow the canonical title/url/ main_sml_updated fields the UI depends on - Forge transport failures (connection refused, timeout) become the structured 502 the UI renders, not an unhandled 500, via a shared _rule_drafts_git_common.request() helper that also holds the branch-name and main.sml Require helpers - main.sml is rejected as a draft path: wholesale-replacing the engine entry point is not a draft; wiring a rule in is the controlled wire_into_main append Adopter docs for the env vars are in docs/user/manage.md. Follow-ups add the rule-editor UI, a GitLab backend, and a Tangled (ATProto) backend. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VZ4RQtuHCCgurfpjfPXXAM
bcd5a3e to
5333ad8
Compare
|
screenshots and more context in #394 |
![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)
osprey_worker/src/osprey/worker/ui_api/osprey/views/tests/test_rule_drafts.py (1)
386-427: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a shared helper for repeated GitHub mock scaffolding.
Several tests (happy path, main.sml wiring, wiring skip, 409 conflict, GHE) repeat near-identical
requests_mocksetup forgit/ref/heads/main,contents/..., andpulls. A small factory (e.g._mock_happy_path_github(m, repo_url, path, ...)) could reduce duplication without hurting readability.
[optional_and_nice_to_have]Also applies to: 430-477, 480-520, 522-548, 770-808
🤖 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 `@osprey_worker/src/osprey/worker/ui_api/osprey/views/tests/test_rule_drafts.py` around lines 386 - 427, Several rule draft tests repeat the same GitHub `requests_mock` setup for `git/ref/heads/main`, `contents/...`, and `pulls`, so factor that scaffolding into a shared helper to reduce duplication. Add a small test utility (for example, a mock factory used by `test_submit_happy_path_creates_branch_commits_and_opens_pr` and the other affected `test_rule_drafts` cases) that registers the common endpoints and accepts the varying path/status/response details, then update each test to call it instead of inlining the repeated mocks.osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_github.py (1)
103-112: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate GET+404-handling logic between
_get_file_shaand_fetch_file_on_ref.Both functions perform the same
GET .../contents/{path}?ref=...call, 404 handling, and "payload is a list → treat as missing" check;_fetch_file_on_refadditionally decodes content. Consider having_get_file_shaderive its result from_fetch_file_on_ref(or vice versa) to avoid divergence.Also applies to: 157-173
🤖 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 `@osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_github.py` around lines 103 - 112, Duplicate the shared GitHub contents-fetch logic so `_get_file_sha` and `_fetch_file_on_ref` do not each repeat the same GET, 404 handling, and “list payload means missing” checks. Refactor one helper to be the single source of truth for fetching `/contents/{path}?ref=...`, then have `_get_file_sha` derive the SHA from that helper’s result (or have `_fetch_file_on_ref` build on `_get_file_sha`) while keeping the existing missing-file behavior intact.
🤖 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 `@osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_github.py`:
- Around line 270-311: `list_pending_drafts` is doing sequential per-PR file
lookups and only reads the first page of open PRs, which can make the endpoint
slow and silently miss drafts. Update `list_pending_drafts` in
`_rule_drafts_github.py` to either paginate through all open pulls explicitly or
enforce and document a clear cap, and reduce the N+1 latency by fetching each
PR’s `/files` data in parallel or otherwise batching the work while keeping the
existing `PendingDraft` construction logic intact.
In `@osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_local.py`:
- Around line 67-106: The filesystem operations in submit_draft are not
protected by the backend error contract, so wrap the mkdir/write/read/write path
for target and main_path in RuleDraftBackendError handling. In
_rule_drafts_local.py, update submit_draft to catch OSError (and any related I/O
failures) around target.parent.mkdir, target.write_text, main_path.read_text,
and main_path.write_text, then rethrow as RuleDraftBackendError with a clear
message and appropriate status_code so callers only see the documented error
type.
- Around line 78-98: The local draft save flow in _rule_drafts_local.py is
vulnerable to races because create/check/write on target and the
read-modify-write of main.sml can interleave across concurrent requests. Update
the write path in the draft save method to use an exclusive create or atomic
write for new drafts instead of target.exists() followed by write_text(), and
protect the main.sml update in the wire_into_main branch with a lock or
compare-and-swap style recheck before appending the Require line. Make the fix
around the existing self._resolve, target.write_text, and
git_common.append_require_to_main logic so concurrent submissions cannot
overwrite each other.
In `@osprey_worker/src/osprey/worker/ui_api/osprey/views/rule_drafts.py`:
- Around line 49-54: In rule_drafts.py, the identifier extraction around
msg.span.ast_node currently swallows all failures with a bare except Exception:
pass, which hides unexpected errors. Update the exception handling in this block
to catch only the expected error type if possible, or log the exception at debug
level with enough context before continuing so failures in extracting the
Name.identifier remain diagnosable.
- Around line 357-358: In the catch-all exception handler in rule_drafts.py,
stop returning the raw exception text from the assemble sources path. Update the
except Exception as exc branch to send a generic user-facing error response from
the rule drafts flow, and log the detailed exception server-side using the
existing logger or error handling pattern instead of interpolating {exc} into
jsonify. Keep the ValidationFailed branch unchanged and make the generic
response consistent with the other public API errors in Osprey.
---
Nitpick comments:
In `@osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_github.py`:
- Around line 103-112: Duplicate the shared GitHub contents-fetch logic so
`_get_file_sha` and `_fetch_file_on_ref` do not each repeat the same GET, 404
handling, and “list payload means missing” checks. Refactor one helper to be the
single source of truth for fetching `/contents/{path}?ref=...`, then have
`_get_file_sha` derive the SHA from that helper’s result (or have
`_fetch_file_on_ref` build on `_get_file_sha`) while keeping the existing
missing-file behavior intact.
In
`@osprey_worker/src/osprey/worker/ui_api/osprey/views/tests/test_rule_drafts.py`:
- Around line 386-427: Several rule draft tests repeat the same GitHub
`requests_mock` setup for `git/ref/heads/main`, `contents/...`, and `pulls`, so
factor that scaffolding into a shared helper to reduce duplication. Add a small
test utility (for example, a mock factory used by
`test_submit_happy_path_creates_branch_commits_and_opens_pr` and the other
affected `test_rule_drafts` cases) that registers the common endpoints and
accepts the varying path/status/response details, then update each test to call
it instead of inlining the repeated mocks.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9598989a-1aad-4814-aad2-7df9ec0eef35
📒 Files selected for processing (13)
.gitignoredocs/user/manage.mdosprey_worker/src/osprey/worker/lib/acls/definitions/super_user.jsonosprey_worker/src/osprey/worker/lib/osprey_engine.pyosprey_worker/src/osprey/worker/ui_api/osprey/app.pyosprey_worker/src/osprey/worker/ui_api/osprey/lib/abilities.pyosprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_backend.pyosprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_git_common.pyosprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_github.pyosprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_local.pyosprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_null.pyosprey_worker/src/osprey/worker/ui_api/osprey/views/rule_drafts.pyosprey_worker/src/osprey/worker/ui_api/osprey/views/tests/test_rule_drafts.py
| def list_pending_drafts(self) -> list[PendingDraft]: | ||
| cfg = self._cfg | ||
| res = _get( | ||
| cfg, | ||
| f'{cfg.repo_url}/pulls', | ||
| params={'state': 'open', 'base': cfg.base_branch, 'per_page': 30}, | ||
| ) | ||
| _raise_for_github(res, 'listing open pull requests') | ||
| open_prs = res.json() | ||
|
|
||
| out: list[PendingDraft] = [] | ||
| for pr in open_prs: | ||
| number = pr.get('number') | ||
| if number is None: | ||
| continue | ||
| files_res = _get(cfg, f'{cfg.repo_url}/pulls/{number}/files', params={'per_page': 50}) | ||
| if not files_res.ok: | ||
| continue | ||
| files = files_res.json() | ||
| touched = [ | ||
| f['filename'] | ||
| for f in files | ||
| if isinstance(f.get('filename'), str) | ||
| and (not cfg.rules_path or f['filename'].startswith(cfg.rules_path + '/')) | ||
| and f['filename'].endswith('.sml') | ||
| ] | ||
| if not touched: | ||
| continue | ||
| out.append( | ||
| PendingDraft( | ||
| title=pr.get('title', ''), | ||
| url=pr.get('html_url', ''), | ||
| author=(pr.get('user') or {}).get('login', ''), | ||
| created_at=pr.get('created_at', ''), | ||
| touched_files=touched, | ||
| extras={ | ||
| 'pr_number': number, | ||
| 'branch': (pr.get('head') or {}).get('ref', ''), | ||
| }, | ||
| ) | ||
| ) | ||
| return out |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
N+1 HTTP calls and unbounded pagination in list_pending_drafts.
For every open PR returned (up to 30, since per_page=30 and no pagination loop), a separate GET .../pulls/{number}/files call is issued sequentially. With the 15s default timeout per call, a slow or degraded GitHub/Enterprise instance can make this endpoint take tens of seconds on a single Flask request thread. Additionally, any PRs beyond the first page of 30 are silently dropped from the pending list with no indication to the caller.
Consider capping/paginating explicitly and documenting the limit, and/or fetching PR file lists in parallel (e.g., via a thread pool) to bound latency.
🤖 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 `@osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_github.py`
around lines 270 - 311, `list_pending_drafts` is doing sequential per-PR file
lookups and only reads the first page of open PRs, which can make the endpoint
slow and silently miss drafts. Update `list_pending_drafts` in
`_rule_drafts_github.py` to either paginate through all open pulls explicitly or
enforce and document a clear cap, and reduce the N+1 latency by fetching each
PR’s `/files` data in parallel or otherwise batching the work while keeping the
existing `PendingDraft` construction logic intact.
| def submit_draft( | ||
| self, | ||
| *, | ||
| draft_path: str, | ||
| sml_source: str, | ||
| rule_name: str, | ||
| summary: str, | ||
| author_email: str, | ||
| is_new_rule: bool, | ||
| wire_into_main: bool, | ||
| ) -> SubmissionResult: | ||
| target = self._resolve(draft_path) | ||
| if is_new_rule and target.exists(): | ||
| raise RuleDraftBackendError( | ||
| f'A file already exists at {draft_path!r}. Pick a different filename or edit the existing rule.', | ||
| status_code=409, | ||
| ) | ||
| target.parent.mkdir(parents=True, exist_ok=True) | ||
| target.write_text(sml_source, encoding='utf-8') | ||
|
|
||
| main_sml_updated = False | ||
| if wire_into_main: | ||
| main_path = self._cfg.rules_dir / 'main.sml' | ||
| if not main_path.exists(): | ||
| raise RuleDraftBackendError( | ||
| f'wire_into_main requested but main.sml does not exist at {main_path}.', | ||
| status_code=409, | ||
| ) | ||
| main_contents = main_path.read_text(encoding='utf-8') | ||
| if not git_common.require_already_present(main_contents, draft_path): | ||
| main_path.write_text(git_common.append_require_to_main(main_contents, draft_path), encoding='utf-8') | ||
| main_sml_updated = True | ||
|
|
||
| verb = 'Created' if is_new_rule else 'Updated' | ||
| return SubmissionResult( | ||
| title=f'{verb} {draft_path} in local rules directory', | ||
| url=None, | ||
| main_sml_updated=main_sml_updated, | ||
| extras={'path_on_disk': str(target)}, | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Filesystem errors aren't wrapped in RuleDraftBackendError.
target.parent.mkdir(...), target.write_text(...), main_path.read_text(...), and main_path.write_text(...) can raise OSError (permission denied, disk full, read-only mount) that isn't caught here. This breaks the backend's own documented contract ("Implementations raise RuleDraftBackendError for any failure path") and, per path instructions, risks leaking an internal exception/stack trace to the client if the view doesn't have a blanket handler for non-RuleDraftBackendError exceptions.
🛡️ Proposed fix
- target.parent.mkdir(parents=True, exist_ok=True)
- target.write_text(sml_source, encoding='utf-8')
+ try:
+ target.parent.mkdir(parents=True, exist_ok=True)
+ target.write_text(sml_source, encoding='utf-8')
+ except OSError as exc:
+ raise RuleDraftBackendError(f'Could not write {draft_path!r}: {exc}', status_code=502) from exc
main_sml_updated = False
if wire_into_main:
main_path = self._cfg.rules_dir / 'main.sml'
if not main_path.exists():
raise RuleDraftBackendError(
f'wire_into_main requested but main.sml does not exist at {main_path}.',
status_code=409,
)
- main_contents = main_path.read_text(encoding='utf-8')
- if not git_common.require_already_present(main_contents, draft_path):
- main_path.write_text(git_common.append_require_to_main(main_contents, draft_path), encoding='utf-8')
- main_sml_updated = True
+ try:
+ main_contents = main_path.read_text(encoding='utf-8')
+ if not git_common.require_already_present(main_contents, draft_path):
+ main_path.write_text(
+ git_common.append_require_to_main(main_contents, draft_path), encoding='utf-8'
+ )
+ main_sml_updated = True
+ except OSError as exc:
+ raise RuleDraftBackendError(f'Could not update main.sml: {exc}', status_code=502) from exc📝 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 submit_draft( | |
| self, | |
| *, | |
| draft_path: str, | |
| sml_source: str, | |
| rule_name: str, | |
| summary: str, | |
| author_email: str, | |
| is_new_rule: bool, | |
| wire_into_main: bool, | |
| ) -> SubmissionResult: | |
| target = self._resolve(draft_path) | |
| if is_new_rule and target.exists(): | |
| raise RuleDraftBackendError( | |
| f'A file already exists at {draft_path!r}. Pick a different filename or edit the existing rule.', | |
| status_code=409, | |
| ) | |
| target.parent.mkdir(parents=True, exist_ok=True) | |
| target.write_text(sml_source, encoding='utf-8') | |
| main_sml_updated = False | |
| if wire_into_main: | |
| main_path = self._cfg.rules_dir / 'main.sml' | |
| if not main_path.exists(): | |
| raise RuleDraftBackendError( | |
| f'wire_into_main requested but main.sml does not exist at {main_path}.', | |
| status_code=409, | |
| ) | |
| main_contents = main_path.read_text(encoding='utf-8') | |
| if not git_common.require_already_present(main_contents, draft_path): | |
| main_path.write_text(git_common.append_require_to_main(main_contents, draft_path), encoding='utf-8') | |
| main_sml_updated = True | |
| verb = 'Created' if is_new_rule else 'Updated' | |
| return SubmissionResult( | |
| title=f'{verb} {draft_path} in local rules directory', | |
| url=None, | |
| main_sml_updated=main_sml_updated, | |
| extras={'path_on_disk': str(target)}, | |
| ) | |
| def submit_draft( | |
| self, | |
| *, | |
| draft_path: str, | |
| sml_source: str, | |
| rule_name: str, | |
| summary: str, | |
| author_email: str, | |
| is_new_rule: bool, | |
| wire_into_main: bool, | |
| ) -> SubmissionResult: | |
| target = self._resolve(draft_path) | |
| if is_new_rule and target.exists(): | |
| raise RuleDraftBackendError( | |
| f'A file already exists at {draft_path!r}. Pick a different filename or edit the existing rule.', | |
| status_code=409, | |
| ) | |
| try: | |
| target.parent.mkdir(parents=True, exist_ok=True) | |
| target.write_text(sml_source, encoding='utf-8') | |
| except OSError as exc: | |
| raise RuleDraftBackendError(f'Could not write {draft_path!r}: {exc}', status_code=502) from exc | |
| main_sml_updated = False | |
| if wire_into_main: | |
| main_path = self._cfg.rules_dir / 'main.sml' | |
| if not main_path.exists(): | |
| raise RuleDraftBackendError( | |
| f'wire_into_main requested but main.sml does not exist at {main_path}.', | |
| status_code=409, | |
| ) | |
| try: | |
| main_contents = main_path.read_text(encoding='utf-8') | |
| if not git_common.require_already_present(main_contents, draft_path): | |
| main_path.write_text( | |
| git_common.append_require_to_main(main_contents, draft_path), encoding='utf-8' | |
| ) | |
| main_sml_updated = True | |
| except OSError as exc: | |
| raise RuleDraftBackendError(f'Could not update main.sml: {exc}', status_code=502) from exc | |
| verb = 'Created' if is_new_rule else 'Updated' | |
| return SubmissionResult( | |
| title=f'{verb} {draft_path} in local rules directory', | |
| url=None, | |
| main_sml_updated=main_sml_updated, | |
| extras={'path_on_disk': str(target)}, | |
| ) |
🤖 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 `@osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_local.py`
around lines 67 - 106, The filesystem operations in submit_draft are not
protected by the backend error contract, so wrap the mkdir/write/read/write path
for target and main_path in RuleDraftBackendError handling. In
_rule_drafts_local.py, update submit_draft to catch OSError (and any related I/O
failures) around target.parent.mkdir, target.write_text, main_path.read_text,
and main_path.write_text, then rethrow as RuleDraftBackendError with a clear
message and appropriate status_code so callers only see the documented error
type.
Source: Path instructions
| target = self._resolve(draft_path) | ||
| if is_new_rule and target.exists(): | ||
| raise RuleDraftBackendError( | ||
| f'A file already exists at {draft_path!r}. Pick a different filename or edit the existing rule.', | ||
| status_code=409, | ||
| ) | ||
| target.parent.mkdir(parents=True, exist_ok=True) | ||
| target.write_text(sml_source, encoding='utf-8') | ||
|
|
||
| main_sml_updated = False | ||
| if wire_into_main: | ||
| main_path = self._cfg.rules_dir / 'main.sml' | ||
| if not main_path.exists(): | ||
| raise RuleDraftBackendError( | ||
| f'wire_into_main requested but main.sml does not exist at {main_path}.', | ||
| status_code=409, | ||
| ) | ||
| main_contents = main_path.read_text(encoding='utf-8') | ||
| if not git_common.require_already_present(main_contents, draft_path): | ||
| main_path.write_text(git_common.append_require_to_main(main_contents, draft_path), encoding='utf-8') | ||
| main_sml_updated = True |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
printf '\n== file outline ==\n'
ast-grep outline osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_local.py --view expanded || true
printf '\n== relevant slice ==\n'
sed -n '1,220p' osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_local.py | cat -n
printf '\n== search for locking / atomic helpers / related backend ==\n'
rg -n "flock|filelock|lockfile|atomic|require_already_present|append_require_to_main|write_text\\(|open\\(mode='x'|mode=\"x\"|main.sml|RuleDraftBackendError" osprey_worker/src/osprey/worker -SRepository: roostorg/osprey
Length of output: 25030
🏁 Script executed:
set -euo pipefail
printf '\n== adjacent files ==\n'
fd -a "_rule_drafts_.*py$|rule_drafts|github.*draft" osprey_worker/src/osprey/worker/ui_api/osprey/views -t f
printf '\n== inspect GitHub backend for comparison ==\n'
sed -n '1,260p' osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_github.py | cat -n
printf '\n== inspect git_common helpers ==\n'
python3 - <<'PY'
from pathlib import Path
p = Path("osprey_worker/src/osprey/worker/ui_api/osprey/views")
for name in ["git_common.py", "_rule_drafts_local.py", "_rule_drafts_github.py"]:
path = p / name
if path.exists():
print(f"\n--- {path} ({sum(1 for _ in path.open())} lines) ---")
PY
sed -n '1,260p' osprey_worker/src/osprey/worker/ui_api/osprey/views/git_common.py | cat -nRepository: roostorg/osprey
Length of output: 12042
🏁 Script executed:
echo 'waiting for results'Repository: roostorg/osprey
Length of output: 173
🏁 Script executed:
set -euo pipefail
# Show the exact file and any concurrency-related patterns nearby.
sed -n '1,220p' osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_local.py | nl -ba | sed -n '1,220p'
printf '\n---\n'
# Search for explicit serialization or locking in the same feature area.
rg -n "flock|filelock|Lock\\(|RLock\\(|Semaphore\\(|asyncio\\.Lock|append_require_to_main|require_already_present|open\\(.*mode=['\"]x['\"]|write_text\\(" osprey_worker/src/osprey/worker/ui_api/osprey/views -SRepository: roostorg/osprey
Length of output: 193
Guard the local write path against concurrent submissions. target.exists()/write_text() and the main.sml read-modify-write can interleave, so two requests can both pass the checks and one can overwrite the other's Require(...) line. Use exclusive-create for new drafts and a lock or CAS-style update for main.sml.
🤖 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 `@osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_local.py`
around lines 78 - 98, The local draft save flow in _rule_drafts_local.py is
vulnerable to races because create/check/write on target and the
read-modify-write of main.sml can interleave across concurrent requests. Update
the write path in the draft save method to use an exclusive create or atomic
write for new drafts instead of target.exists() followed by write_text(), and
protect the main.sml update in the wire_into_main branch with a lock or
compare-and-swap style recheck before appending the Require line. Make the fix
around the existing self._resolve, target.write_text, and
git_common.append_require_to_main logic so concurrent submissions cannot
overwrite each other.
Feedback on roostorg#402 was to drop the github/gitlab/etc submission backends and instead keep drafts in Osprey itself. This does that: rule drafts now live in a rule_drafts Postgres table that the people who operate Osprey can reference, edit, and deploy from, with no external code host. - Add the RuleDraft model (one row per rule path, upserted on save) and register it so the table is created. - Rework the view around the table: create/list/get plus a deploy endpoint that re-validates, writes the SML into OSPREY_RULES_LOCAL_PATH, and optionally wires a Require line into main.sml. Keep the backend-agnostic authoring endpoints (source, validate, vocabulary, parse-into-builder). - Remove the five _rule_drafts_* backend modules and the OSPREY_RULES_SUBMISSION_BACKEND config surface. - Rewrite the tests for the table workflow; update docs and CHANGELOG. A DB-backed SourcesProvider that loads deployed drafts straight from the table (removing the filesystem hand-off) is noted as future direction. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TEcfpqhza3dPepZUVWw36X
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
osprey_worker/src/osprey/worker/ui_api/osprey/views/rule_drafts.py (1)
130-132: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winValidate decoded JSON before accessing or coercing fields.
request.get_json()may return a list or scalar, causing.get()to raise a 500. Additionally,bool("false")enableswire_into_main. Parse these bodies with Pydantic models and return 400 for invalid shapes or field types.As per coding guidelines: "Use Pydantic for data models throughout the Python codebase." As per path instructions: "Validate request bodies and query params (Pydantic preferred)."
Also applies to: 340-344, 454-455, 693-695
🤖 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 `@osprey_worker/src/osprey/worker/ui_api/osprey/views/rule_drafts.py` around lines 130 - 132, Validate decoded request JSON with Pydantic models before accessing fields in the request-body handling paths around payload/path/source_text and the additional affected sections. Reject non-object bodies and invalid field types with HTTP 400, and use strict boolean parsing so values such as "false" do not enable wire_into_main.Sources: Coding guidelines, Path instructions
🧹 Nitpick comments (1)
osprey_worker/src/osprey/worker/ui_api/osprey/views/tests/test_rule_drafts.py (1)
113-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover
get_draftin the authorization test.This is the only new lifecycle endpoint omitted here. Add an unauthenticated
GET rule_drafts.get_draftassertion to protect the ID-based view from ACL regressions.As per path instructions: "Verify each view enforces authentication."
🤖 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 `@osprey_worker/src/osprey/worker/ui_api/osprey/views/tests/test_rule_drafts.py` around lines 113 - 118, Add an unauthenticated GET assertion for the rule_drafts.get_draft view in the existing authorization test, supplying a draft_id such as 1 and verifying it returns 401 alongside list_drafts, create_draft, and deploy_draft.Source: Path instructions
🤖 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/user/manage.md`:
- Line 81: Align the OSPREY_RULES_LOCAL_PATH documentation with the validation
performed by _rules_dir_or_error: either enforce rules_dir.is_absolute() in that
function, or remove “Absolute” from the documented path description. Keep the
documentation and runtime contract consistent.
In `@osprey_worker/src/osprey/worker/lib/storage/rule_drafts.py`:
- Around line 70-80: Update the path upsert logic in the class method containing
the session query so concurrent creates are handled atomically: use PostgreSQL
ON CONFLICT DO UPDATE, or catch a unique-constraint flush failure, roll back,
and retry the update. Preserve all existing field assignments and ensure the
upsert does not propagate a concurrency conflict as a 500.
In `@osprey_worker/src/osprey/worker/ui_api/osprey/views/rule_drafts.py`:
- Around line 470-480: Move the wire_into_main preflight in the rule-draft
creation flow before writing draft.sml: when wire_into_main is true, validate
that main.sml exists and obtain its contents before target.write_text is
reached. Preserve the existing 409 response for a missing main.sml, then perform
the draft and main.sml writes only after all checks pass, using the pre-read
contents for the require update.
---
Outside diff comments:
In `@osprey_worker/src/osprey/worker/ui_api/osprey/views/rule_drafts.py`:
- Around line 130-132: Validate decoded request JSON with Pydantic models before
accessing fields in the request-body handling paths around
payload/path/source_text and the additional affected sections. Reject non-object
bodies and invalid field types with HTTP 400, and use strict boolean parsing so
values such as "false" do not enable wire_into_main.
---
Nitpick comments:
In
`@osprey_worker/src/osprey/worker/ui_api/osprey/views/tests/test_rule_drafts.py`:
- Around line 113-118: Add an unauthenticated GET assertion for the
rule_drafts.get_draft view in the existing authorization test, supplying a
draft_id such as 1 and verifying it returns 401 alongside list_drafts,
create_draft, and deploy_draft.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 39b3a969-0eef-46b6-b51c-3446eeac5ed3
📒 Files selected for processing (6)
CHANGELOG.mddocs/user/manage.mdosprey_worker/src/osprey/worker/lib/storage/postgres.pyosprey_worker/src/osprey/worker/lib/storage/rule_drafts.pyosprey_worker/src/osprey/worker/ui_api/osprey/views/rule_drafts.pyosprey_worker/src/osprey/worker/ui_api/osprey/views/tests/test_rule_drafts.py
|
Huge thanks to @ThisIsMissEm Your feedback made the design a lot simpler. I've reworked the PR to drop the git submission backends entirely. Draft rules now live in a I also think there's a follow-up to be noted: a DB-backed |
julietshen
changed the title
The backend pivoted from git submission backends to a rule_drafts table (roostorg#402), so the editor's submit/pending calls no longer exist. Rewire the UI to the new endpoints: - submit (POST rule-drafts/submit) -> create (POST rule-drafts) - pending (GET rule-drafts/pending) -> list (GET rule-drafts) - add a Deploy action (POST rule-drafts/<id>/deploy) carrying wire_into_main The editor is now save-draft then deploy rather than open-a-PR, and the Rules page lists in-progress drafts (status tag, link to edit) instead of pending-review pull requests. The source/validate/vocabulary/parse endpoints were unchanged, so those calls stay. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TEcfpqhza3dPepZUVWw36X
SML rule names are global identifiers, so two drafts sharing a name would collide once both deploy. Server-side validation only sees deployed rules, not other rows in the rule_drafts table, so it can't catch the draft-vs-draft case. Add RuleDraft.other_with_rule_name() and have create_draft return 409 when a different path already uses the name (re-saving the same path is still an in-place update, not a conflict). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TEcfpqhza3dPepZUVWw36X
ThisIsMissEm
left a comment
ThisIsMissEm
left a comment
There was a problem hiding this comment.
Have left a bunch of comments, it's definitely looking better though.
You could probably expose a config endpoint that says whether or not publishing rules is enabled, and then gate that UI feature based on that.
There's also the path sanitisation and validation that should be the same across all codepaths that perform that sort of logic.
| id: int = Column(BigInteger, primary_key=True, autoincrement=True) | ||
| path: str = Column(Text, nullable=False, unique=True) | ||
| rule_name: str = Column(Text, nullable=False) | ||
| sml_source: str = Column(Text, nullable=False) |
There was a problem hiding this comment.
You may also want to add a "cid" column here, or "content id" which the the normalised sml_source hashed, and then embedded that into the rules output written to disk so you can actually compare "rule as deploy" vs "rule as draft" (that way a deployed rule doesn't need to be deleted). Though this probably touches more on having a Rules table with a rule identifier/name, and then multiple Rule Versions which are content hashed, and the rules table points to the currently live version (allowing for time-travel to see how a rule evolved over time)
| rule_name: str = Column(Text, nullable=False) | ||
| sml_source: str = Column(Text, nullable=False) | ||
| summary: str = Column(Text, nullable=False, default='') | ||
| author_email: str = Column(Text, nullable=False) |
There was a problem hiding this comment.
Might be worth a comment that osprey doesn't actually have a Users table somewhere, instead it's just that you have an email with ACLs applied.
| for draft in drafts: | ||
| session.expunge(draft) |
There was a problem hiding this comment.
I think you could probably just do session.expunge_all()
There was a problem hiding this comment.
that's based on reading https://docs.sqlalchemy.org/en/21/orm/session_api.html#sqlalchemy.orm.Session.expunge
|
|
||
| blueprint = Blueprint('rule_drafts', __name__) | ||
|
|
||
| _VALID_PATH = re.compile(r'^[A-Za-z0-9_./-]+\.sml$') |
There was a problem hiding this comment.
I'd probably remove the . from the regexp's range-match, so it's just A-Za-z0-9_-/ as the range of characters, though the / will give you weirdness on windows if someone enjoys deployment pain that much.
There was a problem hiding this comment.
Also validate that the path doesn't begin with / and then you can scope it to the configured rules directory. Another way to do this without regex, something like:
from pathlib import Path
base = Path(base_dir).resolve()
target = (base / user_input).resolve()
if not target.is_relative_to(base):
raise ValueError("escapes base")| from osprey.engine.ast.grammar import ( | ||
| List as AstList, | ||
| ) | ||
| from osprey.engine.ast.sources import Sources |
There was a problem hiding this comment.
you could probably introduce a osprey.engine.ast.builder import Builder which contains all the builder related logic from this file.
| @blueprint.route('/rule-drafts', methods=['GET']) | ||
| @require_ability(CanEditRuleDrafts) | ||
| def list_drafts() -> Any: | ||
| """The draft rules table: every staged draft, newest-edited first.""" | ||
| return jsonify({'drafts': [d.to_json() for d in RuleDraft.list_all()]}) |
There was a problem hiding this comment.
would it be worth having a GET /rules which is all the rules currently persisted to disk?
| deployed = RuleDraft.mark_deployed(draft_id) | ||
| result = deployed.to_json() if deployed is not None else draft.to_json() | ||
| result['main_sml_updated'] = main_sml_updated | ||
| result['path_on_disk'] = str(target) |
There was a problem hiding this comment.
this could leak sensitive config to the end-user, potentially consider removing the rules_dir prefix from target
| return engine.execution_graph.validated_sources.sources.to_dict() | ||
|
|
||
|
|
||
| @blueprint.route('/rule-drafts/source', methods=['GET']) |
There was a problem hiding this comment.
I don't think this is loading a rule draft source specifically, but any deployed rule's source?
There was a problem hiding this comment.
Yep, that's on purpose. This endpoint hands back the rule that's already on disk so you can edit an existing rule. A draft's own text comes from the table instead, through GET /rule-drafts/. I'll make the docstring say that clearly.
There was a problem hiding this comment.
Yeah, perhaps the URLs should be /rules/:cid/source or something for getting an existing on disk rule versus /rules/drafts/:id/source
| ) | ||
| created_at: datetime = Column(DateTime(timezone=True), nullable=False, default=_now) | ||
| updated_at: datetime = Column(DateTime(timezone=True), nullable=False, default=_now, onupdate=_now) | ||
| deployed_at: datetime | None = Column(DateTime(timezone=True), nullable=True) # type: ignore[misc] |
There was a problem hiding this comment.
What happens if the rule draft is deployed but then deleted from disk by a system admin?
There was a problem hiding this comment.
oh good catch. Right now the row would still say "deployed" even though the file is gone. I'm treating that as a known gap for now. The content-hash idea you mentioned below is probably the proper fix, since it'd let us compare what's in the table against what's actually on disk
Feedback from CodeRabbit and @ThisIsMissEm on the rule-drafts backend: - Don't leak raw exception text to clients; log server-side and return a generic message when sources can't be assembled (both /validate and the create/deploy re-validation). - Share one `_validate_draft_source` helper between /validate and the server-side re-validation so the two can't drift. - Catch the specific RuntimeError/AttributeError from `span.ast_node` instead of a bare `except: pass` when extracting the identifier. - Deploy: verify main.sml exists before writing the rule file, so a missing main.sml no longer leaves the file written while the request 409s. - Deploy: report `path_on_disk` relative to the rules directory rather than leaking the absolute server path. - Make the path upsert atomic with INSERT ... ON CONFLICT DO UPDATE so two concurrent saves of the same path can't race the unique constraint. - Reject absolute paths in `_validate_path`; drop the stray `.` from the path character class; use `expunge_all()`; note that Osprey has no users table (identity is an email + ACLs). - Tests: clear the rule_drafts table between tests (the DB is session-scoped) and assert the deploy 409 leaves no file behind. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TEcfpqhza3dPepZUVWw36X
…s-api # Conflicts: # CHANGELOG.md
@ThisIsMissEm noted the /rule-drafts/source endpoint reads any deployed rule's source, not a draft's. Reword the 404 to "No rule found at ..." (it doesn't consult the drafts table, so "draft" would mislead) and add a docstring saying it's for editing an existing on-disk rule, while a draft's own SML comes from GET /rule-drafts/<id>. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TEcfpqhza3dPepZUVWw36X
chimosky
left a comment
chimosky
left a comment
There was a problem hiding this comment.
I'd also like to suggest being able to export/import rules, this way SCM wouldn't be the only way to share rules.
| """ | ||
| suggested: set[str] = set() | ||
| for err in errors: | ||
| for path in err.get('defined_in_source_paths') or []: |
There was a problem hiding this comment.
The pythonic thing here would be to use [ ] as a default.
|
|
||
| def _validate_path(path: str) -> str | None: | ||
| if not _VALID_PATH.match(path): | ||
| return f'Path {path!r} is not a valid SML source path (must end .sml and contain only [A-Za-z0-9_/-]).' |
There was a problem hiding this comment.
A better error would be f'Path {path!r} is not a valid SML source path (must end .sml and contain only alphanumeric characters and the special characters "/-").' might not be this exact term, errors aren't supposed to cryptic.
| SourcesProvider that lets the engine read deployed drafts straight from this | ||
| table would remove that dependency entirely; see the PR notes. | ||
| """ | ||
| raw = os.environ.get('OSPREY_RULES_LOCAL_PATH', '').strip() |
There was a problem hiding this comment.
For consistency, it'll be great to get this from config.
Feedback from @chimosky on the rule-drafts view: - Read OSPREY_RULES_LOCAL_PATH via CONFIG.get_str() instead of os.environ, for consistency with how the rest of the UI API reads configuration. The deploy tests set it directly on the bound config since CONFIG binds once at app setup. - Make the invalid-path error human-readable ("letters, numbers, underscores, slashes, and hyphens") instead of echoing the raw character class. - Use `[]` as the default in _suggest_imports_from_errors rather than `or []`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TEcfpqhza3dPepZUVWw36X
|
Thank you @ThisIsMissEm and @chimosky for the reviews! I've addressed all your feedback in my latest commits. |
The backend pivoted from git submission backends to a rule_drafts table (roostorg#402), so the editor's submit/pending calls no longer exist. Rewire the UI to the new endpoints: - submit (POST rule-drafts/submit) -> create (POST rule-drafts) - pending (GET rule-drafts/pending) -> list (GET rule-drafts) - add a Deploy action (POST rule-drafts/<id>/deploy) carrying wire_into_main The editor is now save-draft then deploy rather than open-a-PR, and the Rules page lists in-progress drafts (status tag, link to edit) instead of pending-review pull requests. The source/validate/vocabulary/parse endpoints were unchanged, so those calls stay. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TEcfpqhza3dPepZUVWw36X
4 participants
What
Adds experimental in-app rule authoring to the UI API. Users draft SML
rules, the drafts validate against the live engine, and they save to a
rule_draftsPostgres table that the people who operate Osprey canreference, edit, and deploy from. No external code host is involved.
Why this changed from the original PR
The first version of this PR routed drafts through pluggable git
submission backends (GitHub, local, null) so a "Submit" button opened a
pull request. Review feedback was to avoid github/gitlab/etc entirely and
keep drafts inside Osprey. This revision does that: the git backends are
gone and the draft table is the source of truth.
How it works
lib/storage/rule_drafts.py):RuleDraftmodel, onerow per rule path, upserted on save. Editing a deployed draft moves it
back to
draftso the table reflects that the in-flight SML no longermatches what was deployed.
CAN_EDIT_RULE_DRAFTSability):POST /rule-drafts— re-validate server-side, then upsert.GET /rule-drafts— list every draft.GET /rule-drafts/<id>— fetch one.POST /rule-drafts/<id>/deploy— re-validate, write the SML into theconfigured rules directory (
OSPREY_RULES_LOCAL_PATH), and mark thedraft deployed.
wire_into_main: truealso appends aRequire(rule=...)line tomain.smlso the rule takes effect.source,validate,vocabulary,parse-into-builder.Deploy model
Deploy writes into a rules directory the engine's sources provider already
loads, a filesystem hand-off. A DB-backed
SourcesProviderthat loadsdeployed drafts straight from the table (removing the filesystem
dependency) is noted in the docs as future direction; this PR keeps the
filesystem deploy and makes the table the source of truth for drafts.
Tests
test_rule_drafts.pycovers the authoring endpoints plus the tableworkflow: create / upsert / list / get, and deploy including
wire-into-main, idempotency, and the 409 / 503 / 404 branches. Docs
(
docs/user/manage.md) and the CHANGELOG are updated.🤖 Generated with Claude Code
Summary by CodeRabbit
main.sml.OSPREY_RULES_LOCAL_PATHbehavior (including 503 when unset).