Skip to content

S0: own-fix subscriptions candidates (Part A: extractor metadata) - #285

Merged
PhysShell merged 5 commits into
mainfrom
claude/s0-fix-candidates
Jul 16, 2026
Merged

S0: own-fix subscriptions candidates (Part A: extractor metadata)#285
PhysShell merged 5 commits into
mainfrom
claude/s0-fix-candidates

Conversation

@PhysShell

@PhysShell PhysShell commented Jul 16, 2026

Copy link
Copy Markdown
Owner

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-candidates flag.

  • Flag OFF ⇒ facts JSON byte-for-byte identical (verified via a git stash pre-S0 build diff), ownir_version unchanged (0).
  • Flag ON ⇒ top-level fix_candidates_version: 1; component qualified_name + is_partial/is_nested/declaration_count/is_generated; a namespaced fix block per eligible non-timer +=.

fix honours 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-keyed released); 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.py cover 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 candidates collector

Selection request / safety envelope; versioned line-independent finding_id (SHA-256 over the extractor's identity constituents); pinned target_api; allowed_actions = [convert_acquire, manual_review]; deterministic ordered candidates.json with per-file SHA-256. Follows the Part-A implementation-check.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features
    • Added opt-in --fix-candidates mode to emit additive fix-candidate metadata for event subscription facts.
    • Introduced own-fix subscriptions candidates to generate deterministic candidates.json from --fix-candidates facts.
    • Added fixed-style positioning/identity metadata and stable, line-independent finding IDs.
  • Bug Fixes
    • Ensured unresolved/ambiguous computed receiver/handler scenarios are never labeled as “exact”.
    • Preserved identical extractor output when fix-candidates mode is disabled.
  • Tests
    • Added sample-based S0 validation, schema/negative-case checks, and a new flag-off golden to enforce byte-for-byte stability.
    • Added Python test coverage for determinism, ordering, SHA pinning, and error handling.

…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;
@coderabbitai

coderabbitai Bot commented Jul 16, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@PhysShell, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 35 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 92c47dad-02f3-4977-b1d5-0105ccf3c0e1

📥 Commits

Reviewing files that changed from the base of the PR and between d99bedd and e52ccbc.

📒 Files selected for processing (4)
  • ownlang/__main__.py
  • ownlang/config.py
  • ownlang/fix_candidates.py
  • tests/test_fix_candidates.py
📝 Walkthrough

Walkthrough

Changes

The extractor adds opt-in --fix-candidates metadata for subscriptions. A new ownlang collector and CLI command produce deterministic candidate bundles with configuration, identity, source-hash, and selection constraints. Samples, tests, goldens, documentation, and CI validate the contracts and flag-off compatibility.

Fix-candidate extraction and collection

Layer / File(s) Summary
Extractor mode and metadata generation
frontend/roslyn/OwnSharp.Extractor/Program.cs
Adds opt-in fix metadata, stable identities, teardown classification, occurrence ordinals, and additive JSON fields.
Subscription scenario coverage
frontend/roslyn/samples/FixCandidatesSample.cs
Adds event, teardown, nesting, identity, blocker, ordinal, overload, and handler regression scenarios.
Candidate contract and CLI collection
ownlang/fix_candidates.py, ownlang/config.py, ownlang/__main__.py, spec/CLI.md
Adds target pinning, deterministic finding IDs and candidate bundles, source hashing, class validation, filtering, and the own-fix subscriptions candidates command.
Contract validation, tests, and baseline parity
tests/check_fix_candidates_facts.py, tests/test_fix_candidates.py, tests/goldens/*, .github/workflows/ci.yml
Validates extractor and collector invariants, error cases, deterministic output, and byte-for-byte flag-off parity in CI.

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
Loading
🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description does not follow the required template and omits the mandated sections for purpose, change type, verification, issues, and checklist. Rewrite it to match the template headings exactly and fill in all required sections, including change type, verification steps, related issues, and checklist.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title is concise and clearly points to the extractor metadata work, though it mentions only Part A of a larger two-part change.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/s0-fix-candidates

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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".

Comment on lines +661 to +664
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);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

@github-actions

Copy link
Copy Markdown

@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;

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
tests/check_fix_candidates_facts.py (1)

68-189: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Validate the complete new metadata contract.

The checker never asserts is_partial, declaration_count, is_generated, complete span coordinates/length, or the full required fix key 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

📥 Commits

Reviewing files that changed from the base of the PR and between ff21d4a and 5f3b7e8.

📒 Files selected for processing (6)
  • .github/workflows/ci.yml
  • frontend/roslyn/OwnSharp.Extractor/Program.cs
  • frontend/roslyn/samples/FixCandidatesSample.cs
  • tests/check_fix_candidates_facts.py
  • tests/goldens/README.md
  • tests/goldens/fix_candidates_off.golden.json

Comment thread frontend/roslyn/OwnSharp.Extractor/Program.cs
_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
@PhysShell
PhysShell force-pushed the claude/s0-fix-candidates branch from ba4ec1f to 52ec06b Compare July 16, 2026 12:39
…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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 5f3b7e8 and d99bedd.

📒 Files selected for processing (10)
  • .github/workflows/ci.yml
  • frontend/roslyn/OwnSharp.Extractor/Program.cs
  • frontend/roslyn/samples/FixCandidatesSample.cs
  • ownlang/__main__.py
  • ownlang/config.py
  • ownlang/fix_candidates.py
  • spec/CLI.md
  • tests/check_fix_candidates_facts.py
  • tests/goldens/fix_candidates_off.golden.json
  • tests/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

Comment thread ownlang/__main__.py Outdated
Comment thread ownlang/config.py Outdated
Comment thread ownlang/fix_candidates.py Outdated
Comment thread ownlang/fix_candidates.py Outdated
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants