feat(P-035): declared weak-subscribe wrapper = accepted release (B0+B1)#284
Conversation
Implements P-035 Increment B behind a minimal P-015 config carrier (B0), per the
arbiter decision. A project declares its own weak-subscribe wrapper API and the
extractor treats a matching call as a first-class, already-released subscription
instead of assuming the BCL WeakEventManager.
B0 — minimal P-015 carrier (config only; no auto-discovery / severity / env):
* ownlang/config.py — read [weak-subscription].subscribe from an explicit own.toml
(tomllib). Only that one table/key; each entry must be exact "SimpleType.Method".
A malformed declaration is a HARD error, never a silent skip.
* `python -m ownlang config <own.toml>` prints the declared names for own-check.
* scripts/own-check.sh: `--config own.toml` forwards them to the extractor via the
internal `--weak-subscribe` transport flag; a bad config exits non-zero.
B1 — extractor detection + released recognition (only declared exact pairs; zero
behaviour without a declaration):
* A call whose (containing-type simple name, method name) is declared, with the MVP
(source, handler) positional-arg contract, becomes one subscription fact with
released:true. Match resolves via the method symbol, or a conservative syntactic
receiver-name fallback for an unresolved external package.
* A declared wrapper named `Subscribe` is suppressed in the Rx `.Subscribe()` matcher
so it is not double-counted.
* Empty allowlist => the detector is inert and the Rx guard is a no-op => facts are
byte-for-byte unchanged.
Acceptance contract (#1-#10) covered by:
* tests/test_weak_subscribe.py — the config carrier, incl. every malformed-config
hard-error case (auto-discovered by run_tests.py; SDK-free).
* frontend/roslyn/samples/WeakSubscribeAllowlistSample.cs + a new CI step in the
"C# leak extractor" job that runs the extractor with/without --weak-subscribe and
asserts the facts via tests/check_weak_subscribe_facts.py, plus the own-check
--config plumbing and the malformed-config hard error end-to-end.
Not in scope (arbiter): Increment A (unsubscribe wrapper), C (project-aware fix
text / 007 wiring), a general AddHandler heuristic, and the rest of P-015.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JxKjqdGEFzq4UzZupw379G
|
Warning Review limit reached
Next review available in: 44 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 (1)
📝 WalkthroughWalkthroughAdds strict ChangesWeak-subscription allowlist
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Action
participant OwnCheck
participant OwnLangConfig
participant OwnSharpExtractor
participant FactChecker
Action->>OwnCheck: pass --config own.toml
OwnCheck->>OwnLangConfig: load weak-subscription declarations
OwnLangConfig-->>OwnCheck: return wrapper pairs
OwnCheck->>OwnSharpExtractor: pass --weak-subscribe pairs
OwnSharpExtractor-->>FactChecker: write subscription facts
FactChecker-->>Action: validate released and ordinary findings
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 |
![github-advanced-security[bot]](https://cdn.github.com/in/57789?s=60&v=4){kind=small}
| { | ||
| public OrdinaryPlusEquals(ISettings settings) | ||
| { | ||
| settings.PropertyChanged += OnChanged; |
![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: 4e6a911925
ℹ️ 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".
| return _weak_subscribe_from(data, path) | ||
|
|
||
|
|
||
| def _weak_subscribe_from(data: dict, path: str) -> list[str]: |
There was a problem hiding this comment.
The lint job in .github/workflows/ci.yml runs mypy over ownlang with strict = true from pyproject.toml; this new bare dict annotation makes the gate fail with Missing type arguments for generic type "dict" before tests run. Use a parameterized shape such as dict[str, object]/Mapping[str, object] for the TOML payload so the documented mypy gate stays green.
Useful? React with 👍 / 👎.
| --config) | ||
| [[ $# -ge 2 ]] || { echo "own-check: --config requires a value" >&2; exit 2; } | ||
| config="$2"; shift 2 ;; |
There was a problem hiding this comment.
This makes --config available only through the bash wrapper, but the documented Windows/Visual Studio entrypoint is scripts/own-check.ps1, and I checked that it still has no -Config parameter or forwarding to python -m ownlang config / --weak-subscribe. In Windows workflows the same own.toml declaration is therefore unusable, so declared weak wrappers continue to report OWN001 there; please add equivalent PowerShell plumbing when making this surface public.
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: 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 @.github/workflows/ci.yml:
- Around line 1583-1586: Update the malformed-config assertion around
scripts/own-check.sh to capture its output and verify the command fails before
leak detection runs. Do not rely solely on the exit status, since the
intentional OrdinaryPlusEquals finding in $sample also produces a non-zero
result; assert that the captured output does not contain that positive-control
finding while preserving the expected failure for malformed bad.toml.
In `@ownlang/config.py`:
- Line 43: Fix the mypy generic-type error in _weak_subscribe_from by importing
Any from typing and changing the data parameter annotation from an
unparameterized dict to a dict keyed and valued with Any.
In `@scripts/own-check.sh`:
- Around line 16-21: Update the -h|--help branch in scripts/own-check.sh to have
sed print through line 38, ensuring the complete usage, option, and requirements
documentation is displayed.
- Around line 67-69: Add a -Config parameter to the scripts/own-check.ps1
entrypoint and wire it through the same configuration-selection flow used by the
existing --config handling in own-check.sh. Ensure the supplied value is
validated and passed to the underlying check command so PowerShell users can
select a config file.
🪄 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: e520a0cd-e580-46d8-bd1a-f53cd3d53104
📒 Files selected for processing (8)
.github/workflows/ci.ymlfrontend/roslyn/OwnSharp.Extractor/Program.csfrontend/roslyn/samples/WeakSubscribeAllowlistSample.csownlang/__main__.pyownlang/config.pyscripts/own-check.shtests/check_weak_subscribe_facts.pytests/test_weak_subscribe.py
| printf '[weak-subscription]\nsubscribe = ["bad_no_dot"]\n' > "$RUNNER_TEMP/bad.toml" | ||
| if scripts/own-check.sh --config "$RUNNER_TEMP/bad.toml" "$sample" >/dev/null 2>&1; then | ||
| echo "FAIL: malformed --config was silently accepted"; exit 1 | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Fix the false positive in the malformed config test.
The current test logic for the hard error on malformed config relies on a non-zero exit code. Because $sample contains an intentional leak (OrdinaryPlusEquals), scripts/own-check.sh will exit non-zero when it successfully runs the leak check.
If a bug were introduced where bad.toml is silently ignored, the script would run the leak detector, find OrdinaryPlusEquals, and exit non-zero. The if condition would then incorrectly pass the test, masking the configuration bug. To make this assertion robust, capture the output and ensure the leak detector never ran by checking for the absence of the positive control finding.
🐛 Proposed fix to assert early abort
- if scripts/own-check.sh --config "$RUNNER_TEMP/bad.toml" "$sample" >/dev/null 2>&1; then
- echo "FAIL: malformed --config was silently accepted"; exit 1
- fi
+ bad_out=$(scripts/own-check.sh --config "$RUNNER_TEMP/bad.toml" "$sample" 2>&1 || true)
+ if echo "$bad_out" | grep -q "OrdinaryPlusEquals"; then
+ echo "FAIL: malformed --config was silently accepted (leak check ran)"; exit 1
+ fi📝 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.
| printf '[weak-subscription]\nsubscribe = ["bad_no_dot"]\n' > "$RUNNER_TEMP/bad.toml" | |
| if scripts/own-check.sh --config "$RUNNER_TEMP/bad.toml" "$sample" >/dev/null 2>&1; then | |
| echo "FAIL: malformed --config was silently accepted"; exit 1 | |
| fi | |
| printf '[weak-subscription]\nsubscribe = ["bad_no_dot"]\n' > "$RUNNER_TEMP/bad.toml" | |
| bad_out=$(scripts/own-check.sh --config "$RUNNER_TEMP/bad.toml" "$sample" 2>&1 || true) | |
| if echo "$bad_out" | grep -q "OrdinaryPlusEquals"; then | |
| echo "FAIL: malformed --config was silently accepted (leak check ran)"; exit 1 | |
| fi |
🤖 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 @.github/workflows/ci.yml around lines 1583 - 1586, Update the
malformed-config assertion around scripts/own-check.sh to capture its output and
verify the command fails before leak detection runs. Do not rely solely on the
exit status, since the intentional OrdinaryPlusEquals finding in $sample also
produces a non-zero result; assert that the captured output does not contain
that positive-control finding while preserving the expected failure for
malformed bad.toml.
| # [--emit-facts <path>] [--config <own.toml>] [--root <own.net checkout>] | ||
| # [--] <path|file> [more ...] | ||
| # | ||
| # --config <own.toml> reads the project's [weak-subscription].subscribe wrapper | ||
| # names (P-035) and teaches the extractor to treat those calls as already-released | ||
| # weak subscriptions. A malformed config is a hard error. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the truncated --help output.
The addition of these documentation lines pushes the bottom of the comment block down. As a result, the sed -n '2,30p' "$0" command at line 75 now truncates the help text, cutting off the existing --flow-locals, --legacy, and requirements documentation.
Please update the help command in line 75 to print the entire block.
🛠️ Suggested fix for line 75
# Update line 75 to print the full block (up to line 38):
- -h|--help) sed -n '2,30p' "$0"; exit 0 ;;
+ -h|--help) sed -n '2,38p' "$0"; exit 0 ;;🤖 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 `@scripts/own-check.sh` around lines 16 - 21, Update the -h|--help branch in
scripts/own-check.sh to have sed print through line 38, ensuring the complete
usage, option, and requirements documentation is displayed.
| --config) | ||
| [[ $# -ge 2 ]] || { echo "own-check: --config requires a value" >&2; exit 2; } | ||
| config="$2"; shift 2 ;; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if scripts/own-check.ps1 exists and if it implements the new --config flag.
if [ -f scripts/own-check.ps1 ]; then
if rg -q '\-\-config' scripts/own-check.ps1; then
echo "Parity maintained: --config is present in own-check.ps1"
else
echo "Action required: own-check.ps1 exists but is missing --config support"
fi
else
echo "No action required: own-check.ps1 does not exist in this repository"
fiRepository: PhysShell/Own.NET
Length of output: 225
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'own-check.sh:\n'
ast-grep outline scripts/own-check.sh --view expanded || true
printf '\nown-check.ps1 exists?\n'
if [ -f scripts/own-check.ps1 ]; then
wc -l scripts/own-check.ps1
printf '\nRelevant lines from scripts/own-check.ps1:\n'
rg -n --context 3 -- '--config|config|own-check' scripts/own-check.ps1
else
echo 'missing'
fiRepository: PhysShell/Own.NET
Length of output: 958
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '47,95p' scripts/own-check.ps1Repository: PhysShell/Own.NET
Length of output: 1995
Add -Config to scripts/own-check.ps1 The Windows entrypoint still has no equivalent for --config, so PowerShell users can’t select a config file.
🤖 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 `@scripts/own-check.sh` around lines 67 - 69, Add a -Config parameter to the
scripts/own-check.ps1 entrypoint and wire it through the same
configuration-selection flow used by the existing --config handling in
own-check.sh. Ensure the supplied value is validated and passed to the
underlying check command so PowerShell users can select a config file.
Source: Coding guidelines
mypy --strict flagged 'Missing type arguments for generic type dict' on the _weak_subscribe_from param. dict[str, object] (tomllib's shape) -- ruff + mypy both clean now. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JxKjqdGEFzq4UzZupw379G
|
@coderabbitai review |
✅ Action performedReview finished.
|
…ent-leaks, fallback, Action, docs) Addresses the REQUEST CHANGES review on PR #284. Scope stays B0+B1. 1. Handler-shape guard. MatchesDeclaredWeakSubscribe now runs the second argument through NormalizeHandler + IsHandler, so a declared overload whose 2nd parameter is not a delegate -- AddPropertyChanged(source, 42) -- is NOT minted as a subscription. New negative control NonHandlerSecondArgument (zero facts). 2. --no-event-leaks respected. The new detector is gated on `emitEvents && weakSubscribe.Count > 0` (was just Count > 0), and the Rx-Subscribe suppression only fires when emitEvents is on. New facts case: sample + --no-event-leaks + declared -> neither the weak wrapper nor the ordinary += is emitted. 3. Real unresolved-external fixture. WeakSubscribeUnresolvedSample.cs calls External.WeakEvents.AddPropertyChanged (type not in the compilation) so the syntactic receiver-name fallback is exercised (acceptance #8), plus an unresolved negative control with a different final receiver type name. 4. Composite Action exposes the feature. action.yml gains an optional `config:` input, forwarded as OWN_CONFIG and passed to own-check (--config) on both the SARIF and the normal path. A real Action-level test (action-marketplace-readiness.yml, uses: ./): a weak wrapper is a finding WITHOUT config and silent WITH it, and a malformed config hard-errors through the Action. 5. Docs synced to reality. P-035 status + "What exists today" (method-call wrapper is detected, config is consumed; method detector folded into P-035; A and C deferred); P-015 (format resolved: explicit --config own.toml TOML carrier, rest deferred); spec/CLI.md (the `config` command + own-check --config). Verified locally via WSL: extractor facts for all four configs (on/off/no-events/ unresolved) pass tests/check_weak_subscribe_facts.py; the WeakBus fixture is OWN001 without --config and silent with it; ruff + mypy --strict + run_tests.py all green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JxKjqdGEFzq4UzZupw379G
| { | ||
| // Token ignored on purpose: a leak WITHOUT the declaration, an accepted | ||
| // weak subscription WITH it. | ||
| WeakBus.Subscribe(source, OnNext); |
… truth Arbiter round-3 blockers on B0+B1: 1. Rx suppression was gated on `emitEvents` -- so under `--no-event-leaks` a declared IDisposable-returning wrapper named `Subscribe` (e.g. `WeakBus.Subscribe`) fell back to the Rx dropped-token matcher and the OFF switch INVENTED a finding. Suppression is now UNCONDITIONAL (`!MatchesDeclaredWeakSubscribe(...)`); only the weak-fact emission stays `emitEvents`-gated. A declaration classifies the API regardless of event-fact emission. 2. Regression: WeakSubscribeRxNoEventsSample.cs (WeakBus.Subscribe returning IDisposable) scanned with `--weak-subscribe WeakBus.Subscribe --no-event-leaks` must yield ZERO subscriptions (no weak fact: pass off; no Rx fact: declared -> suppressed). A contrast run WITHOUT the declaration proves the Rx dropped-token finding still fires, so it is the declaration -- not the flag -- doing the suppression. 3. Docs: P-035 no longer claims a `+=`-only detector (B1 mints the wrapper call as a released:true subscription), and its fix-text section now quotes OWN001's real text (which names no WeakEventManager) with the manager named only as the deferred Increment-C alternative. Scope unchanged: B0+B1, no Increment A/C, no general AddHandler inference. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JxKjqdGEFzq4UzZupw379G
| { | ||
| public RxCollisionSubscriber() | ||
| { | ||
| WeakBus.Subscribe(this, _ => { }); |
The round-3 Rx-collision regression (WeakBus.Subscribe(this, _ => {}) under
--no-event-leaks) failed in CI: MatchesDeclaredWeakSubscribe returned false, so
the Rx dropped-token matcher still flagged it. Cause: the handler-shape guard
gated arg[1] on IsHandler alone, which accepts only IdentifierName /
MemberAccess -- a lambda is neither. Its own comment already promised "method
group / delegate / new H(...) / lambda", and the `+=` detector's main path does
count a lambda RHS (IsHandler gates only its unresolved fallback), so the guard
was inconsistent and too strict.
Fix: accept `IsHandler(...) || IsLambdaHandler(...)`. The int-literal negative
control (AddPropertyChanged(source, 42)) is still rejected (neither shape), so
NonHandlerSecondArgument stays empty. Now the declared lambda-handler wrapper is
recognised -> minted released:true when events are on, and unconditionally
suppressed from the Rx matcher when they are off.
Verified locally (Windows extractor + WSL python), 7/7 incl. the regression and
its no-declaration contrast (which still fires the Rx fact).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JxKjqdGEFzq4UzZupw379G
2 participants
Implements P-035 Increment B behind a minimal P-015 config carrier (B0), per the arbiter decision. One PR = B0+B1; no Increment A / C / general
AddHandlerinference.B0 — minimal P-015 carrier
own-check --config <own.toml>→ the only public surface. Reads only[weak-subscription].subscribe(a list of"SimpleType.Method"). No auto-discovery / severity / per-path / env.ownlang/config.py(tomllib): a malformed declaration is a hard error, never a silent skip.python -m ownlang configprints the names;scripts/own-check.shforwards them to the extractor via the internal--weak-subscribetransport flag.B1 — extractor detection + released recognition
(containing-type simple name, method name)is declared, with the MVP(source, handler)positional contract, becomes one subscription fact withreleased: true. Match resolves via the method symbol, or a conservative syntactic receiver-name fallback for an unresolved external package.Subscribeis suppressed in the Rx.Subscribe()matcher (no double-count).Acceptance contract (#1–#10)
tests/test_weak_subscribe.py— the config carrier incl. every malformed-config hard-error case (auto-discovered byrun_tests.py, SDK-free). 15/15 locally.frontend/roslyn/samples/WeakSubscribeAllowlistSample.cs+ a new step in the C# leak extractor CI job: runs the extractor with/without--weak-subscribe, asserts the facts viatests/check_weak_subscribe_facts.py(positive released:true;+=still leaks; same-name-different-type / too-few-args / no-flag all produce no fact), plusown-check --configplumbing and the malformed-config hard error end-to-end.Verified locally via WSL:
run_tests.pygreen (incl. weak-subscribe config 15/15, no parity drift), extractor facts match the contract for the resolved and unresolved-external paths.Out of scope (arbiter)
Increment A (unsubscribe wrapper — needs event identity first), C (project-aware fix text / 007 wiring), general
AddHandlerheuristic, rest of P-015.🤖 Generated with Claude Code
Summary by CodeRabbit
own.tomlwith strict TOML validation.--weak-subscribeto recognize declared wrapper APIs as already released.ownlang config(and--configsupport inown-check/ the marketplace action) to surface configured weak-subscribe entries.+=.