Declare renderer ownership so the host enforces the scoping the package only applied at runtime - #51
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Summary by CodeRabbit
WalkthroughThe PR adds renderer ownership metadata for three context commands. It introduces ChangesRenderer ownership
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Reviewer's GuideThis PR scopes pm-context’s toon/json renderer overrides to the extension’s own commands by declaring renderer ownership with a shared result discriminator, updates the renderer implementation to use that discriminator, adds tests validating ownership and behavior, and records the change in pm agent task/history files. Sequence diagram for scoped renderer ownership and shared discriminatorsequenceDiagram
actor User
participant ExtensionHost
participant pm_context_extension
User->>ExtensionHost: run context-pack --json
ExtensionHost->>pm_context_extension: activate(api)
pm_context_extension->>ExtensionHost: api.registerRenderer("json", renderCommandResult, rendererOwnership)
pm_context_extension->>ExtensionHost: api.registerRenderer("toon", renderCommandResult, rendererOwnership)
User->>ExtensionHost: run context-pack --json
alt [command in rendererOwnership.commands]
ExtensionHost->>pm_context_extension: renderCommandResult(context)
pm_context_extension->>pm_context_extension: isRenderedCommandResult(context.result)
alt [isRenderedCommandResult(context.result)]
pm_context_extension-->>ExtensionHost: output
else [not isRenderedCommandResult(context.result)]
pm_context_extension-->>ExtensionHost: null
end
else [command not in rendererOwnership.commands]
ExtensionHost->>ExtensionHost: [native format renderer]
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Greptile SummaryThe PR scopes the
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| index.ts | Extracts the marked-result predicate and applies shared command/result ownership to both renderer registrations. |
| test/renderer-ownership.test.ts | Exercises ownership registration, accepted marked results, independent command and result rejection, and parity with registered commands. |
| dist/index.js | Rebuilds the distributed runtime artifact with the source ownership changes. |
| CHANGELOG.md | Adds an Unreleased entry describing renderer ownership enforcement. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
A[Command result] --> B{Owned command?}
B -- No --> N[Native rendering]
B -- Yes --> C{Marked pm-context result?}
C -- No --> N
C -- Yes --> R[pm-context renderer]
R --> D{Callback validation}
D -- Valid --> O[Rendered output]
D -- Invalid --> N
Reviews (6): Last reviewed commit: "Guard the renderer ownership list agains..." | Re-trigger Greptile
|
@coderabbitai full review @sourcery-ai review @greptileai review Reviewers: the risk in this change is not the ownership object — it is the command list. Declaring So the finding I most want is a command that can produce this package's rendered-result marker but is missing from the declared list. The paths were derived by reading this package's own Second, if you can see a way for the extracted result predicate and the callback to disagree about ownership now that they share one definition, that is worth flagging too — keeping them from diverging is half the point of the extraction. |
Rate Limit Exceeded
|
…oping The pm-context renderer overrides self-scoped by checking the private pmContextRendered marker and returning null for foreign results. That is correct but invisible to the host: an unscoped renderer claims the whole format globally, and one refactor away from the marker check could replace every --json command's output in the CLI. Passing a shared ownership object to both the toon and json registerRenderer calls makes the host enforce the command paths (context-pack, context-handoff, context-usage) and the result discriminator before the callback ever runs. The runtime null return stays as defence in depth — ownership is enforced by the host, the null return by the package, and neither depends on the other. The resultDiscriminator reuses the same isRenderedCommandResult predicate the callback applies internally, extracted to one definition rather than duplicated. A new test asserts the renderer still renders its own marked result, declines a foreign one, and that the registered commands and discriminator match the source.
The decline test passed a context foreign in both dimensions at once — an
unowned command and a result that fails the discriminator — so a single
assertion over both could not tell which filter did the work. Dropping the
commands declaration entirely left the suite green, because the
resultDiscriminator still rejected the foreign result and the unowned
command was never exercised independently.
Replace it with three cases per format, each isolating one axis:
1. Result rejection with ownership matching — the command is one pm-context
owns (context-pack), the result is foreign (another package's marker) or
bare ({ output: "x" }). The commands filter passes; resultDiscriminator
must decline. Fails if the discriminator regresses.
2. Command rejection with the result matching — the command is one pm-context
does not own, the result carries the pmContextRendered marker. The host's
commands filter must decline before the renderer runs. This is the case
that protects the ownership boundary: it fails if the commands declaration
is dropped, because then resultDiscriminator alone would let the renderer
claim a marked result emitted under a foreign command path.
3. Both foreign — the original belt-and-braces check, kept as a third case.
Each case asserts overridden=false, rendered=null, and no warnings for both
toon and json. The harness (createExtensionTestHarness / runRendererOverride)
honours the command field on RendererOverrideContext, so the command path is
driven through pm's real renderer dispatch — no hand-rolled api double.
Closes the implementation tracker pm-context-lhse with full closure metadata
(resolution, expected_result, actual_result) and regenerates the changelog so
the entry lands in the Unreleased section.
CI failed `changelog:check` on all four renderer-ownership branches while the same check passed locally. The cause was not the changelog and not stale tags: the daily release bumped every package to 2026.8.4 and this branch was still based on the commit before it, so the generator ran against 2026.8.3 locally and 2026.8.4 in CI. Under 2026.8.3 the freshly closed item grouped into the `## 2026.8.4` heading, because that version had not been released yet from the branch's point of view. Under 2026.8.4 it belongs in `## Unreleased`, above the released section. Both outputs are correct for the version they were generated against, which is exactly why the local check could not see it. Rebased onto origin/main and regenerated with the package's own `changelog:check` command minus `--check`, so the generation mode cannot drift from the mode the gate compares against.
5d19eb9 to
4f2ea04
Compare
Round addressed — split tests, item closed, and a mid-PR release rebasedThe test split (CodeRabbit + Greptile, raised independently)The decline test passed a context foreign in both dimensions at once, so a green result proved only that at least one ownership filter fired. Three cases now isolate each axis:
Verified by mutation, not by inspection. Deleting the …and the rest stay green. The declaration is now pinned by a test that notices its absence, which the combined assertion never was. This is meaningful because the harness enforces the real thing: Tracker closed in the implementing PRThe item is closed with closure metadata and the changelog regenerated after the close. Ordering matters: Why CI went red, and what it was actually telling us
Fixed by rebasing onto Verification
@coderabbitai full review @sourcery-ai review @greptileai review Re-review focus: the command list, still. The tests now prove the declared commands are enforced, but they cannot prove the list is complete — a command that emits this package's marked result and is missing from |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- Changed files and PR diff summary ---'
git diff --stat origin/main...HEAD
git diff --unified=40 origin/main...HEAD -- index.ts test/renderer-ownership.test.ts
printf '%s\n' '--- AST outline: index.ts ---'
ast-grep outline index.ts --items all --view expanded
printf '%s\n' '--- AST outline: renderer ownership test ---'
ast-grep outline test/renderer-ownership.test.ts --items all --view expanded
printf '%s\n' '--- Command registrations and rendered-result references ---'
rg -n -C 5 -P '\bregisterCommand\s*\(|\bisRenderedCommandResult\b|rendered\s*:\s*true|renderedOutput|renderCommandResult\s*\(' \
-g '*.ts' -g '*.tsx' -g '*.mts' -g '*.cts' .
printf '%s\n' '--- Command-name literals near registration and result construction ---'
rg -n -C 8 -P "['\"][a-z][a-z0-9-]*['\"]" -g '*.ts' -g '*.tsx' -g '*.mts' -g '*.cts' .Length of output: 205782
The command list is complete in the current source:
I found no other The predicate and callback also cannot disagree in the current implementation. The split tests correctly isolate the host command filter from the result discriminator. ✏️ Learnings added
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 18 minutes. |
CodeRabbit raised this on pm-ops as a future-regression gap and it applies to every package that declares renderer ownership. The ownership list is right the day it is written and wrong the day a command is added. The new command registers, returns a rendered result, and silently falls back to native rendering because the host declines it. Nothing fails: not the build, not the types, and not an ownership test whose expected list carries the same omission -- which is exactly the shape of the test this branch already added. So the list is now checked against what the extension actually registers. `renderer ownership covers every command the extension registers` compares the declared `commands` against `activation.registrations.commands`, which is the only assertion that notices a command the ownership list forgot. The test keeps its own independent literal rather than importing the source's, because a test that restates the source's list shares its mistakes and proves nothing. On pm-ops the declaration itself now derives from `OPS_COMMAND_PATHS`, the list every ops command already registers its parser through, making the duplication impossible rather than merely guarded.
Drift guard added — the finding you raised on pm-ops applies here too@coderabbitai — you cleared the command list as complete today and then flagged the thing that actually matters: the list is right the day it is written and wrong the day a command is added. A new command registers, returns a marked result, and silently falls back to native rendering because the host declines it. Nothing fails — not the build, not the types, and not the ownership test, whose expected list carries the same omission. That last part is the sharp end, because the test this branch added earlier in the round has exactly that shape. You raised it on test("renderer ownership covers every command the extension registers", async () => {
const registered = ext.activation.registrations.commands.map((e) => e.command).sort(...);
assert.deepEqual(registered, [...OWNED_COMMANDS].sort(...));
});Comparing against One deliberate deviation from your suggestion. You proposed the test use the shared list directly. I kept the test's literal independent instead: a test that restates the source's list shares its mistakes and proves nothing — the exact failure mode you identified, relocated. The source-side deduplication you asked for is done where it belongs, in the declaration: on pm-ops Not applied to pm-starter, and worth saying why rather than leaving it looking like an oversight: only
@sourcery-ai @greptileai — re-review welcome on the guard itself. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- PR diff for relevant source and tests ---'
git diff --unified=50 origin/main...HEAD -- index.ts test/renderer-ownership.test.ts
printf '%s\n' '--- Structure ---'
ast-grep outline index.ts --items all --view expanded
ast-grep outline test/renderer-ownership.test.ts --items all --view expanded
printf '%s\n' '--- Command registration, ownership declaration, and marked-result production ---'
rg -n -C 6 -P '\bregisterCommand\s*\(|\bcommands\s*:|\bOWNED_COMMANDS\b|\brenderedCommandResult\s*\(|\bpmContextRendered\b|\bisRenderedCommandResult\b' \
-g '*.ts' -g '*.tsx' -g '*.mts' -g '*.cts' .
printf '%s\n' '--- Test harness ownership path ---'
rg -n -C 6 -P '\brunRegisteredRendererOverrideForTest\s*\(|\brunRendererOverride\s*\(|\brendererOwnsContext\s*\(' \
-g '*.ts' -g '*.tsx' -g '*.mts' -g '*.cts' .Length of output: 41135
The guard has the required independent comparison:
Equality is valid for The renderer callback and I did not execute the test suite in the inspection sandbox. The source review is consistent with the reported test result. 🐇 ✓ ✏️ Learnings added
You are interacting with an AI system. |
Merging — round closedAll threads answered, voted and resolved; every check green; What the round produced@coderabbitai and @greptileai independently identified the same weak assertion from opposite directions — the result discriminator after commands match, and command rejection with a matching result. Two reviewers converging on one weak test was the strongest signal of the round, and the fix was verified by mutation rather than inspection: deleting the @coderabbitai then cleared the command list as complete and flagged the thing that outlives this PR — a literal list is right today and stale after the next command is added, and an ownership test that restates the same omission stays green. That is now guarded against What this change does and does not buyIt makes scoping host-enforced rather than merely conventional. The runtime It does not clear the Functionally verified end to end with the globally installed pm CLI, not only the harness: the owned command renders its pack, and |
|
Tip For best results, initiate chat on the files or code changes.
The end-to-end check confirms the required boundary: owned commands use the extension renderer, and The remaining 🐇 ✓ ✏️ Learnings added
You are interacting with an AI system. |
|
Clean round. The mutation-verified fix for the weak discriminator assertion, and the drift guard against |
1 participant
What changed
pm-contextregistered itstoonandjsonrenderer overrides without the ownership argument:registerRenderer(format, renderer, ownership?)takes a third argument the host enforces before invoking the callback —commands(an unrelated command never reaches it) andresultDiscriminator(a false predicate preserves native rendering). This PR passes it:The predicate is extracted once and used by both the ownership declaration and the callback, so the two can no longer disagree about what this package owns.
Why it matters — the failure this prevents
A renderer registered without ownership claims the whole format, globally. I reproduced the consequence on pm CLI 2026.8.3 with a probe extension: an unscoped
jsonrenderer replaced the entire output ofpm health --jsonwith the extension's own literal. Every--jsoncommand in the CLI was destroyed, silently.pm-contextwas not exhibiting that — the callback already returnednullfor any result lacking itsisRenderedCommandResultmarker, which is correct and deliberate. But that scoping lived only in the callback body: invisible to the host, unenforceable, and one refactor away from the failure above. Declaring it moves the guarantee from "this package is careful" to "the host will not call us".The runtime
nullreturn stays, as defence in depth. Ownership is enforced by the host; thenullreturn is enforced by the package; neither should depend on the other.pm-changelogalready did this and is the pattern followed here.The risk this change carries, and how it is covered
The failure mode of declaring
commandsis a renderer that silently stops rendering — output reverts to native and nothing errors. The command paths were therefore derived by reading this package's ownregisterCommandcalls rather than assumed, and 4 tests assert both directions: the renderer still renders its own marked result, it declines a foreign one, and the registered ownership matches what the source declares.Tests use the SDK harness (
createExtensionTestHarness) — no hand-rolledapidouble, noactivate(api as any).Verification
npm run typecheck— clean.npm run coverage— thresholds met.npm run changelog:check— up to date.pm item
pm-context-lhse— in_progress, with a note recording the derived command paths and where they were read from. Committed with its siblinghistory/pm-context-lhse.jsonl.What this does not do
It does not clear the
extension_renderer_collisionwarnings inpm health. The host's collision check groups renderer overrides by format alone and ignores the ownership it enforces, so a correctly scoped ecosystem still reportsok: false— filed upstream as unbraind/pm-cli#897 with the reproduction. No registration was deleted to silence a warning; the registrations are correct. This change is what makes the fleet already-correct on the day that upstream bug is fixed.Summary by cubic
Declare renderer ownership for
toonandjsonso the host enforces scoping topm-contextcommands and the result marker before the renderer runs. Prevents global format overrides while keeping thenullreturn as a fallback.registerRendererwith commandscontext-pack,context-handoff,context-usageand aresultDiscriminatorusingisRenderedCommandResult.isRenderedCommandResultand reused it in both the ownership registration and the renderer callback.Written for commit 7b9ecc3. Summary will update on new commits.