S1: invoke planner — render + validate-plan (candidates → o7 invoke → validated plan)#286
Conversation
|
Warning Review limit reached
Next review available in: 23 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
📝 WalkthroughWalkthroughAdds an S1 subscription fix-plan flow: candidate bundle validation, deterministic prompt/schema rendering, constrained plan validation, CLI subcommands, shell orchestration through ChangesS1 fix-plan flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant own_fix
participant fix_plan
participant o7_invoke
participant validated_plan
own_fix->>fix_plan: render candidates into prompt and schema
fix_plan->>o7_invoke: provide prompt and schema
o7_invoke->>own_fix: write result.json
own_fix->>fix_plan: validate result.json with candidates
fix_plan->>validated_plan: write validated plan atomically
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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 |
![chatgpt-codex-connector[bot]](https://cdn.github.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d10a805f29
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try: | ||
| validated = validate_plan(bundle, plan) | ||
| except PlanError as exc: | ||
| print(f"own-fix: {exc}", file=sys.stderr) |
There was a problem hiding this comment.
When the candidates file is valid JSON but violates the bundle contract, validate_plan() raises CollectError from validate_candidates_bundle(), while this handler catches only PlanError. For example, a bundle with version 2 produces an uncaught traceback and exit code 1 instead of the documented controlled hard error (exit code 2); catch CollectError here or translate it inside validate_plan().
Useful? React with 👍 / 👎.
| "decisions": { | ||
| "type": "array", | ||
| "minItems": count, | ||
| "maxItems": count, | ||
| "items": {"oneOf": decision_schemas}, |
There was a problem hiding this comment.
With two or more candidates, the fixed array length does not enforce the promised finding-ID bijection because every element independently may match the same oneOf branch. An o7 result containing candidate A twice is therefore schema-valid but is subsequently rejected by validate_plan() as duplicate/missing, making a schema-constrained invocation fail despite satisfying the generated schema; use per-position schemas or otherwise encode ID uniqueness and coverage.
Useful? React with 👍 / 👎.
| "minItems": count, | ||
| "maxItems": count, | ||
| "items": {"oneOf": decision_schemas}, |
There was a problem hiding this comment.
The collector can legitimately produce a bundle with zero candidates when the selected class has no eligible unreleased subscriptions, but this renders oneOf: []. Draft 2020-12 requires oneOf to contain at least one schema, so the orchestration's schema consumer can reject the schema before an empty decisions array is returned; either reject or short-circuit empty bundles, or emit a valid schema that permits exactly an empty array.
Useful? React with 👍 / 👎.
| sel = _field(bundle, "selection", "dict", "candidates") | ||
| allowed_types = _field(sel, "allowed_types", "list", "candidates.selection") | ||
| type_names = set() | ||
| for i, t in enumerate(allowed_types): |
There was a problem hiding this comment.
An edited or malformed bundle can include multiple entries in allowed_types while retaining max_types_changed: 1, because this loop validates each entry but never checks the list cardinality. validate_plan() then republishes that broader selection in a supposedly validated plan, violating the locked one-type safety envelope; require exactly one allowed type and reject duplicates or additional entries.
Useful? React with 👍 / 👎.
| if cfile not in source_paths: | ||
| raise CollectError(f"{cctx}: file {cfile!r} is not in source_files") | ||
| if ctype not in type_names: | ||
| raise CollectError( | ||
| f"{cctx}: containing_type {ctype!r} is not in selection.allowed_types" |
There was a problem hiding this comment.
These independent membership checks accept an inconsistent bundle such as allowed type N.C in A.cs, source file B.cs, and a candidate claiming type N.C in B.cs. The resulting validated decision targets a file not declared for that type, bypassing the intended type/file containment relationship; validate (containing_type, file) against the exact pairs in selection.allowed_types.
Useful? React with 👍 / 👎.
| # inside the work dir first, then moved into place, so a failure leaves no partial out. | ||
| python -m ownlang own-fix subscriptions validate-plan "$candidates" "$result" \ | ||
| --output "$work/validated-plan.json" | ||
| mv "$work/validated-plan.json" "$out" |
There was a problem hiding this comment.
When the requested output is on a different filesystem from mktemp's directory, this mv cannot rename and falls back to copying; mv --help confirms that ordinary operation may copy when renaming fails via its --no-copy description. A failed or interrupted copy can therefore leave the partial validated artifact that the script promises to avoid, so stage the temporary file in the destination directory and rename it there.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@ownlang/fix_candidates.py`:
- Around line 361-387: Tighten the selection validation around allowed_types and
source_files to require exactly one entry in each, then require the sole allowed
type’s file to match the sole source-file path. Validate selected_findings when
non-null as a collection of unique IDs, with every ID present in the candidate
set, before copying the selection metadata into the validated plan.
- Around line 340-342: Strengthen _require_canonical_relpath so it rejects empty
paths, ".", "./file" forms, repeated separators, and trailing separators, while
preserving the existing absolute, backslash, drive-prefix, and parent-segment
checks. Accept only normalized root-relative paths with nonempty segments and no
"." aliases.
In `@ownlang/fix_plan.py`:
- Around line 102-107: Update validate_plan around validate_candidates_bundle so
any candidate-validation failure, including CollectError, is caught and
re-raised as PlanError. Preserve the original failure as the cause and ensure
_cmd_validate_plan continues to handle the normalized error through its existing
PlanError catch.
- Around line 71-96: Update the schema construction in the render flow around
decision_schemas and the decisions.items definition to handle zero candidates
without emitting oneOf: []. When candidates is empty, use a schema that rejects
all items, such as false, while preserving the existing oneOf decision_schemas
behavior for non-empty candidates.
In `@scripts/own-fix-plan.sh`:
- Around line 52-56: Update the validate-plan invocation in
scripts/own-fix-plan.sh to pass "$out" directly as its --output destination, and
remove the intermediate "$work/validated-plan.json" path and subsequent mv.
Preserve validation while relying on validate-plan’s _write_atomic behavior to
stage and replace beside the final destination.
🪄 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: 748db9fa-6d40-48fe-9dbe-b1313516c220
📒 Files selected for processing (9)
.github/workflows/ci.ymlownlang/__main__.pyownlang/fix_candidates.pyownlang/fix_plan.pyscripts/own-fix-plan.shtests/fixtures/o7-invoke/subscription-fix-plan-v1/candidates.jsontests/fixtures/o7-invoke/subscription-fix-plan-v1/expected-validated-plan.jsontests/fixtures/o7-invoke/subscription-fix-plan-v1/o7-result.jsontests/test_fix_plan.py
…nvoke -> plan) The S1 increment of the subscription-autofix pipeline: turn a candidates.json into a prompt + per-candidate JSON Schema, run the closed-world model call through the existing `o7 invoke`, and validate the UNTRUSTED result back into an executable plan. Still analysis/planning only — nothing edits source, Own.NET never calls o7, and the model authors nothing but an enum action per finding. Architecture (as locked): Own.NET owns render + domain validation; 007 owns `o7 invoke` / engine / envelope; `scripts/own-fix-plan.sh` is glue only; apply stays S2. - `own-fix subscriptions render <candidates.json> --prompt <f> --schema <f>`: a byte-deterministic prompt (a minimal projection — finding id, event context, allowed actions; NO source, spans, SHA, or absolute paths) and a schema whose `decisions` is a fixed-length array (minItems==maxItems==candidate count) of `oneOf` objects, each binding a finding_id `const` to ITS OWN allowed-actions enum, additionalProperties false throughout. - `own-fix subscriptions validate-plan <candidates.json> <fix-plan.json> --output <f>`: validates the untrusted plan and materializes it. Requires a full bijection between candidate and decision finding ids; `action` is the only model-authored field and must be in that candidate's allowed_actions; every other field (file, acquire_span, target_api, selection, source SHA) is re-derived from the candidates; decisions are emitted in candidates order; `input_bundle_sha256` is the canonical hash of the input. Hard errors: malformed JSON, wrong version/operation, unknown/extra field (rejects code/patch/diff/command/reason/evidence/confidence), unknown/missing/duplicate id, out-of-scope or not-allowed action, invalid candidates bundle. Written atomically. - Shared `validate_candidates_bundle` (fix_candidates.py) enforces the S0 envelope for both commands (version/operation, pinned target, one type/one file, canonical root-relative paths + SHA shape, unique ids, S1-only allowed_actions, file/type containment). - Conformance fixture `tests/fixtures/o7-invoke/subscription-fix-plan-v1/` (Own.NET- owned): candidates.json + o7-result.json -> expected-validated-plan.json. - `scripts/own-fix-plan.sh`: render -> `o7 invoke` (read-only-data, explicit schema, never `o7 run`) -> validate-plan; temp workdir, aborts on any non-zero, leaves no partial artifact, touches no source. Tests: `tests/test_fix_plan.py` (29 checks — the locked acceptance set) + a CI glue step exercising the shell orchestration with a fake o7. ruff + mypy (ownlang) + run_tests green. Live-wiring evidence: `own-fix-plan.sh` with the real `o7 invoke` + claude 2.1.162 produced the correct validated plan (convert_acquire for the INotifyPropertyChanged finding, manual_review for the name_only one). `--json-schema` enforcement on 2.1.162 is best-effort, so the model occasionally narrates in prose (a FAIL o7 catches and the glue aborts cleanly); a retry lands valid JSON. Frozen S1 boundary: no rewriter / source modification / stale-SHA-vs-current-file / convert_exact_teardown / remove API / model spans / model code / direct Claude-Codex call from Own.NET / new o7 adapter / more than one type+file. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JxKjqdGEFzq4UzZupw379G
d10a805 to
0c30e00
Compare
…ndidate schema, glue atomicity
Closes the four Part-S1 safety-contract blockers (arbiter). No new scope.
Blocker 1 — a valid plan over a malformed candidates bundle leaked a CollectError
(the CLI caught only PlanError -> traceback/exit 1). validate_plan now normalizes
the bundle-validation CollectError into PlanError, so the CLI's single except gives
a controlled exit 2 with no output. CLI regression added.
Blocker 2 — the shared validator did not hold the frozen envelope. It now requires
EXACTLY one allowed_type and one source_file with a matching path, each candidate to
match the EXACT (containing_type, file) pair (not two independent membership tests),
selected_findings to be null or a unique-id list whose set equals the candidate ids,
canonical paths made only of real segments (rejects ""/"."/"./x"/"A//x"/"A/."/trailing
"/"), a known event_contract, and the frozen S0 tiering (convert_acquire only for an
inotify_property_changed contract). validate_plan additionally MATERIALIZES target_api
/ selection / source_files as a canonical projection of their known keys (not a whole-
dict copy), so no unknown nested field can ride into the executable artifact.
Blocker 3 — a zero-candidate bundle is legitimate but produced `{"oneOf": []}`, which
is invalid Draft 2020-12. render now emits `{"type":"array","minItems":0,"maxItems":0,
"items":false}` for that case; validate_plan materializes an empty plan.
Blocker 4 — own-fix-plan.sh wrote into the work dir then `mv`d to $out (a cross-
filesystem copy+delete, non-atomic). It now passes $out directly to validate-plan,
whose writer does adjacent-temp + os.replace, so the final artifact is atomic.
tests/test_fix_plan.py grows to 44 checks (the new envelope / tiering / path / zero-
candidate / normalized-error regressions, incl. the CLI leg). ruff + mypy (ownlang) +
run_tests green; the fixture's expected-validated-plan.json is regenerated (the
canonical projection).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JxKjqdGEFzq4UzZupw379G
1 participant
S1 — invoke planner (candidates.json → o7 invoke → validated plan)
Analysis/planning only; nothing edits source, Own.NET never calls o7, and the model authors nothing but an enum action per finding. Apply stays S2.
Separation (locked): Own.NET owns
render+ domain validation; 007 owns the existingo7 invoke/ engine / envelope;scripts/own-fix-plan.shis glue only.own-fix subscriptions render— byte-deterministic prompt (minimal projection: finding id, event context, allowed actions — no source/spans/SHA/paths) + a schema whosedecisionsis a fixed-lengthoneOfarray binding eachfinding_idconstto its own allowed-actions enum,additionalProperties:falsethroughout.own-fix subscriptions validate-plan— validates the untrusted plan and materializes it: full bijection with candidate ids;actionis the only model-authored field (∈ that candidate's allowed_actions); every other field (file, acquire_span, target_api, selection, source SHA) is re-derived from the candidates; decisions in candidates order;input_bundle_sha256over the canonical input. Hard errors for malformed JSON / wrong version / unknown-extra field (rejects code/patch/evidence/confidence) / unknown-missing-duplicate id / out-of-scope or not-allowed action / invalid bundle. Atomic write.validate_candidates_bundleenforces the S0 envelope for both commands.tests/fixtures/o7-invoke/subscription-fix-plan-v1/(Own.NET-owned).scripts/own-fix-plan.sh— render →o7 invoke(read-only-data, explicit schema, nevero7 run) → validate; temp workdir, aborts on non-zero, no partial artifact, no source touched.Tests:
tests/test_fix_plan.py(29 checks — the locked acceptance set) + a CI glue step with a fake o7.Live-wiring evidence:
own-fix-plan.sh+ realo7 invoke+ claude produced the correct validated plan (convert_acquire for the INotifyPropertyChanged finding, manual_review for name_only).Frozen: no rewriter / source mod / stale-SHA / convert_exact_teardown / remove API / model spans or code / direct Claude-Codex call from Own.NET / new o7 adapter / >1 type+file.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
own-fix subscriptions candidates,render, andvalidate-plancommands.Tests