S0: own-fix subscriptions candidates (Part A: extractor metadata) - #285
Conversation
…iption fixes Part A of S0 (`own-fix subscriptions candidates`). Roslyn extractor only; the Python collector is Part B. Analysis-only, strictly additive, arbiter contract. New internal flag `--fix-candidates`. With it OFF the facts JSON is byte-for-byte identical (verified: flag-off vs pre-S0 `git stash` build diff clean) and `ownir_version` is unchanged (0). With it ON: - top-level `fix_candidates_version: 1` (additive metadata, NOT an ownir_version bump — no new resource-kind or analysis-routing value); - each component gains `qualified_name` (one documented FQN format) + type-shape metadata (`is_partial`, `is_nested`, `declaration_count`, `is_generated`); - each eligible non-timer `+=` subscription gains a namespaced `fix` block. The `fix` block honours the locked amendments: - **Full TextSpan** (`start`/`length` UTF-16 + start/end line/column), not a bare line — a future rewriter needs the absolute span; two subs on one line differ. - **Semantic INotifyPropertyChanged**, never name-matching: `event_contract` is `inotify_property_changed` only for the actual `INotifyPropertyChanged.PropertyChanged` member or a proven implementation; a same-named event on an unrelated type is `name_only`; anything else `other`. - **Symbol-based teardown**, separate from the existing string-keyed `released` (which is untouched): `teardown.status` is `none` / `exact` (one `-=` site with equal event symbol + receiver identity + handler identity) / `ambiguous` (multiple sites, or any identity unresolved); each candidate carries its span. - **Line-independent identity constituents** emitted for Part B to hash: `enclosing_member`, `event_identity`, `source_identity`, `handler_identity` (method symbol, else a normalized syntax fingerprint), `occurrence_ordinal`. - **Nested-type boundary**: a `fix` block is attached only when the acquire's IMMEDIATE containing type is this class (a nested class's subscription is fixed under the nested type's own iteration, never the outer's). Existing subscription facts (and `released`) are unchanged. Sample `FixCandidatesSample.cs` + `tests/check_fix_candidates_facts.py` cover: exact/none/ambiguous teardown, INPC vs name_only vs other, two-subs-one-line distinct spans, wrapped-delegate normalization (`+= new H(m)` / `-= m` = exact), nested isolation, and flag-off additive-cleanliness. CI step added; ruff + run_tests green locally. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JxKjqdGEFzq4UzZupw379G
| // INPC subscription, NO teardown -> status none. | ||
| public sealed class InpcNoTeardown | ||
| { | ||
| public InpcNoTeardown(IPub pub) => pub.PropertyChanged += OnChanged; |
|
|
||
| public sealed class NameOnlySubscriber | ||
| { | ||
| public NameOnlySubscriber(FakePub p) => p.PropertyChanged += OnChanged; |
|
|
||
| public sealed class OtherEventSubscriber | ||
| { | ||
| public OtherEventSubscriber(ClickPub p) => p.Clicked += OnClick; |
| // apart, but their full spans (start/length) must differ. | ||
| public sealed class TwoOnOneLine | ||
| { | ||
| public TwoOnOneLine(IPub a, IPub b) { a.PropertyChanged += OnA; b.PropertyChanged += OnB; } |
| // apart, but their full spans (start/length) must differ. | ||
| public sealed class TwoOnOneLine | ||
| { | ||
| public TwoOnOneLine(IPub a, IPub b) { a.PropertyChanged += OnA; b.PropertyChanged += OnB; } |
| // OUTER component; the outer's own subscription still does. | ||
| public sealed class OuterWithNested | ||
| { | ||
| public OuterWithNested(IPub pub) => pub.PropertyChanged += OnOuter; |
|
|
||
| public sealed class Nested | ||
| { | ||
| public Nested(IPub pub) => pub.PropertyChanged += OnNested; |
|
|
||
| public sealed class Nested | ||
| { | ||
| public Nested(IPub pub) => pub.PropertyChanged += OnNested; |
|
Warning Review limit reached
Next review available in: 35 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 (4)
📝 WalkthroughWalkthroughChangesThe extractor adds opt-in Fix-candidate extraction and collection
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant CI as tests job
participant Extractor as OwnSharp.Extractor
participant Facts as fix-candidate facts
participant CLI as own-fix CLI
participant Collector as collect_candidates
participant Output as candidates.json
CI->>Extractor: generate facts with --fix-candidates
Extractor-->>Facts: emit subscription fix metadata
CI->>CLI: invoke subscriptions candidates
CLI->>Collector: pass facts and target configuration
Collector->>Facts: resolve eligible subscriptions
Collector-->>Output: write deterministic candidate bundles
CI->>Output: assert candidate and failure contracts
🚥 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5c1e1c4786
ℹ️ 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".
| var recv = FixEventReceiver(a.Left); | ||
| ISymbol? sourceSym = recv is null ? model.GetDeclaredSymbol(cls) : model.GetSymbolInfo(recv).Symbol; | ||
| var sourceText = recv is null ? "this" : recv.ToString(); | ||
| var sourceId = sourceSym is not null ? sourceSym.ToDisplayString(FixFqnFormat()) : FixNormWs(sourceText); |
There was a problem hiding this comment.
Keep call receivers out of exact teardown matches
When the event receiver is an expression such as GetPublisher().Changed, GetSymbolInfo(recv).Symbol resolves to the method/property symbol, so a later GetPublisher().Changed -= OnChanged will be classified as an exact teardown even though the two calls can return different publisher instances. This affects --fix-candidates metadata for call/property receivers and can make the fix pipeline treat an unsafe or unrelated teardown as exact; only stable lvalues like fields/locals/parameters/this should be exact, while computed receivers should remain ambiguous.
Useful? React with 👍 / 👎.
|
@coderabbitai review |
…rity gate Addresses the Part-A CHANGES-REQUESTED blockers (arbiter). No new architecture. **Blocker 1 — computed receiver/handler must never be `exact`.** Symbol equality over-claimed: `GetPublisher().PropertyChanged += H` / `-= H` resolve to the same IMethodSymbol yet may return different instances; likewise a property receiver, or `a.Publisher` vs `b.Publisher` (same field member, different root). New conservative classifiers `FixReceiverIdentity` / `FixHandlerIdentity` return a kind — `stable_symbol` (this / local / parameter / instance-field-of-this / static field; method group / delegate local/parameter/field) vs `computed` (invocation / property / indexer / conditional access / field-of-another-value / lambda) vs `unresolved`. An `exact` teardown now requires the single `-=` to match by STABLE symbol on event + receiver + handler; anything computed/unresolved or text-only tops out at `ambiguous`. The `fix` block emits `source_identity_kind` / `handler_identity_kind`, and each teardown candidate carries `match: stable|text`. **Blocker 2 — occurrence ordinal scoped by enclosing member.** The ordinal key now includes `enclosing_member`, so identical acquires in different members are each 0 and inserting a subscription into another member never shifts an existing one's identity. `FixFqnFormat` gains `IncludeParamsRefOut` so `M(int)` and `M(ref int)` get distinct enclosing signatures. **Blocker 3 — real byte-parity gate.** `tests/goldens/fix_candidates_off.golden.json` is the pre-S0 output for FixCandidatesSample.cs, generated by the base extractor at the branch point (ff21d4a) — verified byte-identical to the current flag-off run. CI now `diff`s flag-off output against it byte-for-byte (not a key search). The checker also strips every additive field from the flag-ON facts and asserts the result EQUALS flag-off (same records, order, and old values). Regression cases added to the sample + checker: computed-invocation / computed-property / different-roots / computed-handler (none of them `exact`); ordinal-across-members (each 0) / ordinal-within-member (0,1) / ref-vs-value overload (distinct enclosing). Local: extractor build clean, ruff + run_tests green, check + golden byte-parity pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JxKjqdGEFzq4UzZupw379G
| { | ||
| _a = a; | ||
| _b = b; | ||
| _a.Publisher.PropertyChanged += OnChanged; |
| public OrdinalAcrossMembers(IPub pub) | ||
| { | ||
| _pub = pub; | ||
| _pub.PropertyChanged += OnChanged; |
| _pub.PropertyChanged += OnChanged; | ||
| } | ||
|
|
||
| public void Reattach() => _pub.PropertyChanged += OnChanged; |
| { | ||
| public OrdinalWithinMember(IPub pub) | ||
| { | ||
| pub.PropertyChanged += OnChanged; |
| public OrdinalWithinMember(IPub pub) | ||
| { | ||
| pub.PropertyChanged += OnChanged; | ||
| pub.PropertyChanged += OnChanged; |
| private readonly IPub _pub; | ||
|
|
||
| public RefOverloadEnclosing(IPub pub) => _pub = pub; | ||
| public void Attach(int x) => _pub.PropertyChanged += OnChanged; |
|
|
||
| public RefOverloadEnclosing(IPub pub) => _pub = pub; | ||
| public void Attach(int x) => _pub.PropertyChanged += OnChanged; | ||
| public void Attach(ref int x) => _pub.PropertyChanged += OnChanged; |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/check_fix_candidates_facts.py (1)
68-189: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winValidate the complete new metadata contract.
The checker never asserts
is_partial,declaration_count,is_generated, complete span coordinates/length, or the full requiredfixkey set. Their omission could therefore pass CI. Add shared schema assertions, including default values for the current fixtures and positive partial/generated coverage where practical.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/check_fix_candidates_facts.py` around lines 68 - 189, Extend the shared validation in the test checker around only_fix and component assertions to enforce the complete metadata schema: require the full fix key set, validate complete span coordinates and length, and assert is_partial, declaration_count, and is_generated with their fixture defaults. Add or use fixtures that positively cover partial and generated components, while preserving the existing candidate-specific checks.
🤖 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 `@frontend/roslyn/OwnSharp.Extractor/Program.cs`:
- Around line 649-690: Update FixHandlerIdentity and the exact-matching identity
flow to retain a method group’s receiver/target alongside its method symbol, so
handlers from different instances do not compare equal. For local, parameter,
and field handlers, stop treating storage as unconditionally stable; verify the
value remains unchanged between the add and remove operations before allowing
exact matching, otherwise classify it as computed or unresolved.
---
Nitpick comments:
In `@tests/check_fix_candidates_facts.py`:
- Around line 68-189: Extend the shared validation in the test checker around
only_fix and component assertions to enforce the complete metadata schema:
require the full fix key set, validate complete span coordinates and length, and
assert is_partial, declaration_count, and is_generated with their fixture
defaults. Add or use fixtures that positively cover partial and generated
components, while preserving the existing candidate-specific checks.
🪄 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: 79579390-6938-4f7a-8e19-a276480c57c8
📒 Files selected for processing (6)
.github/workflows/ci.ymlfrontend/roslyn/OwnSharp.Extractor/Program.csfrontend/roslyn/samples/FixCandidatesSample.cstests/check_fix_candidates_facts.pytests/goldens/README.mdtests/goldens/fix_candidates_off.golden.json
| _pub = pub; | ||
| _left = left; | ||
| _right = right; | ||
| _pub.PropertyChanged += _left.OnChanged; |
…this` method groups Closes the remaining half of Part-A blocker 1 (arbiter). FixHandlerIdentity called every IMethodSymbol/local/parameter/field "stable", but a method SYMBOL is not a delegate identity and a storage SYMBOL is not its (mutable) value, so `exact` (which compares symbols) produced two false positives: _pub.PropertyChanged += _left.OnChanged; // same IMethodSymbol, _pub.PropertyChanged -= _right.OnChanged; // different target -> not the same delegate _handler = OnFirst; _pub.PropertyChanged += _handler; _handler = OnSecond; _pub.PropertyChanged -= _handler; // same IFieldSymbol, different value `stable_symbol` is now limited to the delegate-fixed forms provable WITHOUT dataflow: a static method group (null target), or an instance method group on `this` (bare `OnChanged` / `this.OnChanged`). A method group on any other receiver, a delegate held in a local/parameter/field/property, and a lambda are all `computed` and can never ground an `exact`. The core STS shape (`source.PropertyChanged += OnChanged; -= OnChanged;`) stays `exact`. Regressions added: HandlerDifferentTarget (-> none), HandlerReassignedField (-> ambiguous), plus the preserved positive controls InpcExactTeardown / WrappedDelegate (still exact). Golden regenerated from the base extractor (byte-identical to flag-off); check + ruff + run_tests green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JxKjqdGEFzq4UzZupw379G
ba4ec1f to
52ec06b
Compare
…ndidate collector Part B of S0 (Part A shipped the extractor `fix` metadata). A new `python -m ownlang own-fix subscriptions candidates <facts.json> --config <own.toml> --class <FQN> [--finding-id <ID>]... --output <candidates.json> [--root <dir>]` turns the `--fix-candidates` facts into a deterministic candidates.json. It changes no source — the C# `fix` block already carries the semantics; this only assembles, identifies, filters and orders. Per the locked contract: - `--class` is an EXACT fully-qualified name; a partial / nested / generated / duplicate-FQN / unknown type is a hard error. - `finding_id` is versioned and LINE-INDEPENDENT: SHA256(version . containing_type . enclosing_member . event_identity . source_identity . handler_identity . occurrence_ordinal) — span/line are location metadata, never in the id. - `target_api` is PINNED from config (`[weak-subscription].target`, or the sole `subscribe` entry) — several entries with no target is a hard error, never a silent first-pick. - `allowed_actions` = `[convert_acquire, manual_review]` only for a proven INotifyPropertyChanged contract, else `[manual_review]`. `convert_exact_teardown` is NOT offered in S0 (deferred to S2 with a pinned remove API); teardown metadata is carried but grants no conversion. - a released subscription is not a leak, so not a candidate; candidates are deterministically ordered (file, span start, id); every source file gets a SHA-256 (S2 refuses if the file changed); the output carries the selection-request safety envelope (allowed_types, selected_findings, constraints). `--finding-id` filters and hard-fails on any unknown id (the chicken-and-egg fix: no ids = discover all eligible; ids = explicit final selection). `ownlang/fix_candidates.py` (core), `ownlang/config.py::load_target_subscribe`, `own-fix` dispatch in `__main__.py`, `tests/test_fix_candidates.py` (18 checks: line-independence, INPC permission tiering, released-skip, per-file SHA, ordering, partial/nested/generated/dup/unknown/unknown-id rejections, target pinning), an end-to-end CI step over the real extractor facts, and a `spec/CLI.md` row. Local: ruff + mypy (ownlang) + run_tests (18/0) green; end-to-end own-fix over the extractor facts verified. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JxKjqdGEFzq4UzZupw379G
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/__main__.py`:
- Around line 485-487: Wrap the output file write in the CLI error-handling flow
around the existing open/write block in __main__.py, catching OSError for
missing parent directories, permission failures, and directory-valued outputs.
Report the failure as a user-facing CLI error and exit through the established
error path instead of allowing the exception to escape; keep the success message
only after the write completes.
In `@ownlang/config.py`:
- Around line 93-99: Update the weak-subscription parsing flow around the
explicit target branch so the entire table is validated before returning target.
Ensure unsupported keys and malformed subscribe values are still rejected by
invoking the existing _weak_subscribe_from validation path, while preserving
target’s string and entry validation and its returned value.
In `@ownlang/fix_candidates.py`:
- Around line 164-176: The candidate collection flow must validate the
fix-candidate schema before calling _resolve_class. Add a validator that rejects
missing, boolean, unsupported, or malformed fix_candidates_version values and
validates required component, subscription, and fix shapes, converting malformed
external JSON into CollectError; invoke it at the start of the affected
collector before resolving the class.
- Around line 75-82: Update _sha_file to validate that rel resolves to a regular
file within root before opening it, rejecting absolute paths, parent-directory
traversal, and symlink escapes with CollectError. Preserve the existing
source-file hashing and error-reporting behavior for valid paths.
🪄 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: febc9809-cffa-4689-8791-a8412903c83d
📒 Files selected for processing (10)
.github/workflows/ci.ymlfrontend/roslyn/OwnSharp.Extractor/Program.csfrontend/roslyn/samples/FixCandidatesSample.csownlang/__main__.pyownlang/config.pyownlang/fix_candidates.pyspec/CLI.mdtests/check_fix_candidates_facts.pytests/goldens/fix_candidates_off.golden.jsontests/test_fix_candidates.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/goldens/fix_candidates_off.golden.json
- frontend/roslyn/OwnSharp.Extractor/Program.cs
- tests/check_fix_candidates_facts.py
…fined source paths Closes the three Part-B safety-contract blockers (arbiter). No new architecture — these harden guarantees the collector already claimed. Blocker 1 — the collector ignored fix_candidates_version and indexed shapes blindly. `collect_candidates` now requires `fix_candidates_version` to be integer 1 (missing / bool / string / other number -> CollectError), and every field it reads or republishes is shape-checked (`_field` + `_validate_span` / `_validate_teardown` / `_validate_fix`, plus component + subscription shapes) so a malformed component/subscription/fix/span/ teardown surfaces as a controlled CollectError, never a KeyError/TypeError traceback. It is a narrow check of the consumed S0 contract, not a full JSON Schema. Blocker 2 — an explicit config `target` bypassed table validation. `load_target_subscribe` now calls `_weak_subscribe_from` (full-table validation: unknown keys, malformed `subscribe`) BEFORE honouring an explicit `target`, so `target = "..."` can no longer smuggle a `subscribes` typo or a non-list `subscribe` past the fail-loud contract. The "explicit target wins over a valid subscribe list" semantics are unchanged. Blocker 3 — a facts-supplied `file` could escape --root. New `_resolve_source` canonicalizes with realpath, confirms containment via commonpath, and rejects a `..` escape, an absolute path outside root, a symlink pointing out, and a non-regular file; it returns one canonical root-relative (`/`) path used in the candidate, allowed_types, the sort key, and source_files. `_sha_of` reads the confined absolute path. Non-blocker: the `--output` write is now wrapped, so a bad parent/dir path is a clean non-zero error instead of a traceback. tests/test_fix_candidates.py grows to 29 checks: missing/bool/v2 version, missing identity field, malformed span/teardown, `..`/absolute/symlink path escapes, and target+unsupported-key / target+malformed-subscribe config rejections. ruff + mypy (ownlang) + run_tests green; end-to-end own-fix over the real extractor facts still emits a canonical root-relative path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JxKjqdGEFzq4UzZupw379G
S0 —
own-fix subscriptions candidates(analysis-only)First increment of the subscription-autofix pipeline (Own.NET candidates → o7 invoke enum fix-plan → deterministic Roslyn rewriter → 007 gates → human apply). Analysis-only; no source is modified. One PR, two parts:
Part A (this commit) — Roslyn extractor metadata
Additive internal
--fix-candidatesflag.git stashpre-S0 build diff),ownir_versionunchanged (0).fix_candidates_version: 1; componentqualified_name+is_partial/is_nested/declaration_count/is_generated; a namespacedfixblock per eligible non-timer+=.fixhonours the locked amendments: full TextSpan (start/length + line/column); semantic INotifyPropertyChanged (inotify_property_changed/name_only/other, never name-matching); symbol-based teardown (none/exact/ambiguous, separate from the untouched string-keyedreleased); line-independent identity constituents (enclosing_member,event_identity,source_identity,handler_identity,occurrence_ordinal) for Part B to hash; nested-type boundary (fix attaches only to the immediate type's own acquires).Sample
FixCandidatesSample.cs+tests/check_fix_candidates_facts.pycover exact/none/ambiguous teardown, INPC vs name_only vs other, two-subs-one-line distinct spans, wrapped-delegate normalization, nested isolation, and flag-off additive-cleanliness. CI step added.Part B (pending) — Python
own-fix subscriptions candidatescollectorSelection request / safety envelope; versioned line-independent
finding_id(SHA-256 over the extractor's identity constituents); pinnedtarget_api;allowed_actions = [convert_acquire, manual_review]; deterministic orderedcandidates.jsonwith per-file SHA-256. Follows the Part-A implementation-check.🤖 Generated with Claude Code
Summary by CodeRabbit
--fix-candidatesmode to emit additive fix-candidate metadata for event subscription facts.own-fix subscriptions candidatesto generate deterministiccandidates.jsonfrom--fix-candidatesfacts.fixed-style positioning/identity metadata and stable, line-independent finding IDs.