Add rule authoring/editing UI backed by the rule-drafts API - #403
Open
julietshen wants to merge 11 commits into
Open
Add rule authoring/editing UI backed by the rule-drafts API#403julietshen wants to merge 11 commits into
julietshen wants to merge 11 commits into
Conversation
Contributor
📝 WalkthroughWalkthroughAdds experimental in-app rule authoring with persisted drafts, AST validation, vocabulary discovery, visual/code editing, protected deployment APIs, and rules registry integration. ChangesRule drafting
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant RuleEditorPage
participant rule_drafts
participant OspreyEngine
participant RuleDraft
RuleEditorPage->>rule_drafts: validate draft source
rule_drafts->>OspreyEngine: validate_sources
OspreyEngine-->>rule_drafts: validation results
RuleEditorPage->>rule_drafts: save draft
rule_drafts->>RuleDraft: upsert draft
RuleEditorPage->>rule_drafts: deploy draft
rule_drafts->>RuleDraft: mark deployed
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
New ui-api blueprint for authoring SML rule drafts from the UI, with submission routed through a pluggable backend so each deployment picks where drafts go for review. Endpoints (all gated by a new CAN_EDIT_RULE_DRAFTS ability, granted to super_user): get-source, validate, vocabulary, submit, pending, and parse-into-builder. Validate and submit splice the draft into the engine's loaded sources and re-run the same AST validation the engine uses; submit re-validates server-side before touching any backend. Backends implement the RuleSubmissionBackend Protocol and are selected by OSPREY_RULES_SUBMISSION_BACKEND: - null (default): fails fast with 503 so an unconfigured install never writes anything - github: opens a PR via the REST API; supports GitHub Enterprise - local: writes into a mounted rules directory Contract and safety details: - SubmissionResult/PendingDraft.to_json spread extras first so a backend-specific extra can't shadow the canonical title/url/ main_sml_updated fields the UI depends on - Forge transport failures (connection refused, timeout) become the structured 502 the UI renders, not an unhandled 500, via a shared _rule_drafts_git_common.request() helper that also holds the branch-name and main.sml Require helpers - main.sml is rejected as a draft path: wholesale-replacing the engine entry point is not a draft; wiring a rule in is the controlled wire_into_main append Adopter docs for the env vars are in docs/user/manage.md. Follow-ups add the rule-editor UI, a GitLab backend, and a Tangled (ATProto) backend. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VZ4RQtuHCCgurfpjfPXXAM
julietshen
force-pushed
the
julietshen/rule-editor-ui
branch
2 times, most recently
from
July 3, 2026 18:27
2b301ec to
0333d51
Compare
Feedback on roostorg#402 was to drop the github/gitlab/etc submission backends and instead keep drafts in Osprey itself. This does that: rule drafts now live in a rule_drafts Postgres table that the people who operate Osprey can reference, edit, and deploy from, with no external code host. - Add the RuleDraft model (one row per rule path, upserted on save) and register it so the table is created. - Rework the view around the table: create/list/get plus a deploy endpoint that re-validates, writes the SML into OSPREY_RULES_LOCAL_PATH, and optionally wires a Require line into main.sml. Keep the backend-agnostic authoring endpoints (source, validate, vocabulary, parse-into-builder). - Remove the five _rule_drafts_* backend modules and the OSPREY_RULES_SUBMISSION_BACKEND config surface. - Rewrite the tests for the table workflow; update docs and CHANGELOG. A DB-backed SourcesProvider that loads deployed drafts straight from the table (removing the filesystem hand-off) is noted as future direction. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TEcfpqhza3dPepZUVWw36X
SML rule names are global identifiers, so two drafts sharing a name would collide once both deploy. Server-side validation only sees deployed rules, not other rows in the rule_drafts table, so it can't catch the draft-vs-draft case. Add RuleDraft.other_with_rule_name() and have create_draft return 409 when a different path already uses the name (re-saving the same path is still an in-place update, not a conflict). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TEcfpqhza3dPepZUVWw36X
julietshen
marked this pull request as ready for review
July 16, 2026 17:18
julietshen
requested review from
a team,
EXBreder,
ayubun,
haileyok and
vinaysrao1
as code owners
July 16, 2026 17:18
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 19
🧹 Nitpick comments (2)
osprey_worker/src/osprey/worker/ui_api/osprey/views/rule_drafts.py (1)
47-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDo not silently discard AST and UDF introspection failures.
These broad catches can hide malformed span metadata or broken UDF registrations, leaving validation and vocabulary results incomplete without diagnostics. Catch expected exceptions or log before applying the fallback.
As per coding guidelines, do not silently swallow
Exceptionwithout logging or rethrowing.Also applies to: 216-230
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@osprey_worker/src/osprey/worker/ui_api/osprey/views/rule_drafts.py` around lines 47 - 54, Update _format_validation_message and the corresponding logic around the additional referenced range to avoid silently swallowing broad Exception failures during AST/UDF introspection. Catch only expected exception types where possible; otherwise log the exception before applying the existing fallback, preserving normal validation and vocabulary behavior.Source: Coding guidelines
osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_backend.py (1)
51-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConstrain backend extras to JSON-serializable values.
Anypermits objects that will fail duringjsonify(). Define a recursiveJsonValuetype and usedict[str, JsonValue]for both fields.As per coding guidelines, “Avoid new
Anyin Python.” <coding_guidelines>Also applies to: 74-74
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_backend.py` at line 51, Define a recursive JsonValue type in the rule-drafts backend covering JSON primitives, lists, and string-keyed objects, then replace Any with JsonValue in both extras fields. Use dict[str, JsonValue] for each field so values passed to jsonify are constrained to JSON-serializable data.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/user/manage.md`:
- Line 62: Update the submission behavior description to be backend-agnostic:
explain that submission routes through the configured backend, with GitHub
opening a pull request and the local backend writing changes directly without
opening a review unit.
In `@osprey_ui/src/actions/RulesActions.tsx`:
- Around line 35-36: Update the response.error handling in the rule validation
action to validate that response.error.response.data contains the expected
RuleDraftValidationResponse envelope with errors and warnings before returning
it. For 401, 403, 500, proxy, or other malformed payloads, throw instead of
returning the unchecked cast, while preserving valid validation responses.
In `@osprey_ui/src/components/rules/ruleBuilderSml.ts`:
- Around line 111-114: Update both import-generation branches in
ruleBuilderSml.ts: the vocabulary-derived mapping around lines 111-114 and the
suggested-path mapping around lines 191-197 must pass each path through
smlString before embedding it in the generated SML. Preserve the existing
formatting and ordering while ensuring both branches serialize paths safely.
- Around line 169-189: Replace the regex-based import discovery in
applyMissingImports with syntax-aware editing, or a scanner that ignores
comments and string literals, so only the actual top-level Import(rules=[...])
block is located and replaced. Preserve deduplication, sorting, and the
unchanged-source behavior when all paths already exist.
In `@osprey_ui/src/components/rules/RuleEditorPage.module.css`:
- Line 51: Update all five font-family declarations in RuleEditorPage.module.css
to remove the quotes around SFMono-Regular, while leaving the remaining fallback
fonts and declaration values unchanged.
In `@osprey_ui/src/components/rules/RuleEditorPage.tsx`:
- Around line 51-63: Preserve the draft summary in the bootstrap data used by
RuleEditorPage: initialize the summary to an empty string for new or live rules,
and to draft.summary when loading an existing draft. Update the BootstrapData
definition and all affected bootstrap construction/consumption points so
subsequent saves retain the existing summary.
- Around line 204-225: Update onModeChange to guard the asynchronous
parseRuleDraftIntoBuilder response against edits made while the request is
pending. Track the latest codeSource with a ref or request sequence token, and
before applying parsed.model or switching modes, discard the response unless it
still matches the current source/request.
- Around line 100-104: Update the RuleEditorPage editPath flow around
getRuleDraftSource and the save handling near the referenced conflict logic so
an existing draft for the same path is detected before loading or saving.
Redirect to the matching draft when available, or block saving with the existing
conflict behavior; do not allow edit mode to overwrite pending draft contents
while pathOverwritesDraft remains disabled.
- Around line 142-180: Update RuleEditorPage’s validation state and canSave
logic to track the exact {path, source} associated with the latest validation
result. Record that snapshot only when validation completes, clear or invalidate
it when path or effectiveSource changes, and require both values to match the
current editor state before enabling Save during debounce or in-flight
validation.
In `@osprey_ui/src/types/RulesTypes.tsx`:
- Around line 34-39: Update the validation response contract used by
validateRuleDraft and the corresponding rule_drafts.py 400-response paths so
malformed input always returns a discriminated, consistently shaped envelope,
including required fields such as rendered, errors, and warnings. If the backend
must retain the {"error": ...} shape, introduce a separate error union and
narrow it before inline rendering; do not cast both response shapes directly to
RuleDraftValidationResponse.
In
`@osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_git_common.py`:
- Around line 35-39: Sanitize backend errors in both affected paths: in
_rule_drafts_git_common.py, log the caught requests.RequestException server-side
and raise RuleDraftBackendError with a stable transport-error message instead of
exc details; in _rule_drafts_github.py, log the bounded response body
server-side and omit it from the client-facing RuleDraftBackendError message.
Ensure neither response exposes internal exception messages or stack traces.
In `@osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_github.py`:
- Around line 199-255: The submission flow around _create_branch, _commit_file,
and _open_pr must avoid orphaned branches and commits. Perform all read-only
preflight checks, including main.sml existence and require-state validation,
before creating the branch; then wrap subsequent branch commits and PR creation
so any failure triggers branch cleanup using the existing branch-deletion
mechanism, while preserving successful submission behavior.
- Around line 270-310: Update list_pending_drafts to paginate the open
pull-request listing beyond the first 30 results, aggregating all pages before
processing them. Replace the silent files_res.ok skip with
_raise_for_github(files_res, 'listing pull request files') so file-list failures
are surfaced while preserving normal file filtering.
In `@osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_local.py`:
- Around line 78-98: Update the submission flow around the draft write and
wire_into_main handling to validate all prerequisites, including main.sml,
before persisting the rule. Protect the complete read-modify-write sequence with
a cross-process file lock, and use atomic replacement for both the draft file
and updated main.sml so concurrent submissions cannot lose Require entries or
leave partial writes.
In `@osprey_worker/src/osprey/worker/ui_api/osprey/views/rule_drafts.py`:
- Around line 143-152: Update the exception handlers in the affected
source-parsing and AST branches, including the handler around Sources.from_dict,
to log the original exception server-side and replace str(exc) in API error
responses with a stable sanitized message. Preserve the existing structured
error shape and status code while ensuring no raw exception details are returned
to clients.
- Around line 483-487: Update the rule parsing/support-detection flow around
get_func_identifier and the statements loop so Import/Require calls,
FormatString descriptions, and missing or non-list rules_any/then arguments
cannot be reported as supported unless their semantics are preserved through
regeneration. Either retain these constructs in the model/generator or return
supported: false before the successful result is produced, including all
affected validation paths.
- Around line 257-286: Update the _walk traversal so it records only top-level
outcome calls from the then argument, rather than recursively adding calls
nested within another call’s arguments. Preserve traversal through AstList and
Assign wrappers as needed, but do not walk Call arguments after recording the
outer call identifier.
- Around line 128-136: Update submit_draft and the related rule-draft endpoints
to validate that request.get_json(silent=True) returns an object before
accessing fields, returning HTTP 400 for scalar or list bodies. Validate each
request field against its expected type, especially wire_into_main, and reject
string values such as "false" instead of coercing them with bool().
- Around line 377-385: Add the editor-facing GET /rule-drafts, POST
/rule-drafts, and GET /rule-drafts/{id} handlers alongside pending_drafts, using
the existing backend and RuleDraftBackendError patterns. Implement list, create,
and single-draft retrieval through the table-backed backend APIs, returning the
expected JSON responses and status codes while preserving the existing /submit
and /pending routes.
---
Nitpick comments:
In `@osprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_backend.py`:
- Line 51: Define a recursive JsonValue type in the rule-drafts backend covering
JSON primitives, lists, and string-keyed objects, then replace Any with
JsonValue in both extras fields. Use dict[str, JsonValue] for each field so
values passed to jsonify are constrained to JSON-serializable data.
In `@osprey_worker/src/osprey/worker/ui_api/osprey/views/rule_drafts.py`:
- Around line 47-54: Update _format_validation_message and the corresponding
logic around the additional referenced range to avoid silently swallowing broad
Exception failures during AST/UDF introspection. Catch only expected exception
types where possible; otherwise log the exception before applying the existing
fallback, preserving normal validation and vocabulary behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4f91b5b9-c0d2-4811-9c27-37bb0af9fae8
📒 Files selected for processing (21)
.gitignoredocs/user/manage.mdosprey_ui/src/App.tsxosprey_ui/src/Constants.tsxosprey_ui/src/actions/RulesActions.tsxosprey_ui/src/components/rules/RuleEditorPage.module.cssosprey_ui/src/components/rules/RuleEditorPage.tsxosprey_ui/src/components/rules/RulesPage.tsxosprey_ui/src/components/rules/ruleBuilderSml.tsosprey_ui/src/types/RulesTypes.tsxosprey_worker/src/osprey/worker/lib/acls/definitions/super_user.jsonosprey_worker/src/osprey/worker/lib/osprey_engine.pyosprey_worker/src/osprey/worker/ui_api/osprey/app.pyosprey_worker/src/osprey/worker/ui_api/osprey/lib/abilities.pyosprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_backend.pyosprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_git_common.pyosprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_github.pyosprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_local.pyosprey_worker/src/osprey/worker/ui_api/osprey/views/_rule_drafts_null.pyosprey_worker/src/osprey/worker/ui_api/osprey/views/rule_drafts.pyosprey_worker/src/osprey/worker/ui_api/osprey/views/tests/test_rule_drafts.py
|
|
||
| ## 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. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Describe submission behavior without assuming a git backend.
The local backend writes immediately and opens no review unit. State that submission routes through the configured backend, with GitHub opening a PR and local writing directly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/user/manage.md` at line 62, Update the submission behavior description
to be backend-agnostic: explain that submission routes through the configured
backend, with GitHub opening a pull request and the local backend writing
changes directly without opening a review unit.
Comment on lines
+35
to
+36
| if (response.error.response?.data) { | ||
| return response.error.response.data as RuleDraftValidationResponse; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate the error envelope before returning it.
A 401, 403, 500, or proxy response may contain {error: ...} rather than errors and warnings. The unchecked cast then gives callers a malformed RuleDraftValidationResponse, potentially crashing validation rendering. Only return payloads matching the envelope; otherwise throw.
Proposed fix
- if (response.error.response?.data) {
- return response.error.response.data as RuleDraftValidationResponse;
+ const payload = response.error.response?.data;
+ if (
+ typeof payload === 'object' &&
+ payload !== null &&
+ 'ok' in payload &&
+ typeof payload.ok === 'boolean' &&
+ 'errors' in payload &&
+ Array.isArray(payload.errors) &&
+ 'warnings' in payload &&
+ Array.isArray(payload.warnings)
+ ) {
+ return payload as RuleDraftValidationResponse;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (response.error.response?.data) { | |
| return response.error.response.data as RuleDraftValidationResponse; | |
| const payload = response.error.response?.data; | |
| if ( | |
| typeof payload === 'object' && | |
| payload !== null && | |
| 'ok' in payload && | |
| typeof payload.ok === 'boolean' && | |
| 'errors' in payload && | |
| Array.isArray(payload.errors) && | |
| 'warnings' in payload && | |
| Array.isArray(payload.warnings) | |
| ) { | |
| return payload as RuleDraftValidationResponse; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@osprey_ui/src/actions/RulesActions.tsx` around lines 35 - 36, Update the
response.error handling in the rule validation action to validate that
response.error.response.data contains the expected RuleDraftValidationResponse
envelope with errors and warnings before returning it. For 401, 403, 500, proxy,
or other malformed payloads, throw instead of returning the unchecked cast,
while preserving valid validation responses.
Comment on lines
+111
to
+114
| const entries = sorted | ||
| .map((p) => { | ||
| return ` '${p}',`; | ||
| }) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Serialize every import path with smlString.
Both import-generation branches directly interpolate paths, producing invalid SML for paths containing quotes, backslashes, or newlines.
osprey_ui/src/components/rules/ruleBuilderSml.ts#L111-L114: render each vocabulary-derived path withsmlString(p).osprey_ui/src/components/rules/ruleBuilderSml.ts#L191-L197: render each suggested path with the same helper.
📍 Affects 1 file
osprey_ui/src/components/rules/ruleBuilderSml.ts#L111-L114(this comment)osprey_ui/src/components/rules/ruleBuilderSml.ts#L191-L197
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@osprey_ui/src/components/rules/ruleBuilderSml.ts` around lines 111 - 114,
Update both import-generation branches in ruleBuilderSml.ts: the
vocabulary-derived mapping around lines 111-114 and the suggested-path mapping
around lines 191-197 must pass each path through smlString before embedding it
in the generated SML. Preserve the existing formatting and ordering while
ensuring both branches serialize paths safely.
Comment on lines
+169
to
+189
| 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); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not locate top-level SML imports with this regex.
The match can select Import(...) text inside a comment or string, causing “Fix imports” to rewrite inert content while leaving the actual import unresolved. Use a syntax-aware backend edit, or at minimum a scanner that skips comments and string literals before replacing a top-level block.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@osprey_ui/src/components/rules/ruleBuilderSml.ts` around lines 169 - 189,
Replace the regex-based import discovery in applyMissingImports with
syntax-aware editing, or a scanner that ignores comments and string literals, so
only the actual top-level Import(rules=[...]) block is located and replaced.
Preserve deduplication, sorting, and the unchanged-source behavior when all
paths already exist.
| } | ||
|
|
||
| .codeArea { | ||
| font-family: 'SFMono-Regular', Consolas, 'Liberation Mono', Menlo, monospace !important; |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the quotes around SFMono-Regular.
Stylelint reports font-family-name-quotes errors at all five declarations.
- font-family: 'SFMono-Regular', Consolas, 'Liberation Mono', Menlo, monospace;
+ font-family: SFMono-Regular, Consolas, 'Liberation Mono', Menlo, monospace;Also applies to: 67-67, 94-94, 131-131, 137-137
🧰 Tools
🪛 Stylelint (17.14.0)
[error] 51-51: Expected no quotes around "SFMono-Regular" (font-family-name-quotes)
(font-family-name-quotes)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@osprey_ui/src/components/rules/RuleEditorPage.module.css` at line 51, Update
all five font-family declarations in RuleEditorPage.module.css to remove the
quotes around SFMono-Regular, while leaving the remaining fallback fonts and
declaration values unchanged.
Source: Linters/SAST tools
Comment on lines
+128
to
+136
| 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 |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file structure first, then inspect the relevant regions.
ast-grep outline osprey_worker/src/osprey/worker/ui_api/osprey/views/rule_drafts.py --view expanded || true
echo
echo '--- file size ---'
wc -l osprey_worker/src/osprey/worker/ui_api/osprey/views/rule_drafts.py
echo
echo '--- lines 1-220 ---'
sed -n '1,220p' osprey_worker/src/osprey/worker/ui_api/osprey/views/rule_drafts.py
echo
echo '--- boolean / wire_into_main references ---'
rg -n "wire_into_main|request.get_json|bool\(|source_text|path_err|validate_path|Pydantic|BaseModel" osprey_worker/src/osprey/worker/ui_api/osprey/views/rule_drafts.pyRepository: roostorg/osprey
Length of output: 10387
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect other reported line ranges to see whether the same pattern is duplicated.
sed -n '280,340p' osprey_worker/src/osprey/worker/ui_api/osprey/views/rule_drafts.py
echo
sed -n '560,620p' osprey_worker/src/osprey/worker/ui_api/osprey/views/rule_drafts.pyRepository: roostorg/osprey
Length of output: 4115
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '340,390p' osprey_worker/src/osprey/worker/ui_api/osprey/views/rule_drafts.pyRepository: roostorg/osprey
Length of output: 1983
Validate request JSON before coercing fields
request.get_json(silent=True) or {} still lets scalar/list bodies reach .get(), which can 500 these endpoints. In submit_draft, bool(payload.get('wire_into_main', False)) also treats strings like "false" as True, so a malformed client body can flip behavior unexpectedly. Use a request schema and reject non-object / wrong-typed fields with HTTP 400.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@osprey_worker/src/osprey/worker/ui_api/osprey/views/rule_drafts.py` around
lines 128 - 136, Update submit_draft and the related rule-draft endpoints to
validate that request.get_json(silent=True) returns an object before accessing
fields, returning HTTP 400 for scalar or list bodies. Validate each request
field against its expected type, especially wire_into_main, and reject string
values such as "false" instead of coercing them with bool().
Sources: Coding guidelines, Path instructions
Comment on lines
+257
to
+286
| 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) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Collect only top-level outcome calls as effects.
The recursive walk also records helper calls nested inside arguments. For example, Notify(target=ResolveUser(...)) exposes both functions as selectable outcomes, although only Notify is an effect.
Proposed adjustment
- _walk(then_arg.value)
+ if isinstance(then_arg.value, AstList):
+ for item in then_arg.value.items:
+ if isinstance(item, Call):
+ ident = get_func_identifier(item)
+ if ident:
+ seen.add(ident)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@osprey_worker/src/osprey/worker/ui_api/osprey/views/rule_drafts.py` around
lines 257 - 286, Update the _walk traversal so it records only top-level outcome
calls from the then argument, rather than recursively adding calls nested within
another call’s arguments. Preserve traversal through AstList and Assign wrappers
as needed, but do not walk Call arguments after recording the outer call
identifier.
Comment on lines
+483
to
+487
| for stmt in statements: | ||
| if isinstance(stmt, Call): | ||
| ident = get_func_identifier(stmt) | ||
| if ident in ('Import', 'Require'): | ||
| continue |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Reject or preserve every construct before returning supported: true.
The parser discards Import/Require statements, converts FormatString descriptions into ordinary strings during regeneration, and accepts missing or non-list rules_any/then arguments. Switching Code Editor → Builder → Code Editor can therefore silently change rule semantics. Preserve these constructs in the model/generator or return supported: false.
Also applies to: 519-528, 547-568
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@osprey_worker/src/osprey/worker/ui_api/osprey/views/rule_drafts.py` around
lines 483 - 487, Update the rule parsing/support-detection flow around
get_func_identifier and the statements loop so Import/Require calls,
FormatString descriptions, and missing or non-list rules_any/then arguments
cannot be reported as supported unless their semantics are preserved through
regeneration. Either retain these constructs in the model/generator or return
supported: false before the successful result is produced, including all
affected validation paths.
Feedback from CodeRabbit and @ThisIsMissEm on the rule-drafts backend: - Don't leak raw exception text to clients; log server-side and return a generic message when sources can't be assembled (both /validate and the create/deploy re-validation). - Share one `_validate_draft_source` helper between /validate and the server-side re-validation so the two can't drift. - Catch the specific RuntimeError/AttributeError from `span.ast_node` instead of a bare `except: pass` when extracting the identifier. - Deploy: verify main.sml exists before writing the rule file, so a missing main.sml no longer leaves the file written while the request 409s. - Deploy: report `path_on_disk` relative to the rules directory rather than leaking the absolute server path. - Make the path upsert atomic with INSERT ... ON CONFLICT DO UPDATE so two concurrent saves of the same path can't race the unique constraint. - Reject absolute paths in `_validate_path`; drop the stray `.` from the path character class; use `expunge_all()`; note that Osprey has no users table (identity is an email + ACLs). - Tests: clear the rule_drafts table between tests (the DB is session-scoped) and assert the deploy 409 leaves no file behind. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TEcfpqhza3dPepZUVWw36X
…s-api # Conflicts: # CHANGELOG.md
@ThisIsMissEm noted the /rule-drafts/source endpoint reads any deployed rule's source, not a draft's. Reword the 404 to "No rule found at ..." (it doesn't consult the drafts table, so "draft" would mislead) and add a docstring saying it's for editing an existing on-disk rule, while a draft's own SML comes from GET /rule-drafts/<id>. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TEcfpqhza3dPepZUVWw36X
Feedback from @chimosky on the rule-drafts view: - Read OSPREY_RULES_LOCAL_PATH via CONFIG.get_str() instead of os.environ, for consistency with how the rest of the UI API reads configuration. The deploy tests set it directly on the bound config since CONFIG binds once at app setup. - Make the invalid-path error human-readable ("letters, numbers, underscores, slashes, and hyphens") instead of echoing the raw character class. - Use `[]` as the default in _suggest_imports_from_errors rather than `or []`. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TEcfpqhza3dPepZUVWw36X
New RuleEditorPage at /rules/new and /rules/edit?path=<file>: - 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VZ4RQtuHCCgurfpjfPXXAM
The backend pivoted from git submission backends to a rule_drafts table (roostorg#402), so the editor's submit/pending calls no longer exist. Rewire the UI to the new endpoints: - submit (POST rule-drafts/submit) -> create (POST rule-drafts) - pending (GET rule-drafts/pending) -> list (GET rule-drafts) - add a Deploy action (POST rule-drafts/<id>/deploy) carrying wire_into_main The editor is now save-draft then deploy rather than open-a-PR, and the Rules page lists in-progress drafts (status tag, link to edit) instead of pending-review pull requests. The source/validate/vocabulary/parse endpoints were unchanged, so those calls stay. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TEcfpqhza3dPepZUVWw36X
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/<id>) 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.
julietshen
force-pushed
the
julietshen/rule-editor-ui
branch
from
July 22, 2026 16:23
4bc1acd to
20e851c
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
osprey_worker/src/osprey/worker/lib/storage/rule_drafts.py (1)
111-123: 🗄️ Data Integrity & Integration | 🔵 Trivial
rule_nameuniqueness is guarded only in application code (TOCTOU).
other_with_rule_name()is a check-then-act guard. Two concurrentcreate_draftcalls with the samerule_nameat differentpaths can both observe "no conflict" and bothupsert, leaving two drafts sharing a global rule name that will collide at deploy. Unlike thepathuniqueness (backed by the DB unique constraint +ON CONFLICT),rule_namehas no DB-level protection. Consider a partial unique index (e.g. onrule_name) so the invariant holds even under concurrent writes. This is admin-only (super_user) so contention is low, but the guarantee is otherwise racy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@osprey_worker/src/osprey/worker/lib/storage/rule_drafts.py` around lines 111 - 123, Add database-level uniqueness enforcement for RuleDraft.rule_name, preferably via a partial unique index covering rule_name, and update the model/migration definitions accordingly. Keep other_with_rule_name() as validation feedback, but ensure concurrent create_draft upserts cannot persist duplicate rule names and handle the resulting uniqueness conflict consistently with existing path uniqueness behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@osprey_worker/src/osprey/worker/lib/storage/rule_drafts.py`:
- Around line 111-123: Add database-level uniqueness enforcement for
RuleDraft.rule_name, preferably via a partial unique index covering rule_name,
and update the model/migration definitions accordingly. Keep
other_with_rule_name() as validation feedback, but ensure concurrent
create_draft upserts cannot persist duplicate rule names and handle the
resulting uniqueness conflict consistently with existing path uniqueness
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 00a1a321-85c1-4f77-9bdf-c786c2d65982
📒 Files selected for processing (14)
CHANGELOG.mddocs/user/manage.mdosprey_ui/src/App.tsxosprey_ui/src/Constants.tsxosprey_ui/src/actions/RulesActions.tsxosprey_ui/src/components/rules/RuleEditorPage.module.cssosprey_ui/src/components/rules/RuleEditorPage.tsxosprey_ui/src/components/rules/RulesPage.tsxosprey_ui/src/components/rules/ruleBuilderSml.tsosprey_ui/src/types/RulesTypes.tsxosprey_worker/src/osprey/worker/lib/storage/postgres.pyosprey_worker/src/osprey/worker/lib/storage/rule_drafts.pyosprey_worker/src/osprey/worker/ui_api/osprey/views/rule_drafts.pyosprey_worker/src/osprey/worker/ui_api/osprey/views/tests/test_rule_drafts.py
🚧 Files skipped from review as they are similar to previous changes (7)
- osprey_ui/src/App.tsx
- osprey_ui/src/components/rules/RuleEditorPage.tsx
- osprey_ui/src/components/rules/RulesPage.tsx
- osprey_ui/src/Constants.tsx
- osprey_ui/src/components/rules/ruleBuilderSml.ts
- osprey_ui/src/types/RulesTypes.tsx
- osprey_ui/src/actions/RulesActions.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #402. Review that first; this PR's diff will show #402's commits until it merges.
Adds a RuleEditorPage at
/rules/newand/rules/edit?path=<file>, built on the rule-drafts API./rule-drafts/parse-into-builder; files outside the builder's subset fall back to the Code Editor./rule-drafts/submitand surfaces the review URL returned by the configured backend./rule-drafts/pending(soft-fails when no backend is configured).Security/correctness hardening in the SML generator: backslashes and newlines are escaped (not just quotes) before interpolation, the rule name is constrained to an SML identifier before it reaches the generated file and the submit gate, and switching between Builder and Code Editor now carries content across so a tab flip can't submit stale or empty source.
Checklist
pnpm run typecheck,pnpm run lint(0 errors),pnpm run format:checkpassCHANGELOG.mdSummary by CodeRabbit