Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions .github/workflows/action-marketplace-readiness.yml
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,45 @@ jobs:
format: github
fail-on-finding: "true"

# P-035: the `config:` input must reach the extractor through the Action border.
# The weak-wrapper fixture is a real finding WITHOUT a declaration and silent WITH
# one, so the pair of runs proves the input plumbs (an Action-level test, not a
# direct scripts/own-check.sh call).
- name: Owen check — weak wrapper, NO config (finds the dropped token)
id: weaknocfg
uses: ./
continue-on-error: true # expected to fail — asserted below
with:
path: fixtures/marketplace-consumer-demo/WeakSubscribeConsumer.cs
format: github
fail-on-finding: "true"
- name: Assert the un-declared weak wrapper WAS flagged
run: |
[ "${{ steps.weaknocfg.outcome }}" = "failure" ] \
|| { echo "FAIL: weak wrapper without --config should be a finding, got '${{ steps.weaknocfg.outcome }}'"; exit 1; }
echo "OK: without a [weak-subscription] declaration the wrapper is a finding"
- name: Owen check — weak wrapper, WITH config (recognised, silent)
uses: ./
with:
path: fixtures/marketplace-consumer-demo/WeakSubscribeConsumer.cs
config: fixtures/marketplace-consumer-demo/own.toml
format: github
fail-on-finding: "true"
- name: Owen check — a malformed config is a hard error through the Action
id: badcfg
uses: ./
continue-on-error: true # expected to fail — asserted below
with:
path: fixtures/marketplace-consumer-demo/Clean.cs
config: fixtures/marketplace-consumer-demo/own-bad.toml
format: github
fail-on-finding: "false"
- name: Assert the malformed config failed the Action step
run: |
[ "${{ steps.badcfg.outcome }}" = "failure" ] \
|| { echo "FAIL: a malformed --config must hard-error through the Action, got '${{ steps.badcfg.outcome }}'"; exit 1; }
echo "OK: own.toml plumbs through the Action; a malformed config is a hard error"

# Fork PRs get a read-only GITHUB_TOKEN (GitHub's fork-PR token policy),
# so security-events:write is never actually granted no matter what
# this job requests — skip the SARIF/upload steps there instead of
Expand Down
57 changes: 57 additions & 0 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -1550,6 +1550,63 @@ jobs:
python -m ownlang explain --json "$RUNNER_TEMP/proj.sarif" | grep -q "OWN001" \
|| { echo "FAIL: explain --json did not harvest OWN001 from the SARIF log"; exit 1; }
echo "OK: explain answers a code and harvests codes from a real findings/SARIF file"
# P-035 (Increment B0+B1): a project-DECLARED weak-subscribe wrapper is a
# first-class, already-released subscription. own.toml is the surface
# (own-check --config); the extractor's --weak-subscribe is internal transport.
- name: P-035 weak-subscribe — declared wrapper is an accepted release
run: |
sample=frontend/roslyn/samples/WeakSubscribeAllowlistSample.cs
unresolved=frontend/roslyn/samples/WeakSubscribeUnresolvedSample.cs
rxsample=frontend/roslyn/samples/WeakSubscribeRxNoEventsSample.cs
decl="WeakEvents.AddPropertyChanged"
# WITH the declared wrapper; WITHOUT (byte-for-byte baseline); WITH but
# --no-event-leaks (event analysis off); and the unresolved-external sample.
dotnet run --project frontend/roslyn/OwnSharp.Extractor -- \
"$sample" --weak-subscribe "$decl" -o "$RUNNER_TEMP/ws_on.json"
dotnet run --project frontend/roslyn/OwnSharp.Extractor -- \
"$sample" -o "$RUNNER_TEMP/ws_off.json"
dotnet run --project frontend/roslyn/OwnSharp.Extractor -- \
"$sample" --weak-subscribe "$decl" --no-event-leaks -o "$RUNNER_TEMP/ws_noev.json"
dotnet run --project frontend/roslyn/OwnSharp.Extractor -- \
"$unresolved" --weak-subscribe "$decl" -o "$RUNNER_TEMP/ws_unres.json"
# Rx-collision regression: a declared `Subscribe`-named IDisposable wrapper under
# --no-event-leaks must stay fully silent (suppression is unconditional).
dotnet run --project frontend/roslyn/OwnSharp.Extractor -- \
"$rxsample" --weak-subscribe "WeakBus.Subscribe" --no-event-leaks -o "$RUNNER_TEMP/ws_rxnoev.json"
# The Increment-B acceptance contract, at the fact level.
python tests/check_weak_subscribe_facts.py \
"$RUNNER_TEMP/ws_on.json" "$RUNNER_TEMP/ws_off.json" \
"$RUNNER_TEMP/ws_noev.json" "$RUNNER_TEMP/ws_unres.json" \
"$RUNNER_TEMP/ws_rxnoev.json"
# Contrast that proves it is the DECLARATION (not --no-event-leaks) doing the
# suppression: the same sample under --no-event-leaks with NO declaration still
# emits the Rx dropped-token subscription for RxCollisionSubscriber.
dotnet run --project frontend/roslyn/OwnSharp.Extractor -- \
"$rxsample" --no-event-leaks -o "$RUNNER_TEMP/ws_rxbase.json"
python -c "import json,sys; d=json.load(open(sys.argv[1])); c=[x for x in d['components'] if x['name']=='RxCollisionSubscriber']; subs=(c[0].get('subscriptions') or []) if c else []; sys.exit(0 if any(s.get('resource')=='subscribe' for s in subs) else 1)" \
"$RUNNER_TEMP/ws_rxbase.json" \
|| { echo "FAIL: without a declaration, --no-event-leaks should still leave the Rx dropped-token finding (regression baseline broken)"; exit 1; }
# End-to-end through the core: the ordinary += still surfaces (OWN001); the
# declared wrapper does not.
out=$(python -m ownlang ownir "$RUNNER_TEMP/ws_on.json" || true)
echo "$out" | grep -q "OrdinaryPlusEquals" \
|| { echo "FAIL: ordinary += leak not flagged"; exit 1; }
! echo "$out" | grep -q "WeaklySubscribed" \
|| { echo "FAIL: declared weak wrapper was flagged as a leak"; exit 1; }
# Action plumbing: own-check --config own.toml parses [weak-subscription] and
# forwards it to the extractor, so the wrapper is silent end-to-end.
printf '[weak-subscription]\nsubscribe = ["WeakEvents.AddPropertyChanged"]\n' > "$RUNNER_TEMP/own.toml"
cout=$(scripts/own-check.sh --config "$RUNNER_TEMP/own.toml" "$sample" || true)
! echo "$cout" | grep -q "WeaklySubscribed" \
|| { echo "FAIL: own-check --config did not silence the declared wrapper"; exit 1; }
echo "$cout" | grep -q "OrdinaryPlusEquals" \
|| { echo "FAIL: own-check --config lost the ordinary += leak"; exit 1; }
# A malformed config is a hard error (non-zero), never a silent skip.
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
Comment on lines +1605 to +1608

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 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.

Suggested change
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.

echo "OK: declared weak-subscribe wrapper = accepted release; += unaffected; own.toml plumbed; malformed config is a hard error"

# The OwnTS frontend spike (P-020 Own.React): the SAME OwnIR seam, fed from a
# React .tsx instead of C#. Two analyses over the one core: (1) a useEffect
Expand Down
19 changes: 17 additions & 2 deletions action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,14 @@ inputs:
description: "File(s) or directory to scan (directories are walked for *.cs)."
required: false
default: "."
config:
description: >-
Explicit Owen configuration file (own.toml). Today it carries the P-035
[weak-subscription].subscribe allowlist of your project's own weak-subscribe
wrapper API ("SimpleType.Method"); a matching call is treated as an already-
released subscription. Left empty, no config is read.
required: false
default: ""
format:
description: >-
Finding surface: github (PR annotations), msbuild, human, or sarif — a SARIF
Expand Down Expand Up @@ -77,8 +85,15 @@ runs:
OWN_SEVERITY: ${{ inputs.severity }}
OWN_FAIL_ON_FINDING: ${{ inputs.fail-on-finding }}
OWN_SARIF_FILE: ${{ inputs.sarif-file }}
OWN_CONFIG: ${{ inputs.config }}
run: |
check="${{ github.action_path }}/scripts/own-check.sh"
# P-035: forward an explicit own.toml to own-check when the caller set one.
# Passed as data via OWN_CONFIG (never interpolated into the script body).
config_args=()
if [ -n "$OWN_CONFIG" ]; then
config_args+=(--config "$OWN_CONFIG")
fi
if [ "$OWN_FORMAT" = "sarif" ]; then
# The code-scanning surface: write a SARIF 2.1.0 log to a file and expose
# its path as an output, so the caller hands it to
Expand All @@ -91,7 +106,7 @@ runs:
sarif="${OWN_SARIF_FILE:-$RUNNER_TEMP/owen.sarif}"
set +e
"$check" --root "${{ github.action_path }}" --format sarif \
--severity "$OWN_SEVERITY" --fail-on-finding -- "$OWN_PATH" > "$sarif"
--severity "$OWN_SEVERITY" "${config_args[@]}" --fail-on-finding -- "$OWN_PATH" > "$sarif"
rc=$?
set -e
echo "sarif-file=$sarif" >> "$GITHUB_OUTPUT"
Expand All @@ -115,7 +130,7 @@ runs:
fi
exit 0
fi
args=(--root "${{ github.action_path }}" --format "$OWN_FORMAT" --severity "$OWN_SEVERITY")
args=(--root "${{ github.action_path }}" --format "$OWN_FORMAT" --severity "$OWN_SEVERITY" "${config_args[@]}")
if [ "$OWN_FAIL_ON_FINDING" = "true" ]; then
args+=(--fail-on-finding)
fi
Expand Down
16 changes: 11 additions & 5 deletions docs/proposals/P-015-configuration-surface.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
# P-015 — Configuration surface: check selection & per-category severity

- **Status:** draft (**stub** — to be fleshed out; P2, quality-of-life, not blocking)
- **Status:** draft — but a **first minimal slice has shipped** (via P-035, PR #284):
an **explicit `own-check --config own.toml`** (no auto-discovery) that reads **only**
the `[weak-subscription].subscribe` table, in **TOML** (`ownlang/config.py`,
`tomllib`; a malformed config is a hard error). Everything else below —
auto-discovery, per-category enable/severity, per-path overrides, env — remains
**deferred**. The format question is therefore **settled: TOML.**
- **Origin:** [P-014](P-014-semantic-resolution.md) Open Question 2. P-014 ships a
single `--event-leaks` gate as *instance #1* of the need this proposal generalises.
- **Depends on / reconciles with:**
Expand Down Expand Up @@ -96,10 +101,11 @@ is the central implementation question.

## Open questions

1. **File format.** TOML (`own.toml`, typed, ecosystem-standard) vs an
`.editorconfig`-style INI vs JSON. Leaning TOML.
2. **Discovery & precedence.** Nearest-file-up vs explicit `--config PATH`;
exact precedence vs CLI and vs `[OwnIgnore]` (draft order above).
1. **File format — RESOLVED: TOML.** Shipped as `own.toml` (`tomllib`) in the P-035
slice; not reopening INI/JSON.
2. **Discovery & precedence.** The shipped slice is **explicit `--config PATH` only**
(no discovery), so nearest-file-up walking and CLI/`[OwnIgnore]` precedence remain
open for when a broader surface lands.
3. **Enforcement point.** Core-side only (clean "one checker") vs extractor-side
skip for disabled categories (cheaper) — likely both, with the core
authoritative.
Expand Down
85 changes: 56 additions & 29 deletions docs/proposals/P-035-custom-weak-subscription.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
# P-035 — Project-declared weak-subscription conventions

- **Status:** draft.
- **Status:** partially implemented. **B0+B1 shipped** — a project declares its
weak-subscribe wrapper API in `own.toml` (`[weak-subscription].subscribe`), read via
`own-check --config`, and the extractor mints a matching call as a first-class,
already-released subscription. **Increment A** (unsubscribe wrapper — needs event
identity first) and **Increment C** (project-aware fix text / 007 wiring) remain
**deferred**, as does a general `AddHandler` heuristic.
- **Depends on / reconciles with:**
- [P-004](P-004-wpf-lifetime-profile.md) — the WPF lifetime profile. Its Open
Question #4 (P-004:142-143) proposes recognising *"`WeakEventManager` / weak
Expand Down Expand Up @@ -66,19 +71,27 @@ weakly here."*
whose publisher holds subscribers weakly (one entry: `CommandManager.RequerySuggested`).
Deliberately curated and compiled-in — "extend only when another sibling's
weak-reference implementation is independently confirmed."
- **Subscription detection** — only the C# `event += handler` operator mints an
`acquire` (`Program.cs:3491-3502`, P-014 Tier A). A method call such as
`Mgr.AddHandler(src, h)` or `WeakEvents.AddPropertyChanged(src, h)` is **invisible**
to the subscription detector; the only recognised method-call subscription is the
Rx `X.Subscribe(…)` IDisposable-token shape.
- **Subscription detection** — the C# `event += handler` operator mints an `acquire`
(P-014 Tier A). In addition, **B1** now mints one for a **declared** weak-subscribe
wrapper *method call* (`MatchesDeclaredWeakSubscribe`): an exact `(containing-type
simple name, method name)` on the allowlist, with the MVP `(source, handler)`
positional contract and a real handler second argument (`IsHandler`), resolved via
the method symbol or a syntactic receiver-name fallback for an unresolved external
package. A non-declared method call is still invisible; there is **no** general
`AddHandler` heuristic. The Rx `X.Subscribe(…)` IDisposable-token shape is unchanged,
and a declared wrapper named `Subscribe` is suppressed there so it is not
double-counted.
- **Per-site suppression** — `[OwnIgnore("reason")]` is read from source
(`OwnIgnoreReason`, `Program.cs:4388`, issue #209).
- **No project-wide config is consumed yet** — the only external extractor inputs are
assembly-reference dirs (`--ref-dir` / `OWN_EXTRA_REF_DIRS`), not semantic-role
declarations. P-015's config file is still a draft.
- **A project-wide config is now consumed (B0)** — `own-check --config own.toml` reads
the `[weak-subscription].subscribe` allowlist (`ownlang/config.py`, `tomllib`; a
malformed table is a hard error) and forwards it to the extractor via the internal
`--weak-subscribe` transport flag. This is the first, deliberately narrow slice of
P-015; discovery / severity / per-path / env stay deferred there. The composite
Action exposes it as the optional `config:` input.

So the two things this proposal needs are: (1) a place to *declare* the convention
(P-015's config), and (2) two small consumers of it (recognition + fix text).
The two consumers this proposal named — recognition (shipped, B1) and fix text
(deferred, Increment C) — now sit on that config seam.

## Design

Expand Down Expand Up @@ -114,21 +127,25 @@ when it is absent):
— it is config-extensible rather than curated, because a project's own wrapper
cannot be "independently confirmed" in Own.NET's tree.

> Note: for a project that has *already* converted (STS after the fix), the code is
> a method call, so today's `+=`-only detector is silent anyway — no false positive
> exists **yet**. Recognition earns its keep the moment method-call subscriptions are
> detected (so mixed `+=`/wrapper codebases don't get half-flagged), and it makes the
> wrapper a first-class, auditable release instead of an invisible one.
> Note: **before B1** a converted call was silent only by accident — the extractor saw
> only `event += handler`, so a method-call wrapper was *invisible* (no fact at all).
> **B1 (shipped in this PR) removes that**: the extractor now detects a declared wrapper
> call and emits it as a first-class, `released:true` subscription. So the wrapper is
> silent-and-**recognised** (an auditable release), not silent-and-invisible — and a
> mixed `+=`/wrapper codebase is no longer half-analysed.

### 3. Fix-text / autofix consumer (suggest the *project's* weak API)

Own.NET does not ship a code-fix (by policy — the fix is applied by an agent under
the 007 harness's `o7 run`). Two touch-points:

- **The OWN001 explanation** (`ownlang/diagnostics.py:122-130`) currently offers a
fixed *"unsubscribe (`-=`) in Dispose/Unloaded, or WeakEventManager"* text. When a
`[weak-subscription]` convention is configured, the weak alternative it names
should be the **declared** `subscribe` API, not the BCL manager.
fixed *"unsubscribe (`-=`) in Dispose/Unloaded, dispose the owned field in the
owner's Dispose, or capture and dispose the IDisposable a `Subscribe()` returns"*
text — it names **no** weak-events manager at all today. Increment C would make it
name the project's **declared** `subscribe` API as the weak alternative when a
`[weak-subscription]` convention is configured (rather than hard-coding the BCL
`WeakEventManager`, which the STS case proves may not fit).
- **The agent fix task** (007) should be handed the convention so a converting agent
emits `WeakEvents.AddPropertyChanged`, not a `WeakEventManager` that — as the STS
case proves — may not compile or may not work in that layer.
Expand All @@ -151,13 +168,23 @@ regression fixture for the recognition half.
- **Shipping a weak-events helper.** Own.NET recommends a shape; the project owns the
implementation (cf. P-027's stance that Own.NET ships no mandated fix type).

## Open questions

1. Config format & discovery — deferred to P-015 (`.ownrc` vs `own.toml`), this is
one more table in it.
2. Should a declared `unsubscribe` also be recognised as a release for a *`+=`*
subscription (i.e. a project that hides `-=` behind `WeakEvents.RemovePropertyChanged`)?
Probably yes, via the same `(type, method)` match feeding the `unsub` set
(`Program.cs:3401`).
3. Method-call subscription detection is a prerequisite for the recognition half and
is itself a P-014 increment; sequence it there or fold it in here?
## Resolved decisions

1. **Config format & discovery — RESOLVED.** The carrier is an **explicit
`own-check --config own.toml`** (no auto-discovery). Format is **TOML**
(`tomllib`), the minimal P-015 slice — see [P-015](P-015-configuration-surface.md).
2. **Method-call subscription detection — RESOLVED: folded into P-035**, not carved
out as a separate P-014 increment. It has *no* standalone behaviour — it fires only
for an explicitly declared `(type, method)` pair, so it cannot exist without a
P-035 declaration.

## Open questions (deferred)

- **Increment A** — should a declared `unsubscribe` also be recognised as a release
for a *`+=`* subscription (a project that hides `-=` behind
`WeakEvents.RemovePropertyChanged`)? **Deferred**: `(source, handler)` alone cannot
pin the release to a specific `event +=` on the same source, so a sound design needs
event identity first (arbiter). Not implemented.
- **Increment C** — the project-aware fix text (the OWN001 explanation naming the
declared API instead of `WeakEventManager`) and the 007 fix-agent wiring. **Deferred**
to a separate scope with write-capable remediation plumbing.
Loading
Loading