From 5333ad8aac0ad41c2034c4b13c179989cb7c89d4 Mon Sep 17 00:00:00 2001 From: Juliet Shen Date: Thu, 2 Jul 2026 18:02:33 -0400 Subject: [PATCH 1/9] Add rule-drafts API with pluggable submission backends (GitHub, local) 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 Claude-Session: https://claude.ai/code/session_01VZ4RQtuHCCgurfpjfPXXAM --- .gitignore | 3 + docs/user/manage.md | 39 + .../lib/acls/definitions/super_user.json | 4 + .../src/osprey/worker/lib/osprey_engine.py | 8 + .../src/osprey/worker/ui_api/osprey/app.py | 2 + .../worker/ui_api/osprey/lib/abilities.py | 1 + .../osprey/views/_rule_drafts_backend.py | 142 +++ .../osprey/views/_rule_drafts_git_common.py | 62 ++ .../osprey/views/_rule_drafts_github.py | 311 ++++++ .../ui_api/osprey/views/_rule_drafts_local.py | 110 +++ .../ui_api/osprey/views/_rule_drafts_null.py | 41 + .../worker/ui_api/osprey/views/rule_drafts.py | 602 ++++++++++++ .../osprey/views/tests/test_rule_drafts.py | 900 ++++++++++++++++++ 13 files changed, 2225 insertions(+) create mode 100644 osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_backend.py create mode 100644 osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_git_common.py create mode 100644 osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_github.py create mode 100644 osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_local.py create mode 100644 osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_null.py create mode 100644 osprey_worker/src/osprey/worker/ui_api/osprey/views/rule_drafts.py create mode 100644 osprey_worker/src/osprey/worker/ui_api/osprey/views/tests/test_rule_drafts.py diff --git a/.gitignore b/.gitignore index afa35534..fdea2ca3 100644 --- a/.gitignore +++ b/.gitignore @@ -307,5 +307,8 @@ Cargo.lock .claude +# Local docker compose overrides (env vars, port remaps, secrets) +docker-compose.override.yaml + # docs output book/ diff --git a/docs/user/manage.md b/docs/user/manage.md index 36ed4225..d5ef0642 100644 --- a/docs/user/manage.md +++ b/docs/user/manage.md @@ -56,3 +56,42 @@ The list is paginated (50 per page) and can be filtered and sorted: - **Sort**: by name, most referenced, or least referenced Each row shows the rule's name, source file, description, reference count, and line number within the source file. + +## Rule Authoring (Experimental feature) + +Users can draft SML rules directly in the UI. Submit opens a review unit against a configured git remote so authoring, review, and merge use the same tools users already have. + +The editor validates every keystroke against the same AST validator the running engine uses, so compile-time errors surface before the pull request opens. The Rule Builder view expresses the common shape (name, conditions, outcomes) as a form and generates SML; the Code Editor view accepts arbitrary SML for anything the builder can't represent. + +### Rule submission backends + +The Submit button routes drafts through a pluggable backend. Pick one for your deployment by setting `OSPREY_RULES_SUBMISSION_BACKEND` on the `osprey-ui-api` process: + +| Value | What it does | Required env vars | +|---|---|---| +| `null` (default) | Returns 503 on any submit or list call. Ships as the default so an unconfigured install never writes anything. | none | +| `github` | Opens a pull request on a configured repo. Works with github.com and GitHub Enterprise. | `OSPREY_RULES_REPO`, `OSPREY_GITHUB_TOKEN` (+ optionals) | +| `local` | Writes SML directly to a mounted directory. For self-hosted setups whose deploy pipeline already syncs a rules directory into the engine. | `OSPREY_RULES_LOCAL_PATH` | + +Env vars shared across every backend that targets a git host: + +- `OSPREY_RULES_BASE_BRANCH` (default `main`) — the branch the review targets. +- `OSPREY_RULES_PATH_IN_REPO` (default empty) — subdirectory inside the target repo where rule files live, e.g. `example_rules`. Leave empty if rules sit at the repo root. + +#### `github` + +| Var | Default | Notes | +|---|---|---| +| `OSPREY_RULES_REPO` | _required_ | `owner/name` of the repo to PR against. | +| `OSPREY_GITHUB_TOKEN` | _required_ | Fine-grained PAT with `Contents: read/write` and `Pull requests: read/write` on the repo. | +| `OSPREY_GITHUB_API_URL` | `https://api.github.com` | Set for GitHub Enterprise: e.g. `https://github.acme.example/api/v3`. | + +#### `local` + +| Var | Default | Notes | +|---|---|---| +| `OSPREY_RULES_LOCAL_PATH` | _required_ | Absolute path to the directory the backend writes SML into. Must already exist. Submissions take effect immediately; there's no review queue. | + +### Adding a rule submission backend + +Add a Python module next to `_rule_drafts_github.py` that implements the `RuleSubmissionBackend` Protocol defined in `_rule_drafts_backend.py`, then wire it into `load_backend()`. See the module docstring on `_rule_drafts_backend.py` for the contract; the existing HTTP-backed module (`_rule_drafts_github.py`) is a working template. diff --git a/osprey_worker/src/osprey/worker/lib/acls/definitions/super_user.json b/osprey_worker/src/osprey/worker/lib/acls/definitions/super_user.json index c824783c..2ee45783 100644 --- a/osprey_worker/src/osprey/worker/lib/acls/definitions/super_user.json +++ b/osprey_worker/src/osprey/worker/lib/acls/definitions/super_user.json @@ -40,6 +40,10 @@ { "name": "CAN_VIEW_EVENTS_BY_ACTION", "allow_all": true + }, + { + "name": "CAN_EDIT_RULE_DRAFTS", + "allow_all": true } ], "ability_groups": ["CAN_VIEW_BASIC_USER_DATA"] diff --git a/osprey_worker/src/osprey/worker/lib/osprey_engine.py b/osprey_worker/src/osprey/worker/lib/osprey_engine.py index 2e6318d4..45618ae8 100644 --- a/osprey_worker/src/osprey/worker/lib/osprey_engine.py +++ b/osprey_worker/src/osprey/worker/lib/osprey_engine.py @@ -157,6 +157,14 @@ def _handle_updated_sources(self) -> None: def execution_graph(self) -> ExecutionGraph: return self._execution_graph + @property + def udf_registry(self) -> UDFRegistry: + return self._udf_registry + + @property + def validator_registry(self) -> ValidatorRegistry: + return self._validator_registry + @property def config(self) -> SourcesConfig: return self._execution_graph.validated_sources.sources.config diff --git a/osprey_worker/src/osprey/worker/ui_api/osprey/app.py b/osprey_worker/src/osprey/worker/ui_api/osprey/app.py index 407462fe..4292b3df 100644 --- a/osprey_worker/src/osprey/worker/ui_api/osprey/app.py +++ b/osprey_worker/src/osprey/worker/ui_api/osprey/app.py @@ -68,6 +68,7 @@ def create_app() -> Flask: events, features, queries, + rule_drafts, rules, rules_visualizer, saved_queries, @@ -111,6 +112,7 @@ def create_app() -> Flask: _register_with_prefix(app, events.blueprint) _register_with_prefix(app, features.blueprint) _register_with_prefix(app, rules.blueprint) + _register_with_prefix(app, rule_drafts.blueprint) _register_with_prefix(app, queries.blueprint) _register_with_prefix(app, config.blueprint) _register_with_prefix(app, docs.blueprint) diff --git a/osprey_worker/src/osprey/worker/ui_api/osprey/lib/abilities.py b/osprey_worker/src/osprey/worker/ui_api/osprey/lib/abilities.py index e56fffd2..3bdf1748 100644 --- a/osprey_worker/src/osprey/worker/ui_api/osprey/lib/abilities.py +++ b/osprey_worker/src/osprey/worker/ui_api/osprey/lib/abilities.py @@ -535,6 +535,7 @@ def _get_query_filter(self) -> dict[str, Any] | None: CanViewSavedQueries = register_ability('CAN_VIEW_SAVED_QUERIES')(make_marker_ability()) CanCreateAndEditSavedQueries = register_ability('CAN_CREATE_AND_EDIT_SAVED_QUERIES')(make_marker_ability()) CanBulkAction = register_ability('CAN_BULK_ACTION')(make_marker_ability()) +CanEditRuleDrafts = register_ability('CAN_EDIT_RULE_DRAFTS')(make_marker_ability()) def require_ability_with_request(request_model: ModelT, ability_class: Type[Ability[ModelT, ItemT]]) -> None: diff --git a/osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_backend.py b/osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_backend.py new file mode 100644 index 00000000..755f1eb3 --- /dev/null +++ b/osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_backend.py @@ -0,0 +1,142 @@ +"""Backend abstraction for rule-draft submission. + +The Osprey engine doesn't care where rules live. Different deployments use +different hosting: GitHub or Enterprise, GitLab, Tangled, an internal Gerrit, +or a filesystem on a shared volume. Each is one implementation of the +RuleSubmissionBackend Protocol below. + +`load_backend()` reads `OSPREY_RULES_SUBMISSION_BACKEND` and instantiates the +chosen backend with its own env vars. Defaults to `null` so an unconfigured +install ships safe; adopters opt into a backend explicitly. + +Adopter docs (env vars per backend, how to choose one): see +`docs/user/manage.md`. + +Adding a new backend: implement a class with `submit_draft` and +`list_pending_drafts` matching the Protocol below, add a case in +`load_backend()`, and update the "unknown backend" error message here plus +the "no backend configured" message in `_rule_drafts_null.py`. The existing +`_rule_drafts_github.py` module is a working template for HTTP-backed +adapters; `_rule_drafts_local.py` for filesystem. +""" + +from __future__ import annotations + +import os +from dataclasses import dataclass, field +from typing import Any, Protocol + + +class RuleDraftBackendError(Exception): + """Raised by any backend method when the operation cannot complete.""" + + def __init__(self, message: str, status_code: int = 502): + super().__init__(message) + self.message = message + self.status_code = status_code + + +@dataclass(frozen=True) +class SubmissionResult: + """Backend-neutral submit_draft return value. + + `title` and `url` are what the UI surfaces in the success banner; `extras` + carries backend-specific fields (PR number, branch, etc.) for adopters + whose UI variants want to render more detail. + """ + + title: str + url: str | None + main_sml_updated: bool = False + extras: dict[str, Any] = field(default_factory=dict) + + def to_json(self) -> dict[str, Any]: + # Spread extras first so the canonical fields always win: a backend that + # happens to name an extra `title`/`url`/`main_sml_updated` can't shadow + # the contract fields the UI depends on. + return { + **self.extras, + 'title': self.title, + 'url': self.url, + 'main_sml_updated': self.main_sml_updated, + } + + +@dataclass(frozen=True) +class PendingDraft: + """Backend-neutral entry for the pending-drafts list.""" + + title: str + url: str + author: str + created_at: str + touched_files: list[str] + extras: dict[str, Any] = field(default_factory=dict) + + def to_json(self) -> dict[str, Any]: + # Spread extras first so backend-specific keys can't shadow the + # canonical fields the UI depends on. + return { + **self.extras, + 'title': self.title, + 'url': self.url, + 'author': self.author, + 'created_at': self.created_at, + 'touched_files': self.touched_files, + } + + +class RuleSubmissionBackend(Protocol): + """The contract every submission backend implements. + + Implementations: + - submit a draft (create whatever the backend's review unit is) + - optionally wire the new rule into main.sml as part of the same submission + - list whatever's currently in review + + Implementations raise `RuleDraftBackendError` for any failure path. + """ + + name: str + + 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: ... + + def list_pending_drafts(self) -> list[PendingDraft]: ... + + +def load_backend() -> RuleSubmissionBackend: + """Select and instantiate the configured backend. + + `OSPREY_RULES_SUBMISSION_BACKEND` picks one of: github, local, null. + Unset or empty defaults to `null`. Unknown values raise so a typo doesn't + silently degrade to no-op submission. + """ + name = (os.environ.get('OSPREY_RULES_SUBMISSION_BACKEND') or 'null').strip().lower() + + # Imports are deferred to keep the Protocol module dependency-free. + if name == 'null': + from ._rule_drafts_null import NullBackend + + return NullBackend() + if name == 'github': + from ._rule_drafts_github import GitHubBackend + + return GitHubBackend.from_env() + if name == 'local': + from ._rule_drafts_local import LocalBackend + + return LocalBackend.from_env() + raise RuleDraftBackendError( + f'Unknown OSPREY_RULES_SUBMISSION_BACKEND {name!r}; valid values are github, local, null.', + status_code=500, + ) diff --git a/osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_git_common.py b/osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_git_common.py new file mode 100644 index 00000000..5f9ad575 --- /dev/null +++ b/osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_git_common.py @@ -0,0 +1,62 @@ +"""Shared helpers for git-forge submission backends. + +The GitHub, GitLab, and Tangled adapters all need the same three things: a +cosmetic branch name, a check for whether main.sml already wires a rule in, and +the append that adds the wiring. They also all talk to a remote over HTTP and +must turn a dropped connection into the same structured error the UI renders +rather than an unhandled 500. This module is the one place those live. +""" + +from __future__ import annotations + +import re +import time +from typing import Any + +import requests + +from ._rule_drafts_backend import RuleDraftBackendError + +DEFAULT_TIMEOUT_SECONDS = 15 + + +def request(method: str, url: str, *, error_action: str, **kwargs: Any) -> requests.Response: + """Issue an HTTP request, converting transport failures to RuleDraftBackendError. + + A forge outage (connection refused, DNS failure, timeout) is an expected + operational state for a backend whose job is talking to a remote host, so it + should surface as the 502 JSON shape the editor knows how to display, not as + an unhandled Flask 500. HTTP status errors are left for the caller to map, + since the right status code depends on what was being attempted. + """ + kwargs.setdefault('timeout', DEFAULT_TIMEOUT_SECONDS) + try: + return requests.request(method, url, **kwargs) + except requests.RequestException as exc: + raise RuleDraftBackendError( + f'Could not reach the git host while {error_action}: {exc}', + status_code=502, + ) from exc + + +def generate_branch_name(rule_name: str, author_email: str, *, prefix: str = 'rule-draft') -> str: + """Cosmetic source-branch label. Timestamped so retries don't collide.""" + short_email = author_email.split('@', 1)[0] + slug = re.sub(r'[^A-Za-z0-9_-]+', '-', short_email).strip('-') or 'osprey-ui' + rule_slug = re.sub(r'[^A-Za-z0-9_-]+', '-', rule_name).strip('-') or 'rule' + return f'{prefix}/{slug}/{rule_slug}-{int(time.time())}' + + +def require_already_present(main_sml: str, draft_path: str) -> bool: + pattern = re.compile( + r"Require\s*\(\s*rule\s*=\s*['\"]" + re.escape(draft_path) + r"['\"]\s*\)", + re.MULTILINE, + ) + return bool(pattern.search(main_sml)) + + +def append_require_to_main(main_sml: str, draft_path: str) -> str: + suffix = f"\nRequire(rule='{draft_path}')\n" + if not main_sml.endswith('\n'): + suffix = '\n' + suffix + return main_sml + suffix diff --git a/osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_github.py b/osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_github.py new file mode 100644 index 00000000..1e9678e6 --- /dev/null +++ b/osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_github.py @@ -0,0 +1,311 @@ +"""GitHub submission backend. + +Implements RuleSubmissionBackend by opening a pull request against a configured +repo. Works with github.com and with GitHub Enterprise (set OSPREY_GITHUB_API_URL +to the Enterprise API root, e.g. https://github.mycompany.com/api/v3). +""" + +from __future__ import annotations + +import base64 +import os +from dataclasses import dataclass +from typing import Any + +import requests + +from . import _rule_drafts_git_common as git_common +from ._rule_drafts_backend import ( + PendingDraft, + RuleDraftBackendError, + SubmissionResult, +) + +DEFAULT_GITHUB_API = 'https://api.github.com' + + +@dataclass(frozen=True) +class GitHubConfig: + api_url: str + repo: str + base_branch: str + rules_path: str + token: str + + @property + def repo_url(self) -> str: + return f'{self.api_url.rstrip("/")}/repos/{self.repo}' + + @classmethod + def from_env(cls) -> 'GitHubConfig': + api_url = (os.environ.get('OSPREY_GITHUB_API_URL') or DEFAULT_GITHUB_API).strip() + repo = os.environ.get('OSPREY_RULES_REPO', '').strip() + base = os.environ.get('OSPREY_RULES_BASE_BRANCH', 'main').strip() or 'main' + rules_path = os.environ.get('OSPREY_RULES_PATH_IN_REPO', '').strip().strip('/') + token = os.environ.get('OSPREY_GITHUB_TOKEN', '').strip() + if not repo: + raise RuleDraftBackendError( + 'OSPREY_RULES_REPO is not configured; set it to "owner/name" to enable PR submission.', + status_code=503, + ) + if not token: + raise RuleDraftBackendError( + 'OSPREY_GITHUB_TOKEN is not configured; set a service-account PAT with repo write access.', + status_code=503, + ) + return cls(api_url=api_url, repo=repo, base_branch=base, rules_path=rules_path, token=token) + + +def _headers(cfg: GitHubConfig) -> dict[str, str]: + return { + 'Authorization': f'Bearer {cfg.token}', + 'Accept': 'application/vnd.github+json', + 'X-GitHub-Api-Version': '2022-11-28', + } + + +def _full_path(cfg: GitHubConfig, draft_path: str) -> str: + draft_path = draft_path.lstrip('/') + if cfg.rules_path: + return f'{cfg.rules_path}/{draft_path}' + return draft_path + + +def _get(cfg: GitHubConfig, url: str, **kwargs: Any) -> requests.Response: + return git_common.request('GET', url, error_action='contacting GitHub', headers=_headers(cfg), **kwargs) + + +def _post(cfg: GitHubConfig, url: str, json: dict[str, Any]) -> requests.Response: + return git_common.request('POST', url, error_action='contacting GitHub', headers=_headers(cfg), json=json) + + +def _put(cfg: GitHubConfig, url: str, json: dict[str, Any]) -> requests.Response: + return git_common.request('PUT', url, error_action='contacting GitHub', headers=_headers(cfg), json=json) + + +def _raise_for_github(response: requests.Response, action: str) -> None: + if response.ok: + return + body = response.text[:500] + # 4xx from GitHub is still a backend failure from the caller's POV. + raise RuleDraftBackendError( + f'GitHub returned {response.status_code} while {action}: {body}', + status_code=502, + ) + + +def _get_base_sha(cfg: GitHubConfig) -> str: + res = _get(cfg, f'{cfg.repo_url}/git/ref/heads/{cfg.base_branch}') + _raise_for_github(res, f'looking up base branch {cfg.base_branch!r}') + return res.json()['object']['sha'] + + +def _get_file_sha(cfg: GitHubConfig, path_in_repo: str, ref: str) -> str | None: + res = _get(cfg, f'{cfg.repo_url}/contents/{path_in_repo}', params={'ref': ref}) + if res.status_code == 404: + return None + _raise_for_github(res, f'reading {path_in_repo!r} on {ref!r}') + payload = res.json() + if isinstance(payload, list): + return None + return payload.get('sha') + + +def _create_branch(cfg: GitHubConfig, branch: str, sha: str) -> None: + res = _post(cfg, f'{cfg.repo_url}/git/refs', json={'ref': f'refs/heads/{branch}', 'sha': sha}) + if res.status_code == 422: + raise RuleDraftBackendError( + f'Branch {branch!r} already exists on {cfg.repo}. Pick a different name.', + status_code=409, + ) + _raise_for_github(res, f'creating branch {branch!r}') + + +def _commit_file( + cfg: GitHubConfig, + branch: str, + path_in_repo: str, + contents: str, + message: str, + existing_sha: str | None, +) -> None: + payload: dict[str, Any] = { + 'message': message, + 'content': base64.b64encode(contents.encode('utf-8')).decode('ascii'), + 'branch': branch, + } + if existing_sha is not None: + payload['sha'] = existing_sha + res = _put(cfg, f'{cfg.repo_url}/contents/{path_in_repo}', json=payload) + _raise_for_github(res, f'committing {path_in_repo!r} to {branch!r}') + + +def _open_pr(cfg: GitHubConfig, branch: str, title: str, body: str) -> dict[str, Any]: + res = _post( + cfg, + f'{cfg.repo_url}/pulls', + json={'title': title, 'head': branch, 'base': cfg.base_branch, 'body': body}, + ) + _raise_for_github(res, f'opening PR from {branch!r}') + return res.json() + + +def _main_sml_path(cfg: GitHubConfig) -> str: + return _full_path(cfg, 'main.sml') + + +def _fetch_file_on_ref(cfg: GitHubConfig, path_in_repo: str, ref: str) -> tuple[str, str] | None: + res = _get(cfg, f'{cfg.repo_url}/contents/{path_in_repo}', params={'ref': ref}) + if res.status_code == 404: + return None + _raise_for_github(res, f'reading {path_in_repo!r} on {ref!r}') + payload = res.json() + if isinstance(payload, list): + return None + encoded = payload.get('content', '') + sha = payload.get('sha') + if sha is None: + return None + try: + decoded = base64.b64decode(encoded).decode('utf-8') + except Exception as exc: + raise RuleDraftBackendError(f'could not decode {path_in_repo!r}: {exc}', status_code=502) + return decoded, sha + + +class GitHubBackend: + name = 'github' + + def __init__(self, cfg: GitHubConfig): + self._cfg = cfg + + @classmethod + def from_env(cls) -> 'GitHubBackend': + return cls(GitHubConfig.from_env()) + + 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: + cfg = self._cfg + path_in_repo = _full_path(cfg, draft_path) + base_sha = _get_base_sha(cfg) + existing_sha = _get_file_sha(cfg, path_in_repo, cfg.base_branch) + + if is_new_rule and existing_sha is not None: + raise RuleDraftBackendError( + f'A file already exists at {path_in_repo!r} on {cfg.base_branch}. ' + 'Pick a different filename, or edit the existing rule instead of creating a new one.', + status_code=409, + ) + + branch = git_common.generate_branch_name(rule_name, author_email) + _create_branch(cfg, branch, base_sha) + + verb = 'Add' if is_new_rule else 'Update' + commit_message = f'{verb} rule {rule_name}\n\nAuthored via Osprey UI by {author_email}.' + _commit_file( + cfg, + branch=branch, + path_in_repo=path_in_repo, + contents=sml_source, + message=commit_message, + existing_sha=existing_sha, + ) + + main_sml_updated = False + if wire_into_main: + main_path = _main_sml_path(cfg) + fetched = _fetch_file_on_ref(cfg, main_path, cfg.base_branch) + if fetched is None: + raise RuleDraftBackendError( + f'wire_into_main requested but {main_path!r} does not exist on {cfg.base_branch}.', + status_code=409, + ) + main_contents, main_sha = fetched + if not git_common.require_already_present(main_contents, draft_path): + new_main = git_common.append_require_to_main(main_contents, draft_path) + _commit_file( + cfg, + branch=branch, + path_in_repo=main_path, + contents=new_main, + message=f'Wire {rule_name} into main.sml\n\nAuthored via Osprey UI by {author_email}.', + existing_sha=main_sha, + ) + main_sml_updated = True + + title = f'{verb} rule {rule_name}' + touched = f'`{path_in_repo}`' + if main_sml_updated: + touched += f', `{_main_sml_path(cfg)}`' + body = ( + f'{summary.strip() or "_(no summary provided)_"}\n\n' + f'---\n' + f'Drafted in the Osprey rules UI by `{author_email}`.\n' + f'Touches: {touched}.' + ) + pr = _open_pr(cfg, branch=branch, title=title, body=body) + pr_number = pr.get('number') + pr_url = pr.get('html_url') + return SubmissionResult( + title=f'Pull request #{pr_number} opened', + url=pr_url, + main_sml_updated=main_sml_updated, + extras={ + 'pr_number': pr_number, + 'pr_url': pr_url, + 'branch': branch, + 'path_in_repo': path_in_repo, + }, + ) + + 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 diff --git a/osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_local.py b/osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_local.py new file mode 100644 index 00000000..2d815eb6 --- /dev/null +++ b/osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_local.py @@ -0,0 +1,110 @@ +"""Filesystem submission backend for self-hosted setups. + +Writes the SML straight to a configured rules directory. No review, no PR. +Adopters whose deploy pipeline already syncs a rules directory into the engine +(etcd push, file watcher, etc.) wire this backend up so the UI drops the SML +in the right place and lets the downstream pipeline take it from there. +""" + +from __future__ import annotations + +import os +from dataclasses import dataclass +from pathlib import Path + +from . import _rule_drafts_git_common as git_common +from ._rule_drafts_backend import ( + PendingDraft, + RuleDraftBackendError, + SubmissionResult, +) + + +@dataclass(frozen=True) +class LocalConfig: + rules_dir: Path + + @classmethod + def from_env(cls) -> 'LocalConfig': + raw = os.environ.get('OSPREY_RULES_LOCAL_PATH', '').strip() + if not raw: + raise RuleDraftBackendError( + 'OSPREY_RULES_LOCAL_PATH is not configured; set it to the directory the local backend should write to.', + status_code=503, + ) + path = Path(raw) + if not path.is_dir(): + raise RuleDraftBackendError( + f'OSPREY_RULES_LOCAL_PATH {raw!r} is not a directory.', + status_code=503, + ) + return cls(rules_dir=path) + + +class LocalBackend: + name = 'local' + + def __init__(self, cfg: LocalConfig): + self._cfg = cfg + + @classmethod + def from_env(cls) -> 'LocalBackend': + return cls(LocalConfig.from_env()) + + def _resolve(self, draft_path: str) -> Path: + """Resolve a draft path within rules_dir, refusing anything that would + escape via `..` or symlink traversal.""" + candidate = (self._cfg.rules_dir / draft_path).resolve() + try: + candidate.relative_to(self._cfg.rules_dir.resolve()) + except ValueError as exc: + raise RuleDraftBackendError( + f'Draft path {draft_path!r} escapes the configured rules directory.', + status_code=400, + ) from exc + return candidate + + 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 list_pending_drafts(self) -> list[PendingDraft]: + # Local backend has no review queue; submissions take effect immediately. + return [] diff --git a/osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_null.py b/osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_null.py new file mode 100644 index 00000000..8ee303ab --- /dev/null +++ b/osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_null.py @@ -0,0 +1,41 @@ +"""Default submission backend: nothing is configured, so every call fails fast. + +Ships as the default so an unconfigured upstream install never opens a PR or +writes a file without an adopter explicitly opting into a backend. +""" + +from __future__ import annotations + +from ._rule_drafts_backend import ( + PendingDraft, + RuleDraftBackendError, + SubmissionResult, +) + + +class NullBackend: + name = 'null' + + @staticmethod + def _err() -> RuleDraftBackendError: + return RuleDraftBackendError( + 'No rule-submission backend is configured. Set OSPREY_RULES_SUBMISSION_BACKEND ' + 'to one of: github, local. See docs for what each one needs.', + status_code=503, + ) + + 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: + raise self._err() + + def list_pending_drafts(self) -> list[PendingDraft]: + raise self._err() diff --git a/osprey_worker/src/osprey/worker/ui_api/osprey/views/rule_drafts.py b/osprey_worker/src/osprey/worker/ui_api/osprey/views/rule_drafts.py new file mode 100644 index 00000000..588a9942 --- /dev/null +++ b/osprey_worker/src/osprey/worker/ui_api/osprey/views/rule_drafts.py @@ -0,0 +1,602 @@ +from __future__ import annotations + +import logging +import re +from collections.abc import Iterable +from typing import Any + +from flask import Blueprint, jsonify, request +from osprey.engine.ast.error_utils import SpanWithHint +from osprey.engine.ast.grammar import ( + Assign, + BinaryComparison, + Call, + FormatString, + Name, + Not, + Number, + Source, + String, + UnaryOperation, +) +from osprey.engine.ast.grammar import ( + List as AstList, +) +from osprey.engine.ast.sources import Sources +from osprey.engine.ast_validator import validate_sources +from osprey.engine.ast_validator.validation_context import ( + ValidationError, + ValidationFailed, + ValidationWarning, +) +from osprey.worker.lib.singletons import ENGINE +from osprey.worker.ui_api.osprey.lib.abilities import CanEditRuleDrafts, require_ability +from osprey.worker.ui_api.osprey.lib.auth import get_current_user_email + +from . import _rule_drafts_backend as backends +from ._engine_ast_utils import get_func_identifier + +logger = logging.getLogger(__name__) + +blueprint = Blueprint('rule_drafts', __name__) + +_VALID_PATH = re.compile(r'^[A-Za-z0-9_./-]+\.sml$') +_VALID_RULE_NAME = re.compile(r'^[A-Za-z_][A-Za-z0-9_]*$') + + +def _format_validation_message(msg: ValidationError | ValidationWarning) -> dict[str, Any]: + identifier: str | None = None + try: + node = msg.span.ast_node + if isinstance(node, Name): + identifier = node.identifier + except Exception: + pass + + defined_in: list[str] = [] + for additional in msg.additional_spans: + span = additional.span if isinstance(additional, SpanWithHint) else additional + defined_in.append(span.source.path) + + return { + 'message': msg.message, + 'hint': msg.hint, + 'source_path': msg.source.path, + 'line': msg.span.start_line, + 'column': msg.span.start_pos, + 'rendered': msg.rendered(), + 'identifier': identifier, + 'defined_in_source_paths': defined_in, + } + + +def _suggest_imports_from_errors( + draft_path: str, + errors: list[dict[str, Any]], +) -> list[str]: + """Collect source paths the draft references but doesn't import. + + Pulled from each error's `defined_in_source_paths`. main.sml is the engine + entry point and is never importable; the draft can't import itself either. + """ + suggested: set[str] = set() + for err in errors: + for path in err.get('defined_in_source_paths') or []: + if path == 'main.sml' or path == draft_path: + continue + suggested.add(path) + return sorted(suggested) + + +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_./-]).' + if '..' in path.split('/'): + return f'Path {path!r} contains a parent-directory segment.' + return None + + +def _current_sources_dict() -> dict[str, str]: + engine = ENGINE.instance() + return engine.execution_graph.validated_sources.sources.to_dict() + + +@blueprint.route('/rule-drafts/source', methods=['GET']) +@require_ability(CanEditRuleDrafts) +def get_source() -> Any: + path = request.args.get('path', '').strip() + err = _validate_path(path) + if err: + return jsonify({'error': err}), 400 + + engine = ENGINE.instance() + source: Source | None = engine.execution_graph.validated_sources.sources.get_by_path(path) + if source is None: + return jsonify({'error': f'No source found at {path!r}.'}), 404 + return jsonify({'path': source.path, 'contents': source.contents}) + + +@blueprint.route('/rule-drafts/validate', methods=['POST']) +@require_ability(CanEditRuleDrafts) +def validate_draft() -> Any: + """Splice the draft into the engine's sources and re-run AST validation. + + A 200 with `{ok: false, errors: [...]}` means the SML failed validation; the + response is still JSON so the editor can render structured errors inline. + A 400 means the request itself was malformed (bad path, missing source). + """ + payload = request.get_json(silent=True) or {} + path = (payload.get('path') or '').strip() + source_text = payload.get('source', '') + + path_err = _validate_path(path) + if path_err: + return jsonify({'error': path_err}), 400 + if not isinstance(source_text, str): + return jsonify({'error': 'source must be a string.'}), 400 + + spliced = _current_sources_dict() + spliced[path] = source_text + + try: + sources = Sources.from_dict(spliced) + except Exception as exc: + # Sources.from_dict asserts on shape (e.g., missing main.sml). Surface as a structured error + # so the editor can show "you broke main.sml" without crashing. + return jsonify( + { + 'ok': False, + 'errors': [{'message': str(exc), 'hint': '', 'source_path': path, 'line': 0, 'column': 0}], + 'warnings': [], + } + ), 400 + + engine = ENGINE.instance() + try: + validated = validate_sources( + sources, + udf_registry=engine.udf_registry, + validator_registry=engine.validator_registry, + ) + except ValidationFailed as exc: + formatted_errors = [_format_validation_message(e) for e in exc.errors] + return jsonify( + { + 'ok': False, + 'errors': formatted_errors, + 'warnings': [_format_validation_message(w) for w in exc.warnings], + 'suggested_imports': _suggest_imports_from_errors(path, formatted_errors), + } + ) + + return jsonify( + { + 'ok': True, + 'errors': [], + 'warnings': [_format_validation_message(w) for w in validated.warnings], + 'suggested_imports': [], + } + ) + + +def _iter_top_level_assigns(sources: Iterable[Source]) -> Iterable[tuple[Source, Assign]]: + for source in sources: + for statement in source.ast_root.statements: + if isinstance(statement, Assign): + yield source, statement + + +def _is_rule_call(node: Any) -> bool: + return isinstance(node, Call) and get_func_identifier(node) == 'Rule' + + +def _collect_features(sources: Iterable[Source]) -> list[dict[str, Any]]: + out: list[dict[str, Any]] = [] + seen: set[str] = set() + for source, assign in _iter_top_level_assigns(sources): + name = assign.target.identifier + if name in seen: + continue + # Skip `MyRule = Rule(...)` assigns; the builder dropdown is for values + # a user can reference inside conditions, not for rule definitions. + if _is_rule_call(assign.value): + continue + seen.add(name) + out.append( + { + 'name': name, + 'source_path': source.path, + 'source_line': assign.span.start_line, + } + ) + out.sort(key=lambda item: item['name']) + return out + + +def _collect_udfs() -> list[dict[str, Any]]: + engine = ENGINE.instance() + udf_registry = engine.udf_registry + out: list[dict[str, Any]] = [] + for func in sorted(udf_registry.iter_functions(), key=lambda f: f.__name__): + try: + args_type = func.get_arguments_type() + rvalue_type = func.get_rvalue_type() + except Exception: + continue + arguments: list[dict[str, Any]] = [] + try: + items = args_type.items().items() + except Exception: + items = [] + for arg_name, arg_type in items: + arguments.append( + { + 'name': arg_name, + 'type_name': getattr(arg_type, '__name__', str(arg_type)), + } + ) + out.append( + { + 'name': func.__name__, + 'return_type': getattr(rvalue_type, '__name__', str(rvalue_type)), + 'arguments': arguments, + } + ) + return out + + +def _collect_effects(sources: Iterable[Source]) -> list[str]: + """Names of UDFs that appear inside a `WhenRules(then=[...])` block. + + Used as the effect dropdown. Sourced from real usage rather than from the + UDF registry because the registry holds every UDF and we want a shortlist + of things users actually use as actions. + """ + seen: set[str] = set() + + def _walk(node: Any) -> None: + if isinstance(node, Call): + ident = get_func_identifier(node) + if ident: + seen.add(ident) + for arg in node.arguments: + _walk(arg.value) + elif isinstance(node, AstList): + for item in node.items: + _walk(item) + elif isinstance(node, Assign): + _walk(node.value) + + for source in sources: + for statement in source.ast_root.statements: + call_node: Call | None = None + if isinstance(statement, Call) and get_func_identifier(statement) == 'WhenRules': + call_node = statement + elif ( + isinstance(statement, Assign) + and isinstance(statement.value, Call) + and get_func_identifier(statement.value) == 'WhenRules' + ): + call_node = statement.value + if call_node is None: + continue + then_arg = call_node.find_argument('then') + if then_arg is None: + continue + _walk(then_arg.value) + + return sorted(seen) + + +@blueprint.route('/rule-drafts/vocabulary', methods=['GET']) +@require_ability(CanEditRuleDrafts) +def vocabulary() -> Any: + engine = ENGINE.instance() + sources = list(engine.execution_graph.validated_sources.sources) + features = _collect_features(sources) + udfs = _collect_udfs() + effects = _collect_effects(sources) + source_files = sorted(s.path for s in sources) + return jsonify( + { + 'features': features, + 'udfs': udfs, + 'effects': effects, + 'source_files': source_files, + } + ) + + +@blueprint.route('/rule-drafts/submit', methods=['POST']) +@require_ability(CanEditRuleDrafts) +def submit_draft() -> Any: + payload = request.get_json(silent=True) or {} + path = (payload.get('path') or '').strip() + source_text = payload.get('source', '') + rule_name = (payload.get('rule_name') or '').strip() + summary = (payload.get('summary') or '').strip() + is_new_rule = bool(payload.get('is_new_rule', False)) + wire_into_main = bool(payload.get('wire_into_main', False)) + + path_err = _validate_path(path) + if path_err: + return jsonify({'error': path_err}), 400 + if path == 'main.sml': + # main.sml is the engine entry point. Submitting a draft *as* main.sml + # would wholesale-replace it (immediate live effect on the local + # backend). Wiring a rule in is a controlled one-line append handled by + # the wire_into_main option, not a draft submission. + return jsonify( + { + 'error': 'main.sml is the engine entry point and cannot be submitted as a draft. ' + 'Use "turn this rule on" to add a Require line instead.' + } + ), 400 + if not isinstance(source_text, str) or not source_text.strip(): + return jsonify({'error': 'source must be a non-empty string.'}), 400 + if not _VALID_RULE_NAME.match(rule_name): + return jsonify({'error': 'rule_name must be a valid SML identifier ([A-Za-z_][A-Za-z0-9_]*).'}), 400 + + # Re-validate server-side so a client that skips the validate step still cannot push uncompilable SML. + spliced = _current_sources_dict() + spliced[path] = source_text + try: + sources = Sources.from_dict(spliced) + validate_sources( + sources, + udf_registry=ENGINE.instance().udf_registry, + validator_registry=ENGINE.instance().validator_registry, + ) + except ValidationFailed as exc: + return jsonify( + { + 'error': 'Validation failed; fix errors before submitting.', + 'errors': [_format_validation_message(e) for e in exc.errors], + } + ), 400 + except Exception as exc: + return jsonify({'error': f'Could not assemble sources: {exc}'}), 400 + + try: + backend = backends.load_backend() + result = backend.submit_draft( + draft_path=path, + sml_source=source_text, + rule_name=rule_name, + summary=summary, + author_email=get_current_user_email(), + is_new_rule=is_new_rule, + wire_into_main=wire_into_main, + ) + except backends.RuleDraftBackendError as exc: + return jsonify({'error': exc.message}), exc.status_code + + return jsonify(result.to_json()) + + +@blueprint.route('/rule-drafts/pending', methods=['GET']) +@require_ability(CanEditRuleDrafts) +def pending_drafts() -> Any: + try: + backend = backends.load_backend() + drafts = backend.list_pending_drafts() + except backends.RuleDraftBackendError as exc: + return jsonify({'error': exc.message, 'pending': []}), exc.status_code + return jsonify({'pending': [d.to_json() for d in drafts]}) + + +# The set of comparator strings the Rule Builder UI can render. +_BUILDER_COMPARATORS = {'==', '!=', '>', '<', '>=', '<='} + + +def _condition_from_value(node: Any, feature: str, operator: str) -> dict[str, Any] | None: + """Build a builder Condition row from an RHS node, returning None if the node + isn't a literal or a bare Name (the only RHS shapes the builder supports).""" + if isinstance(node, Name): + return {'feature': feature, 'operator': operator, 'rhs': node.identifier, 'rhsIsFeature': True} + if isinstance(node, String): + return {'feature': feature, 'operator': operator, 'rhs': node.value, 'rhsIsFeature': False} + if isinstance(node, Number): + return {'feature': feature, 'operator': operator, 'rhs': str(node.value), 'rhsIsFeature': False} + return None + + +def _parse_text_contains_call(call: Call, operator: str) -> dict[str, Any] | None: + """Convert a `TextContains(text=Name, phrase=...)` call to an includes/excludes row. + + Returns None if the call doesn't have the exact shape the builder emits. + """ + if get_func_identifier(call) != 'TextContains': + return None + text_arg = call.find_argument('text') + phrase_arg = call.find_argument('phrase') + if text_arg is None or phrase_arg is None: + return None + if not isinstance(text_arg.value, Name): + return None + feature = text_arg.value.identifier + return _condition_from_value(phrase_arg.value, feature, operator) + + +def _parse_condition(node: Any) -> dict[str, Any] | None: + if isinstance(node, UnaryOperation) and isinstance(node.operator, Not) and isinstance(node.operand, Call): + return _parse_text_contains_call(node.operand, 'excludes') + if isinstance(node, Call): + return _parse_text_contains_call(node, 'includes') + if isinstance(node, BinaryComparison): + if not isinstance(node.left, Name): + return None + operator = node.comparator.original_comparator + if operator not in _BUILDER_COMPARATORS: + return None + return _condition_from_value(node.right, node.left.identifier, operator) + return None + + +def _parse_outcome_arg(arg: Any) -> dict[str, Any] | None: + """Convert one `Call.arguments[i]` into a builder OutcomeArg, or None for + anything richer than a literal or bare Name reference.""" + val = arg.value + if isinstance(val, Name): + return {'name': arg.name, 'value': val.identifier, 'isFeature': True} + if isinstance(val, String): + return {'name': arg.name, 'value': val.value, 'isFeature': False} + if isinstance(val, Number): + return {'name': arg.name, 'value': str(val.value), 'isFeature': False} + return None + + +def _parse_outcome(node: Any) -> dict[str, Any] | None: + if not isinstance(node, Call): + return None + effect = get_func_identifier(node) + if effect is None: + return None + args: list[dict[str, Any]] = [] + for arg in node.arguments: + parsed = _parse_outcome_arg(arg) + if parsed is None: + return None + args.append(parsed) + return {'effect': effect, 'args': args} + + +def _parse_into_builder_model(source: Source) -> dict[str, Any]: + """Walk the AST of a single draft Source and either return a populated + builder model JSON or `{supported: False, reason: ...}`. + + The builder's expressible subset is deliberately narrow: optional Import + and Require statements (ignored for the model), exactly one + `RuleName = Rule(when_all=[...], description='...')`, and an optional + `WhenRules(rules_any=[RuleName], then=[...])` whose `then` entries are + UDF calls with literal or Name arguments. Anything richer means the file + can't round-trip and the user must use Code Editor. + """ + try: + statements = source.ast_root.statements + except Exception as exc: + return {'supported': False, 'reason': f'could not parse SML: {exc}'} + + rule_assign: Assign | None = None + when_rules_call: Call | None = None + + for stmt in statements: + if isinstance(stmt, Call): + ident = get_func_identifier(stmt) + if ident in ('Import', 'Require'): + continue + if ident == 'WhenRules': + if when_rules_call is not None: + return { + 'supported': False, + 'reason': 'multiple WhenRules blocks; Rule Builder edits one rule at a time', + } + when_rules_call = stmt + continue + return {'supported': False, 'reason': f'top-level call to `{ident}` is not supported by Rule Builder'} + if isinstance(stmt, Assign) and isinstance(stmt.value, Call) and get_func_identifier(stmt.value) == 'Rule': + if rule_assign is not None: + return { + 'supported': False, + 'reason': 'multiple Rule definitions in one file; Rule Builder edits one rule at a time', + } + rule_assign = stmt + continue + if isinstance(stmt, Assign): + return { + 'supported': False, + 'reason': f'helper assignment `{stmt.target.identifier} = ...` is not supported by Rule Builder', + } + return {'supported': False, 'reason': f'unsupported top-level statement: {type(stmt).__name__}'} + + if rule_assign is None: + return {'supported': False, 'reason': 'no Rule(...) definition found in this file'} + + rule_name = rule_assign.target.identifier + rule_call = rule_assign.value + assert isinstance(rule_call, Call) + + description = '' + description_arg = rule_call.find_argument('description') + if description_arg is not None: + if isinstance(description_arg.value, String): + description = description_arg.value.value + elif isinstance(description_arg.value, FormatString): + # Round-trip the raw template; the builder doesn't expose format-string editing. + description = description_arg.value.format_string + else: + return {'supported': False, 'reason': 'rule description must be a string literal'} + + when_all_arg = rule_call.find_argument('when_all') + if when_all_arg is None or not isinstance(when_all_arg.value, AstList): + return {'supported': False, 'reason': 'Rule must have `when_all=[...]`'} + + conditions: list[dict[str, Any]] = [] + for item in when_all_arg.value.items: + cond = _parse_condition(item) + if cond is None: + return { + 'supported': False, + 'reason': 'one or more conditions use expressions Rule Builder cannot represent', + } + conditions.append(cond) + if not conditions: + # Builder needs at least one row to render anything sensible; matching the EMPTY_BUILDER_MODEL default. + conditions = [{'feature': '', 'operator': '==', 'rhs': '', 'rhsIsFeature': False}] + + outcomes: list[dict[str, Any]] = [] + if when_rules_call is not None: + rules_any_arg = when_rules_call.find_argument('rules_any') + if rules_any_arg is not None and isinstance(rules_any_arg.value, AstList): + for item in rules_any_arg.value.items: + if not isinstance(item, Name) or item.identifier != rule_name: + return { + 'supported': False, + 'reason': 'WhenRules.rules_any must reference only the rule being edited', + } + then_arg = when_rules_call.find_argument('then') + if then_arg is not None and isinstance(then_arg.value, AstList): + for item in then_arg.value.items: + outcome = _parse_outcome(item) + if outcome is None: + return { + 'supported': False, + 'reason': 'one or more outcomes use expressions Rule Builder cannot represent', + } + outcomes.append(outcome) + if not outcomes: + outcomes = [{'effect': '', 'args': []}] + + return { + 'supported': True, + 'model': { + 'ruleName': rule_name, + 'description': description, + 'conditions': conditions, + 'outcomes': outcomes, + }, + } + + +@blueprint.route('/rule-drafts/parse-into-builder', methods=['POST']) +@require_ability(CanEditRuleDrafts) +def parse_into_builder() -> Any: + """Attempt to render an existing SML file as a Rule Builder model. + + Returns `{supported: true, model: {...}}` if the file fits the builder's + expressible subset, or `{supported: false, reason: "..."}` otherwise. The + UI uses this to decide whether to enable the Rule Builder toggle when + editing an existing rule. + """ + payload = request.get_json(silent=True) or {} + path = (payload.get('path') or '').strip() + source_text = payload.get('source', '') + + path_err = _validate_path(path) + if path_err: + return jsonify({'error': path_err}), 400 + if not isinstance(source_text, str): + return jsonify({'error': 'source must be a string.'}), 400 + + source = Source(path=path, contents=source_text) + return jsonify(_parse_into_builder_model(source)) diff --git a/osprey_worker/src/osprey/worker/ui_api/osprey/views/tests/test_rule_drafts.py b/osprey_worker/src/osprey/worker/ui_api/osprey/views/tests/test_rule_drafts.py new file mode 100644 index 00000000..dbe18e43 --- /dev/null +++ b/osprey_worker/src/osprey/worker/ui_api/osprey/views/tests/test_rule_drafts.py @@ -0,0 +1,900 @@ +import base64 +import json +import tempfile +from pathlib import Path +from unittest.mock import patch + +import pytest +import requests +import requests_mock as requests_mock_module +from flask import Response, url_for +from flask.testing import FlaskClient +from osprey.worker.lib.snowflake import Snowflake + + +@pytest.fixture(autouse=True) +def _mock_audit_snowflake(): + # The after_request audit hook mints a snowflake id, which normally means an + # HTTP call to the snowflake-id-worker service. Tests that wrap a request in + # a requests_mock.Mocker would otherwise trip NoMockAddress on that call, so + # neutralize it here (the audit log's persist() is already mocked in the + # shared conftest). + with patch('osprey.worker.ui_api.osprey.lib.audit.generate_snowflake', return_value=Snowflake(1)): + yield + + +def _set_github_backend(monkeypatch: pytest.MonkeyPatch, **overrides: str) -> None: + """Configure the github backend with sensible defaults for tests; overrides win.""" + defaults = { + 'OSPREY_RULES_SUBMISSION_BACKEND': 'github', + 'OSPREY_RULES_REPO': 'roostorg/osprey-rules', + 'OSPREY_GITHUB_TOKEN': 'gh_fake_token', + 'OSPREY_RULES_BASE_BRANCH': 'main', + } + for k, v in {**defaults, **overrides}.items(): + monkeypatch.setenv(k, v) + + +_acl_with_draft_ability = json.dumps( + { + 'ui_config': {}, + 'labels': {}, + 'acl': { + 'users': { + 'local-dev@localhost': { + 'abilities': [ + {'name': 'CAN_VIEW_DOCS', 'allow_all': True}, + {'name': 'CAN_EDIT_RULE_DRAFTS', 'allow_all': True}, + ], + }, + }, + }, + } +) + +_acl_without_draft_ability = json.dumps( + { + 'ui_config': {}, + 'labels': {}, + 'acl': { + 'users': { + 'local-dev@localhost': {'abilities': [{'name': 'CAN_VIEW_DOCS', 'allow_all': True}]}, + }, + }, + } +) + +_base_sources = { + 'config.yaml': _acl_with_draft_ability, + 'models/base.sml': """ + UserId: str = JsonData(path='$.user_id') + PostText: str = JsonData(path='$.post_text') + """, + 'main.sml': """ + Import(rules=['models/base.sml']) + + ContainsHello = Rule( + when_all=[PostText == 'hello'], + description='Post contains hello', + ) + + WhenRules( + rules_any=[ContainsHello], + then=[DeclareVerdict(verdict=UserId)], + ) + """, +} + + +@pytest.mark.use_rules_sources(_base_sources) +def test_get_source_returns_contents(client: 'FlaskClient[Response]') -> None: + res = client.get(url_for('rule_drafts.get_source'), query_string={'path': 'main.sml'}) + assert res.status_code == 200 + assert res.json is not None + assert res.json['path'] == 'main.sml' + assert 'ContainsHello' in res.json['contents'] + + +@pytest.mark.use_rules_sources(_base_sources) +def test_get_source_rejects_bad_path(client: 'FlaskClient[Response]') -> None: + res = client.get(url_for('rule_drafts.get_source'), query_string={'path': '../etc/passwd.sml'}) + assert res.status_code == 400 + + +@pytest.mark.use_rules_sources(_base_sources) +def test_get_source_404_for_unknown_path(client: 'FlaskClient[Response]') -> None: + res = client.get(url_for('rule_drafts.get_source'), query_string={'path': 'rules/does_not_exist.sml'}) + assert res.status_code == 404 + + +@pytest.mark.use_rules_sources( + { + 'config.yaml': _acl_without_draft_ability, + 'main.sml': "UserId: str = JsonData(path='$.user_id')", + } +) +def test_endpoints_require_can_edit_rule_drafts(client: 'FlaskClient[Response]') -> None: + res = client.get(url_for('rule_drafts.get_source'), query_string={'path': 'main.sml'}) + assert res.status_code == 401 + res = client.post( + url_for('rule_drafts.validate_draft'), + json={'path': 'rules/x.sml', 'source': ''}, + ) + assert res.status_code == 401 + res = client.post( + url_for('rule_drafts.parse_into_builder'), + json={'path': 'rules/x.sml', 'source': ''}, + ) + assert res.status_code == 401 + res = client.get(url_for('rule_drafts.vocabulary')) + assert res.status_code == 401 + res = client.get(url_for('rule_drafts.pending_drafts')) + assert res.status_code == 401 + + +@pytest.mark.use_rules_sources(_base_sources) +def test_validate_clean_draft_returns_ok(client: 'FlaskClient[Response]') -> None: + res = client.post( + url_for('rule_drafts.validate_draft'), + json={ + 'path': 'rules/new_rule.sml', + 'source': "Import(rules=['models/base.sml'])\nAnotherRule = Rule(when_all=[PostText == 'bye'], description='bye')", + }, + ) + assert res.status_code == 200 + body = res.json + assert body is not None + assert body['ok'] is True + assert body['errors'] == [] + + +@pytest.mark.use_rules_sources(_base_sources) +def test_validate_broken_draft_returns_structured_errors(client: 'FlaskClient[Response]') -> None: + res = client.post( + url_for('rule_drafts.validate_draft'), + json={ + 'path': 'rules/broken.sml', + 'source': 'this is not valid SML at all *** !!!', + }, + ) + assert res.status_code == 200 + body = res.json + assert body is not None + assert body['ok'] is False + assert len(body['errors']) >= 1 + err = body['errors'][0] + assert set(err.keys()) >= {'message', 'hint', 'source_path', 'line', 'column', 'rendered'} + + +@pytest.mark.use_rules_sources( + { + 'config.yaml': _acl_with_draft_ability, + 'main.sml': "Import(rules=['models/post.sml'])", + 'models/post.sml': "PostText: str = JsonData(path='$.post_text')", + } +) +def test_validate_returns_suggested_imports_for_unimported_identifier(client: 'FlaskClient[Response]') -> None: + res = client.post( + url_for('rule_drafts.validate_draft'), + json={ + 'path': 'rules/uses_post_text.sml', + 'source': "MyRule = Rule(when_all=[PostText == 'hi'], description='hi')", + }, + ) + assert res.status_code == 200 + body = res.json + assert body is not None + assert body['ok'] is False + assert body['suggested_imports'] == ['models/post.sml'] + assert any(e.get('identifier') == 'PostText' for e in body['errors']) + + +@pytest.mark.use_rules_sources(_base_sources) +def test_validate_clean_draft_has_empty_suggested_imports(client: 'FlaskClient[Response]') -> None: + res = client.post( + url_for('rule_drafts.validate_draft'), + json={ + 'path': 'rules/new_rule.sml', + 'source': "Import(rules=['models/base.sml'])\nAnotherRule = Rule(when_all=[PostText == 'bye'], description='bye')", + }, + ) + assert res.status_code == 200 + assert res.json is not None + assert res.json['suggested_imports'] == [] + + +@pytest.mark.use_rules_sources(_base_sources) +def test_validate_rejects_bad_path(client: 'FlaskClient[Response]') -> None: + res = client.post( + url_for('rule_drafts.validate_draft'), + json={'path': 'rules/x.txt', 'source': ''}, + ) + assert res.status_code == 400 + + +@pytest.mark.use_rules_sources(_base_sources) +def test_vocabulary_returns_features_udfs_effects(client: 'FlaskClient[Response]') -> None: + res = client.get(url_for('rule_drafts.vocabulary')) + assert res.status_code == 200 + body = res.json + assert body is not None + assert set(body.keys()) == {'features', 'udfs', 'effects', 'source_files'} + + feature_names = {f['name'] for f in body['features']} + assert {'UserId', 'PostText'}.issubset(feature_names) + assert 'ContainsHello' not in feature_names + + udf_names = {u['name'] for u in body['udfs']} + assert 'JsonData' in udf_names + assert 'Rule' in udf_names + assert 'DeclareVerdict' in body['effects'] + + assert 'main.sml' in body['source_files'] + + +@pytest.mark.use_rules_sources(_base_sources) +def test_submit_returns_503_when_no_backend_configured( + client: 'FlaskClient[Response]', monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.delenv('OSPREY_RULES_SUBMISSION_BACKEND', raising=False) + res = client.post( + url_for('rule_drafts.submit_draft'), + json={ + 'path': 'rules/new_rule.sml', + 'source': "Import(rules=['models/base.sml'])\nAnotherRule = Rule(when_all=[PostText == 'bye'], description='bye')", + 'rule_name': 'AnotherRule', + 'summary': 'demo', + 'is_new_rule': True, + }, + ) + assert res.status_code == 503 + body = res.json + assert body is not None + assert 'No rule-submission backend is configured' in body['error'] + + +@pytest.mark.use_rules_sources(_base_sources) +def test_submit_returns_503_when_github_missing_required_env( + client: 'FlaskClient[Response]', monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.setenv('OSPREY_RULES_SUBMISSION_BACKEND', 'github') + monkeypatch.delenv('OSPREY_RULES_REPO', raising=False) + monkeypatch.delenv('OSPREY_GITHUB_TOKEN', raising=False) + res = client.post( + url_for('rule_drafts.submit_draft'), + json={ + 'path': 'rules/new_rule.sml', + 'source': "Import(rules=['models/base.sml'])\nAnotherRule = Rule(when_all=[PostText == 'bye'], description='bye')", + 'rule_name': 'AnotherRule', + 'summary': 'demo', + 'is_new_rule': True, + }, + ) + assert res.status_code == 503 + body = res.json + assert body is not None + assert 'OSPREY_RULES_REPO' in body['error'] + + +@pytest.mark.use_rules_sources(_base_sources) +def test_submit_blocks_invalid_sml_before_calling_github( + client: 'FlaskClient[Response]', monkeypatch: pytest.MonkeyPatch +) -> None: + _set_github_backend(monkeypatch) + + with requests_mock_module.Mocker() as m: + res = client.post( + url_for('rule_drafts.submit_draft'), + json={ + 'path': 'rules/bad.sml', + 'source': 'this is not valid SML !!!', + 'rule_name': 'BadRule', + 'summary': 'should not submit', + 'is_new_rule': True, + }, + ) + + assert res.status_code == 400 + assert m.call_count == 0 + body = res.json + assert body is not None + assert body['error'].startswith('Validation failed') + + +@pytest.mark.use_rules_sources(_base_sources) +def test_submit_rejects_bad_rule_name(client: 'FlaskClient[Response]', monkeypatch: pytest.MonkeyPatch) -> None: + _set_github_backend(monkeypatch) + res = client.post( + url_for('rule_drafts.submit_draft'), + json={ + 'path': 'rules/new.sml', + 'source': "X = Rule(when_all=[PostText == 'x'], description='x')", + 'rule_name': '1-not-an-identifier', + 'summary': '', + 'is_new_rule': True, + }, + ) + assert res.status_code == 400 + + +def test_submission_result_extras_cannot_shadow_canonical_fields() -> None: + from osprey.worker.ui_api.osprey.views._rule_drafts_backend import SubmissionResult + + result = SubmissionResult( + title='real title', + url='https://real.example/pr/1', + extras={'title': 'spoofed', 'url': 'https://evil.example', 'pr_number': 7}, + ) + out = result.to_json() + assert out['title'] == 'real title' + assert out['url'] == 'https://real.example/pr/1' + assert out['main_sml_updated'] is False + # Non-colliding extras still pass through for adopters that want them. + assert out['pr_number'] == 7 + + +@pytest.mark.use_rules_sources(_base_sources) +def test_submit_rejects_main_sml_as_draft_path( + client: 'FlaskClient[Response]', monkeypatch: pytest.MonkeyPatch +) -> None: + _set_github_backend(monkeypatch) + with requests_mock_module.Mocker() as m: + res = client.post( + url_for('rule_drafts.submit_draft'), + json={ + 'path': 'main.sml', + 'source': 'Import(rules=[])', + 'rule_name': 'Whatever', + 'summary': '', + 'is_new_rule': False, + }, + ) + # Guard fires before any network call: the entry point is never a draft. + assert m.call_count == 0 + assert res.status_code == 400 + body = res.json + assert body is not None + assert 'main.sml' in body['error'] + + +@pytest.mark.use_rules_sources(_base_sources) +def test_submit_surfaces_github_connection_error_as_502( + client: 'FlaskClient[Response]', monkeypatch: pytest.MonkeyPatch +) -> None: + _set_github_backend(monkeypatch) + repo_url = 'https://api.github.com/repos/roostorg/osprey-rules' + with requests_mock_module.Mocker() as m: + m.get(f'{repo_url}/git/ref/heads/main', exc=requests.exceptions.ConnectTimeout) + res = client.post( + url_for('rule_drafts.submit_draft'), + json={ + 'path': 'rules/new_rule.sml', + 'source': "Import(rules=['models/base.sml'])\nAnotherRule = Rule(when_all=[PostText == 'bye'], description='bye')", + 'rule_name': 'AnotherRule', + 'summary': '', + 'is_new_rule': True, + }, + ) + # A forge outage is a structured 502, not an unhandled 500. + assert res.status_code == 502 + body = res.json + assert body is not None + assert 'Could not reach the git host' in body['error'] + + +@pytest.mark.use_rules_sources(_base_sources) +def test_submit_happy_path_creates_branch_commits_and_opens_pr( + client: 'FlaskClient[Response]', monkeypatch: pytest.MonkeyPatch +) -> None: + _set_github_backend(monkeypatch, OSPREY_RULES_PATH_IN_REPO='rules') + + repo_url = 'https://api.github.com/repos/roostorg/osprey-rules' + + with requests_mock_module.Mocker() as m: + m.get(f'{repo_url}/git/ref/heads/main', json={'object': {'sha': 'BASE_SHA'}}) + m.get(f'{repo_url}/contents/rules/new_rule.sml', status_code=404) + m.post(f'{repo_url}/git/refs', status_code=201, json={}) + m.put(f'{repo_url}/contents/rules/new_rule.sml', status_code=201, json={}) + m.post( + f'{repo_url}/pulls', + status_code=201, + json={'number': 42, 'html_url': 'https://github.com/roostorg/osprey-rules/pull/42'}, + ) + + res = client.post( + url_for('rule_drafts.submit_draft'), + json={ + # Bare filename: OSPREY_RULES_PATH_IN_REPO='rules' prepends the + # subdirectory, so the file lands at rules/new_rule.sml. + 'path': 'new_rule.sml', + 'source': "Import(rules=['models/base.sml'])\nAnotherRule = Rule(when_all=[PostText == 'bye'], description='bye')", + 'rule_name': 'AnotherRule', + 'summary': 'add bye rule', + 'is_new_rule': True, + }, + ) + + assert res.status_code == 200 + body = res.json + assert body is not None + assert body['title'] == 'Pull request #42 opened' + assert body['url'] == 'https://github.com/roostorg/osprey-rules/pull/42' + assert body['main_sml_updated'] is False + # GitHub-specific extras are surfaced for adopters that want them. + assert body['pr_number'] == 42 + assert body['pr_url'] == 'https://github.com/roostorg/osprey-rules/pull/42' + assert body['path_in_repo'] == 'rules/new_rule.sml' + assert body['branch'].startswith('rule-draft/local-dev/AnotherRule-') + + +@pytest.mark.use_rules_sources(_base_sources) +def test_submit_wire_into_main_appends_require_line( + client: 'FlaskClient[Response]', monkeypatch: pytest.MonkeyPatch +) -> None: + _set_github_backend(monkeypatch) + + repo_url = 'https://api.github.com/repos/roostorg/osprey-rules' + existing_main = "Import(rules=['models/post.sml'])\n\nRequire(rule='rules/post_contains_hello.sml')\n" + encoded_main = base64.b64encode(existing_main.encode('utf-8')).decode('ascii') + + with requests_mock_module.Mocker() as m: + m.get(f'{repo_url}/git/ref/heads/main', json={'object': {'sha': 'BASE_SHA'}}) + m.get(f'{repo_url}/contents/rules/new_rule.sml', status_code=404) + m.post(f'{repo_url}/git/refs', status_code=201, json={}) + m.put(f'{repo_url}/contents/rules/new_rule.sml', status_code=201, json={}) + m.get(f'{repo_url}/contents/main.sml', json={'sha': 'MAIN_SHA', 'content': encoded_main, 'type': 'file'}) + main_put = m.put(f'{repo_url}/contents/main.sml', status_code=200, json={}) + m.post( + f'{repo_url}/pulls', + status_code=201, + json={'number': 99, 'html_url': 'https://github.com/roostorg/osprey-rules/pull/99'}, + ) + + res = client.post( + url_for('rule_drafts.submit_draft'), + json={ + 'path': 'rules/new_rule.sml', + 'source': "Import(rules=['models/base.sml'])\nAnotherRule = Rule(when_all=[PostText == 'bye'], description='bye')", + 'rule_name': 'AnotherRule', + 'summary': 'add bye rule and wire it in', + 'is_new_rule': True, + 'wire_into_main': True, + }, + ) + + assert res.status_code == 200 + body = res.json + assert body is not None + assert body['main_sml_updated'] is True + + main_put_request = main_put.last_request + assert main_put_request is not None + posted = main_put_request.json() + decoded_new_main = base64.b64decode(posted['content']).decode('utf-8') + assert "Require(rule='rules/new_rule.sml')" in decoded_new_main + # The existing Require for post_contains_hello.sml should still be present untouched. + assert "Require(rule='rules/post_contains_hello.sml')" in decoded_new_main + + +@pytest.mark.use_rules_sources(_base_sources) +def test_submit_wire_into_main_skips_when_require_already_present( + client: 'FlaskClient[Response]', monkeypatch: pytest.MonkeyPatch +) -> None: + _set_github_backend(monkeypatch) + + repo_url = 'https://api.github.com/repos/roostorg/osprey-rules' + existing_main = "Require(rule='rules/already_here.sml')\n" + encoded_main = base64.b64encode(existing_main.encode('utf-8')).decode('ascii') + + with requests_mock_module.Mocker() as m: + m.get(f'{repo_url}/git/ref/heads/main', json={'object': {'sha': 'BASE_SHA'}}) + m.get(f'{repo_url}/contents/rules/already_here.sml', status_code=404) + m.post(f'{repo_url}/git/refs', status_code=201, json={}) + m.put(f'{repo_url}/contents/rules/already_here.sml', status_code=201, json={}) + m.get(f'{repo_url}/contents/main.sml', json={'sha': 'MAIN_SHA', 'content': encoded_main, 'type': 'file'}) + main_put = m.put(f'{repo_url}/contents/main.sml', status_code=200, json={}) + m.post( + f'{repo_url}/pulls', + status_code=201, + json={'number': 100, 'html_url': 'https://github.com/roostorg/osprey-rules/pull/100'}, + ) + + res = client.post( + url_for('rule_drafts.submit_draft'), + json={ + 'path': 'rules/already_here.sml', + 'source': "Import(rules=['models/base.sml'])\nAnotherRule = Rule(when_all=[PostText == 'bye'], description='bye')", + 'rule_name': 'AnotherRule', + 'summary': '', + 'is_new_rule': True, + 'wire_into_main': True, + }, + ) + + assert res.status_code == 200 + body = res.json + assert body is not None + assert body['main_sml_updated'] is False + # main.sml fetch happens but the PUT to update it must not. + assert main_put.call_count == 0 + + +@pytest.mark.use_rules_sources(_base_sources) +def test_submit_409_if_new_rule_file_already_exists( + client: 'FlaskClient[Response]', monkeypatch: pytest.MonkeyPatch +) -> None: + _set_github_backend(monkeypatch) + + repo_url = 'https://api.github.com/repos/roostorg/osprey-rules' + + with requests_mock_module.Mocker() as m: + m.get(f'{repo_url}/git/ref/heads/main', json={'object': {'sha': 'BASE_SHA'}}) + m.get( + f'{repo_url}/contents/new_rule.sml', + json={'sha': 'EXISTING_BLOB_SHA', 'type': 'file'}, + ) + + res = client.post( + url_for('rule_drafts.submit_draft'), + json={ + 'path': 'new_rule.sml', + 'source': "Import(rules=['models/base.sml'])\nAnotherRule = Rule(when_all=[PostText == 'bye'], description='bye')", + 'rule_name': 'AnotherRule', + 'summary': '', + 'is_new_rule': True, + }, + ) + + assert res.status_code == 409 + + +@pytest.mark.use_rules_sources(_base_sources) +def test_pending_filters_to_rules_path_and_sml( + client: 'FlaskClient[Response]', monkeypatch: pytest.MonkeyPatch +) -> None: + _set_github_backend(monkeypatch, OSPREY_RULES_PATH_IN_REPO='rules') + + repo_url = 'https://api.github.com/repos/roostorg/osprey-rules' + + with requests_mock_module.Mocker() as m: + m.get( + f'{repo_url}/pulls', + json=[ + { + 'number': 1, + 'title': 'Add rule X', + 'html_url': 'https://github.com/roostorg/osprey-rules/pull/1', + 'head': {'ref': 'rule-draft/local-dev/X-1'}, + 'user': {'login': 'someone'}, + 'created_at': '2026-06-30T12:00:00Z', + }, + { + 'number': 2, + 'title': 'Update README', + 'html_url': 'https://github.com/roostorg/osprey-rules/pull/2', + 'head': {'ref': 'docs/readme'}, + 'user': {'login': 'someone'}, + 'created_at': '2026-06-30T13:00:00Z', + }, + ], + ) + m.get( + f'{repo_url}/pulls/1/files', + json=[{'filename': 'rules/x.sml'}], + ) + m.get( + f'{repo_url}/pulls/2/files', + json=[{'filename': 'README.md'}], + ) + + res = client.get(url_for('rule_drafts.pending_drafts')) + + assert res.status_code == 200 + body = res.json + assert body is not None + assert len(body['pending']) == 1 + entry = body['pending'][0] + assert entry['title'] == 'Add rule X' + assert entry['url'] == 'https://github.com/roostorg/osprey-rules/pull/1' + assert entry['touched_files'] == ['rules/x.sml'] + # GitHub-specific extras carried through for adopters that want them. + assert entry['pr_number'] == 1 + + +@pytest.mark.use_rules_sources(_base_sources) +def test_parse_into_builder_round_trips_a_builder_shaped_rule(client: 'FlaskClient[Response]') -> None: + source = """ +Import(rules=['models/post.sml']) + +ContainsCat = Rule( + when_all=[ + TextContains(text=PostText, phrase='cat'), + EventType == 'create_post', + ], + description='looks for cat', +) + +WhenRules( + rules_any=[ContainsCat], + then=[ + LabelAdd(entity=UserId, label='meow'), + ], +) +""" + res = client.post( + url_for('rule_drafts.parse_into_builder'), + json={'path': 'rules/contains_cat.sml', 'source': source}, + ) + assert res.status_code == 200 + body = res.json + assert body is not None + assert body['supported'] is True + model = body['model'] + assert model['ruleName'] == 'ContainsCat' + assert model['description'] == 'looks for cat' + assert model['conditions'] == [ + {'feature': 'PostText', 'operator': 'includes', 'rhs': 'cat', 'rhsIsFeature': False}, + {'feature': 'EventType', 'operator': '==', 'rhs': 'create_post', 'rhsIsFeature': False}, + ] + assert model['outcomes'] == [ + { + 'effect': 'LabelAdd', + 'args': [ + {'name': 'entity', 'value': 'UserId', 'isFeature': True}, + {'name': 'label', 'value': 'meow', 'isFeature': False}, + ], + } + ] + + +@pytest.mark.use_rules_sources(_base_sources) +def test_parse_into_builder_handles_excludes(client: 'FlaskClient[Response]') -> None: + source = "BlocksCat = Rule(when_all=[not TextContains(text=PostText, phrase='cat')], description='no cat')\n" + res = client.post( + url_for('rule_drafts.parse_into_builder'), + json={'path': 'rules/blocks_cat.sml', 'source': source}, + ) + assert res.status_code == 200 + body = res.json + assert body is not None + assert body['supported'] is True + assert body['model']['conditions'] == [ + {'feature': 'PostText', 'operator': 'excludes', 'rhs': 'cat', 'rhsIsFeature': False}, + ] + + +@pytest.mark.use_rules_sources(_base_sources) +def test_parse_into_builder_rejects_multiple_rules(client: 'FlaskClient[Response]') -> None: + source = ( + "A = Rule(when_all=[PostText == 'a'], description='a')\nB = Rule(when_all=[PostText == 'b'], description='b')\n" + ) + res = client.post( + url_for('rule_drafts.parse_into_builder'), + json={'path': 'rules/multi.sml', 'source': source}, + ) + assert res.status_code == 200 + body = res.json + assert body is not None + assert body['supported'] is False + assert 'multiple Rule definitions' in body['reason'] + + +@pytest.mark.use_rules_sources(_base_sources) +def test_parse_into_builder_rejects_helper_assigns(client: 'FlaskClient[Response]') -> None: + source = "Helper = 'cat'\nA = Rule(when_all=[PostText == Helper], description='a')\n" + res = client.post( + url_for('rule_drafts.parse_into_builder'), + json={'path': 'rules/helper.sml', 'source': source}, + ) + assert res.status_code == 200 + body = res.json + assert body is not None + assert body['supported'] is False + assert 'helper assignment' in body['reason'] + + +@pytest.mark.use_rules_sources(_base_sources) +def test_parse_into_builder_rejects_complex_condition(client: 'FlaskClient[Response]') -> None: + # A boolean operator inside `when_all` would need Code Editor; the builder is AND-only via row repetition. + source = "A = Rule(when_all=[PostText == 'a' and EventType == 'create_post'], description='a')\n" + res = client.post( + url_for('rule_drafts.parse_into_builder'), + json={'path': 'rules/complex.sml', 'source': source}, + ) + assert res.status_code == 200 + body = res.json + assert body is not None + assert body['supported'] is False + assert 'Rule Builder cannot represent' in body['reason'] + + +@pytest.mark.use_rules_sources(_base_sources) +def test_parse_into_builder_rejects_file_with_no_rule(client: 'FlaskClient[Response]') -> None: + source = "Import(rules=['models/post.sml'])\n" + res = client.post( + url_for('rule_drafts.parse_into_builder'), + json={'path': 'rules/empty.sml', 'source': source}, + ) + assert res.status_code == 200 + body = res.json + assert body is not None + assert body['supported'] is False + assert 'no Rule(...)' in body['reason'] + + +@pytest.mark.use_rules_sources(_base_sources) +def test_parse_into_builder_rejects_syntax_error(client: 'FlaskClient[Response]') -> None: + res = client.post( + url_for('rule_drafts.parse_into_builder'), + json={'path': 'rules/broken.sml', 'source': 'this is not valid SML at all !!!'}, + ) + assert res.status_code == 200 + body = res.json + assert body is not None + assert body['supported'] is False + assert 'could not parse SML' in body['reason'] + + +@pytest.mark.use_rules_sources(_base_sources) +def test_unknown_backend_value_returns_500(client: 'FlaskClient[Response]', monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setenv('OSPREY_RULES_SUBMISSION_BACKEND', 'gerrit') + res = client.post( + url_for('rule_drafts.submit_draft'), + json={ + 'path': 'rules/new_rule.sml', + 'source': "Import(rules=['models/base.sml'])\nAnotherRule = Rule(when_all=[PostText == 'bye'], description='bye')", + 'rule_name': 'AnotherRule', + 'summary': '', + 'is_new_rule': True, + }, + ) + assert res.status_code == 500 + body = res.json + assert body is not None + assert "Unknown OSPREY_RULES_SUBMISSION_BACKEND 'gerrit'" in body['error'] + + +@pytest.mark.use_rules_sources(_base_sources) +def test_pending_returns_empty_list_for_null_backend( + client: 'FlaskClient[Response]', monkeypatch: pytest.MonkeyPatch +) -> None: + monkeypatch.delenv('OSPREY_RULES_SUBMISSION_BACKEND', raising=False) + res = client.get(url_for('rule_drafts.pending_drafts')) + assert res.status_code == 503 + body = res.json + assert body is not None + assert body['pending'] == [] + + +@pytest.mark.use_rules_sources(_base_sources) +def test_submit_github_enterprise_url_is_threaded_into_requests( + client: 'FlaskClient[Response]', monkeypatch: pytest.MonkeyPatch +) -> None: + """GitHub Enterprise customers point OSPREY_GITHUB_API_URL at their own host; every API call must use it.""" + _set_github_backend( + monkeypatch, + OSPREY_GITHUB_API_URL='https://github.acme.test/api/v3', + OSPREY_RULES_REPO='acme/rules', + ) + + repo_url = 'https://github.acme.test/api/v3/repos/acme/rules' + + with requests_mock_module.Mocker() as m: + m.get(f'{repo_url}/git/ref/heads/main', json={'object': {'sha': 'BASE_SHA'}}) + m.get(f'{repo_url}/contents/rules/new_rule.sml', status_code=404) + m.post(f'{repo_url}/git/refs', status_code=201, json={}) + m.put(f'{repo_url}/contents/rules/new_rule.sml', status_code=201, json={}) + m.post( + f'{repo_url}/pulls', + status_code=201, + json={'number': 5, 'html_url': 'https://github.acme.test/acme/rules/pull/5'}, + ) + + res = client.post( + url_for('rule_drafts.submit_draft'), + json={ + 'path': 'rules/new_rule.sml', + 'source': "Import(rules=['models/base.sml'])\nAnotherRule = Rule(when_all=[PostText == 'bye'], description='bye')", + 'rule_name': 'AnotherRule', + 'summary': '', + 'is_new_rule': True, + }, + ) + + assert res.status_code == 200 + body = res.json + assert body is not None + assert body['url'] == 'https://github.acme.test/acme/rules/pull/5' + # If any call had hit api.github.com instead, the Mocker would have raised NoMockAddress. + + +@pytest.mark.use_rules_sources(_base_sources) +def test_local_backend_writes_file_and_returns_no_url( + client: 'FlaskClient[Response]', monkeypatch: pytest.MonkeyPatch +) -> None: + with tempfile.TemporaryDirectory() as tmpdir: + rules_dir = Path(tmpdir) + (rules_dir / 'main.sml').write_text("Import(rules=['models/post.sml'])\n", encoding='utf-8') + + monkeypatch.setenv('OSPREY_RULES_SUBMISSION_BACKEND', 'local') + monkeypatch.setenv('OSPREY_RULES_LOCAL_PATH', str(rules_dir)) + + res = client.post( + url_for('rule_drafts.submit_draft'), + json={ + 'path': 'rules/new_rule.sml', + 'source': "Import(rules=['models/base.sml'])\nAnotherRule = Rule(when_all=[PostText == 'bye'], description='bye')", + 'rule_name': 'AnotherRule', + 'summary': 'add bye rule', + 'is_new_rule': True, + 'wire_into_main': False, + }, + ) + + assert res.status_code == 200 + body = res.json + assert body is not None + assert body['url'] is None + assert 'rules/new_rule.sml' in body['title'] + assert body['main_sml_updated'] is False + + written = (rules_dir / 'rules' / 'new_rule.sml').read_text(encoding='utf-8') + assert 'AnotherRule' in written + + +@pytest.mark.use_rules_sources(_base_sources) +def test_local_backend_wires_into_main_when_requested( + client: 'FlaskClient[Response]', monkeypatch: pytest.MonkeyPatch +) -> None: + with tempfile.TemporaryDirectory() as tmpdir: + rules_dir = Path(tmpdir) + (rules_dir / 'main.sml').write_text("Import(rules=['models/post.sml'])\n", encoding='utf-8') + + monkeypatch.setenv('OSPREY_RULES_SUBMISSION_BACKEND', 'local') + monkeypatch.setenv('OSPREY_RULES_LOCAL_PATH', str(rules_dir)) + + res = client.post( + url_for('rule_drafts.submit_draft'), + json={ + 'path': 'rules/new_rule.sml', + 'source': "Import(rules=['models/base.sml'])\nAnotherRule = Rule(when_all=[PostText == 'bye'], description='bye')", + 'rule_name': 'AnotherRule', + 'summary': '', + 'is_new_rule': True, + 'wire_into_main': True, + }, + ) + + assert res.status_code == 200 + body = res.json + assert body is not None + assert body['main_sml_updated'] is True + updated_main = (rules_dir / 'main.sml').read_text(encoding='utf-8') + assert "Require(rule='rules/new_rule.sml')" in updated_main + + +@pytest.mark.use_rules_sources(_base_sources) +def test_local_backend_rejects_path_traversal(client: 'FlaskClient[Response]', monkeypatch: pytest.MonkeyPatch) -> None: + with tempfile.TemporaryDirectory() as tmpdir: + rules_dir = Path(tmpdir) + monkeypatch.setenv('OSPREY_RULES_SUBMISSION_BACKEND', 'local') + monkeypatch.setenv('OSPREY_RULES_LOCAL_PATH', str(rules_dir)) + + # The view layer already rejects '..' in the path, so to actually exercise the + # local backend's own guard we bypass the view-level check by going around the + # API: instantiate the backend and call submit_draft directly with a traversal path. + from osprey.worker.ui_api.osprey.views._rule_drafts_backend import RuleDraftBackendError + from osprey.worker.ui_api.osprey.views._rule_drafts_local import LocalBackend, LocalConfig + + backend = LocalBackend(LocalConfig(rules_dir=rules_dir)) + with pytest.raises(RuleDraftBackendError) as exc_info: + backend.submit_draft( + draft_path='../escape.sml', + sml_source='x = 1', + rule_name='X', + summary='', + author_email='test@local', + is_new_rule=True, + wire_into_main=False, + ) + assert 'escapes the configured rules directory' in exc_info.value.message From 8a483fb143778f60240a6df2e7fd8be92d55e0df Mon Sep 17 00:00:00 2001 From: Juliet Shen Date: Mon, 13 Jul 2026 21:52:07 -0400 Subject: [PATCH 2/9] Replace rule-submission backends with a draft rules table Feedback on #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 Claude-Session: https://claude.ai/code/session_01TEcfpqhza3dPepZUVWw36X --- CHANGELOG.md | 1 + docs/user/manage.md | 38 +- .../src/osprey/worker/lib/storage/postgres.py | 1 + .../osprey/worker/lib/storage/rule_drafts.py | 110 +++ .../osprey/views/_rule_drafts_backend.py | 142 ---- .../osprey/views/_rule_drafts_git_common.py | 62 -- .../osprey/views/_rule_drafts_github.py | 311 ------- .../ui_api/osprey/views/_rule_drafts_local.py | 110 --- .../ui_api/osprey/views/_rule_drafts_null.py | 41 - .../worker/ui_api/osprey/views/rule_drafts.py | 202 +++-- .../osprey/views/tests/test_rule_drafts.py | 766 ++++-------------- 11 files changed, 422 insertions(+), 1362 deletions(-) create mode 100644 osprey_worker/src/osprey/worker/lib/storage/rule_drafts.py delete mode 100644 osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_backend.py delete mode 100644 osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_git_common.py delete mode 100644 osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_github.py delete mode 100644 osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_local.py delete mode 100644 osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_null.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 0715b425..10473d2e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,7 @@ For more information about each release including git tags and artifacts, see [R - Per-action health metrics in the executor ([#191](https://github.com/roostorg/osprey/pull/191) by [@cmttt](https://github.com/cmttt)) - Option to suppress cached errors to reduce metric bloat ([#180](https://github.com/roostorg/osprey/pull/180) by [@lithium-powered](https://github.com/lithium-powered)) - Experimental asyncio-native worker with metrics and engine/coordinator improvements ([#341](https://github.com/roostorg/osprey/pull/341) by [@cmttt](https://github.com/cmttt)) +- Experimental in-app rule authoring: draft SML rules validate against the live engine, save to a `rule_drafts` table, and deploy into the configured rules directory ([#402](https://github.com/roostorg/osprey/pull/402) by [@julietshen](https://github.com/julietshen)) ### Changed diff --git a/docs/user/manage.md b/docs/user/manage.md index d5ef0642..4d1c54ea 100644 --- a/docs/user/manage.md +++ b/docs/user/manage.md @@ -59,39 +59,25 @@ Each row shows the rule's name, source file, description, reference count, and l ## Rule Authoring (Experimental feature) -Users can draft SML rules directly in the UI. Submit opens a review unit against a configured git remote so authoring, review, and merge use the same tools users already have. +Users can draft SML rules directly in the UI. Drafts are saved to a `rule_drafts` table so the people who operate Osprey can reference, edit, and deploy them without any external code host. -The editor validates every keystroke against the same AST validator the running engine uses, so compile-time errors surface before the pull request opens. The Rule Builder view expresses the common shape (name, conditions, outcomes) as a form and generates SML; the Code Editor view accepts arbitrary SML for anything the builder can't represent. +The editor validates every keystroke against the same AST validator the running engine uses, so compile-time errors surface before a draft is saved. The Rule Builder view expresses the common shape (name, conditions, outcomes) as a form and generates SML; the Code Editor view accepts arbitrary SML for anything the builder can't represent. -### Rule submission backends +### The draft rules table -The Submit button routes drafts through a pluggable backend. Pick one for your deployment by setting `OSPREY_RULES_SUBMISSION_BACKEND` on the `osprey-ui-api` process: +Drafts live in a Postgres table (`rule_drafts`), one row per rule file path. The API (all gated by the `CAN_EDIT_RULE_DRAFTS` ability, granted to `super_user`): -| Value | What it does | Required env vars | -|---|---|---| -| `null` (default) | Returns 503 on any submit or list call. Ships as the default so an unconfigured install never writes anything. | none | -| `github` | Opens a pull request on a configured repo. Works with github.com and GitHub Enterprise. | `OSPREY_RULES_REPO`, `OSPREY_GITHUB_TOKEN` (+ optionals) | -| `local` | Writes SML directly to a mounted directory. For self-hosted setups whose deploy pipeline already syncs a rules directory into the engine. | `OSPREY_RULES_LOCAL_PATH` | - -Env vars shared across every backend that targets a git host: +- `POST /rule-drafts` — re-validates the SML server-side, then upserts the draft. +- `GET /rule-drafts` — lists every draft (the table operators work from). +- `GET /rule-drafts/` — fetches a single draft. +- `POST /rule-drafts//deploy` — re-validates, writes the SML into the rules directory, and marks the draft `deployed`. Pass `wire_into_main: true` to also append a `Require(rule=...)` line to `main.sml` so the rule takes effect (a rule file is inert until something requires it). -- `OSPREY_RULES_BASE_BRANCH` (default `main`) — the branch the review targets. -- `OSPREY_RULES_PATH_IN_REPO` (default empty) — subdirectory inside the target repo where rule files live, e.g. `example_rules`. Leave empty if rules sit at the repo root. +### Deploying -#### `github` +Deploy writes the draft's SML into a rules directory that the engine's sources provider already loads (a filesystem hand-off: whatever pipeline syncs that directory activates the rule). | Var | Default | Notes | |---|---|---| -| `OSPREY_RULES_REPO` | _required_ | `owner/name` of the repo to PR against. | -| `OSPREY_GITHUB_TOKEN` | _required_ | Fine-grained PAT with `Contents: read/write` and `Pull requests: read/write` on the repo. | -| `OSPREY_GITHUB_API_URL` | `https://api.github.com` | Set for GitHub Enterprise: e.g. `https://github.acme.example/api/v3`. | - -#### `local` - -| Var | Default | Notes | -|---|---|---| -| `OSPREY_RULES_LOCAL_PATH` | _required_ | Absolute path to the directory the backend writes SML into. Must already exist. Submissions take effect immediately; there's no review queue. | - -### Adding a rule submission backend +| `OSPREY_RULES_LOCAL_PATH` | _required for deploy_ | Absolute path to the rules directory the engine loads. Deploy writes SML here; must already exist. If unset, `POST /deploy` returns 503 (drafting and validation still work). | -Add a Python module next to `_rule_drafts_github.py` that implements the `RuleSubmissionBackend` Protocol defined in `_rule_drafts_backend.py`, then wire it into `load_backend()`. See the module docstring on `_rule_drafts_backend.py` for the contract; the existing HTTP-backed module (`_rule_drafts_github.py`) is a working template. +> **Future direction:** a DB-backed `SourcesProvider` could let the engine load deployed drafts straight from the `rule_drafts` table, removing the filesystem hand-off and making rule management work with zero external infrastructure. This PR keeps the filesystem deploy; the table is already the source of truth for drafts. diff --git a/osprey_worker/src/osprey/worker/lib/storage/postgres.py b/osprey_worker/src/osprey/worker/lib/storage/postgres.py index b0f67045..fb302e1b 100644 --- a/osprey_worker/src/osprey/worker/lib/storage/postgres.py +++ b/osprey_worker/src/osprey/worker/lib/storage/postgres.py @@ -56,6 +56,7 @@ def _init(config: Config) -> None: bulk_label_task, pg_stored_execution, queries, + rule_drafts, temporary_ability_token, ) diff --git a/osprey_worker/src/osprey/worker/lib/storage/rule_drafts.py b/osprey_worker/src/osprey/worker/lib/storage/rule_drafts.py new file mode 100644 index 00000000..ac520021 --- /dev/null +++ b/osprey_worker/src/osprey/worker/lib/storage/rule_drafts.py @@ -0,0 +1,110 @@ +from __future__ import annotations + +from datetime import datetime, timezone +from enum import StrEnum + +from sqlalchemy import BigInteger, Column, DateTime, Enum, Text + +from .postgres import Model, scoped_session + + +def _now() -> datetime: + return datetime.now(timezone.utc) + + +class RuleDraftStatus(StrEnum): + DRAFT = 'draft' + DEPLOYED = 'deployed' + + +class RuleDraft(Model): + """A staged SML rule draft. + + Drafts are authored and validated in the UI and live here so the people who + operate Osprey can reference, edit, and deploy them without any external code + host. Deploying writes the SML into the configured rules directory; the draft + row stays as the record of what was deployed. + + One row per rule `path` (upserted on submit), so the table reads as the + current set of drafts rather than an append-only history. + """ + + __tablename__ = 'rule_drafts' + + 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) + summary: str = Column(Text, nullable=False, default='') + author_email: str = Column(Text, nullable=False) + status: RuleDraftStatus = Column( + Enum(RuleDraftStatus, native_enum=False, length=32), + nullable=False, + default=RuleDraftStatus.DRAFT, + ) + 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] + + def to_json(self) -> dict[str, object]: + return { + 'id': self.id, + 'path': self.path, + 'rule_name': self.rule_name, + 'source': self.sml_source, + 'summary': self.summary, + 'author': self.author_email, + 'status': str(self.status), + 'created_at': self.created_at.isoformat() if self.created_at else None, + 'updated_at': self.updated_at.isoformat() if self.updated_at else None, + 'deployed_at': self.deployed_at.isoformat() if self.deployed_at else None, + } + + @classmethod + def upsert(cls, *, path: str, rule_name: str, sml_source: str, summary: str, author_email: str) -> 'RuleDraft': + """Create the draft for `path`, or update it in place if one already exists. + + Editing a deployed draft moves it back to `DRAFT` so the table reflects + that the in-flight SML no longer matches what was last deployed. + """ + with scoped_session(commit=True) as session: + draft = session.query(cls).filter(cls.path == path).first() + if draft is None: + draft = cls(path=path) + session.add(draft) + draft.rule_name = rule_name + draft.sml_source = sml_source + draft.summary = summary + draft.author_email = author_email + draft.status = RuleDraftStatus.DRAFT + session.flush() + session.expunge(draft) + return draft + + @classmethod + def list_all(cls) -> list['RuleDraft']: + with scoped_session() as session: + drafts = session.query(cls).order_by(cls.updated_at.desc()).all() + for draft in drafts: + session.expunge(draft) + return drafts + + @classmethod + def get_one(cls, draft_id: int) -> 'RuleDraft | None': + with scoped_session() as session: + draft = session.query(cls).filter(cls.id == draft_id).first() + if draft is not None: + session.expunge(draft) + return draft + + @classmethod + def mark_deployed(cls, draft_id: int) -> 'RuleDraft | None': + with scoped_session(commit=True) as session: + draft = session.query(cls).filter(cls.id == draft_id).first() + if draft is None: + return None + draft.status = RuleDraftStatus.DEPLOYED + draft.deployed_at = _now() + session.flush() + session.expunge(draft) + return draft diff --git a/osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_backend.py b/osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_backend.py deleted file mode 100644 index 755f1eb3..00000000 --- a/osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_backend.py +++ /dev/null @@ -1,142 +0,0 @@ -"""Backend abstraction for rule-draft submission. - -The Osprey engine doesn't care where rules live. Different deployments use -different hosting: GitHub or Enterprise, GitLab, Tangled, an internal Gerrit, -or a filesystem on a shared volume. Each is one implementation of the -RuleSubmissionBackend Protocol below. - -`load_backend()` reads `OSPREY_RULES_SUBMISSION_BACKEND` and instantiates the -chosen backend with its own env vars. Defaults to `null` so an unconfigured -install ships safe; adopters opt into a backend explicitly. - -Adopter docs (env vars per backend, how to choose one): see -`docs/user/manage.md`. - -Adding a new backend: implement a class with `submit_draft` and -`list_pending_drafts` matching the Protocol below, add a case in -`load_backend()`, and update the "unknown backend" error message here plus -the "no backend configured" message in `_rule_drafts_null.py`. The existing -`_rule_drafts_github.py` module is a working template for HTTP-backed -adapters; `_rule_drafts_local.py` for filesystem. -""" - -from __future__ import annotations - -import os -from dataclasses import dataclass, field -from typing import Any, Protocol - - -class RuleDraftBackendError(Exception): - """Raised by any backend method when the operation cannot complete.""" - - def __init__(self, message: str, status_code: int = 502): - super().__init__(message) - self.message = message - self.status_code = status_code - - -@dataclass(frozen=True) -class SubmissionResult: - """Backend-neutral submit_draft return value. - - `title` and `url` are what the UI surfaces in the success banner; `extras` - carries backend-specific fields (PR number, branch, etc.) for adopters - whose UI variants want to render more detail. - """ - - title: str - url: str | None - main_sml_updated: bool = False - extras: dict[str, Any] = field(default_factory=dict) - - def to_json(self) -> dict[str, Any]: - # Spread extras first so the canonical fields always win: a backend that - # happens to name an extra `title`/`url`/`main_sml_updated` can't shadow - # the contract fields the UI depends on. - return { - **self.extras, - 'title': self.title, - 'url': self.url, - 'main_sml_updated': self.main_sml_updated, - } - - -@dataclass(frozen=True) -class PendingDraft: - """Backend-neutral entry for the pending-drafts list.""" - - title: str - url: str - author: str - created_at: str - touched_files: list[str] - extras: dict[str, Any] = field(default_factory=dict) - - def to_json(self) -> dict[str, Any]: - # Spread extras first so backend-specific keys can't shadow the - # canonical fields the UI depends on. - return { - **self.extras, - 'title': self.title, - 'url': self.url, - 'author': self.author, - 'created_at': self.created_at, - 'touched_files': self.touched_files, - } - - -class RuleSubmissionBackend(Protocol): - """The contract every submission backend implements. - - Implementations: - - submit a draft (create whatever the backend's review unit is) - - optionally wire the new rule into main.sml as part of the same submission - - list whatever's currently in review - - Implementations raise `RuleDraftBackendError` for any failure path. - """ - - name: str - - 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: ... - - def list_pending_drafts(self) -> list[PendingDraft]: ... - - -def load_backend() -> RuleSubmissionBackend: - """Select and instantiate the configured backend. - - `OSPREY_RULES_SUBMISSION_BACKEND` picks one of: github, local, null. - Unset or empty defaults to `null`. Unknown values raise so a typo doesn't - silently degrade to no-op submission. - """ - name = (os.environ.get('OSPREY_RULES_SUBMISSION_BACKEND') or 'null').strip().lower() - - # Imports are deferred to keep the Protocol module dependency-free. - if name == 'null': - from ._rule_drafts_null import NullBackend - - return NullBackend() - if name == 'github': - from ._rule_drafts_github import GitHubBackend - - return GitHubBackend.from_env() - if name == 'local': - from ._rule_drafts_local import LocalBackend - - return LocalBackend.from_env() - raise RuleDraftBackendError( - f'Unknown OSPREY_RULES_SUBMISSION_BACKEND {name!r}; valid values are github, local, null.', - status_code=500, - ) diff --git a/osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_git_common.py b/osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_git_common.py deleted file mode 100644 index 5f9ad575..00000000 --- a/osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_git_common.py +++ /dev/null @@ -1,62 +0,0 @@ -"""Shared helpers for git-forge submission backends. - -The GitHub, GitLab, and Tangled adapters all need the same three things: a -cosmetic branch name, a check for whether main.sml already wires a rule in, and -the append that adds the wiring. They also all talk to a remote over HTTP and -must turn a dropped connection into the same structured error the UI renders -rather than an unhandled 500. This module is the one place those live. -""" - -from __future__ import annotations - -import re -import time -from typing import Any - -import requests - -from ._rule_drafts_backend import RuleDraftBackendError - -DEFAULT_TIMEOUT_SECONDS = 15 - - -def request(method: str, url: str, *, error_action: str, **kwargs: Any) -> requests.Response: - """Issue an HTTP request, converting transport failures to RuleDraftBackendError. - - A forge outage (connection refused, DNS failure, timeout) is an expected - operational state for a backend whose job is talking to a remote host, so it - should surface as the 502 JSON shape the editor knows how to display, not as - an unhandled Flask 500. HTTP status errors are left for the caller to map, - since the right status code depends on what was being attempted. - """ - kwargs.setdefault('timeout', DEFAULT_TIMEOUT_SECONDS) - try: - return requests.request(method, url, **kwargs) - except requests.RequestException as exc: - raise RuleDraftBackendError( - f'Could not reach the git host while {error_action}: {exc}', - status_code=502, - ) from exc - - -def generate_branch_name(rule_name: str, author_email: str, *, prefix: str = 'rule-draft') -> str: - """Cosmetic source-branch label. Timestamped so retries don't collide.""" - short_email = author_email.split('@', 1)[0] - slug = re.sub(r'[^A-Za-z0-9_-]+', '-', short_email).strip('-') or 'osprey-ui' - rule_slug = re.sub(r'[^A-Za-z0-9_-]+', '-', rule_name).strip('-') or 'rule' - return f'{prefix}/{slug}/{rule_slug}-{int(time.time())}' - - -def require_already_present(main_sml: str, draft_path: str) -> bool: - pattern = re.compile( - r"Require\s*\(\s*rule\s*=\s*['\"]" + re.escape(draft_path) + r"['\"]\s*\)", - re.MULTILINE, - ) - return bool(pattern.search(main_sml)) - - -def append_require_to_main(main_sml: str, draft_path: str) -> str: - suffix = f"\nRequire(rule='{draft_path}')\n" - if not main_sml.endswith('\n'): - suffix = '\n' + suffix - return main_sml + suffix diff --git a/osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_github.py b/osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_github.py deleted file mode 100644 index 1e9678e6..00000000 --- a/osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_github.py +++ /dev/null @@ -1,311 +0,0 @@ -"""GitHub submission backend. - -Implements RuleSubmissionBackend by opening a pull request against a configured -repo. Works with github.com and with GitHub Enterprise (set OSPREY_GITHUB_API_URL -to the Enterprise API root, e.g. https://github.mycompany.com/api/v3). -""" - -from __future__ import annotations - -import base64 -import os -from dataclasses import dataclass -from typing import Any - -import requests - -from . import _rule_drafts_git_common as git_common -from ._rule_drafts_backend import ( - PendingDraft, - RuleDraftBackendError, - SubmissionResult, -) - -DEFAULT_GITHUB_API = 'https://api.github.com' - - -@dataclass(frozen=True) -class GitHubConfig: - api_url: str - repo: str - base_branch: str - rules_path: str - token: str - - @property - def repo_url(self) -> str: - return f'{self.api_url.rstrip("/")}/repos/{self.repo}' - - @classmethod - def from_env(cls) -> 'GitHubConfig': - api_url = (os.environ.get('OSPREY_GITHUB_API_URL') or DEFAULT_GITHUB_API).strip() - repo = os.environ.get('OSPREY_RULES_REPO', '').strip() - base = os.environ.get('OSPREY_RULES_BASE_BRANCH', 'main').strip() or 'main' - rules_path = os.environ.get('OSPREY_RULES_PATH_IN_REPO', '').strip().strip('/') - token = os.environ.get('OSPREY_GITHUB_TOKEN', '').strip() - if not repo: - raise RuleDraftBackendError( - 'OSPREY_RULES_REPO is not configured; set it to "owner/name" to enable PR submission.', - status_code=503, - ) - if not token: - raise RuleDraftBackendError( - 'OSPREY_GITHUB_TOKEN is not configured; set a service-account PAT with repo write access.', - status_code=503, - ) - return cls(api_url=api_url, repo=repo, base_branch=base, rules_path=rules_path, token=token) - - -def _headers(cfg: GitHubConfig) -> dict[str, str]: - return { - 'Authorization': f'Bearer {cfg.token}', - 'Accept': 'application/vnd.github+json', - 'X-GitHub-Api-Version': '2022-11-28', - } - - -def _full_path(cfg: GitHubConfig, draft_path: str) -> str: - draft_path = draft_path.lstrip('/') - if cfg.rules_path: - return f'{cfg.rules_path}/{draft_path}' - return draft_path - - -def _get(cfg: GitHubConfig, url: str, **kwargs: Any) -> requests.Response: - return git_common.request('GET', url, error_action='contacting GitHub', headers=_headers(cfg), **kwargs) - - -def _post(cfg: GitHubConfig, url: str, json: dict[str, Any]) -> requests.Response: - return git_common.request('POST', url, error_action='contacting GitHub', headers=_headers(cfg), json=json) - - -def _put(cfg: GitHubConfig, url: str, json: dict[str, Any]) -> requests.Response: - return git_common.request('PUT', url, error_action='contacting GitHub', headers=_headers(cfg), json=json) - - -def _raise_for_github(response: requests.Response, action: str) -> None: - if response.ok: - return - body = response.text[:500] - # 4xx from GitHub is still a backend failure from the caller's POV. - raise RuleDraftBackendError( - f'GitHub returned {response.status_code} while {action}: {body}', - status_code=502, - ) - - -def _get_base_sha(cfg: GitHubConfig) -> str: - res = _get(cfg, f'{cfg.repo_url}/git/ref/heads/{cfg.base_branch}') - _raise_for_github(res, f'looking up base branch {cfg.base_branch!r}') - return res.json()['object']['sha'] - - -def _get_file_sha(cfg: GitHubConfig, path_in_repo: str, ref: str) -> str | None: - res = _get(cfg, f'{cfg.repo_url}/contents/{path_in_repo}', params={'ref': ref}) - if res.status_code == 404: - return None - _raise_for_github(res, f'reading {path_in_repo!r} on {ref!r}') - payload = res.json() - if isinstance(payload, list): - return None - return payload.get('sha') - - -def _create_branch(cfg: GitHubConfig, branch: str, sha: str) -> None: - res = _post(cfg, f'{cfg.repo_url}/git/refs', json={'ref': f'refs/heads/{branch}', 'sha': sha}) - if res.status_code == 422: - raise RuleDraftBackendError( - f'Branch {branch!r} already exists on {cfg.repo}. Pick a different name.', - status_code=409, - ) - _raise_for_github(res, f'creating branch {branch!r}') - - -def _commit_file( - cfg: GitHubConfig, - branch: str, - path_in_repo: str, - contents: str, - message: str, - existing_sha: str | None, -) -> None: - payload: dict[str, Any] = { - 'message': message, - 'content': base64.b64encode(contents.encode('utf-8')).decode('ascii'), - 'branch': branch, - } - if existing_sha is not None: - payload['sha'] = existing_sha - res = _put(cfg, f'{cfg.repo_url}/contents/{path_in_repo}', json=payload) - _raise_for_github(res, f'committing {path_in_repo!r} to {branch!r}') - - -def _open_pr(cfg: GitHubConfig, branch: str, title: str, body: str) -> dict[str, Any]: - res = _post( - cfg, - f'{cfg.repo_url}/pulls', - json={'title': title, 'head': branch, 'base': cfg.base_branch, 'body': body}, - ) - _raise_for_github(res, f'opening PR from {branch!r}') - return res.json() - - -def _main_sml_path(cfg: GitHubConfig) -> str: - return _full_path(cfg, 'main.sml') - - -def _fetch_file_on_ref(cfg: GitHubConfig, path_in_repo: str, ref: str) -> tuple[str, str] | None: - res = _get(cfg, f'{cfg.repo_url}/contents/{path_in_repo}', params={'ref': ref}) - if res.status_code == 404: - return None - _raise_for_github(res, f'reading {path_in_repo!r} on {ref!r}') - payload = res.json() - if isinstance(payload, list): - return None - encoded = payload.get('content', '') - sha = payload.get('sha') - if sha is None: - return None - try: - decoded = base64.b64decode(encoded).decode('utf-8') - except Exception as exc: - raise RuleDraftBackendError(f'could not decode {path_in_repo!r}: {exc}', status_code=502) - return decoded, sha - - -class GitHubBackend: - name = 'github' - - def __init__(self, cfg: GitHubConfig): - self._cfg = cfg - - @classmethod - def from_env(cls) -> 'GitHubBackend': - return cls(GitHubConfig.from_env()) - - 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: - cfg = self._cfg - path_in_repo = _full_path(cfg, draft_path) - base_sha = _get_base_sha(cfg) - existing_sha = _get_file_sha(cfg, path_in_repo, cfg.base_branch) - - if is_new_rule and existing_sha is not None: - raise RuleDraftBackendError( - f'A file already exists at {path_in_repo!r} on {cfg.base_branch}. ' - 'Pick a different filename, or edit the existing rule instead of creating a new one.', - status_code=409, - ) - - branch = git_common.generate_branch_name(rule_name, author_email) - _create_branch(cfg, branch, base_sha) - - verb = 'Add' if is_new_rule else 'Update' - commit_message = f'{verb} rule {rule_name}\n\nAuthored via Osprey UI by {author_email}.' - _commit_file( - cfg, - branch=branch, - path_in_repo=path_in_repo, - contents=sml_source, - message=commit_message, - existing_sha=existing_sha, - ) - - main_sml_updated = False - if wire_into_main: - main_path = _main_sml_path(cfg) - fetched = _fetch_file_on_ref(cfg, main_path, cfg.base_branch) - if fetched is None: - raise RuleDraftBackendError( - f'wire_into_main requested but {main_path!r} does not exist on {cfg.base_branch}.', - status_code=409, - ) - main_contents, main_sha = fetched - if not git_common.require_already_present(main_contents, draft_path): - new_main = git_common.append_require_to_main(main_contents, draft_path) - _commit_file( - cfg, - branch=branch, - path_in_repo=main_path, - contents=new_main, - message=f'Wire {rule_name} into main.sml\n\nAuthored via Osprey UI by {author_email}.', - existing_sha=main_sha, - ) - main_sml_updated = True - - title = f'{verb} rule {rule_name}' - touched = f'`{path_in_repo}`' - if main_sml_updated: - touched += f', `{_main_sml_path(cfg)}`' - body = ( - f'{summary.strip() or "_(no summary provided)_"}\n\n' - f'---\n' - f'Drafted in the Osprey rules UI by `{author_email}`.\n' - f'Touches: {touched}.' - ) - pr = _open_pr(cfg, branch=branch, title=title, body=body) - pr_number = pr.get('number') - pr_url = pr.get('html_url') - return SubmissionResult( - title=f'Pull request #{pr_number} opened', - url=pr_url, - main_sml_updated=main_sml_updated, - extras={ - 'pr_number': pr_number, - 'pr_url': pr_url, - 'branch': branch, - 'path_in_repo': path_in_repo, - }, - ) - - 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 diff --git a/osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_local.py b/osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_local.py deleted file mode 100644 index 2d815eb6..00000000 --- a/osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_local.py +++ /dev/null @@ -1,110 +0,0 @@ -"""Filesystem submission backend for self-hosted setups. - -Writes the SML straight to a configured rules directory. No review, no PR. -Adopters whose deploy pipeline already syncs a rules directory into the engine -(etcd push, file watcher, etc.) wire this backend up so the UI drops the SML -in the right place and lets the downstream pipeline take it from there. -""" - -from __future__ import annotations - -import os -from dataclasses import dataclass -from pathlib import Path - -from . import _rule_drafts_git_common as git_common -from ._rule_drafts_backend import ( - PendingDraft, - RuleDraftBackendError, - SubmissionResult, -) - - -@dataclass(frozen=True) -class LocalConfig: - rules_dir: Path - - @classmethod - def from_env(cls) -> 'LocalConfig': - raw = os.environ.get('OSPREY_RULES_LOCAL_PATH', '').strip() - if not raw: - raise RuleDraftBackendError( - 'OSPREY_RULES_LOCAL_PATH is not configured; set it to the directory the local backend should write to.', - status_code=503, - ) - path = Path(raw) - if not path.is_dir(): - raise RuleDraftBackendError( - f'OSPREY_RULES_LOCAL_PATH {raw!r} is not a directory.', - status_code=503, - ) - return cls(rules_dir=path) - - -class LocalBackend: - name = 'local' - - def __init__(self, cfg: LocalConfig): - self._cfg = cfg - - @classmethod - def from_env(cls) -> 'LocalBackend': - return cls(LocalConfig.from_env()) - - def _resolve(self, draft_path: str) -> Path: - """Resolve a draft path within rules_dir, refusing anything that would - escape via `..` or symlink traversal.""" - candidate = (self._cfg.rules_dir / draft_path).resolve() - try: - candidate.relative_to(self._cfg.rules_dir.resolve()) - except ValueError as exc: - raise RuleDraftBackendError( - f'Draft path {draft_path!r} escapes the configured rules directory.', - status_code=400, - ) from exc - return candidate - - 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 list_pending_drafts(self) -> list[PendingDraft]: - # Local backend has no review queue; submissions take effect immediately. - return [] diff --git a/osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_null.py b/osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_null.py deleted file mode 100644 index 8ee303ab..00000000 --- a/osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_null.py +++ /dev/null @@ -1,41 +0,0 @@ -"""Default submission backend: nothing is configured, so every call fails fast. - -Ships as the default so an unconfigured upstream install never opens a PR or -writes a file without an adopter explicitly opting into a backend. -""" - -from __future__ import annotations - -from ._rule_drafts_backend import ( - PendingDraft, - RuleDraftBackendError, - SubmissionResult, -) - - -class NullBackend: - name = 'null' - - @staticmethod - def _err() -> RuleDraftBackendError: - return RuleDraftBackendError( - 'No rule-submission backend is configured. Set OSPREY_RULES_SUBMISSION_BACKEND ' - 'to one of: github, local. See docs for what each one needs.', - status_code=503, - ) - - 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: - raise self._err() - - def list_pending_drafts(self) -> list[PendingDraft]: - raise self._err() diff --git a/osprey_worker/src/osprey/worker/ui_api/osprey/views/rule_drafts.py b/osprey_worker/src/osprey/worker/ui_api/osprey/views/rule_drafts.py index 588a9942..f0edc5e1 100644 --- a/osprey_worker/src/osprey/worker/ui_api/osprey/views/rule_drafts.py +++ b/osprey_worker/src/osprey/worker/ui_api/osprey/views/rule_drafts.py @@ -1,8 +1,10 @@ from __future__ import annotations import logging +import os import re from collections.abc import Iterable +from pathlib import Path from typing import Any from flask import Blueprint, jsonify, request @@ -30,10 +32,10 @@ ValidationWarning, ) from osprey.worker.lib.singletons import ENGINE +from osprey.worker.lib.storage.rule_drafts import RuleDraft from osprey.worker.ui_api.osprey.lib.abilities import CanEditRuleDrafts, require_ability from osprey.worker.ui_api.osprey.lib.auth import get_current_user_email -from . import _rule_drafts_backend as backends from ._engine_ast_utils import get_func_identifier logger = logging.getLogger(__name__) @@ -307,29 +309,50 @@ def vocabulary() -> Any: ) -@blueprint.route('/rule-drafts/submit', methods=['POST']) +def _revalidate(path: str, source_text: str) -> tuple[Any, int] | None: + """Re-run the engine's AST validation with the draft spliced into the loaded + sources. Returns a Flask error tuple on failure, or None if it validates.""" + spliced = _current_sources_dict() + spliced[path] = source_text + try: + sources = Sources.from_dict(spliced) + validate_sources( + sources, + udf_registry=ENGINE.instance().udf_registry, + validator_registry=ENGINE.instance().validator_registry, + ) + except ValidationFailed as exc: + return jsonify( + { + 'error': 'Validation failed; fix errors before submitting.', + 'errors': [_format_validation_message(e) for e in exc.errors], + } + ), 400 + except Exception as exc: + return jsonify({'error': f'Could not assemble sources: {exc}'}), 400 + return None + + +@blueprint.route('/rule-drafts', methods=['POST']) @require_ability(CanEditRuleDrafts) -def submit_draft() -> Any: +def create_draft() -> Any: + """Validate a draft and upsert it into the rule_drafts table (one row per path).""" payload = request.get_json(silent=True) or {} path = (payload.get('path') or '').strip() source_text = payload.get('source', '') rule_name = (payload.get('rule_name') or '').strip() summary = (payload.get('summary') or '').strip() - is_new_rule = bool(payload.get('is_new_rule', False)) - wire_into_main = bool(payload.get('wire_into_main', False)) path_err = _validate_path(path) if path_err: return jsonify({'error': path_err}), 400 if path == 'main.sml': - # main.sml is the engine entry point. Submitting a draft *as* main.sml - # would wholesale-replace it (immediate live effect on the local - # backend). Wiring a rule in is a controlled one-line append handled by - # the wire_into_main option, not a draft submission. + # main.sml is the engine entry point; a draft never replaces it wholesale. + # Deploying a draft optionally wires it into main.sml with a single Require line. return jsonify( { - 'error': 'main.sml is the engine entry point and cannot be submitted as a draft. ' - 'Use "turn this rule on" to add a Require line instead.' + 'error': 'main.sml is the engine entry point and cannot be saved as a draft. ' + 'Deploy a rule with wire_into_main to add a Require line instead.' } ), 400 if not isinstance(source_text, str) or not source_text.strip(): @@ -337,52 +360,131 @@ def submit_draft() -> Any: if not _VALID_RULE_NAME.match(rule_name): return jsonify({'error': 'rule_name must be a valid SML identifier ([A-Za-z_][A-Za-z0-9_]*).'}), 400 - # Re-validate server-side so a client that skips the validate step still cannot push uncompilable SML. - spliced = _current_sources_dict() - spliced[path] = source_text - try: - sources = Sources.from_dict(spliced) - validate_sources( - sources, - udf_registry=ENGINE.instance().udf_registry, - validator_registry=ENGINE.instance().validator_registry, + # Re-validate server-side so a client that skips the validate step still cannot store uncompilable SML. + error = _revalidate(path, source_text) + if error is not None: + return error + + draft = RuleDraft.upsert( + path=path, + rule_name=rule_name, + sml_source=source_text, + summary=summary, + author_email=get_current_user_email(), + ) + return jsonify(draft.to_json()) + + +@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()]}) + + +@blueprint.route('/rule-drafts/', methods=['GET']) +@require_ability(CanEditRuleDrafts) +def get_draft(draft_id: int) -> Any: + draft = RuleDraft.get_one(draft_id) + if draft is None: + return jsonify({'error': f'No draft with id {draft_id}.'}), 404 + return jsonify(draft.to_json()) + + +def _rules_dir_or_error() -> tuple[Path | None, tuple[Any, int] | None]: + """The directory deploy writes into (OSPREY_RULES_LOCAL_PATH), or an error tuple. + + Deploying into the engine's own rules source is a filesystem hand-off, the same + contract the retired `local` backend used: whatever pipeline already syncs that + directory (etcd push, file watcher) activates the rule. A DB-backed + 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() + if not raw: + return None, ( + jsonify( + { + 'error': 'Deploy is not configured. Set OSPREY_RULES_LOCAL_PATH to the rules ' + 'directory the engine loads so deployed drafts are written there.' + } + ), + 503, ) - except ValidationFailed as exc: - return jsonify( - { - 'error': 'Validation failed; fix errors before submitting.', - 'errors': [_format_validation_message(e) for e in exc.errors], - } - ), 400 - except Exception as exc: - return jsonify({'error': f'Could not assemble sources: {exc}'}), 400 + rules_dir = Path(raw) + if not rules_dir.is_dir(): + return None, (jsonify({'error': f'OSPREY_RULES_LOCAL_PATH {raw!r} is not a directory.'}), 503) + return rules_dir, None + +def _resolve_within(rules_dir: Path, draft_path: str) -> Path | None: + """Resolve draft_path inside rules_dir, or None if it would escape via `..`/symlink.""" + candidate = (rules_dir / draft_path).resolve() try: - backend = backends.load_backend() - result = backend.submit_draft( - draft_path=path, - sml_source=source_text, - rule_name=rule_name, - summary=summary, - author_email=get_current_user_email(), - is_new_rule=is_new_rule, - wire_into_main=wire_into_main, - ) - except backends.RuleDraftBackendError as exc: - return jsonify({'error': exc.message}), exc.status_code + candidate.relative_to(rules_dir.resolve()) + except ValueError: + return None + return candidate + + +def _main_requires(main_sml: str, draft_path: str) -> bool: + pattern = re.compile(r"Require\s*\(\s*rule\s*=\s*['\"]" + re.escape(draft_path) + r"['\"]\s*\)", re.MULTILINE) + return bool(pattern.search(main_sml)) - return jsonify(result.to_json()) +def _append_require(main_sml: str, draft_path: str) -> str: + suffix = f"\nRequire(rule='{draft_path}')\n" + if main_sml and not main_sml.endswith('\n'): + suffix = '\n' + suffix + return main_sml + suffix -@blueprint.route('/rule-drafts/pending', methods=['GET']) + +@blueprint.route('/rule-drafts//deploy', methods=['POST']) @require_ability(CanEditRuleDrafts) -def pending_drafts() -> Any: - try: - backend = backends.load_backend() - drafts = backend.list_pending_drafts() - except backends.RuleDraftBackendError as exc: - return jsonify({'error': exc.message, 'pending': []}), exc.status_code - return jsonify({'pending': [d.to_json() for d in drafts]}) +def deploy_draft(draft_id: int) -> Any: + """Write a draft's SML into the configured rules directory and mark it deployed. + + With `wire_into_main`, also append a `Require(rule=...)` line to main.sml so the + rule takes effect (the file on its own is inert until something requires it). + """ + draft = RuleDraft.get_one(draft_id) + if draft is None: + return jsonify({'error': f'No draft with id {draft_id}.'}), 404 + + payload = request.get_json(silent=True) or {} + wire_into_main = bool(payload.get('wire_into_main', False)) + + # Re-validate at deploy time: the loaded sources may have changed since the draft was saved. + error = _revalidate(draft.path, draft.sml_source) + if error is not None: + return error + + rules_dir, dir_error = _rules_dir_or_error() + if dir_error is not None: + return dir_error + assert rules_dir is not None + + target = _resolve_within(rules_dir, draft.path) + if target is None: + return jsonify({'error': f'Draft path {draft.path!r} escapes the rules directory.'}), 400 + target.parent.mkdir(parents=True, exist_ok=True) + target.write_text(draft.sml_source, encoding='utf-8') + + main_sml_updated = False + if wire_into_main: + main_path = rules_dir / 'main.sml' + if not main_path.exists(): + return jsonify({'error': f'wire_into_main requested but main.sml does not exist at {main_path}.'}), 409 + main_contents = main_path.read_text(encoding='utf-8') + if not _main_requires(main_contents, draft.path): + main_path.write_text(_append_require(main_contents, draft.path), encoding='utf-8') + main_sml_updated = True + + 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) + return jsonify(result) # The set of comparator strings the Rule Builder UI can render. diff --git a/osprey_worker/src/osprey/worker/ui_api/osprey/views/tests/test_rule_drafts.py b/osprey_worker/src/osprey/worker/ui_api/osprey/views/tests/test_rule_drafts.py index dbe18e43..fb1d97df 100644 --- a/osprey_worker/src/osprey/worker/ui_api/osprey/views/tests/test_rule_drafts.py +++ b/osprey_worker/src/osprey/worker/ui_api/osprey/views/tests/test_rule_drafts.py @@ -1,12 +1,7 @@ -import base64 import json -import tempfile -from pathlib import Path from unittest.mock import patch import pytest -import requests -import requests_mock as requests_mock_module from flask import Response, url_for from flask.testing import FlaskClient from osprey.worker.lib.snowflake import Snowflake @@ -15,26 +10,13 @@ @pytest.fixture(autouse=True) def _mock_audit_snowflake(): # The after_request audit hook mints a snowflake id, which normally means an - # HTTP call to the snowflake-id-worker service. Tests that wrap a request in - # a requests_mock.Mocker would otherwise trip NoMockAddress on that call, so - # neutralize it here (the audit log's persist() is already mocked in the - # shared conftest). + # HTTP call to the snowflake-id-worker service. Neutralize it here so tests + # don't reach for that service (the audit log's persist() is already mocked + # in the shared conftest). with patch('osprey.worker.ui_api.osprey.lib.audit.generate_snowflake', return_value=Snowflake(1)): yield -def _set_github_backend(monkeypatch: pytest.MonkeyPatch, **overrides: str) -> None: - """Configure the github backend with sensible defaults for tests; overrides win.""" - defaults = { - 'OSPREY_RULES_SUBMISSION_BACKEND': 'github', - 'OSPREY_RULES_REPO': 'roostorg/osprey-rules', - 'OSPREY_GITHUB_TOKEN': 'gh_fake_token', - 'OSPREY_RULES_BASE_BRANCH': 'main', - } - for k, v in {**defaults, **overrides}.items(): - monkeypatch.setenv(k, v) - - _acl_with_draft_ability = json.dumps( { 'ui_config': {}, @@ -128,7 +110,11 @@ def test_endpoints_require_can_edit_rule_drafts(client: 'FlaskClient[Response]') assert res.status_code == 401 res = client.get(url_for('rule_drafts.vocabulary')) assert res.status_code == 401 - res = client.get(url_for('rule_drafts.pending_drafts')) + res = client.get(url_for('rule_drafts.list_drafts')) + assert res.status_code == 401 + res = client.post(url_for('rule_drafts.create_draft'), json={}) + assert res.status_code == 401 + res = client.post(url_for('rule_drafts.deploy_draft', draft_id=1), json={}) assert res.status_code == 401 @@ -232,669 +218,209 @@ def test_vocabulary_returns_features_udfs_effects(client: 'FlaskClient[Response] assert 'main.sml' in body['source_files'] -@pytest.mark.use_rules_sources(_base_sources) -def test_submit_returns_503_when_no_backend_configured( - client: 'FlaskClient[Response]', monkeypatch: pytest.MonkeyPatch -) -> None: - monkeypatch.delenv('OSPREY_RULES_SUBMISSION_BACKEND', raising=False) - res = client.post( - url_for('rule_drafts.submit_draft'), - json={ - 'path': 'rules/new_rule.sml', - 'source': "Import(rules=['models/base.sml'])\nAnotherRule = Rule(when_all=[PostText == 'bye'], description='bye')", - 'rule_name': 'AnotherRule', - 'summary': 'demo', - 'is_new_rule': True, - }, - ) - assert res.status_code == 503 - body = res.json - assert body is not None - assert 'No rule-submission backend is configured' in body['error'] +# --- Draft table: create / list / get ------------------------------------- + +_VALID_DRAFT = "Import(rules=['models/base.sml'])\nSomeRule = Rule(when_all=[PostText == 'bye'], description='bye')" @pytest.mark.use_rules_sources(_base_sources) -def test_submit_returns_503_when_github_missing_required_env( - client: 'FlaskClient[Response]', monkeypatch: pytest.MonkeyPatch -) -> None: - monkeypatch.setenv('OSPREY_RULES_SUBMISSION_BACKEND', 'github') - monkeypatch.delenv('OSPREY_RULES_REPO', raising=False) - monkeypatch.delenv('OSPREY_GITHUB_TOKEN', raising=False) +def test_create_draft_persists_and_lists(client: 'FlaskClient[Response]') -> None: res = client.post( - url_for('rule_drafts.submit_draft'), - json={ - 'path': 'rules/new_rule.sml', - 'source': "Import(rules=['models/base.sml'])\nAnotherRule = Rule(when_all=[PostText == 'bye'], description='bye')", - 'rule_name': 'AnotherRule', - 'summary': 'demo', - 'is_new_rule': True, - }, + url_for('rule_drafts.create_draft'), + json={'path': 'rules/spam.sml', 'rule_name': 'SomeRule', 'source': _VALID_DRAFT, 'summary': 'catch spam'}, ) - assert res.status_code == 503 - body = res.json - assert body is not None - assert 'OSPREY_RULES_REPO' in body['error'] + assert res.status_code == 200 + draft = res.json + assert draft is not None + assert draft['path'] == 'rules/spam.sml' + assert draft['rule_name'] == 'SomeRule' + assert draft['summary'] == 'catch spam' + assert draft['status'] == 'draft' + assert draft['id'] is not None + + res = client.get(url_for('rule_drafts.list_drafts')) + assert res.status_code == 200 + assert res.json is not None + assert any(d['path'] == 'rules/spam.sml' for d in res.json['drafts']) @pytest.mark.use_rules_sources(_base_sources) -def test_submit_blocks_invalid_sml_before_calling_github( - client: 'FlaskClient[Response]', monkeypatch: pytest.MonkeyPatch -) -> None: - _set_github_backend(monkeypatch) - - with requests_mock_module.Mocker() as m: - res = client.post( - url_for('rule_drafts.submit_draft'), - json={ - 'path': 'rules/bad.sml', - 'source': 'this is not valid SML !!!', - 'rule_name': 'BadRule', - 'summary': 'should not submit', - 'is_new_rule': True, - }, - ) +def test_create_draft_upserts_same_path_in_place(client: 'FlaskClient[Response]') -> None: + first = client.post( + url_for('rule_drafts.create_draft'), + json={'path': 'rules/dupe.sml', 'rule_name': 'SomeRule', 'source': _VALID_DRAFT, 'summary': 'v1'}, + ) + assert first.status_code == 200 + assert first.json is not None + original_id = first.json['id'] - assert res.status_code == 400 - assert m.call_count == 0 - body = res.json - assert body is not None - assert body['error'].startswith('Validation failed') + second = client.post( + url_for('rule_drafts.create_draft'), + json={'path': 'rules/dupe.sml', 'rule_name': 'SomeRule', 'source': _VALID_DRAFT, 'summary': 'v2'}, + ) + assert second.status_code == 200 + assert second.json is not None + assert second.json['id'] == original_id + assert second.json['summary'] == 'v2' + + res = client.get(url_for('rule_drafts.list_drafts')) + assert res.json is not None + assert len([d for d in res.json['drafts'] if d['path'] == 'rules/dupe.sml']) == 1 @pytest.mark.use_rules_sources(_base_sources) -def test_submit_rejects_bad_rule_name(client: 'FlaskClient[Response]', monkeypatch: pytest.MonkeyPatch) -> None: - _set_github_backend(monkeypatch) +def test_create_draft_rejects_invalid_sml(client: 'FlaskClient[Response]') -> None: res = client.post( - url_for('rule_drafts.submit_draft'), + url_for('rule_drafts.create_draft'), json={ - 'path': 'rules/new.sml', - 'source': "X = Rule(when_all=[PostText == 'x'], description='x')", - 'rule_name': '1-not-an-identifier', + 'path': 'rules/broken.sml', + 'rule_name': 'Broken', + 'source': "AnotherRule = Rule(when_all=[NonexistentFeature == 'x'], description='x')", 'summary': '', - 'is_new_rule': True, }, ) assert res.status_code == 400 - - -def test_submission_result_extras_cannot_shadow_canonical_fields() -> None: - from osprey.worker.ui_api.osprey.views._rule_drafts_backend import SubmissionResult - - result = SubmissionResult( - title='real title', - url='https://real.example/pr/1', - extras={'title': 'spoofed', 'url': 'https://evil.example', 'pr_number': 7}, - ) - out = result.to_json() - assert out['title'] == 'real title' - assert out['url'] == 'https://real.example/pr/1' - assert out['main_sml_updated'] is False - # Non-colliding extras still pass through for adopters that want them. - assert out['pr_number'] == 7 + assert res.json is not None + assert 'error' in res.json @pytest.mark.use_rules_sources(_base_sources) -def test_submit_rejects_main_sml_as_draft_path( - client: 'FlaskClient[Response]', monkeypatch: pytest.MonkeyPatch -) -> None: - _set_github_backend(monkeypatch) - with requests_mock_module.Mocker() as m: - res = client.post( - url_for('rule_drafts.submit_draft'), - json={ - 'path': 'main.sml', - 'source': 'Import(rules=[])', - 'rule_name': 'Whatever', - 'summary': '', - 'is_new_rule': False, - }, - ) - # Guard fires before any network call: the entry point is never a draft. - assert m.call_count == 0 +def test_create_draft_rejects_main_sml(client: 'FlaskClient[Response]') -> None: + res = client.post( + url_for('rule_drafts.create_draft'), + json={'path': 'main.sml', 'rule_name': 'SomeRule', 'source': _VALID_DRAFT, 'summary': ''}, + ) assert res.status_code == 400 - body = res.json - assert body is not None - assert 'main.sml' in body['error'] @pytest.mark.use_rules_sources(_base_sources) -def test_submit_surfaces_github_connection_error_as_502( - client: 'FlaskClient[Response]', monkeypatch: pytest.MonkeyPatch -) -> None: - _set_github_backend(monkeypatch) - repo_url = 'https://api.github.com/repos/roostorg/osprey-rules' - with requests_mock_module.Mocker() as m: - m.get(f'{repo_url}/git/ref/heads/main', exc=requests.exceptions.ConnectTimeout) - res = client.post( - url_for('rule_drafts.submit_draft'), - json={ - 'path': 'rules/new_rule.sml', - 'source': "Import(rules=['models/base.sml'])\nAnotherRule = Rule(when_all=[PostText == 'bye'], description='bye')", - 'rule_name': 'AnotherRule', - 'summary': '', - 'is_new_rule': True, - }, - ) - # A forge outage is a structured 502, not an unhandled 500. - assert res.status_code == 502 - body = res.json - assert body is not None - assert 'Could not reach the git host' in body['error'] +def test_create_draft_rejects_bad_rule_name(client: 'FlaskClient[Response]') -> None: + res = client.post( + url_for('rule_drafts.create_draft'), + json={'path': 'rules/x.sml', 'rule_name': '9 not valid', 'source': _VALID_DRAFT, 'summary': ''}, + ) + assert res.status_code == 400 @pytest.mark.use_rules_sources(_base_sources) -def test_submit_happy_path_creates_branch_commits_and_opens_pr( - client: 'FlaskClient[Response]', monkeypatch: pytest.MonkeyPatch -) -> None: - _set_github_backend(monkeypatch, OSPREY_RULES_PATH_IN_REPO='rules') - - repo_url = 'https://api.github.com/repos/roostorg/osprey-rules' - - with requests_mock_module.Mocker() as m: - m.get(f'{repo_url}/git/ref/heads/main', json={'object': {'sha': 'BASE_SHA'}}) - m.get(f'{repo_url}/contents/rules/new_rule.sml', status_code=404) - m.post(f'{repo_url}/git/refs', status_code=201, json={}) - m.put(f'{repo_url}/contents/rules/new_rule.sml', status_code=201, json={}) - m.post( - f'{repo_url}/pulls', - status_code=201, - json={'number': 42, 'html_url': 'https://github.com/roostorg/osprey-rules/pull/42'}, - ) - - res = client.post( - url_for('rule_drafts.submit_draft'), - json={ - # Bare filename: OSPREY_RULES_PATH_IN_REPO='rules' prepends the - # subdirectory, so the file lands at rules/new_rule.sml. - 'path': 'new_rule.sml', - 'source': "Import(rules=['models/base.sml'])\nAnotherRule = Rule(when_all=[PostText == 'bye'], description='bye')", - 'rule_name': 'AnotherRule', - 'summary': 'add bye rule', - 'is_new_rule': True, - }, - ) +def test_get_draft_returns_one_and_404s(client: 'FlaskClient[Response]') -> None: + created = client.post( + url_for('rule_drafts.create_draft'), + json={'path': 'rules/getme.sml', 'rule_name': 'SomeRule', 'source': _VALID_DRAFT, 'summary': ''}, + ) + assert created.json is not None + draft_id = created.json['id'] + res = client.get(url_for('rule_drafts.get_draft', draft_id=draft_id)) assert res.status_code == 200 - body = res.json - assert body is not None - assert body['title'] == 'Pull request #42 opened' - assert body['url'] == 'https://github.com/roostorg/osprey-rules/pull/42' - assert body['main_sml_updated'] is False - # GitHub-specific extras are surfaced for adopters that want them. - assert body['pr_number'] == 42 - assert body['pr_url'] == 'https://github.com/roostorg/osprey-rules/pull/42' - assert body['path_in_repo'] == 'rules/new_rule.sml' - assert body['branch'].startswith('rule-draft/local-dev/AnotherRule-') - - -@pytest.mark.use_rules_sources(_base_sources) -def test_submit_wire_into_main_appends_require_line( - client: 'FlaskClient[Response]', monkeypatch: pytest.MonkeyPatch -) -> None: - _set_github_backend(monkeypatch) - - repo_url = 'https://api.github.com/repos/roostorg/osprey-rules' - existing_main = "Import(rules=['models/post.sml'])\n\nRequire(rule='rules/post_contains_hello.sml')\n" - encoded_main = base64.b64encode(existing_main.encode('utf-8')).decode('ascii') - - with requests_mock_module.Mocker() as m: - m.get(f'{repo_url}/git/ref/heads/main', json={'object': {'sha': 'BASE_SHA'}}) - m.get(f'{repo_url}/contents/rules/new_rule.sml', status_code=404) - m.post(f'{repo_url}/git/refs', status_code=201, json={}) - m.put(f'{repo_url}/contents/rules/new_rule.sml', status_code=201, json={}) - m.get(f'{repo_url}/contents/main.sml', json={'sha': 'MAIN_SHA', 'content': encoded_main, 'type': 'file'}) - main_put = m.put(f'{repo_url}/contents/main.sml', status_code=200, json={}) - m.post( - f'{repo_url}/pulls', - status_code=201, - json={'number': 99, 'html_url': 'https://github.com/roostorg/osprey-rules/pull/99'}, - ) + assert res.json is not None + assert res.json['path'] == 'rules/getme.sml' - res = client.post( - url_for('rule_drafts.submit_draft'), - json={ - 'path': 'rules/new_rule.sml', - 'source': "Import(rules=['models/base.sml'])\nAnotherRule = Rule(when_all=[PostText == 'bye'], description='bye')", - 'rule_name': 'AnotherRule', - 'summary': 'add bye rule and wire it in', - 'is_new_rule': True, - 'wire_into_main': True, - }, - ) + res = client.get(url_for('rule_drafts.get_draft', draft_id=999999)) + assert res.status_code == 404 - assert res.status_code == 200 - body = res.json - assert body is not None - assert body['main_sml_updated'] is True - main_put_request = main_put.last_request - assert main_put_request is not None - posted = main_put_request.json() - decoded_new_main = base64.b64decode(posted['content']).decode('utf-8') - assert "Require(rule='rules/new_rule.sml')" in decoded_new_main - # The existing Require for post_contains_hello.sml should still be present untouched. - assert "Require(rule='rules/post_contains_hello.sml')" in decoded_new_main +# --- Draft table: deploy -------------------------------------------------- @pytest.mark.use_rules_sources(_base_sources) -def test_submit_wire_into_main_skips_when_require_already_present( - client: 'FlaskClient[Response]', monkeypatch: pytest.MonkeyPatch +def test_deploy_writes_sml_and_marks_deployed( + client: 'FlaskClient[Response]', monkeypatch: pytest.MonkeyPatch, tmp_path ) -> None: - _set_github_backend(monkeypatch) - - repo_url = 'https://api.github.com/repos/roostorg/osprey-rules' - existing_main = "Require(rule='rules/already_here.sml')\n" - encoded_main = base64.b64encode(existing_main.encode('utf-8')).decode('ascii') - - with requests_mock_module.Mocker() as m: - m.get(f'{repo_url}/git/ref/heads/main', json={'object': {'sha': 'BASE_SHA'}}) - m.get(f'{repo_url}/contents/rules/already_here.sml', status_code=404) - m.post(f'{repo_url}/git/refs', status_code=201, json={}) - m.put(f'{repo_url}/contents/rules/already_here.sml', status_code=201, json={}) - m.get(f'{repo_url}/contents/main.sml', json={'sha': 'MAIN_SHA', 'content': encoded_main, 'type': 'file'}) - main_put = m.put(f'{repo_url}/contents/main.sml', status_code=200, json={}) - m.post( - f'{repo_url}/pulls', - status_code=201, - json={'number': 100, 'html_url': 'https://github.com/roostorg/osprey-rules/pull/100'}, - ) - - res = client.post( - url_for('rule_drafts.submit_draft'), - json={ - 'path': 'rules/already_here.sml', - 'source': "Import(rules=['models/base.sml'])\nAnotherRule = Rule(when_all=[PostText == 'bye'], description='bye')", - 'rule_name': 'AnotherRule', - 'summary': '', - 'is_new_rule': True, - 'wire_into_main': True, - }, - ) + monkeypatch.setenv('OSPREY_RULES_LOCAL_PATH', str(tmp_path)) + created = client.post( + url_for('rule_drafts.create_draft'), + json={'path': 'rules/deploy.sml', 'rule_name': 'SomeRule', 'source': _VALID_DRAFT, 'summary': ''}, + ) + assert created.json is not None + draft_id = created.json['id'] + res = client.post(url_for('rule_drafts.deploy_draft', draft_id=draft_id), json={}) assert res.status_code == 200 - body = res.json - assert body is not None - assert body['main_sml_updated'] is False - # main.sml fetch happens but the PUT to update it must not. - assert main_put.call_count == 0 - - -@pytest.mark.use_rules_sources(_base_sources) -def test_submit_409_if_new_rule_file_already_exists( - client: 'FlaskClient[Response]', monkeypatch: pytest.MonkeyPatch -) -> None: - _set_github_backend(monkeypatch) - - repo_url = 'https://api.github.com/repos/roostorg/osprey-rules' - - with requests_mock_module.Mocker() as m: - m.get(f'{repo_url}/git/ref/heads/main', json={'object': {'sha': 'BASE_SHA'}}) - m.get( - f'{repo_url}/contents/new_rule.sml', - json={'sha': 'EXISTING_BLOB_SHA', 'type': 'file'}, - ) - - res = client.post( - url_for('rule_drafts.submit_draft'), - json={ - 'path': 'new_rule.sml', - 'source': "Import(rules=['models/base.sml'])\nAnotherRule = Rule(when_all=[PostText == 'bye'], description='bye')", - 'rule_name': 'AnotherRule', - 'summary': '', - 'is_new_rule': True, - }, - ) + assert res.json is not None + assert res.json['status'] == 'deployed' + assert res.json['deployed_at'] is not None + assert res.json['main_sml_updated'] is False - assert res.status_code == 409 + written = tmp_path / 'rules' / 'deploy.sml' + assert written.exists() + assert written.read_text() == _VALID_DRAFT @pytest.mark.use_rules_sources(_base_sources) -def test_pending_filters_to_rules_path_and_sml( - client: 'FlaskClient[Response]', monkeypatch: pytest.MonkeyPatch +def test_deploy_wire_into_main_appends_require( + client: 'FlaskClient[Response]', monkeypatch: pytest.MonkeyPatch, tmp_path ) -> None: - _set_github_backend(monkeypatch, OSPREY_RULES_PATH_IN_REPO='rules') - - repo_url = 'https://api.github.com/repos/roostorg/osprey-rules' - - with requests_mock_module.Mocker() as m: - m.get( - f'{repo_url}/pulls', - json=[ - { - 'number': 1, - 'title': 'Add rule X', - 'html_url': 'https://github.com/roostorg/osprey-rules/pull/1', - 'head': {'ref': 'rule-draft/local-dev/X-1'}, - 'user': {'login': 'someone'}, - 'created_at': '2026-06-30T12:00:00Z', - }, - { - 'number': 2, - 'title': 'Update README', - 'html_url': 'https://github.com/roostorg/osprey-rules/pull/2', - 'head': {'ref': 'docs/readme'}, - 'user': {'login': 'someone'}, - 'created_at': '2026-06-30T13:00:00Z', - }, - ], - ) - m.get( - f'{repo_url}/pulls/1/files', - json=[{'filename': 'rules/x.sml'}], - ) - m.get( - f'{repo_url}/pulls/2/files', - json=[{'filename': 'README.md'}], - ) - - res = client.get(url_for('rule_drafts.pending_drafts')) - - assert res.status_code == 200 - body = res.json - assert body is not None - assert len(body['pending']) == 1 - entry = body['pending'][0] - assert entry['title'] == 'Add rule X' - assert entry['url'] == 'https://github.com/roostorg/osprey-rules/pull/1' - assert entry['touched_files'] == ['rules/x.sml'] - # GitHub-specific extras carried through for adopters that want them. - assert entry['pr_number'] == 1 - - -@pytest.mark.use_rules_sources(_base_sources) -def test_parse_into_builder_round_trips_a_builder_shaped_rule(client: 'FlaskClient[Response]') -> None: - source = """ -Import(rules=['models/post.sml']) - -ContainsCat = Rule( - when_all=[ - TextContains(text=PostText, phrase='cat'), - EventType == 'create_post', - ], - description='looks for cat', -) - -WhenRules( - rules_any=[ContainsCat], - then=[ - LabelAdd(entity=UserId, label='meow'), - ], -) -""" - res = client.post( - url_for('rule_drafts.parse_into_builder'), - json={'path': 'rules/contains_cat.sml', 'source': source}, - ) - assert res.status_code == 200 - body = res.json - assert body is not None - assert body['supported'] is True - model = body['model'] - assert model['ruleName'] == 'ContainsCat' - assert model['description'] == 'looks for cat' - assert model['conditions'] == [ - {'feature': 'PostText', 'operator': 'includes', 'rhs': 'cat', 'rhsIsFeature': False}, - {'feature': 'EventType', 'operator': '==', 'rhs': 'create_post', 'rhsIsFeature': False}, - ] - assert model['outcomes'] == [ - { - 'effect': 'LabelAdd', - 'args': [ - {'name': 'entity', 'value': 'UserId', 'isFeature': True}, - {'name': 'label', 'value': 'meow', 'isFeature': False}, - ], - } - ] - - -@pytest.mark.use_rules_sources(_base_sources) -def test_parse_into_builder_handles_excludes(client: 'FlaskClient[Response]') -> None: - source = "BlocksCat = Rule(when_all=[not TextContains(text=PostText, phrase='cat')], description='no cat')\n" - res = client.post( - url_for('rule_drafts.parse_into_builder'), - json={'path': 'rules/blocks_cat.sml', 'source': source}, + (tmp_path / 'main.sml').write_text("Import(rules=['models/base.sml'])\n") + monkeypatch.setenv('OSPREY_RULES_LOCAL_PATH', str(tmp_path)) + created = client.post( + url_for('rule_drafts.create_draft'), + json={'path': 'rules/wired.sml', 'rule_name': 'SomeRule', 'source': _VALID_DRAFT, 'summary': ''}, ) - assert res.status_code == 200 - body = res.json - assert body is not None - assert body['supported'] is True - assert body['model']['conditions'] == [ - {'feature': 'PostText', 'operator': 'excludes', 'rhs': 'cat', 'rhsIsFeature': False}, - ] - + assert created.json is not None + draft_id = created.json['id'] -@pytest.mark.use_rules_sources(_base_sources) -def test_parse_into_builder_rejects_multiple_rules(client: 'FlaskClient[Response]') -> None: - source = ( - "A = Rule(when_all=[PostText == 'a'], description='a')\nB = Rule(when_all=[PostText == 'b'], description='b')\n" - ) - res = client.post( - url_for('rule_drafts.parse_into_builder'), - json={'path': 'rules/multi.sml', 'source': source}, - ) + res = client.post(url_for('rule_drafts.deploy_draft', draft_id=draft_id), json={'wire_into_main': True}) assert res.status_code == 200 - body = res.json - assert body is not None - assert body['supported'] is False - assert 'multiple Rule definitions' in body['reason'] + assert res.json is not None + assert res.json['main_sml_updated'] is True + assert "Require(rule='rules/wired.sml')" in (tmp_path / 'main.sml').read_text() @pytest.mark.use_rules_sources(_base_sources) -def test_parse_into_builder_rejects_helper_assigns(client: 'FlaskClient[Response]') -> None: - source = "Helper = 'cat'\nA = Rule(when_all=[PostText == Helper], description='a')\n" - res = client.post( - url_for('rule_drafts.parse_into_builder'), - json={'path': 'rules/helper.sml', 'source': source}, +def test_deploy_wire_into_main_is_idempotent( + client: 'FlaskClient[Response]', monkeypatch: pytest.MonkeyPatch, tmp_path +) -> None: + (tmp_path / 'main.sml').write_text("Import(rules=['models/base.sml'])\nRequire(rule='rules/already.sml')\n") + monkeypatch.setenv('OSPREY_RULES_LOCAL_PATH', str(tmp_path)) + created = client.post( + url_for('rule_drafts.create_draft'), + json={'path': 'rules/already.sml', 'rule_name': 'SomeRule', 'source': _VALID_DRAFT, 'summary': ''}, ) - assert res.status_code == 200 - body = res.json - assert body is not None - assert body['supported'] is False - assert 'helper assignment' in body['reason'] + assert created.json is not None + draft_id = created.json['id'] - -@pytest.mark.use_rules_sources(_base_sources) -def test_parse_into_builder_rejects_complex_condition(client: 'FlaskClient[Response]') -> None: - # A boolean operator inside `when_all` would need Code Editor; the builder is AND-only via row repetition. - source = "A = Rule(when_all=[PostText == 'a' and EventType == 'create_post'], description='a')\n" - res = client.post( - url_for('rule_drafts.parse_into_builder'), - json={'path': 'rules/complex.sml', 'source': source}, - ) + res = client.post(url_for('rule_drafts.deploy_draft', draft_id=draft_id), json={'wire_into_main': True}) assert res.status_code == 200 - body = res.json - assert body is not None - assert body['supported'] is False - assert 'Rule Builder cannot represent' in body['reason'] + assert res.json is not None + assert res.json['main_sml_updated'] is False + assert (tmp_path / 'main.sml').read_text().count("Require(rule='rules/already.sml')") == 1 @pytest.mark.use_rules_sources(_base_sources) -def test_parse_into_builder_rejects_file_with_no_rule(client: 'FlaskClient[Response]') -> None: - source = "Import(rules=['models/post.sml'])\n" - res = client.post( - url_for('rule_drafts.parse_into_builder'), - json={'path': 'rules/empty.sml', 'source': source}, +def test_deploy_wire_into_main_409_when_main_missing( + client: 'FlaskClient[Response]', monkeypatch: pytest.MonkeyPatch, tmp_path +) -> None: + monkeypatch.setenv('OSPREY_RULES_LOCAL_PATH', str(tmp_path)) + created = client.post( + url_for('rule_drafts.create_draft'), + json={'path': 'rules/nomain.sml', 'rule_name': 'SomeRule', 'source': _VALID_DRAFT, 'summary': ''}, ) - assert res.status_code == 200 - body = res.json - assert body is not None - assert body['supported'] is False - assert 'no Rule(...)' in body['reason'] - + assert created.json is not None + draft_id = created.json['id'] -@pytest.mark.use_rules_sources(_base_sources) -def test_parse_into_builder_rejects_syntax_error(client: 'FlaskClient[Response]') -> None: - res = client.post( - url_for('rule_drafts.parse_into_builder'), - json={'path': 'rules/broken.sml', 'source': 'this is not valid SML at all !!!'}, - ) - assert res.status_code == 200 - body = res.json - assert body is not None - assert body['supported'] is False - assert 'could not parse SML' in body['reason'] + res = client.post(url_for('rule_drafts.deploy_draft', draft_id=draft_id), json={'wire_into_main': True}) + assert res.status_code == 409 @pytest.mark.use_rules_sources(_base_sources) -def test_unknown_backend_value_returns_500(client: 'FlaskClient[Response]', monkeypatch: pytest.MonkeyPatch) -> None: - monkeypatch.setenv('OSPREY_RULES_SUBMISSION_BACKEND', 'gerrit') - res = client.post( - url_for('rule_drafts.submit_draft'), - json={ - 'path': 'rules/new_rule.sml', - 'source': "Import(rules=['models/base.sml'])\nAnotherRule = Rule(when_all=[PostText == 'bye'], description='bye')", - 'rule_name': 'AnotherRule', - 'summary': '', - 'is_new_rule': True, - }, +def test_deploy_503_when_rules_dir_unset(client: 'FlaskClient[Response]', monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.delenv('OSPREY_RULES_LOCAL_PATH', raising=False) + created = client.post( + url_for('rule_drafts.create_draft'), + json={'path': 'rules/nodir.sml', 'rule_name': 'SomeRule', 'source': _VALID_DRAFT, 'summary': ''}, ) - assert res.status_code == 500 - body = res.json - assert body is not None - assert "Unknown OSPREY_RULES_SUBMISSION_BACKEND 'gerrit'" in body['error'] - + assert created.json is not None + draft_id = created.json['id'] -@pytest.mark.use_rules_sources(_base_sources) -def test_pending_returns_empty_list_for_null_backend( - client: 'FlaskClient[Response]', monkeypatch: pytest.MonkeyPatch -) -> None: - monkeypatch.delenv('OSPREY_RULES_SUBMISSION_BACKEND', raising=False) - res = client.get(url_for('rule_drafts.pending_drafts')) + res = client.post(url_for('rule_drafts.deploy_draft', draft_id=draft_id), json={}) assert res.status_code == 503 - body = res.json - assert body is not None - assert body['pending'] == [] - - -@pytest.mark.use_rules_sources(_base_sources) -def test_submit_github_enterprise_url_is_threaded_into_requests( - client: 'FlaskClient[Response]', monkeypatch: pytest.MonkeyPatch -) -> None: - """GitHub Enterprise customers point OSPREY_GITHUB_API_URL at their own host; every API call must use it.""" - _set_github_backend( - monkeypatch, - OSPREY_GITHUB_API_URL='https://github.acme.test/api/v3', - OSPREY_RULES_REPO='acme/rules', - ) - - repo_url = 'https://github.acme.test/api/v3/repos/acme/rules' - - with requests_mock_module.Mocker() as m: - m.get(f'{repo_url}/git/ref/heads/main', json={'object': {'sha': 'BASE_SHA'}}) - m.get(f'{repo_url}/contents/rules/new_rule.sml', status_code=404) - m.post(f'{repo_url}/git/refs', status_code=201, json={}) - m.put(f'{repo_url}/contents/rules/new_rule.sml', status_code=201, json={}) - m.post( - f'{repo_url}/pulls', - status_code=201, - json={'number': 5, 'html_url': 'https://github.acme.test/acme/rules/pull/5'}, - ) - - res = client.post( - url_for('rule_drafts.submit_draft'), - json={ - 'path': 'rules/new_rule.sml', - 'source': "Import(rules=['models/base.sml'])\nAnotherRule = Rule(when_all=[PostText == 'bye'], description='bye')", - 'rule_name': 'AnotherRule', - 'summary': '', - 'is_new_rule': True, - }, - ) - - assert res.status_code == 200 - body = res.json - assert body is not None - assert body['url'] == 'https://github.acme.test/acme/rules/pull/5' - # If any call had hit api.github.com instead, the Mocker would have raised NoMockAddress. - - -@pytest.mark.use_rules_sources(_base_sources) -def test_local_backend_writes_file_and_returns_no_url( - client: 'FlaskClient[Response]', monkeypatch: pytest.MonkeyPatch -) -> None: - with tempfile.TemporaryDirectory() as tmpdir: - rules_dir = Path(tmpdir) - (rules_dir / 'main.sml').write_text("Import(rules=['models/post.sml'])\n", encoding='utf-8') - - monkeypatch.setenv('OSPREY_RULES_SUBMISSION_BACKEND', 'local') - monkeypatch.setenv('OSPREY_RULES_LOCAL_PATH', str(rules_dir)) - - res = client.post( - url_for('rule_drafts.submit_draft'), - json={ - 'path': 'rules/new_rule.sml', - 'source': "Import(rules=['models/base.sml'])\nAnotherRule = Rule(when_all=[PostText == 'bye'], description='bye')", - 'rule_name': 'AnotherRule', - 'summary': 'add bye rule', - 'is_new_rule': True, - 'wire_into_main': False, - }, - ) - - assert res.status_code == 200 - body = res.json - assert body is not None - assert body['url'] is None - assert 'rules/new_rule.sml' in body['title'] - assert body['main_sml_updated'] is False - - written = (rules_dir / 'rules' / 'new_rule.sml').read_text(encoding='utf-8') - assert 'AnotherRule' in written @pytest.mark.use_rules_sources(_base_sources) -def test_local_backend_wires_into_main_when_requested( - client: 'FlaskClient[Response]', monkeypatch: pytest.MonkeyPatch +def test_deploy_404_for_unknown_draft( + client: 'FlaskClient[Response]', monkeypatch: pytest.MonkeyPatch, tmp_path ) -> None: - with tempfile.TemporaryDirectory() as tmpdir: - rules_dir = Path(tmpdir) - (rules_dir / 'main.sml').write_text("Import(rules=['models/post.sml'])\n", encoding='utf-8') - - monkeypatch.setenv('OSPREY_RULES_SUBMISSION_BACKEND', 'local') - monkeypatch.setenv('OSPREY_RULES_LOCAL_PATH', str(rules_dir)) - - res = client.post( - url_for('rule_drafts.submit_draft'), - json={ - 'path': 'rules/new_rule.sml', - 'source': "Import(rules=['models/base.sml'])\nAnotherRule = Rule(when_all=[PostText == 'bye'], description='bye')", - 'rule_name': 'AnotherRule', - 'summary': '', - 'is_new_rule': True, - 'wire_into_main': True, - }, - ) - - assert res.status_code == 200 - body = res.json - assert body is not None - assert body['main_sml_updated'] is True - updated_main = (rules_dir / 'main.sml').read_text(encoding='utf-8') - assert "Require(rule='rules/new_rule.sml')" in updated_main - - -@pytest.mark.use_rules_sources(_base_sources) -def test_local_backend_rejects_path_traversal(client: 'FlaskClient[Response]', monkeypatch: pytest.MonkeyPatch) -> None: - with tempfile.TemporaryDirectory() as tmpdir: - rules_dir = Path(tmpdir) - monkeypatch.setenv('OSPREY_RULES_SUBMISSION_BACKEND', 'local') - monkeypatch.setenv('OSPREY_RULES_LOCAL_PATH', str(rules_dir)) - - # The view layer already rejects '..' in the path, so to actually exercise the - # local backend's own guard we bypass the view-level check by going around the - # API: instantiate the backend and call submit_draft directly with a traversal path. - from osprey.worker.ui_api.osprey.views._rule_drafts_backend import RuleDraftBackendError - from osprey.worker.ui_api.osprey.views._rule_drafts_local import LocalBackend, LocalConfig - - backend = LocalBackend(LocalConfig(rules_dir=rules_dir)) - with pytest.raises(RuleDraftBackendError) as exc_info: - backend.submit_draft( - draft_path='../escape.sml', - sml_source='x = 1', - rule_name='X', - summary='', - author_email='test@local', - is_new_rule=True, - wire_into_main=False, - ) - assert 'escapes the configured rules directory' in exc_info.value.message + monkeypatch.setenv('OSPREY_RULES_LOCAL_PATH', str(tmp_path)) + res = client.post(url_for('rule_drafts.deploy_draft', draft_id=999999), json={}) + assert res.status_code == 404 From 0ec81dfa57f7601e137ff646ad5bbf49724fae04 Mon Sep 17 00:00:00 2001 From: Juliet Shen Date: Wed, 15 Jul 2026 14:11:23 -0400 Subject: [PATCH 3/9] Reject drafts that reuse another draft's rule name 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 Claude-Session: https://claude.ai/code/session_01TEcfpqhza3dPepZUVWw36X --- .../osprey/worker/lib/storage/rule_drafts.py | 14 ++++++++++ .../worker/ui_api/osprey/views/rule_drafts.py | 10 +++++++ .../osprey/views/tests/test_rule_drafts.py | 26 +++++++++++++++++++ 3 files changed, 50 insertions(+) diff --git a/osprey_worker/src/osprey/worker/lib/storage/rule_drafts.py b/osprey_worker/src/osprey/worker/lib/storage/rule_drafts.py index ac520021..6108fdc2 100644 --- a/osprey_worker/src/osprey/worker/lib/storage/rule_drafts.py +++ b/osprey_worker/src/osprey/worker/lib/storage/rule_drafts.py @@ -97,6 +97,20 @@ def get_one(cls, draft_id: int) -> 'RuleDraft | None': session.expunge(draft) return draft + @classmethod + def other_with_rule_name(cls, rule_name: str, *, exclude_path: str) -> 'RuleDraft | None': + """A draft at a different path that already uses `rule_name`, if one exists. + + Rule names are global identifiers in SML, so two drafts sharing a name would + collide once both deploy. Validation only sees deployed rules, not other + drafts, so this catches the draft-vs-draft case that validation can't. + """ + with scoped_session() as session: + draft = session.query(cls).filter(cls.rule_name == rule_name, cls.path != exclude_path).first() + if draft is not None: + session.expunge(draft) + return draft + @classmethod def mark_deployed(cls, draft_id: int) -> 'RuleDraft | None': with scoped_session(commit=True) as session: diff --git a/osprey_worker/src/osprey/worker/ui_api/osprey/views/rule_drafts.py b/osprey_worker/src/osprey/worker/ui_api/osprey/views/rule_drafts.py index f0edc5e1..98f6b25e 100644 --- a/osprey_worker/src/osprey/worker/ui_api/osprey/views/rule_drafts.py +++ b/osprey_worker/src/osprey/worker/ui_api/osprey/views/rule_drafts.py @@ -365,6 +365,16 @@ def create_draft() -> Any: if error is not None: return error + # Validation only sees deployed rules; guard the draft-vs-draft name collision it can't. + conflict = RuleDraft.other_with_rule_name(rule_name, exclude_path=path) + if conflict is not None: + return jsonify( + { + 'error': f'Another draft ({conflict.path}) already uses the rule name {rule_name!r}. ' + 'Rename this rule, or edit that draft instead.' + } + ), 409 + draft = RuleDraft.upsert( path=path, rule_name=rule_name, diff --git a/osprey_worker/src/osprey/worker/ui_api/osprey/views/tests/test_rule_drafts.py b/osprey_worker/src/osprey/worker/ui_api/osprey/views/tests/test_rule_drafts.py index fb1d97df..2c24afd4 100644 --- a/osprey_worker/src/osprey/worker/ui_api/osprey/views/tests/test_rule_drafts.py +++ b/osprey_worker/src/osprey/worker/ui_api/osprey/views/tests/test_rule_drafts.py @@ -268,6 +268,32 @@ def test_create_draft_upserts_same_path_in_place(client: 'FlaskClient[Response]' assert len([d for d in res.json['drafts'] if d['path'] == 'rules/dupe.sml']) == 1 +@pytest.mark.use_rules_sources(_base_sources) +def test_create_draft_rejects_duplicate_rule_name_across_drafts(client: 'FlaskClient[Response]') -> None: + first = client.post( + url_for('rule_drafts.create_draft'), + json={'path': 'rules/first.sml', 'rule_name': 'SomeRule', 'source': _VALID_DRAFT, 'summary': ''}, + ) + assert first.status_code == 200 + + # A different path reusing the same rule name collides: rule names are global in SML, + # and validation can't see the other draft (it only knows deployed rules). + second = client.post( + url_for('rule_drafts.create_draft'), + json={'path': 'rules/second.sml', 'rule_name': 'SomeRule', 'source': _VALID_DRAFT, 'summary': ''}, + ) + assert second.status_code == 409 + assert second.json is not None + assert 'SomeRule' in second.json['error'] + + # Re-saving the same path with the same name is an update, not a collision. + again = client.post( + url_for('rule_drafts.create_draft'), + json={'path': 'rules/first.sml', 'rule_name': 'SomeRule', 'source': _VALID_DRAFT, 'summary': 'edit'}, + ) + assert again.status_code == 200 + + @pytest.mark.use_rules_sources(_base_sources) def test_create_draft_rejects_invalid_sml(client: 'FlaskClient[Response]') -> None: res = client.post( From 62beabb1e7c2bd0e528ae8d2fd56370200d559d1 Mon Sep 17 00:00:00 2001 From: Juliet Shen Date: Sun, 19 Jul 2026 13:28:57 -0700 Subject: [PATCH 4/9] Address review: harden validation, deploy, and the upsert 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 Claude-Session: https://claude.ai/code/session_01TEcfpqhza3dPepZUVWw36X --- .../osprey/worker/lib/storage/rule_drafts.py | 37 +++-- .../worker/ui_api/osprey/views/rule_drafts.py | 150 +++++++++++------- .../osprey/views/tests/test_rule_drafts.py | 13 ++ 3 files changed, 126 insertions(+), 74 deletions(-) diff --git a/osprey_worker/src/osprey/worker/lib/storage/rule_drafts.py b/osprey_worker/src/osprey/worker/lib/storage/rule_drafts.py index 6108fdc2..069a485a 100644 --- a/osprey_worker/src/osprey/worker/lib/storage/rule_drafts.py +++ b/osprey_worker/src/osprey/worker/lib/storage/rule_drafts.py @@ -4,6 +4,7 @@ from enum import StrEnum from sqlalchemy import BigInteger, Column, DateTime, Enum, Text +from sqlalchemy.dialects.postgresql import insert as pg_insert from .postgres import Model, scoped_session @@ -36,6 +37,8 @@ class RuleDraft(Model): rule_name: str = Column(Text, nullable=False) sml_source: str = Column(Text, nullable=False) summary: str = Column(Text, nullable=False, default='') + # Osprey has no users table; identity is just an email with ACLs applied, so + # this stores the author's email rather than a foreign key. author_email: str = Column(Text, nullable=False) status: RuleDraftStatus = Column( Enum(RuleDraftStatus, native_enum=False, length=32), @@ -64,20 +67,29 @@ def to_json(self) -> dict[str, object]: def upsert(cls, *, path: str, rule_name: str, sml_source: str, summary: str, author_email: str) -> 'RuleDraft': """Create the draft for `path`, or update it in place if one already exists. - Editing a deployed draft moves it back to `DRAFT` so the table reflects - that the in-flight SML no longer matches what was last deployed. + Uses a single `INSERT ... ON CONFLICT DO UPDATE` so two concurrent saves of + the same path can't both see "no row" and then race the unique constraint. + Editing a deployed draft moves it back to `DRAFT` so the table reflects that + the in-flight SML no longer matches what was last deployed. """ + now = _now() + mutable = { + 'rule_name': rule_name, + 'sml_source': sml_source, + 'summary': summary, + 'author_email': author_email, + 'status': RuleDraftStatus.DRAFT, + 'updated_at': now, + } + statement = ( + pg_insert(cls.__table__) + .values(path=path, created_at=now, **mutable) + .on_conflict_do_update(index_elements=[cls.path], set_=mutable) + ) with scoped_session(commit=True) as session: - draft = session.query(cls).filter(cls.path == path).first() - if draft is None: - draft = cls(path=path) - session.add(draft) - draft.rule_name = rule_name - draft.sml_source = sml_source - draft.summary = summary - draft.author_email = author_email - draft.status = RuleDraftStatus.DRAFT + session.execute(statement) session.flush() + draft = session.query(cls).filter(cls.path == path).one() session.expunge(draft) return draft @@ -85,8 +97,7 @@ def upsert(cls, *, path: str, rule_name: str, sml_source: str, summary: str, aut def list_all(cls) -> list['RuleDraft']: with scoped_session() as session: drafts = session.query(cls).order_by(cls.updated_at.desc()).all() - for draft in drafts: - session.expunge(draft) + session.expunge_all() return drafts @classmethod diff --git a/osprey_worker/src/osprey/worker/ui_api/osprey/views/rule_drafts.py b/osprey_worker/src/osprey/worker/ui_api/osprey/views/rule_drafts.py index 98f6b25e..36d1e339 100644 --- a/osprey_worker/src/osprey/worker/ui_api/osprey/views/rule_drafts.py +++ b/osprey_worker/src/osprey/worker/ui_api/osprey/views/rule_drafts.py @@ -4,6 +4,7 @@ import os import re from collections.abc import Iterable +from dataclasses import dataclass, field from pathlib import Path from typing import Any @@ -42,18 +43,19 @@ blueprint = Blueprint('rule_drafts', __name__) -_VALID_PATH = re.compile(r'^[A-Za-z0-9_./-]+\.sml$') +_VALID_PATH = re.compile(r'^[A-Za-z0-9_/-]+\.sml$') _VALID_RULE_NAME = re.compile(r'^[A-Za-z_][A-Za-z0-9_]*$') def _format_validation_message(msg: ValidationError | ValidationWarning) -> dict[str, Any]: identifier: str | None = None try: + # `ast_node` is a property that raises RuntimeError when the span has no node. node = msg.span.ast_node - if isinstance(node, Name): - identifier = node.identifier - except Exception: - pass + except (RuntimeError, AttributeError): + node = None + if isinstance(node, Name): + identifier = node.identifier defined_in: list[str] = [] for additional in msg.additional_spans: @@ -92,7 +94,11 @@ def _suggest_imports_from_errors( 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_./-]).' + return f'Path {path!r} is not a valid SML source path (must end .sml and contain only [A-Za-z0-9_/-]).' + if path.startswith('/'): + # Rule paths are relative to the rules directory; an absolute path would + # escape it. Deploy also guards this with _resolve_within, but reject early. + return f'Path {path!r} must be relative to the rules directory, not absolute.' if '..' in path.split('/'): return f'Path {path!r} contains a parent-directory segment.' return None @@ -118,6 +124,57 @@ def get_source() -> Any: return jsonify({'path': source.path, 'contents': source.contents}) +@dataclass +class _DraftValidation: + """The outcome of validating a draft spliced into the loaded sources.""" + + ok: bool + errors: list[dict[str, Any]] = field(default_factory=list) + warnings: list[dict[str, Any]] = field(default_factory=list) + suggested_imports: list[str] = field(default_factory=list) + # Set when the sources couldn't even be assembled (e.g. broken main.sml), + # which is distinct from the SML compiling but failing validation. + assemble_error: str | None = None + + +def _validate_draft_source(path: str, source_text: str) -> _DraftValidation: + """Splice the draft into the loaded sources and run AST validation. + + Shared by POST /rule-drafts/validate (which reports the result) and the + server-side re-validation on create/deploy (which rejects on failure), so the + two paths can't drift apart. + """ + spliced = _current_sources_dict() + spliced[path] = source_text + try: + sources = Sources.from_dict(spliced) + except Exception: + # Don't echo the raw exception to the client (it can leak internals). + logger.exception('failed to assemble sources for draft at %r', path) + return _DraftValidation(ok=False, assemble_error='Could not assemble the rule sources for validation.') + + engine = ENGINE.instance() + try: + validated = validate_sources( + sources, + udf_registry=engine.udf_registry, + validator_registry=engine.validator_registry, + ) + except ValidationFailed as exc: + errors = [_format_validation_message(e) for e in exc.errors] + return _DraftValidation( + ok=False, + errors=errors, + warnings=[_format_validation_message(w) for w in exc.warnings], + suggested_imports=_suggest_imports_from_errors(path, errors), + ) + + return _DraftValidation( + ok=True, + warnings=[_format_validation_message(w) for w in validated.warnings], + ) + + @blueprint.route('/rule-drafts/validate', methods=['POST']) @require_ability(CanEditRuleDrafts) def validate_draft() -> Any: @@ -137,46 +194,23 @@ def validate_draft() -> Any: if not isinstance(source_text, str): return jsonify({'error': 'source must be a string.'}), 400 - spliced = _current_sources_dict() - spliced[path] = source_text - - try: - sources = Sources.from_dict(spliced) - except Exception as exc: - # Sources.from_dict asserts on shape (e.g., missing main.sml). Surface as a structured error - # so the editor can show "you broke main.sml" without crashing. + result = _validate_draft_source(path, source_text) + if result.assemble_error is not None: + # main.sml (or another source) is broken; surface it so the editor can show it. return jsonify( { 'ok': False, - 'errors': [{'message': str(exc), 'hint': '', 'source_path': path, 'line': 0, 'column': 0}], + 'errors': [{'message': result.assemble_error, 'hint': '', 'source_path': path, 'line': 0, 'column': 0}], 'warnings': [], } ), 400 - engine = ENGINE.instance() - try: - validated = validate_sources( - sources, - udf_registry=engine.udf_registry, - validator_registry=engine.validator_registry, - ) - except ValidationFailed as exc: - formatted_errors = [_format_validation_message(e) for e in exc.errors] - return jsonify( - { - 'ok': False, - 'errors': formatted_errors, - 'warnings': [_format_validation_message(w) for w in exc.warnings], - 'suggested_imports': _suggest_imports_from_errors(path, formatted_errors), - } - ) - return jsonify( { - 'ok': True, - 'errors': [], - 'warnings': [_format_validation_message(w) for w in validated.warnings], - 'suggested_imports': [], + 'ok': result.ok, + 'errors': result.errors, + 'warnings': result.warnings, + 'suggested_imports': result.suggested_imports, } ) @@ -310,26 +344,14 @@ def vocabulary() -> Any: def _revalidate(path: str, source_text: str) -> tuple[Any, int] | None: - """Re-run the engine's AST validation with the draft spliced into the loaded - sources. Returns a Flask error tuple on failure, or None if it validates.""" - spliced = _current_sources_dict() - spliced[path] = source_text - try: - sources = Sources.from_dict(spliced) - validate_sources( - sources, - udf_registry=ENGINE.instance().udf_registry, - validator_registry=ENGINE.instance().validator_registry, - ) - except ValidationFailed as exc: - return jsonify( - { - 'error': 'Validation failed; fix errors before submitting.', - 'errors': [_format_validation_message(e) for e in exc.errors], - } - ), 400 - except Exception as exc: - return jsonify({'error': f'Could not assemble sources: {exc}'}), 400 + """Re-run validation server-side, rejecting on failure. Returns a Flask error + tuple to return, or None if the draft validates. Shares `_validate_draft_source` + with the /validate endpoint so create/deploy can't accept SML the editor rejected.""" + result = _validate_draft_source(path, source_text) + if result.assemble_error is not None: + return jsonify({'error': result.assemble_error}), 400 + if not result.ok: + return jsonify({'error': 'Validation failed; fix errors before submitting.', 'errors': result.errors}), 400 return None @@ -477,14 +499,18 @@ def deploy_draft(draft_id: int) -> Any: target = _resolve_within(rules_dir, draft.path) if target is None: return jsonify({'error': f'Draft path {draft.path!r} escapes the rules directory.'}), 400 + + # If wiring is requested, verify main.sml exists before writing anything, so a + # missing main.sml doesn't leave the rule file written while the deploy 409s. + main_path = rules_dir / 'main.sml' + if wire_into_main and not main_path.exists(): + return jsonify({'error': 'wire_into_main requested but main.sml does not exist in the rules directory.'}), 409 + target.parent.mkdir(parents=True, exist_ok=True) target.write_text(draft.sml_source, encoding='utf-8') main_sml_updated = False if wire_into_main: - main_path = rules_dir / 'main.sml' - if not main_path.exists(): - return jsonify({'error': f'wire_into_main requested but main.sml does not exist at {main_path}.'}), 409 main_contents = main_path.read_text(encoding='utf-8') if not _main_requires(main_contents, draft.path): main_path.write_text(_append_require(main_contents, draft.path), encoding='utf-8') @@ -493,7 +519,9 @@ def deploy_draft(draft_id: int) -> Any: 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) + # Report the path relative to the rules directory, not the absolute server path + # (which would leak the deployment's directory layout to the client). + result['path_on_disk'] = str(target.relative_to(rules_dir.resolve())) return jsonify(result) diff --git a/osprey_worker/src/osprey/worker/ui_api/osprey/views/tests/test_rule_drafts.py b/osprey_worker/src/osprey/worker/ui_api/osprey/views/tests/test_rule_drafts.py index 2c24afd4..d0d8779f 100644 --- a/osprey_worker/src/osprey/worker/ui_api/osprey/views/tests/test_rule_drafts.py +++ b/osprey_worker/src/osprey/worker/ui_api/osprey/views/tests/test_rule_drafts.py @@ -5,6 +5,17 @@ from flask import Response, url_for from flask.testing import FlaskClient from osprey.worker.lib.snowflake import Snowflake +from osprey.worker.lib.storage.postgres import scoped_session +from osprey.worker.lib.storage.rule_drafts import RuleDraft + + +@pytest.fixture(autouse=True) +def _clear_rule_drafts(): + # The test database is session-scoped, so drafts persist across tests. Start each + # test with an empty table, or leaked rows trip the rule-name uniqueness check. + with scoped_session(commit=True) as session: + session.query(RuleDraft).delete() + yield @pytest.fixture(autouse=True) @@ -427,6 +438,8 @@ def test_deploy_wire_into_main_409_when_main_missing( res = client.post(url_for('rule_drafts.deploy_draft', draft_id=draft_id), json={'wire_into_main': True}) assert res.status_code == 409 + # The rule file must not be written when the deploy 409s on a missing main.sml. + assert not (tmp_path / 'rules' / 'nomain.sml').exists() @pytest.mark.use_rules_sources(_base_sources) From 4089033994ff05a8bfec03f64c8f149ebd58516a Mon Sep 17 00:00:00 2001 From: Juliet Shen Date: Sun, 19 Jul 2026 13:46:31 -0700 Subject: [PATCH 5/9] Clarify get_source: it serves on-disk rules, not drafts @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/. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01TEcfpqhza3dPepZUVWw36X --- .../src/osprey/worker/ui_api/osprey/views/rule_drafts.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/osprey_worker/src/osprey/worker/ui_api/osprey/views/rule_drafts.py b/osprey_worker/src/osprey/worker/ui_api/osprey/views/rule_drafts.py index 36d1e339..dd3f1ba1 100644 --- a/osprey_worker/src/osprey/worker/ui_api/osprey/views/rule_drafts.py +++ b/osprey_worker/src/osprey/worker/ui_api/osprey/views/rule_drafts.py @@ -112,6 +112,11 @@ def _current_sources_dict() -> dict[str, str]: @blueprint.route('/rule-drafts/source', methods=['GET']) @require_ability(CanEditRuleDrafts) def get_source() -> Any: + """Return the source of a rule already loaded by the engine (i.e. on disk). + + This is for editing an existing rule; it does not read the rule_drafts table. + A draft's own SML is loaded from the table via `GET /rule-drafts/`. + """ path = request.args.get('path', '').strip() err = _validate_path(path) if err: @@ -120,7 +125,7 @@ def get_source() -> Any: engine = ENGINE.instance() source: Source | None = engine.execution_graph.validated_sources.sources.get_by_path(path) if source is None: - return jsonify({'error': f'No source found at {path!r}.'}), 404 + return jsonify({'error': f'No rule found at {path!r}.'}), 404 return jsonify({'path': source.path, 'contents': source.contents}) From 2c65500d6b2fd0eef679dab5b3fc8d522d77f71d Mon Sep 17 00:00:00 2001 From: Juliet Shen Date: Tue, 21 Jul 2026 22:50:17 -0700 Subject: [PATCH 6/9] Address review: config-read rules dir, clearer path error, list default 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 Claude-Session: https://claude.ai/code/session_01TEcfpqhza3dPepZUVWw36X --- .../worker/ui_api/osprey/views/rule_drafts.py | 12 +++++---- .../osprey/views/tests/test_rule_drafts.py | 25 ++++++++++++++----- 2 files changed, 26 insertions(+), 11 deletions(-) diff --git a/osprey_worker/src/osprey/worker/ui_api/osprey/views/rule_drafts.py b/osprey_worker/src/osprey/worker/ui_api/osprey/views/rule_drafts.py index dd3f1ba1..5af4baa5 100644 --- a/osprey_worker/src/osprey/worker/ui_api/osprey/views/rule_drafts.py +++ b/osprey_worker/src/osprey/worker/ui_api/osprey/views/rule_drafts.py @@ -1,7 +1,6 @@ from __future__ import annotations import logging -import os import re from collections.abc import Iterable from dataclasses import dataclass, field @@ -32,7 +31,7 @@ ValidationFailed, ValidationWarning, ) -from osprey.worker.lib.singletons import ENGINE +from osprey.worker.lib.singletons import CONFIG, ENGINE from osprey.worker.lib.storage.rule_drafts import RuleDraft from osprey.worker.ui_api.osprey.lib.abilities import CanEditRuleDrafts, require_ability from osprey.worker.ui_api.osprey.lib.auth import get_current_user_email @@ -85,7 +84,7 @@ def _suggest_imports_from_errors( """ suggested: set[str] = set() for err in errors: - for path in err.get('defined_in_source_paths') or []: + for path in err.get('defined_in_source_paths', []): if path == 'main.sml' or path == draft_path: continue suggested.add(path) @@ -94,7 +93,10 @@ def _suggest_imports_from_errors( 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_/-]).' + return ( + f'Path {path!r} is not a valid SML source path. It must end in .sml and contain only ' + 'letters, numbers, underscores, slashes, and hyphens.' + ) if path.startswith('/'): # Rule paths are relative to the rules directory; an absolute path would # escape it. Deploy also guards this with _resolve_within, but reject early. @@ -437,7 +439,7 @@ def _rules_dir_or_error() -> tuple[Path | None, tuple[Any, int] | None]: 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() + raw = CONFIG.instance().get_str('OSPREY_RULES_LOCAL_PATH', '').strip() if not raw: return None, ( jsonify( diff --git a/osprey_worker/src/osprey/worker/ui_api/osprey/views/tests/test_rule_drafts.py b/osprey_worker/src/osprey/worker/ui_api/osprey/views/tests/test_rule_drafts.py index d0d8779f..1d0ac5dd 100644 --- a/osprey_worker/src/osprey/worker/ui_api/osprey/views/tests/test_rule_drafts.py +++ b/osprey_worker/src/osprey/worker/ui_api/osprey/views/tests/test_rule_drafts.py @@ -4,11 +4,24 @@ import pytest from flask import Response, url_for from flask.testing import FlaskClient +from osprey.worker.lib.singletons import CONFIG from osprey.worker.lib.snowflake import Snowflake from osprey.worker.lib.storage.postgres import scoped_session from osprey.worker.lib.storage.rule_drafts import RuleDraft +def _set_rules_dir(monkeypatch: pytest.MonkeyPatch, path: object) -> None: + # Deploy reads OSPREY_RULES_LOCAL_PATH via CONFIG, which is bound once at app + # setup, so set it on the already-bound config for the deploy handler to see. + monkeypatch.setenv('OSPREY_RULES_LOCAL_PATH', str(path)) + CONFIG.instance()._config_dict['OSPREY_RULES_LOCAL_PATH'] = str(path) + + +def _unset_rules_dir(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.delenv('OSPREY_RULES_LOCAL_PATH', raising=False) + CONFIG.instance()._config_dict.pop('OSPREY_RULES_LOCAL_PATH', None) + + @pytest.fixture(autouse=True) def _clear_rule_drafts(): # The test database is session-scoped, so drafts persist across tests. Start each @@ -364,7 +377,7 @@ def test_get_draft_returns_one_and_404s(client: 'FlaskClient[Response]') -> None def test_deploy_writes_sml_and_marks_deployed( client: 'FlaskClient[Response]', monkeypatch: pytest.MonkeyPatch, tmp_path ) -> None: - monkeypatch.setenv('OSPREY_RULES_LOCAL_PATH', str(tmp_path)) + _set_rules_dir(monkeypatch, tmp_path) created = client.post( url_for('rule_drafts.create_draft'), json={'path': 'rules/deploy.sml', 'rule_name': 'SomeRule', 'source': _VALID_DRAFT, 'summary': ''}, @@ -389,7 +402,7 @@ def test_deploy_wire_into_main_appends_require( client: 'FlaskClient[Response]', monkeypatch: pytest.MonkeyPatch, tmp_path ) -> None: (tmp_path / 'main.sml').write_text("Import(rules=['models/base.sml'])\n") - monkeypatch.setenv('OSPREY_RULES_LOCAL_PATH', str(tmp_path)) + _set_rules_dir(monkeypatch, tmp_path) created = client.post( url_for('rule_drafts.create_draft'), json={'path': 'rules/wired.sml', 'rule_name': 'SomeRule', 'source': _VALID_DRAFT, 'summary': ''}, @@ -409,7 +422,7 @@ def test_deploy_wire_into_main_is_idempotent( client: 'FlaskClient[Response]', monkeypatch: pytest.MonkeyPatch, tmp_path ) -> None: (tmp_path / 'main.sml').write_text("Import(rules=['models/base.sml'])\nRequire(rule='rules/already.sml')\n") - monkeypatch.setenv('OSPREY_RULES_LOCAL_PATH', str(tmp_path)) + _set_rules_dir(monkeypatch, tmp_path) created = client.post( url_for('rule_drafts.create_draft'), json={'path': 'rules/already.sml', 'rule_name': 'SomeRule', 'source': _VALID_DRAFT, 'summary': ''}, @@ -428,7 +441,7 @@ def test_deploy_wire_into_main_is_idempotent( def test_deploy_wire_into_main_409_when_main_missing( client: 'FlaskClient[Response]', monkeypatch: pytest.MonkeyPatch, tmp_path ) -> None: - monkeypatch.setenv('OSPREY_RULES_LOCAL_PATH', str(tmp_path)) + _set_rules_dir(monkeypatch, tmp_path) created = client.post( url_for('rule_drafts.create_draft'), json={'path': 'rules/nomain.sml', 'rule_name': 'SomeRule', 'source': _VALID_DRAFT, 'summary': ''}, @@ -444,7 +457,7 @@ def test_deploy_wire_into_main_409_when_main_missing( @pytest.mark.use_rules_sources(_base_sources) def test_deploy_503_when_rules_dir_unset(client: 'FlaskClient[Response]', monkeypatch: pytest.MonkeyPatch) -> None: - monkeypatch.delenv('OSPREY_RULES_LOCAL_PATH', raising=False) + _unset_rules_dir(monkeypatch) created = client.post( url_for('rule_drafts.create_draft'), json={'path': 'rules/nodir.sml', 'rule_name': 'SomeRule', 'source': _VALID_DRAFT, 'summary': ''}, @@ -460,6 +473,6 @@ def test_deploy_503_when_rules_dir_unset(client: 'FlaskClient[Response]', monkey def test_deploy_404_for_unknown_draft( client: 'FlaskClient[Response]', monkeypatch: pytest.MonkeyPatch, tmp_path ) -> None: - monkeypatch.setenv('OSPREY_RULES_LOCAL_PATH', str(tmp_path)) + _set_rules_dir(monkeypatch, tmp_path) res = client.post(url_for('rule_drafts.deploy_draft', draft_id=999999), json={}) assert res.status_code == 404 From ba29dff13cd9cbdbf2caf27d3bd3917f9f557aaa Mon Sep 17 00:00:00 2001 From: Juliet Shen Date: Thu, 2 Jul 2026 10:52:01 -0400 Subject: [PATCH 7/9] Add rule authoring/editing UI backed by the rule-drafts API New RuleEditorPage at /rules/new and /rules/edit?path=: - Rule Builder view: form-based conditions and outcomes compiled to SML client-side, with vocabulary (features, UDFs, effects) fetched from the engine; falls back to a plain Code Editor view for any SML the builder subset can't represent (decided via /parse-into-builder) - Live validation against the engine's AST validator with a 600ms debounce, structured inline errors, and a missing-imports quick fix - Submit posts the draft to /rule-drafts/submit and surfaces the resulting review URL; RulesPage gains an Add rule button, per-rule Edit links, and a pending-drafts banner fed by /rule-drafts/pending Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01VZ4RQtuHCCgurfpjfPXXAM --- osprey_ui/src/App.tsx | 7 + osprey_ui/src/Constants.tsx | 2 + osprey_ui/src/actions/RulesActions.tsx | 80 +- .../rules/RuleEditorPage.module.css | 151 ++++ .../src/components/rules/RuleEditorPage.tsx | 754 ++++++++++++++++++ osprey_ui/src/components/rules/RulesPage.tsx | 108 ++- .../src/components/rules/ruleBuilderSml.ts | 210 +++++ osprey_ui/src/types/RulesTypes.tsx | 102 +++ 8 files changed, 1400 insertions(+), 14 deletions(-) create mode 100644 osprey_ui/src/components/rules/RuleEditorPage.module.css create mode 100644 osprey_ui/src/components/rules/RuleEditorPage.tsx create mode 100644 osprey_ui/src/components/rules/ruleBuilderSml.ts diff --git a/osprey_ui/src/App.tsx b/osprey_ui/src/App.tsx index 0b52a3a2..097580bd 100644 --- a/osprey_ui/src/App.tsx +++ b/osprey_ui/src/App.tsx @@ -6,6 +6,7 @@ import { getApplicationConfig } from './actions/ConfigActions'; import UdfDocsView from './components/docs/UdfDocsView'; import BulkJobHistoryView from './components/bulk_job_history/BulkJobHistory'; import { FeaturesPage } from './components/features/FeaturesPage'; +import { RuleEditorPage } from './components/rules/RuleEditorPage'; import { RulesPage } from './components/rules/RulesPage'; import RulesVisualizerView from './components/rules_visualizer/RulesVisualizer'; import EntityViewBar from './components/entities/EntityViewBar'; @@ -105,6 +106,12 @@ const AppRouter: React.FC = () => { + + + + + + diff --git a/osprey_ui/src/Constants.tsx b/osprey_ui/src/Constants.tsx index 7e9769a8..8ebb35d1 100644 --- a/osprey_ui/src/Constants.tsx +++ b/osprey_ui/src/Constants.tsx @@ -6,6 +6,8 @@ export const Routes = { ENTITY: '/entity/:entityType/:entityId', FEATURES: '/features', RULES: '/rules', + RULES_NEW: '/rules/new', + RULES_EDIT: '/rules/edit', SAVED_QUERY: '/saved-query/:savedQueryId', SAVED_QUERY_LATEST: '/saved-query/:savedQueryId/latest', BULK_JOB_HISTORY: '/bulk-job-history', diff --git a/osprey_ui/src/actions/RulesActions.tsx b/osprey_ui/src/actions/RulesActions.tsx index ad5e63b9..6709e918 100644 --- a/osprey_ui/src/actions/RulesActions.tsx +++ b/osprey_ui/src/actions/RulesActions.tsx @@ -1,5 +1,13 @@ import HTTPUtils, { HTTPResponse } from '../utils/HTTPUtils'; -import { RulesListResponse } from '../types/RulesTypes'; +import { + ParseIntoBuilderResponse, + PendingDraftsResponse, + RuleDraftSourceResponse, + RuleDraftSubmitResponse, + RuleDraftValidationResponse, + RuleDraftVocabulary, + RulesListResponse, +} from '../types/RulesTypes'; export async function getRulesList(): Promise { const response: HTTPResponse = await HTTPUtils.get('rules'); @@ -8,3 +16,73 @@ export async function getRulesList(): Promise { } throw new Error(response.error.message ?? 'Failed to fetch rules list'); } + +export async function getRuleDraftSource(path: string): Promise { + const response: HTTPResponse = await HTTPUtils.get('rule-drafts/source', { params: { path } }); + if (response.ok) { + return response.data; + } + throw new Error(response.error.message ?? `Failed to fetch rule source at ${path}`); +} + +export async function validateRuleDraft(path: string, source: string): Promise { + // SML validation errors come back as 200 with {ok: false}; backend-shape problems come back as 400 + // with the same envelope. Both paths surface the structured errors to the UI without throwing. + const response: HTTPResponse = await HTTPUtils.post('rule-drafts/validate', { path, source }); + if (response.ok) { + return response.data; + } + if (response.error.response?.data) { + return response.error.response.data as RuleDraftValidationResponse; + } + throw new Error(response.error.message ?? 'Validation request failed'); +} + +export async function parseRuleDraftIntoBuilder(path: string, source: string): Promise { + const response: HTTPResponse = await HTTPUtils.post('rule-drafts/parse-into-builder', { path, source }); + if (response.ok) { + return response.data; + } + throw new Error(response.error.message ?? 'Failed to parse rule into builder model'); +} + +export async function getRuleDraftVocabulary(): Promise { + const response: HTTPResponse = await HTTPUtils.get('rule-drafts/vocabulary'); + if (response.ok) { + return response.data; + } + throw new Error(response.error.message ?? 'Failed to fetch rule vocabulary'); +} + +export interface SubmitRuleDraftBody { + path: string; + source: string; + rule_name: string; + summary: string; + is_new_rule: boolean; + wire_into_main?: boolean; + branch?: string; +} + +export async function submitRuleDraft(body: SubmitRuleDraftBody): Promise { + const response: HTTPResponse = await HTTPUtils.post('rule-drafts/submit', body); + if (response.ok) { + return response.data; + } + const errPayload = response.error.response?.data as { error?: string } | undefined; + throw new Error(errPayload?.error ?? response.error.message ?? 'Failed to submit rule draft'); +} + +export async function getPendingRuleDrafts(): Promise { + // Returns an empty list rather than throwing on failure so the RulesPage still renders + // when GitHub isn't configured. + const response: HTTPResponse = await HTTPUtils.get('rule-drafts/pending'); + if (response.ok) { + return response.data; + } + const errPayload = response.error.response?.data as PendingDraftsResponse | undefined; + if (errPayload && Array.isArray(errPayload.pending)) { + return errPayload; + } + return { pending: [], error: response.error.message ?? 'Failed to fetch pending drafts' }; +} diff --git a/osprey_ui/src/components/rules/RuleEditorPage.module.css b/osprey_ui/src/components/rules/RuleEditorPage.module.css new file mode 100644 index 00000000..d9f07acf --- /dev/null +++ b/osprey_ui/src/components/rules/RuleEditorPage.module.css @@ -0,0 +1,151 @@ +.viewContainer { + height: 100%; + width: 100%; + display: flex; + flex-direction: column; + overflow: hidden; +} + +.scrollArea { + flex: 1; + overflow-y: auto; + padding: 24px; +} + +.headerRow { + display: flex; + align-items: center; + justify-content: space-between; + gap: 16px; + margin-bottom: 16px; + flex-wrap: wrap; +} + +.headerLeft { + display: flex; + flex-direction: column; + gap: 2px; + min-width: 0; +} + +.headerActions { + display: flex; + gap: 8px; + align-items: center; +} + +.editorGrid { + display: grid; + grid-template-columns: minmax(0, 1fr) 360px; + gap: 16px; + align-items: start; +} + +@media (max-width: 1100px) { + .editorGrid { + grid-template-columns: 1fr; + } +} + +.codeArea { + font-family: 'SFMono-Regular', Consolas, 'Liberation Mono', Menlo, monospace !important; + font-size: 13px; + line-height: 1.5; + min-height: 480px; +} + +.sidePanel { + display: flex; + flex-direction: column; + gap: 12px; + position: sticky; + top: 0; +} + +.validationCard pre { + margin: 0; + font-family: 'SFMono-Regular', Consolas, 'Liberation Mono', Menlo, monospace; + font-size: 12px; + white-space: pre-wrap; + overflow-wrap: break-word; +} + +.errorList { + display: flex; + flex-direction: column; + gap: 8px; +} + +.errorItem { + padding: 8px 10px; + background: var(--background-secondary); + border-left: 3px solid var(--status-error); + border-radius: 2px; +} + +.warningItem { + padding: 8px 10px; + background: var(--background-secondary); + border-left: 3px solid var(--status-warning); + border-radius: 2px; +} + +.errorLocation { + font-family: 'SFMono-Regular', Consolas, 'Liberation Mono', Menlo, monospace; + font-size: 11px; + color: var(--text-light-secondary); +} + +.builderSection { + margin-bottom: 16px; +} + +.builderRow { + display: grid; + grid-template-columns: minmax(140px, 1fr) 130px minmax(140px, 1fr) 32px; + gap: 8px; + align-items: start; + margin-bottom: 8px; +} + +.builderRowOutcome { + display: grid; + grid-template-columns: minmax(140px, 1fr) 32px; + gap: 8px; + align-items: start; + margin-bottom: 8px; +} + +.builderArgsGrid { + display: grid; + grid-template-columns: 120px 1fr; + gap: 6px; + margin-top: 4px; + margin-left: 0; + padding: 8px; + background: var(--background-secondary); + border-radius: 4px; +} + +.builderArgLabel { + font-family: 'SFMono-Regular', Consolas, 'Liberation Mono', Menlo, monospace; + font-size: 12px; + align-self: center; +} + +.previewBlock { + font-family: 'SFMono-Regular', Consolas, 'Liberation Mono', Menlo, monospace; + font-size: 12px; + padding: 12px; + background: var(--background-secondary); + border: 1px solid var(--divider); + border-radius: 4px; + white-space: pre-wrap; + overflow-wrap: break-word; +} + +.footnote { + font-size: 11px; + color: var(--text-light-secondary); + margin-top: 4px; +} diff --git a/osprey_ui/src/components/rules/RuleEditorPage.tsx b/osprey_ui/src/components/rules/RuleEditorPage.tsx new file mode 100644 index 00000000..bc8b0e13 --- /dev/null +++ b/osprey_ui/src/components/rules/RuleEditorPage.tsx @@ -0,0 +1,754 @@ +import * as React from 'react'; +import { + Alert, + Button, + Card, + Checkbox, + Form, + Input, + Segmented, + Select, + Space, + Tag, + Tooltip, + Typography, + message, +} from 'antd'; +import { DeleteOutlined, PlusOutlined, SaveOutlined } from '@ant-design/icons'; +import { useHistory, useLocation } from 'react-router-dom'; + +import { + getRuleDraftSource, + getRuleDraftVocabulary, + parseRuleDraftIntoBuilder, + submitRuleDraft, + validateRuleDraft, +} from '../../actions/RulesActions'; +import usePromiseResult from '../../hooks/usePromiseResult'; +import { + ParseIntoBuilderResponse, + RuleDraftValidationMessage, + RuleDraftValidationResponse, + RuleDraftVocabulary, +} from '../../types/RulesTypes'; +import { renderFromPromiseResult } from '../../utils/PromiseResultUtils'; + +import { + CONDITION_OPERATOR_OPTIONS, + Condition, + ConditionOperator, + EMPTY_BUILDER_MODEL, + Outcome, + OutcomeArg, + RuleBuilderModel, + SML_IDENTIFIER_RE, + applyMissingImports, + generateSmlFromBuilder, + outcomeArgsForEffect, +} from './ruleBuilderSml'; + +import styles from './RuleEditorPage.module.css'; + +const { Title, Text, Paragraph } = Typography; + +type EditorMode = 'builder' | 'code'; + +const VALIDATE_DEBOUNCE_MS = 600; + +interface BootstrapData { + vocabulary: RuleDraftVocabulary; + initialSource: string; + initialPath: string; + isNewRule: boolean; + // For edit mode: the result of round-tripping the loaded source through the + // backend parser. Determines whether the Rule Builder toggle is enabled and + // what model the builder starts from. + initialBuilderParse?: ParseIntoBuilderResponse; +} + +export const RuleEditorPage: React.FC = () => { + // The edit path lives in `?path=` because react-router v5 has no clean + // repeating-segment param and rule paths contain slashes. + const location = useLocation(); + const isNewRule = location.pathname === '/rules/new'; + const editPath = isNewRule ? undefined : (new URLSearchParams(location.search).get('path') ?? undefined); + + const result = usePromiseResult(async () => { + const vocabulary = await getRuleDraftVocabulary(); + if (isNewRule) { + return { + vocabulary, + initialSource: '', + initialPath: 'rules/new_rule.sml', + isNewRule: true, + }; + } + if (!editPath) { + throw new Error('Missing ?path= query parameter; navigate from the Rules page.'); + } + const source = await getRuleDraftSource(editPath); + const initialBuilderParse = await parseRuleDraftIntoBuilder(source.path, source.contents); + return { + vocabulary, + initialSource: source.contents, + initialPath: source.path, + isNewRule: false, + initialBuilderParse, + }; + }, [editPath, isNewRule]); + + return renderFromPromiseResult(result, (data) => { + return ; + }); +}; + +const RuleEditorView: React.FC<{ data: BootstrapData }> = ({ data }) => { + const history = useHistory(); + // Builder is allowed for new rules and for edits whose source round-trips. + const builderAllowed = data.isNewRule || data.initialBuilderParse?.supported === true; + const builderDisabledReason = + !data.isNewRule && data.initialBuilderParse?.supported === false ? data.initialBuilderParse.reason : ''; + const [mode, setMode] = React.useState(builderAllowed && data.isNewRule ? 'builder' : 'code'); + const [path, setPath] = React.useState(data.initialPath); + const [codeSource, setCodeSource] = React.useState(data.initialSource); + const [builder, setBuilder] = React.useState(() => { + if (data.initialBuilderParse?.supported === true) { + return data.initialBuilderParse.model; + } + return EMPTY_BUILDER_MODEL; + }); + const [summary, setSummary] = React.useState(''); + // Off by default: turning a rule on is a deliberate opt-in, so a submit never + // wires a new rule into the live ruleset unless the author checks the box. + const [wireIntoMain, setWireIntoMain] = React.useState(false); + const [validation, setValidation] = React.useState(null); + const [isValidating, setIsValidating] = React.useState(false); + const [submitState, setSubmitState] = React.useState< + | { kind: 'idle' } + | { kind: 'submitting' } + | { kind: 'done'; title: string; prUrl: string | null } + | { kind: 'error'; message: string } + >({ kind: 'idle' }); + + const effectiveSource = mode === 'builder' ? generateSmlFromBuilder(builder, data.vocabulary.features) : codeSource; + + React.useEffect(() => { + let cancelled = false; + if (!effectiveSource.trim()) { + // eslint-disable-next-line react-hooks/set-state-in-effect -- clearing stale validation on input empty + setValidation(null); + setIsValidating(false); + return; + } + setIsValidating(true); + const handle = window.setTimeout(async () => { + try { + const result = await validateRuleDraft(path, effectiveSource); + if (!cancelled) setValidation(result); + } catch (e) { + if (!cancelled) { + setValidation({ + ok: false, + errors: [ + { + message: e instanceof Error ? e.message : String(e), + hint: '', + source_path: path, + line: 0, + column: 0, + rendered: '', + }, + ], + warnings: [], + }); + } + } finally { + if (!cancelled) setIsValidating(false); + } + }, VALIDATE_DEBOUNCE_MS); + return () => { + cancelled = true; + window.clearTimeout(handle); + }; + }, [effectiveSource, path]); + + const ruleNameForSubmit = mode === 'builder' ? builder.ruleName : guessRuleNameFromSource(codeSource); + + const canSubmit = + !!validation?.ok && + SML_IDENTIFIER_RE.test(ruleNameForSubmit) && + submitState.kind !== 'submitting' && + !!effectiveSource.trim(); + + // The builder and code editor hold independent state, so a tab switch has to + // carry content across: builder -> code dumps the generated SML into the + // textarea, code -> builder re-parses the (possibly hand-edited) source so + // the form never silently submits a stale model. + const onModeChange = async (next: EditorMode) => { + if (next === mode) return; + if (next === 'code') { + setCodeSource(generateSmlFromBuilder(builder, data.vocabulary.features)); + setMode('code'); + return; + } + if (!codeSource.trim()) { + setMode('builder'); + return; + } + try { + const parsed = await parseRuleDraftIntoBuilder(path, codeSource); + if (parsed.supported) { + setBuilder(parsed.model); + setMode('builder'); + } else { + message.warning(`Rule Builder can't represent this file: ${parsed.reason}. Keep editing in Code Editor.`); + } + } catch (e) { + message.warning(`Could not parse this file for Rule Builder: ${e instanceof Error ? e.message : String(e)}`); + } + }; + + const onSubmit = async () => { + if (!canSubmit) return; + setSubmitState({ kind: 'submitting' }); + try { + const res = await submitRuleDraft({ + path, + source: effectiveSource, + rule_name: ruleNameForSubmit, + summary, + is_new_rule: data.isNewRule, + wire_into_main: wireIntoMain, + }); + setSubmitState({ kind: 'done', title: res.title, prUrl: res.url }); + const wiredMsg = res.main_sml_updated ? ' (main.sml updated)' : ''; + message.success(`${res.title}${wiredMsg}.`); + } catch (e) { + const msg = e instanceof Error ? e.message : String(e); + setSubmitState({ kind: 'error', message: msg }); + } + }; + + return ( +
+
+
+
+ + {data.isNewRule ? 'Add rule' : 'Edit rule'} + + + Drafts open a pull request against the rules repo. Nothing applies until the PR is merged and the engine + reloads. + +
+
+ + { + void onModeChange(value as EditorMode); + }} + options={[ + { label: 'Rule Builder', value: 'builder', disabled: !builderAllowed }, + { label: 'Code Editor', value: 'code' }, + ]} + /> + + + +
+
+ + + +
+
+ +
+ + setPath(e.target.value)} disabled={!data.isNewRule} /> + + + setSummary(e.target.value)} + placeholder={ + data.isNewRule + ? 'Why do we need this rule? What behaviour does it target?' + : 'What are you changing about this rule, and why?' + } + autoSize={{ minRows: 2, maxRows: 4 }} + /> + + + setWireIntoMain(e.target.checked)}> + Turn this rule on once the review is approved. + +
+ Adds your rule to the list Osprey runs, as part of the same review. If it's already on the + list, nothing changes. +
+ +
+ + + {mode === 'code' && validation?.suggested_imports && validation.suggested_imports.length > 0 && ( + + This file references identifiers defined in: {validation.suggested_imports.join(', ')} + + + } + /> + )} + + {mode === 'builder' ? ( + + ) : ( + + )} + + {mode === 'builder' && ( + + 
{effectiveSource}
+
+ This is the code that will be submitted in a pull request on Github. Make further changes in the Code + Editor view. +
+ + )} +
+ + +
+
+
+ ); +}; + +const SubmitBanner: React.FC<{ + submitState: + | { kind: 'idle' } + | { kind: 'submitting' } + | { kind: 'done'; title: string; prUrl: string | null } + | { kind: 'error'; message: string }; +}> = ({ submitState }) => { + if (submitState.kind === 'idle') return null; + if (submitState.kind === 'submitting') { + return ; + } + if (submitState.kind === 'done') { + return ( + + {submitState.prUrl} + + ) : null + } + /> + ); + } + return ( + + ); +}; + +const CodeEditorMode: React.FC<{ source: string; setSource: (next: string) => void }> = ({ source, setSource }) => { + return ( + + setSource(e.target.value)} + placeholder="MyRule = Rule(when_all=[PostText == 'hello'], description='...')" + autoSize={{ minRows: 24, maxRows: 60 }} + spellCheck={false} + /> + + ); +}; + +const ValidationPanel: React.FC<{ + validation: RuleDraftValidationResponse | null; + isValidating: boolean; +}> = ({ validation, isValidating }) => { + return ( + + Validation + {isValidating && checking…} + {!isValidating && validation?.ok === true && valid} + {!isValidating && validation?.ok === false && errors} + + } + > + {!validation && Start typing to see live validation against the engine.} + {validation?.ok === true && validation.warnings.length === 0 && ( + Engine accepts this draft. + )} + {validation?.errors && validation.errors.length > 0 && ( +
+ {validation.errors.map((err, i) => { + return ; + })} +
+ )} + {validation?.warnings && validation.warnings.length > 0 && ( +
+ {validation.warnings.map((w, i) => { + return ; + })} +
+ )} + + ); +}; + +const ValidationMessageRow: React.FC<{ kind: 'error' | 'warning'; msg: RuleDraftValidationMessage }> = ({ + kind, + msg, +}) => { + return ( +
+
{msg.message}
+ {msg.hint && ( +
+ {msg.hint} +
+ )} +
+ {msg.source_path}:{msg.line}:{msg.column} +
+
+ ); +}; + +const VocabularyPanel: React.FC<{ vocabulary: RuleDraftVocabulary }> = ({ vocabulary }) => { + return ( + + + Variables you can reference inside conditions. + + + {vocabulary.features.slice(0, 60).map((f) => { + return ( + + {f.name} + + ); + })} + {vocabulary.features.length > 60 && +{vocabulary.features.length - 60} more} + + {vocabulary.effects.length > 0 && ( + <> + + Effects used in existing rules. + + + {vocabulary.effects.map((name) => { + return ( + + {name} + + ); + })} + + + )} + + ); +}; + +const RuleBuilderEditor: React.FC<{ + model: RuleBuilderModel; + setModel: React.Dispatch>; + vocabulary: RuleDraftVocabulary; +}> = ({ model, setModel, vocabulary }) => { + const featureOptions = React.useMemo(() => { + return vocabulary.features.map((f) => { + return { label: f.name, value: f.name }; + }); + }, [vocabulary.features]); + + const effectOptions = React.useMemo(() => { + return vocabulary.effects.map((name) => { + return { label: name, value: name }; + }); + }, [vocabulary.effects]); + + const updateCondition = (idx: number, patch: Partial) => { + setModel((prev) => { + const next = [...prev.conditions]; + next[idx] = { ...next[idx], ...patch }; + return { ...prev, conditions: next }; + }); + }; + const addCondition = () => { + setModel((prev) => ({ + ...prev, + conditions: [...prev.conditions, { feature: '', operator: '==', rhs: '', rhsIsFeature: false }], + })); + }; + const removeCondition = (idx: number) => { + setModel((prev) => ({ + ...prev, + conditions: prev.conditions.filter((_, i) => { + return i !== idx; + }), + })); + }; + + const updateOutcome = (idx: number, patch: Partial) => { + setModel((prev) => { + const next = [...prev.outcomes]; + next[idx] = { ...next[idx], ...patch }; + return { ...prev, outcomes: next }; + }); + }; + const updateOutcomeArg = (oIdx: number, aIdx: number, patch: Partial) => { + setModel((prev) => { + const outcomes = [...prev.outcomes]; + const args = [...outcomes[oIdx].args]; + args[aIdx] = { ...args[aIdx], ...patch }; + outcomes[oIdx] = { ...outcomes[oIdx], args }; + return { ...prev, outcomes }; + }); + }; + const addOutcome = () => { + setModel((prev) => ({ ...prev, outcomes: [...prev.outcomes, { effect: '', args: [] }] })); + }; + const removeOutcome = (idx: number) => { + setModel((prev) => ({ + ...prev, + outcomes: prev.outcomes.filter((_, i) => { + return i !== idx; + }), + })); + }; + + return ( + +
+ + setModel((prev) => ({ ...prev, ruleName: e.target.value }))} + placeholder="ContainsHello" + /> + + + setModel((prev) => ({ ...prev, description: e.target.value }))} + autoSize={{ minRows: 1, maxRows: 3 }} + placeholder="What does the rule detect?" + /> + +
+ +
+ + Conditions + + + Every row must be true for the rule to fire. SML's when_all is AND-only. For OR, write the + extra rule in Code Editor. + + {model.conditions.map((cond, idx) => { + return ( +
+ updateCondition(idx, { rhs: value })} + options={featureOptions} + style={{ width: '100%' }} + filterOption={(input, opt) => { + return String(opt?.label).toLowerCase().includes(input.toLowerCase()); + }} + /> + ) : ( + updateCondition(idx, { rhs: e.target.value })} + /> + )} + + +
+ ); + })} + +
+ +
+ + Outcomes + + + Wrapped in a WhenRules(then=[…]) block that fires when the rule matches. + + {model.outcomes.map((outcome, oIdx) => { + return ( +
+
+ updateOutcomeArg(oIdx, aIdx, { value })} + options={featureOptions} + style={{ width: '100%' }} + filterOption={(input, opt) => { + return String(opt?.label).toLowerCase().includes(input.toLowerCase()); + }} + /> + ) : ( + updateOutcomeArg(oIdx, aIdx, { value: e.target.value })} + /> + )} + + + + ); + })} +
+ )} +
+ ); + })} + +
+ + ); +}; + +function guessRuleNameFromSource(source: string): string { + const m = source.match(/^\s*([A-Za-z_][A-Za-z0-9_]*)\s*=\s*Rule\s*\(/m); + return m?.[1] ?? ''; +} diff --git a/osprey_ui/src/components/rules/RulesPage.tsx b/osprey_ui/src/components/rules/RulesPage.tsx index 580abadb..21f01512 100644 --- a/osprey_ui/src/components/rules/RulesPage.tsx +++ b/osprey_ui/src/components/rules/RulesPage.tsx @@ -1,5 +1,7 @@ import * as React from 'react'; import { + Alert, + Button, Card, Collapse, Descriptions, @@ -14,11 +16,12 @@ import { Tooltip, Typography, } from 'antd'; -import { SearchOutlined } from '@ant-design/icons'; +import { EditOutlined, PlusOutlined, SearchOutlined } from '@ant-design/icons'; +import { Link } from 'react-router-dom'; -import { getRulesList } from '../../actions/RulesActions'; -import usePromiseResult from '../../hooks/usePromiseResult'; -import { RuleInfo, RulesListResponse, SortKey } from '../../types/RulesTypes'; +import { getPendingRuleDrafts, getRulesList } from '../../actions/RulesActions'; +import usePromiseResult, { PromiseResultStatus } from '../../hooks/usePromiseResult'; +import { PendingDraft, PendingDraftsResponse, RuleInfo, RulesListResponse, SortKey } from '../../types/RulesTypes'; import { renderFromPromiseResult } from '../../utils/PromiseResultUtils'; import styles from './RulesPage.module.css'; @@ -73,13 +76,19 @@ export const RulesPage: React.FC = () => { const result = usePromiseResult(() => { return getRulesList(); }); + const pendingResult = usePromiseResult(() => { + return getPendingRuleDrafts(); + }); return renderFromPromiseResult(result, (data) => { - return ; + return ; }); }; -const RulesPageContent: React.FC<{ data: RulesListResponse }> = ({ data }) => { +const RulesPageContent: React.FC<{ + data: RulesListResponse; + pendingResult: ReturnType>; +}> = ({ data, pendingResult }) => { const [filters, dispatch] = React.useReducer(filtersReducer, INITIAL_FILTERS); const { rules, total, when_rules_total, unused_total } = data; const { search, unusedOnly, sortKey, page, pageSize } = filters; @@ -132,13 +141,32 @@ const RulesPageContent: React.FC<{ data: RulesListResponse }> = ({ data }) => { return (
- - Rules Registry - - - Named rule definitions across the engine — conditions, descriptions, the features each rule references, and - how many WhenRules blocks include it. - +
+
+ + Rules Registry + + + Named rule definitions across the engine — conditions, descriptions, the features each rule references, + and how many WhenRules blocks include it. + +
+ + + +
+ +
@@ -262,11 +290,65 @@ const RuleHeader: React.FC<{ rule: RuleInfo }> = ({ rule }) => { {rule.referenced_by_whenrules} when-rules )} + + { + // Prevent the surrounding Collapse panel from toggling open. + e.stopPropagation(); + }} + > + + +
); }; +const PendingDraftsBanner: React.FC<{ + pendingResult: ReturnType>; +}> = ({ pendingResult }) => { + if (pendingResult.status !== PromiseResultStatus.Resolved) return null; + const { pending, error } = pendingResult.value; + if (error && pending.length === 0) { + // GitHub backend not configured or unreachable; the rest of the page works without it. + return null; + } + if (pending.length === 0) return null; + return ( + + {pending.slice(0, 8).map((p, i) => { + return ; + })} + {pending.length > 8 && +{pending.length - 8} more in review.} + + } + /> + ); +}; + +const PendingDraftRow: React.FC<{ draft: PendingDraft }> = ({ draft }) => { + return ( +
+ + {draft.title} + {' '} + + by {draft.author}, {draft.touched_files.join(', ')} + +
+ ); +}; + const RuleDetail: React.FC<{ rule: RuleInfo }> = ({ rule }) => { return ( ', label: 'is greater than' }, + { value: '<', label: 'is less than' }, + { value: '>=', label: 'is greater than or equal to' }, + { value: '<=', label: 'is less than or equal to' }, + { value: 'includes', label: 'includes' }, + { value: 'excludes', label: 'excludes' }, +]; + +export const EMPTY_BUILDER_MODEL: RuleBuilderModel = { + ruleName: '', + description: '', + conditions: [{ feature: '', operator: '==', rhs: '', rhsIsFeature: false }], + outcomes: [{ effect: '', args: [] }], +}; + +export const SML_IDENTIFIER_RE = /^[A-Za-z_][A-Za-z0-9_]*$/; + +// SML string literals follow Python escaping rules (the parser is Python's ast +// module), so backslashes must be escaped before quotes or a trailing `\` in +// user input would swallow the closing quote and inject raw SML. +function smlString(value: string): string { + const escaped = value.replace(/\\/g, '\\\\').replace(/'/g, "\\'").replace(/\r/g, '\\r').replace(/\n/g, '\\n'); + return `'${escaped}'`; +} + +function renderRhs(rhs: string, isFeature: boolean): string { + if (isFeature) return rhs; + if (rhs === '') return "''"; + if (/^-?\d+(\.\d+)?$/.test(rhs)) return rhs; + if (rhs === 'true' || rhs === 'false') return rhs; + return smlString(rhs); +} + +function renderConditionExpression(c: RuleBuilderCondition): string { + const lhs = c.feature || '__missing_feature__'; + const rhs = renderRhs(c.rhs, c.rhsIsFeature); + switch (c.operator) { + case 'includes': + return `TextContains(text=${lhs}, phrase=${rhs})`; + case 'excludes': + return `not TextContains(text=${lhs}, phrase=${rhs})`; + default: + return `${lhs} ${c.operator} ${rhs}`; + } +} + +function renderOutcomeCall(o: RuleBuilderOutcome): string { + const effect = o.effect || '__missing_effect__'; + // Skip args left blank: emitting `entity=` is a syntax error, and emitting `expires_after=''` + // forces an empty literal where the UDF would otherwise use its default. + const parts = o.args + .filter((arg) => { + return arg.value !== ''; + }) + .map((arg) => { + return `${arg.name}=${renderRhs(arg.value, arg.isFeature)}`; + }); + return `${effect}(${parts.join(', ')})`; +} + +function collectReferencedFeatures(model: RuleBuilderModel): Set { + const refs = new Set(); + for (const c of model.conditions) { + if (c.feature) refs.add(c.feature); + if (c.rhsIsFeature && c.rhs) refs.add(c.rhs); + } + for (const o of model.outcomes) { + for (const arg of o.args) { + if (arg.isFeature && arg.value) refs.add(arg.value); + } + } + return refs; +} + +function buildImportBlock(refs: Set, features: RuleDraftVocabularyFeature[]): string { + // Map each referenced identifier to the SML file it's defined in, drop main.sml + // (the entry point cannot import itself), dedupe, and emit a single Import block. + const paths = new Set(); + for (const ref of refs) { + const feat = features.find((f) => { + return f.name === ref; + }); + if (!feat) continue; + if (feat.source_path === 'main.sml') continue; + paths.add(feat.source_path); + } + if (paths.size === 0) return ''; + const sorted = [...paths].sort(); + const entries = sorted + .map((p) => { + return ` '${p}',`; + }) + .join('\n'); + return `Import( + rules=[ +${entries} + ] +) + +`; +} + +export function generateSmlFromBuilder(model: RuleBuilderModel, features: RuleDraftVocabularyFeature[] = []): string { + // Never interpolate a non-identifier rule name: `Foo = Rule(...) # ` as a + // "name" would otherwise inject arbitrary SML that still validates. + const ruleName = SML_IDENTIFIER_RE.test(model.ruleName) ? model.ruleName : 'UnnamedRule'; + const whenAll = model.conditions + .map((c) => { + return ` ${renderConditionExpression(c)},`; + }) + .join('\n'); + const ruleBlock = `${ruleName} = Rule( + when_all=[ +${whenAll} + ], + description=${smlString(model.description)}, +)`; + + const imports = buildImportBlock(collectReferencedFeatures(model), features); + + if (model.outcomes.length === 0) { + return `${imports}${ruleBlock}\n`; + } + + const thenBlock = model.outcomes + .map((o) => { + return ` ${renderOutcomeCall(o)},`; + }) + .join('\n'); + + return `${imports}${ruleBlock} + +WhenRules( + rules_any=[${ruleName}], + then=[ +${thenBlock} + ], +) +`; +} + +/** + * Insert or merge the given source paths into the file's top-level + * `Import(rules=[...])` block. If an Import block already exists, missing + * entries are added in-place; otherwise a new block is prepended. + */ +export function applyMissingImports(source: string, pathsToAdd: string[]): string { + if (pathsToAdd.length === 0) return source; + const existingMatch = source.match(/Import\s*\(\s*rules\s*=\s*\[([^\]]*)\]\s*\)/m); + if (existingMatch) { + const existingPathsBlock = existingMatch[1]; + const existing = new Set( + Array.from(existingPathsBlock.matchAll(/['"]([^'"]+)['"]/g)).map((m) => { + return m[1]; + }) + ); + const merged = new Set(existing); + for (const p of pathsToAdd) merged.add(p); + if (merged.size === existing.size) return source; + const sorted = [...merged].sort(); + const entries = sorted + .map((p) => { + return ` '${p}',`; + }) + .join('\n'); + const replacement = `Import(\n rules=[\n${entries}\n ]\n)`; + return source.replace(existingMatch[0], replacement); + } + const sorted = [...pathsToAdd].sort(); + const entries = sorted + .map((p) => { + return ` '${p}',`; + }) + .join('\n'); + return `Import(\n rules=[\n${entries}\n ]\n)\n\n${source}`; +} + +export function outcomeArgsForEffect(effectName: string, udfs: RuleDraftVocabularyUdf[]): RuleBuilderOutcomeArg[] { + const match = udfs.find((u) => { + return u.name === effectName; + }); + if (!match) return []; + return match.arguments.map((arg) => { + // Defaults the value-vs-feature toggle for the form. Wrong guesses are one click to flip. + const looksLikeFeature = arg.name === 'entity' || /Id$|DID$/i.test(arg.name); + return { name: arg.name, value: '', isFeature: looksLikeFeature }; + }); +} diff --git a/osprey_ui/src/types/RulesTypes.tsx b/osprey_ui/src/types/RulesTypes.tsx index a0d78be1..db86833c 100644 --- a/osprey_ui/src/types/RulesTypes.tsx +++ b/osprey_ui/src/types/RulesTypes.tsx @@ -19,3 +19,105 @@ export enum SortKey { MostReferenced = 'most-referenced', LeastReferenced = 'least-referenced', } + +export interface RuleDraftValidationMessage { + message: string; + hint: string; + source_path: string; + line: number; + column: number; + rendered: string; + identifier?: string | null; + defined_in_source_paths?: string[]; +} + +export interface RuleDraftValidationResponse { + ok: boolean; + errors: RuleDraftValidationMessage[]; + warnings: RuleDraftValidationMessage[]; + suggested_imports?: string[]; +} + +export interface RuleDraftSourceResponse { + path: string; + contents: string; +} + +export interface RuleDraftVocabularyFeature { + name: string; + source_path: string; + source_line: number; +} + +export interface RuleDraftVocabularyUdfArgument { + name: string; + type_name: string; +} + +export interface RuleDraftVocabularyUdf { + name: string; + return_type: string; + arguments: RuleDraftVocabularyUdfArgument[]; +} + +export interface RuleDraftVocabulary { + features: RuleDraftVocabularyFeature[]; + udfs: RuleDraftVocabularyUdf[]; + effects: string[]; + source_files: string[]; +} + +export interface RuleDraftSubmitResponse { + // Backend-neutral fields produced by every RuleSubmissionBackend. + title: string; + url: string | null; + main_sml_updated: boolean; + // Backend-specific extras (e.g., pr_number, branch for the GitHub backend; + // path_on_disk for the local backend). + [extra: string]: unknown; +} + +export type ConditionOperator = '==' | '!=' | '>' | '<' | '>=' | '<=' | 'includes' | 'excludes'; + +export interface RuleBuilderCondition { + feature: string; + operator: ConditionOperator; + rhs: string; + rhsIsFeature: boolean; +} + +export interface RuleBuilderOutcomeArg { + name: string; + value: string; + isFeature: boolean; +} + +export interface RuleBuilderOutcome { + effect: string; + args: RuleBuilderOutcomeArg[]; +} + +export interface RuleBuilderModel { + ruleName: string; + description: string; + conditions: RuleBuilderCondition[]; + outcomes: RuleBuilderOutcome[]; +} + +export type ParseIntoBuilderResponse = + | { supported: true; model: RuleBuilderModel } + | { supported: false; reason: string }; + +export interface PendingDraft { + title: string; + url: string; + author: string; + created_at: string; + touched_files: string[]; + [extra: string]: unknown; +} + +export interface PendingDraftsResponse { + pending: PendingDraft[]; + error?: string; +} From 5292f9666748a591f21c604bef04ddcde34a6357 Mon Sep 17 00:00:00 2001 From: Juliet Shen Date: Tue, 14 Jul 2026 23:00:37 -0400 Subject: [PATCH 8/9] Update rule-authoring UI to the draft-table API The backend pivoted from git submission backends to a rule_drafts table (#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//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 Claude-Session: https://claude.ai/code/session_01TEcfpqhza3dPepZUVWw36X --- osprey_ui/src/actions/RulesActions.tsx | 42 +++-- .../src/components/rules/RuleEditorPage.tsx | 155 +++++++++++------- osprey_ui/src/components/rules/RulesPage.tsx | 49 +++--- osprey_ui/src/types/RulesTypes.tsx | 38 ++--- 4 files changed, 167 insertions(+), 117 deletions(-) diff --git a/osprey_ui/src/actions/RulesActions.tsx b/osprey_ui/src/actions/RulesActions.tsx index 6709e918..1f0ed549 100644 --- a/osprey_ui/src/actions/RulesActions.tsx +++ b/osprey_ui/src/actions/RulesActions.tsx @@ -1,9 +1,10 @@ import HTTPUtils, { HTTPResponse } from '../utils/HTTPUtils'; import { + DeployRuleDraftResponse, ParseIntoBuilderResponse, - PendingDraftsResponse, + RuleDraft, RuleDraftSourceResponse, - RuleDraftSubmitResponse, + RuleDraftsListResponse, RuleDraftValidationResponse, RuleDraftVocabulary, RulesListResponse, @@ -54,35 +55,48 @@ export async function getRuleDraftVocabulary(): Promise { throw new Error(response.error.message ?? 'Failed to fetch rule vocabulary'); } -export interface SubmitRuleDraftBody { +export interface CreateRuleDraftBody { path: string; source: string; rule_name: string; summary: string; - is_new_rule: boolean; +} + +// Saves a draft into the rule_drafts table (upserted by path). The draft is staged, +// not live; deployRuleDraft writes it into the rules directory. +export async function createRuleDraft(body: CreateRuleDraftBody): Promise { + const response: HTTPResponse = await HTTPUtils.post('rule-drafts', body); + if (response.ok) { + return response.data; + } + const errPayload = response.error.response?.data as { error?: string } | undefined; + throw new Error(errPayload?.error ?? response.error.message ?? 'Failed to save rule draft'); +} + +export interface DeployRuleDraftBody { + // Also append a Require line to main.sml so the rule takes effect. wire_into_main?: boolean; - branch?: string; } -export async function submitRuleDraft(body: SubmitRuleDraftBody): Promise { - const response: HTTPResponse = await HTTPUtils.post('rule-drafts/submit', body); +export async function deployRuleDraft(id: number, body: DeployRuleDraftBody = {}): Promise { + const response: HTTPResponse = await HTTPUtils.post(`rule-drafts/${id}/deploy`, body); if (response.ok) { return response.data; } const errPayload = response.error.response?.data as { error?: string } | undefined; - throw new Error(errPayload?.error ?? response.error.message ?? 'Failed to submit rule draft'); + throw new Error(errPayload?.error ?? response.error.message ?? 'Failed to deploy rule draft'); } -export async function getPendingRuleDrafts(): Promise { +export async function getRuleDrafts(): Promise { // Returns an empty list rather than throwing on failure so the RulesPage still renders - // when GitHub isn't configured. - const response: HTTPResponse = await HTTPUtils.get('rule-drafts/pending'); + // when the caller lacks the rule-drafts ability. + const response: HTTPResponse = await HTTPUtils.get('rule-drafts'); if (response.ok) { return response.data; } - const errPayload = response.error.response?.data as PendingDraftsResponse | undefined; - if (errPayload && Array.isArray(errPayload.pending)) { + const errPayload = response.error.response?.data as RuleDraftsListResponse | undefined; + if (errPayload && Array.isArray(errPayload.drafts)) { return errPayload; } - return { pending: [], error: response.error.message ?? 'Failed to fetch pending drafts' }; + return { drafts: [] }; } diff --git a/osprey_ui/src/components/rules/RuleEditorPage.tsx b/osprey_ui/src/components/rules/RuleEditorPage.tsx index bc8b0e13..407a724a 100644 --- a/osprey_ui/src/components/rules/RuleEditorPage.tsx +++ b/osprey_ui/src/components/rules/RuleEditorPage.tsx @@ -14,19 +14,21 @@ import { Typography, message, } from 'antd'; -import { DeleteOutlined, PlusOutlined, SaveOutlined } from '@ant-design/icons'; +import { CloudUploadOutlined, DeleteOutlined, PlusOutlined, SaveOutlined } from '@ant-design/icons'; import { useHistory, useLocation } from 'react-router-dom'; import { + createRuleDraft, + deployRuleDraft, getRuleDraftSource, getRuleDraftVocabulary, parseRuleDraftIntoBuilder, - submitRuleDraft, validateRuleDraft, } from '../../actions/RulesActions'; import usePromiseResult from '../../hooks/usePromiseResult'; import { ParseIntoBuilderResponse, + RuleDraft, RuleDraftValidationMessage, RuleDraftValidationResponse, RuleDraftVocabulary, @@ -53,6 +55,16 @@ const { Title, Text, Paragraph } = Typography; type EditorMode = 'builder' | 'code'; +// Saving stages a draft in the rule_drafts table; deploying writes it into the +// rules directory. The saved draft's id is what a subsequent deploy targets. +type SubmitState = + | { kind: 'idle' } + | { kind: 'saving' } + | { kind: 'saved'; draft: RuleDraft } + | { kind: 'deploying'; draft: RuleDraft } + | { kind: 'deployed'; draft: RuleDraft; mainSmlUpdated: boolean; pathOnDisk: string } + | { kind: 'error'; message: string }; + const VALIDATE_DEBOUNCE_MS = 600; interface BootstrapData { @@ -118,17 +130,12 @@ const RuleEditorView: React.FC<{ data: BootstrapData }> = ({ data }) => { return EMPTY_BUILDER_MODEL; }); const [summary, setSummary] = React.useState(''); - // Off by default: turning a rule on is a deliberate opt-in, so a submit never + // Off by default: turning a rule on is a deliberate opt-in, so a deploy never // wires a new rule into the live ruleset unless the author checks the box. const [wireIntoMain, setWireIntoMain] = React.useState(false); const [validation, setValidation] = React.useState(null); const [isValidating, setIsValidating] = React.useState(false); - const [submitState, setSubmitState] = React.useState< - | { kind: 'idle' } - | { kind: 'submitting' } - | { kind: 'done'; title: string; prUrl: string | null } - | { kind: 'error'; message: string } - >({ kind: 'idle' }); + const [submitState, setSubmitState] = React.useState({ kind: 'idle' }); const effectiveSource = mode === 'builder' ? generateSmlFromBuilder(builder, data.vocabulary.features) : codeSource; @@ -174,11 +181,14 @@ const RuleEditorView: React.FC<{ data: BootstrapData }> = ({ data }) => { const ruleNameForSubmit = mode === 'builder' ? builder.ruleName : guessRuleNameFromSource(codeSource); - const canSubmit = - !!validation?.ok && - SML_IDENTIFIER_RE.test(ruleNameForSubmit) && - submitState.kind !== 'submitting' && - !!effectiveSource.trim(); + const isBusy = submitState.kind === 'saving' || submitState.kind === 'deploying'; + const canSave = !!validation?.ok && SML_IDENTIFIER_RE.test(ruleNameForSubmit) && !isBusy && !!effectiveSource.trim(); + // A draft must exist (be saved) before it can be deployed; the deploy targets its id. + const savedDraft = + submitState.kind === 'saved' || submitState.kind === 'deployed' || submitState.kind === 'deploying' + ? submitState.draft + : null; + const canDeploy = savedDraft !== null && !isBusy; // The builder and code editor hold independent state, so a tab switch has to // carry content across: builder -> code dumps the generated SML into the @@ -208,21 +218,37 @@ const RuleEditorView: React.FC<{ data: BootstrapData }> = ({ data }) => { } }; - const onSubmit = async () => { - if (!canSubmit) return; - setSubmitState({ kind: 'submitting' }); + const onSave = async () => { + if (!canSave) return; + setSubmitState({ kind: 'saving' }); try { - const res = await submitRuleDraft({ + const draft = await createRuleDraft({ path, source: effectiveSource, rule_name: ruleNameForSubmit, summary, - is_new_rule: data.isNewRule, - wire_into_main: wireIntoMain, }); - setSubmitState({ kind: 'done', title: res.title, prUrl: res.url }); + setSubmitState({ kind: 'saved', draft }); + message.success('Draft saved.'); + } catch (e) { + const msg = e instanceof Error ? e.message : String(e); + setSubmitState({ kind: 'error', message: msg }); + } + }; + + const onDeploy = async () => { + if (savedDraft === null || isBusy) return; + setSubmitState({ kind: 'deploying', draft: savedDraft }); + try { + const res = await deployRuleDraft(savedDraft.id, { wire_into_main: wireIntoMain }); + setSubmitState({ + kind: 'deployed', + draft: res, + mainSmlUpdated: res.main_sml_updated, + pathOnDisk: res.path_on_disk, + }); const wiredMsg = res.main_sml_updated ? ' (main.sml updated)' : ''; - message.success(`${res.title}${wiredMsg}.`); + message.success(`Deployed to ${res.path_on_disk}${wiredMsg}.`); } catch (e) { const msg = e instanceof Error ? e.message : String(e); setSubmitState({ kind: 'error', message: msg }); @@ -238,8 +264,8 @@ const RuleEditorView: React.FC<{ data: BootstrapData }> = ({ data }) => { {data.isNewRule ? 'Add rule' : 'Edit rule'} - Drafts open a pull request against the rules repo. Nothing applies until the PR is merged and the engine - reloads. + Save stages this rule in the drafts table. Deploy writes it into the rules directory; nothing applies + until the engine reloads.
@@ -262,9 +288,25 @@ const RuleEditorView: React.FC<{ data: BootstrapData }> = ({ data }) => { /> - + + +
@@ -274,12 +316,12 @@ const RuleEditorView: React.FC<{ data: BootstrapData }> = ({ data }) => {
- + setPath(e.target.value)} disabled={!data.isNewRule} /> = ({ data }) => { setWireIntoMain(e.target.checked)}> - Turn this rule on once the review is approved. + Turn this rule on when I deploy it.
- Adds your rule to the list Osprey runs, as part of the same review. If it's already on the - list, nothing changes. + Adds your rule to the list Osprey runs (a Require line in main.sml) as part of the deploy. If + it's already on the list, nothing changes.
@@ -339,8 +381,7 @@ const RuleEditorView: React.FC<{ data: BootstrapData }> = ({ data }) => { 
{effectiveSource}
- This is the code that will be submitted in a pull request on Github. Make further changes in the Code - Editor view. + This is the code that will be saved as the draft. Make further changes in the Code Editor view.
)} @@ -356,42 +397,42 @@ const RuleEditorView: React.FC<{ data: BootstrapData }> = ({ data }) => { ); }; -const SubmitBanner: React.FC<{ - submitState: - | { kind: 'idle' } - | { kind: 'submitting' } - | { kind: 'done'; title: string; prUrl: string | null } - | { kind: 'error'; message: string }; -}> = ({ submitState }) => { +const SubmitBanner: React.FC<{ submitState: SubmitState }> = ({ submitState }) => { if (submitState.kind === 'idle') return null; - if (submitState.kind === 'submitting') { - return ; + if (submitState.kind === 'saving') { + return ; + } + if (submitState.kind === 'deploying') { + return ; + } + if (submitState.kind === 'saved') { + return ( + + ); } - if (submitState.kind === 'done') { + if (submitState.kind === 'deployed') { return ( - {submitState.prUrl} - - ) : null + submitState.mainSmlUpdated + ? 'Added to main.sml. Takes effect when the engine reloads.' + : 'Takes effect once something requires it and the engine reloads.' } /> ); } return ( - + ); }; diff --git a/osprey_ui/src/components/rules/RulesPage.tsx b/osprey_ui/src/components/rules/RulesPage.tsx index 21f01512..b9583c26 100644 --- a/osprey_ui/src/components/rules/RulesPage.tsx +++ b/osprey_ui/src/components/rules/RulesPage.tsx @@ -19,9 +19,9 @@ import { import { EditOutlined, PlusOutlined, SearchOutlined } from '@ant-design/icons'; import { Link } from 'react-router-dom'; -import { getPendingRuleDrafts, getRulesList } from '../../actions/RulesActions'; +import { getRuleDrafts, getRulesList } from '../../actions/RulesActions'; import usePromiseResult, { PromiseResultStatus } from '../../hooks/usePromiseResult'; -import { PendingDraft, PendingDraftsResponse, RuleInfo, RulesListResponse, SortKey } from '../../types/RulesTypes'; +import { RuleDraft, RuleDraftsListResponse, RuleInfo, RulesListResponse, SortKey } from '../../types/RulesTypes'; import { renderFromPromiseResult } from '../../utils/PromiseResultUtils'; import styles from './RulesPage.module.css'; @@ -76,19 +76,19 @@ export const RulesPage: React.FC = () => { const result = usePromiseResult(() => { return getRulesList(); }); - const pendingResult = usePromiseResult(() => { - return getPendingRuleDrafts(); + const draftsResult = usePromiseResult(() => { + return getRuleDrafts(); }); return renderFromPromiseResult(result, (data) => { - return ; + return ; }); }; const RulesPageContent: React.FC<{ data: RulesListResponse; - pendingResult: ReturnType>; -}> = ({ data, pendingResult }) => { + draftsResult: ReturnType>; +}> = ({ data, draftsResult }) => { const [filters, dispatch] = React.useReducer(filtersReducer, INITIAL_FILTERS); const { rules, total, when_rules_total, unused_total } = data; const { search, unusedOnly, sortKey, page, pageSize } = filters; @@ -166,7 +166,7 @@ const RulesPageContent: React.FC<{
- +
@@ -308,42 +308,37 @@ const RuleHeader: React.FC<{ rule: RuleInfo }> = ({ rule }) => { ); }; -const PendingDraftsBanner: React.FC<{ - pendingResult: ReturnType>; -}> = ({ pendingResult }) => { - if (pendingResult.status !== PromiseResultStatus.Resolved) return null; - const { pending, error } = pendingResult.value; - if (error && pending.length === 0) { - // GitHub backend not configured or unreachable; the rest of the page works without it. - return null; - } - if (pending.length === 0) return null; +const DraftsBanner: React.FC<{ + draftsResult: ReturnType>; +}> = ({ draftsResult }) => { + if (draftsResult.status !== PromiseResultStatus.Resolved) return null; + const { drafts } = draftsResult.value; + if (drafts.length === 0) return null; return ( - {pending.slice(0, 8).map((p, i) => { - return ; + {drafts.slice(0, 8).map((draft) => { + return ; })} - {pending.length > 8 && +{pending.length - 8} more in review.} + {drafts.length > 8 && +{drafts.length - 8} more.} } /> ); }; -const PendingDraftRow: React.FC<{ draft: PendingDraft }> = ({ draft }) => { +const DraftRow: React.FC<{ draft: RuleDraft }> = ({ draft }) => { return (
- - {draft.title} - {' '} + {draft.path}{' '} + {draft.status}{' '} - by {draft.author}, {draft.touched_files.join(', ')} + by {draft.author}
); diff --git a/osprey_ui/src/types/RulesTypes.tsx b/osprey_ui/src/types/RulesTypes.tsx index db86833c..02767253 100644 --- a/osprey_ui/src/types/RulesTypes.tsx +++ b/osprey_ui/src/types/RulesTypes.tsx @@ -67,14 +67,24 @@ export interface RuleDraftVocabulary { source_files: string[]; } -export interface RuleDraftSubmitResponse { - // Backend-neutral fields produced by every RuleSubmissionBackend. - title: string; - url: string | null; +export type RuleDraftStatus = 'draft' | 'deployed'; + +export interface RuleDraft { + id: number; + path: string; + rule_name: string; + source: string; + summary: string; + author: string; + status: RuleDraftStatus; + created_at: string | null; + updated_at: string | null; + deployed_at: string | null; +} + +export interface DeployRuleDraftResponse extends RuleDraft { main_sml_updated: boolean; - // Backend-specific extras (e.g., pr_number, branch for the GitHub backend; - // path_on_disk for the local backend). - [extra: string]: unknown; + path_on_disk: string; } export type ConditionOperator = '==' | '!=' | '>' | '<' | '>=' | '<=' | 'includes' | 'excludes'; @@ -108,16 +118,6 @@ export type ParseIntoBuilderResponse = | { supported: true; model: RuleBuilderModel } | { supported: false; reason: string }; -export interface PendingDraft { - title: string; - url: string; - author: string; - created_at: string; - touched_files: string[]; - [extra: string]: unknown; -} - -export interface PendingDraftsResponse { - pending: PendingDraft[]; - error?: string; +export interface RuleDraftsListResponse { + drafts: RuleDraft[]; } From 20e851c6ed9d1bd0d516a0654283a7e4f600c0b5 Mon Sep 17 00:00:00 2001 From: Juliet Shen Date: Wed, 15 Jul 2026 14:30:02 -0400 Subject: [PATCH 9/9] Make rule authoring a draft handoff, not a deploy Authors stage rule drafts for a developer to review and deploy; the UI should never imply the author deploys. Remove the deploy action from the editor entirely (button, wire-into-main checkbox, and deploy states), and reword the copy: "Saves the rule draft into rule_drafts for a developer to review and deploy. This does not change any live rules." Also fix and harden the draft flow: - Edit a draft by ?draftId=, loading its SML from the table (GET /rule-drafts/) instead of from disk, which 404'd for un-deployed drafts. Editing an existing rule file still uses ?path=. - Warn before saving when the rule name is already used by another draft (a hard conflict the server also rejects with 409, so Save is disabled), or when a new rule's path would overwrite a live rule or another draft (a soft warning, since replacing can be intentional). - Plain-language copy for the Outcomes helper text. --- osprey_ui/src/actions/RulesActions.tsx | 18 +- .../src/components/rules/RuleEditorPage.tsx | 196 ++++++++---------- osprey_ui/src/components/rules/RulesPage.tsx | 2 +- osprey_ui/src/types/RulesTypes.tsx | 5 - 4 files changed, 96 insertions(+), 125 deletions(-) diff --git a/osprey_ui/src/actions/RulesActions.tsx b/osprey_ui/src/actions/RulesActions.tsx index 1f0ed549..bd32fbce 100644 --- a/osprey_ui/src/actions/RulesActions.tsx +++ b/osprey_ui/src/actions/RulesActions.tsx @@ -1,6 +1,5 @@ import HTTPUtils, { HTTPResponse } from '../utils/HTTPUtils'; import { - DeployRuleDraftResponse, ParseIntoBuilderResponse, RuleDraft, RuleDraftSourceResponse, @@ -62,8 +61,8 @@ export interface CreateRuleDraftBody { summary: string; } -// Saves a draft into the rule_drafts table (upserted by path). The draft is staged, -// not live; deployRuleDraft writes it into the rules directory. +// Saves a draft into the rule_drafts table (upserted by path). The draft is staged +// for a developer to review and deploy; saving never changes any live rules. export async function createRuleDraft(body: CreateRuleDraftBody): Promise { const response: HTTPResponse = await HTTPUtils.post('rule-drafts', body); if (response.ok) { @@ -73,18 +72,15 @@ export async function createRuleDraft(body: CreateRuleDraftBody): Promise { - const response: HTTPResponse = await HTTPUtils.post(`rule-drafts/${id}/deploy`, body); +// Loads a single draft (its SML lives in the table, not on disk, so editing a draft +// reads it from here rather than from the rules directory). +export async function getRuleDraft(id: number): Promise { + const response: HTTPResponse = await HTTPUtils.get(`rule-drafts/${id}`); if (response.ok) { return response.data; } const errPayload = response.error.response?.data as { error?: string } | undefined; - throw new Error(errPayload?.error ?? response.error.message ?? 'Failed to deploy rule draft'); + throw new Error(errPayload?.error ?? response.error.message ?? 'Failed to load rule draft'); } export async function getRuleDrafts(): Promise { diff --git a/osprey_ui/src/components/rules/RuleEditorPage.tsx b/osprey_ui/src/components/rules/RuleEditorPage.tsx index 407a724a..d719859f 100644 --- a/osprey_ui/src/components/rules/RuleEditorPage.tsx +++ b/osprey_ui/src/components/rules/RuleEditorPage.tsx @@ -1,25 +1,12 @@ import * as React from 'react'; -import { - Alert, - Button, - Card, - Checkbox, - Form, - Input, - Segmented, - Select, - Space, - Tag, - Tooltip, - Typography, - message, -} from 'antd'; -import { CloudUploadOutlined, DeleteOutlined, PlusOutlined, SaveOutlined } from '@ant-design/icons'; +import { Alert, Button, Card, Form, Input, Segmented, Select, Space, Tag, Tooltip, Typography, message } from 'antd'; +import { DeleteOutlined, PlusOutlined, SaveOutlined } from '@ant-design/icons'; import { useHistory, useLocation } from 'react-router-dom'; import { createRuleDraft, - deployRuleDraft, + getRuleDraft, + getRuleDrafts, getRuleDraftSource, getRuleDraftVocabulary, parseRuleDraftIntoBuilder, @@ -55,15 +42,9 @@ const { Title, Text, Paragraph } = Typography; type EditorMode = 'builder' | 'code'; -// Saving stages a draft in the rule_drafts table; deploying writes it into the -// rules directory. The saved draft's id is what a subsequent deploy targets. -type SubmitState = - | { kind: 'idle' } - | { kind: 'saving' } - | { kind: 'saved'; draft: RuleDraft } - | { kind: 'deploying'; draft: RuleDraft } - | { kind: 'deployed'; draft: RuleDraft; mainSmlUpdated: boolean; pathOnDisk: string } - | { kind: 'error'; message: string }; +// Saving stages the rule in the rule_drafts table for a developer to review and +// deploy. Authoring never deploys, so there is no deploy state here. +type SubmitState = { kind: 'idle' } | { kind: 'saving' } | { kind: 'saved' } | { kind: 'error'; message: string }; const VALIDATE_DEBOUNCE_MS = 600; @@ -72,6 +53,9 @@ interface BootstrapData { initialSource: string; initialPath: string; isNewRule: boolean; + // Every existing draft, used to warn about name/path collisions the server-side + // validator can't see (it only knows deployed rules, not other drafts). + existingDrafts: RuleDraft[]; // For edit mode: the result of round-tripping the loaded source through the // backend parser. Determines whether the Rule Builder toggle is enabled and // what model the builder starts from. @@ -79,35 +63,54 @@ interface BootstrapData { } export const RuleEditorPage: React.FC = () => { - // The edit path lives in `?path=` because react-router v5 has no clean - // repeating-segment param and rule paths contain slashes. + // Two edit entry points, both via query params because react-router v5 has no + // clean repeating-segment param and rule paths contain slashes: + // ?draftId=N -> edit a saved draft (its SML lives in the rule_drafts table) + // ?path=X -> edit an existing rule file loaded from the rules directory const location = useLocation(); const isNewRule = location.pathname === '/rules/new'; - const editPath = isNewRule ? undefined : (new URLSearchParams(location.search).get('path') ?? undefined); + const params = new URLSearchParams(location.search); + const draftId = isNewRule ? undefined : (params.get('draftId') ?? undefined); + const editPath = isNewRule ? undefined : (params.get('path') ?? undefined); const result = usePromiseResult(async () => { - const vocabulary = await getRuleDraftVocabulary(); + const [vocabulary, { drafts: existingDrafts }] = await Promise.all([getRuleDraftVocabulary(), getRuleDrafts()]); if (isNewRule) { return { vocabulary, + existingDrafts, initialSource: '', initialPath: 'rules/new_rule.sml', isNewRule: true, }; } + // A draft's SML is in the table, so load it from there rather than from disk. + if (draftId) { + const draft = await getRuleDraft(Number(draftId)); + const initialBuilderParse = await parseRuleDraftIntoBuilder(draft.path, draft.source); + return { + vocabulary, + existingDrafts, + initialSource: draft.source, + initialPath: draft.path, + isNewRule: false, + initialBuilderParse, + }; + } if (!editPath) { - throw new Error('Missing ?path= query parameter; navigate from the Rules page.'); + throw new Error('Missing ?draftId= or ?path= query parameter; navigate from the Rules page.'); } const source = await getRuleDraftSource(editPath); const initialBuilderParse = await parseRuleDraftIntoBuilder(source.path, source.contents); return { vocabulary, + existingDrafts, initialSource: source.contents, initialPath: source.path, isNewRule: false, initialBuilderParse, }; - }, [editPath, isNewRule]); + }, [draftId, editPath, isNewRule]); return renderFromPromiseResult(result, (data) => { return ; @@ -130,9 +133,6 @@ const RuleEditorView: React.FC<{ data: BootstrapData }> = ({ data }) => { return EMPTY_BUILDER_MODEL; }); const [summary, setSummary] = React.useState(''); - // Off by default: turning a rule on is a deliberate opt-in, so a deploy never - // wires a new rule into the live ruleset unless the author checks the box. - const [wireIntoMain, setWireIntoMain] = React.useState(false); const [validation, setValidation] = React.useState(null); const [isValidating, setIsValidating] = React.useState(false); const [submitState, setSubmitState] = React.useState({ kind: 'idle' }); @@ -181,14 +181,21 @@ const RuleEditorView: React.FC<{ data: BootstrapData }> = ({ data }) => { const ruleNameForSubmit = mode === 'builder' ? builder.ruleName : guessRuleNameFromSource(codeSource); - const isBusy = submitState.kind === 'saving' || submitState.kind === 'deploying'; - const canSave = !!validation?.ok && SML_IDENTIFIER_RE.test(ruleNameForSubmit) && !isBusy && !!effectiveSource.trim(); - // A draft must exist (be saved) before it can be deployed; the deploy targets its id. - const savedDraft = - submitState.kind === 'saved' || submitState.kind === 'deployed' || submitState.kind === 'deploying' - ? submitState.draft - : null; - const canDeploy = savedDraft !== null && !isBusy; + // Collisions the server-side validator can't see (it only knows deployed rules): + // a rule name already taken by another draft (a hard conflict — the server rejects + // it on save too), and — for a brand new rule — a path that would overwrite a live + // rule or another draft (a soft warning, since replacing can be intentional). Path + // is only editable for new rules, so path warnings are new-rule-only. + const nameConflictDraft = data.existingDrafts.find((d) => d.rule_name === ruleNameForSubmit && d.path !== path); + const pathOverwritesRule = data.isNewRule && data.vocabulary.source_files.includes(path); + const pathOverwritesDraft = data.isNewRule && data.existingDrafts.some((d) => d.path === path); + + const canSave = + !!validation?.ok && + SML_IDENTIFIER_RE.test(ruleNameForSubmit) && + submitState.kind !== 'saving' && + !!effectiveSource.trim() && + !nameConflictDraft; // The builder and code editor hold independent state, so a tab switch has to // carry content across: builder -> code dumps the generated SML into the @@ -222,13 +229,13 @@ const RuleEditorView: React.FC<{ data: BootstrapData }> = ({ data }) => { if (!canSave) return; setSubmitState({ kind: 'saving' }); try { - const draft = await createRuleDraft({ + await createRuleDraft({ path, source: effectiveSource, rule_name: ruleNameForSubmit, summary, }); - setSubmitState({ kind: 'saved', draft }); + setSubmitState({ kind: 'saved' }); message.success('Draft saved.'); } catch (e) { const msg = e instanceof Error ? e.message : String(e); @@ -236,25 +243,6 @@ const RuleEditorView: React.FC<{ data: BootstrapData }> = ({ data }) => { } }; - const onDeploy = async () => { - if (savedDraft === null || isBusy) return; - setSubmitState({ kind: 'deploying', draft: savedDraft }); - try { - const res = await deployRuleDraft(savedDraft.id, { wire_into_main: wireIntoMain }); - setSubmitState({ - kind: 'deployed', - draft: res, - mainSmlUpdated: res.main_sml_updated, - pathOnDisk: res.path_on_disk, - }); - const wiredMsg = res.main_sml_updated ? ' (main.sml updated)' : ''; - message.success(`Deployed to ${res.path_on_disk}${wiredMsg}.`); - } catch (e) { - const msg = e instanceof Error ? e.message : String(e); - setSubmitState({ kind: 'error', message: msg }); - } - }; - return (
@@ -264,8 +252,8 @@ const RuleEditorView: React.FC<{ data: BootstrapData }> = ({ data }) => { {data.isNewRule ? 'Add rule' : 'Edit rule'} - Save stages this rule in the drafts table. Deploy writes it into the rules directory; nothing applies - until the engine reloads. + Saves the rule draft into rule_drafts for a developer to review and deploy. This does not + change any live rules.
@@ -289,6 +277,7 @@ const RuleEditorView: React.FC<{ data: BootstrapData }> = ({ data }) => { - - -
+ {nameConflictDraft && ( + + )} + {pathOverwritesRule && ( + + )} + {pathOverwritesDraft && !pathOverwritesRule && ( + + )} +
@@ -320,8 +326,9 @@ const RuleEditorView: React.FC<{ data: BootstrapData }> = ({ data }) => { setPath(e.target.value)} disabled={!data.isNewRule} /> = ({ data }) => { autoSize={{ minRows: 2, maxRows: 4 }} /> - - setWireIntoMain(e.target.checked)}> - Turn this rule on when I deploy it. - -
- Adds your rule to the list Osprey runs (a Require line in main.sml) as part of the deploy. If - it's already on the list, nothing changes. -
- @@ -402,9 +400,6 @@ const SubmitBanner: React.FC<{ submitState: SubmitState }> = ({ submitState }) = if (submitState.kind === 'saving') { return ; } - if (submitState.kind === 'deploying') { - return ; - } if (submitState.kind === 'saved') { return ( = ({ submitState }) = showIcon style={{ marginBottom: 12 }} message="Draft saved" - description="Staged in the drafts table. Deploy it to write it into the rules directory." - /> - ); - } - if (submitState.kind === 'deployed') { - return ( - ); } @@ -715,7 +695,7 @@ const RuleBuilderEditor: React.FC<{ Outcomes - Wrapped in a WhenRules(then=[…]) block that fires when the rule matches. + What Osprey does when the conditions above are met, like adding a label or banning the user. {model.outcomes.map((outcome, oIdx) => { return ( diff --git a/osprey_ui/src/components/rules/RulesPage.tsx b/osprey_ui/src/components/rules/RulesPage.tsx index b9583c26..d443e398 100644 --- a/osprey_ui/src/components/rules/RulesPage.tsx +++ b/osprey_ui/src/components/rules/RulesPage.tsx @@ -335,7 +335,7 @@ const DraftsBanner: React.FC<{ const DraftRow: React.FC<{ draft: RuleDraft }> = ({ draft }) => { return (
- {draft.path}{' '} + {draft.path}{' '} {draft.status}{' '} by {draft.author} diff --git a/osprey_ui/src/types/RulesTypes.tsx b/osprey_ui/src/types/RulesTypes.tsx index 02767253..55ce7fb6 100644 --- a/osprey_ui/src/types/RulesTypes.tsx +++ b/osprey_ui/src/types/RulesTypes.tsx @@ -82,11 +82,6 @@ export interface RuleDraft { deployed_at: string | null; } -export interface DeployRuleDraftResponse extends RuleDraft { - main_sml_updated: boolean; - path_on_disk: string; -} - export type ConditionOperator = '==' | '!=' | '>' | '<' | '>=' | '<=' | 'includes' | 'excludes'; export interface RuleBuilderCondition {