Repository navigation
◔ 9/21 [Epic] Plugin system: manifest, handlers, commands and official plugins - #3563
Draft
propr-dev[bot] wants to merge 29 commits into
Draft
propr-dev[bot] wants to merge 29 commits into
propr-dev[bot] wants to merge 29 commits into
Conversation
…, a new `@propr/plugins` registry workspace with `example-pack`, `npm run plugins:check` wired into CI, tests and a docs page. As the issue asked, there are no tables, routes or UI. All 48 tests in the new files and the existing extension library tests pass. The CI workflow and classifier tests also pass, and so do `release:verify` and the typechecks for both packages. Schema (`packages/shared/src/plugins.ts`, exported from the shared index) - It covers every manifest field in the issue. The permission list is closed: `events.read`, `tasks.read`/`create`, `issues.read`/`create`/`comment`, `pull_requests.read`/`comment`, `repositories.read` and `notifications.send`. Commands may only use permissions the plugin requests, and event subscriptions require `events.read`. - Remote and package servers reuse the extension library's schemas rather than redefining them. On top of those, the manifest must record the `integrity` value itself, and each `secretEnv` entry maps a variable to a connection. Version ranges, tags, paths, images, commands and unknown fields are refused. - Cross-reference checks: a server or webhook must point at a declared connection of the right type, names must be unique in each list, and setting defaults must fit their own limits. - `publicPluginManifest()` is the public view. It leaves out the integrity value, launch arguments and environment values. Registry (`packages/plugins`) - `registry.json` lists plugins in order and marks development-only ones. `officialPlugins()` and `getOfficialPlugin()` return each manifest with its asset paths resolved. `example-pack` only appears when `PROPR_PLUGINS_DEVELOPMENT=1` is set. - `example-pack` has one remote OAuth server, one package server, one skill and one automation template. The package server points at the existing `propr-fixture-mcp@1.0.0` test package. I computed its integrity value with the existing npm registry test helper, and two runs gave the same value. - The package is added to the root `test:prepare` build, and `package-lock.json` gains only the new workspace entries. `plugins:check` (`scripts/check-plugins.mjs`, runs through `tsx` so no build is needed) - It runs the schema, then checks that `registry.json` and the plugin folders match one-to-one, that ids match folder names, and that command names are unique across plugins. It also checks that each plugin has a README and its icon, that each skill folder has a `SKILL.md` with a matching name, and that each automation template passes the existing template checks. - It reports every problem in one run and exits 1 if there are any. - `--online [--registry <url>]` compares recorded integrity values with the npm registry. Run against public npm it reports `example-pack`'s package as not found, because the test package isn't published there. - CI: there is no standalone CI step for `automation-templates:check` (it only runs through the test suite). So I added a "Plugins: Manifest Check" section to the Run Checks step in `pr-build-check.yml` instead. It runs on every pull request. Tests - `packages/shared/test/plugins.test.ts` covers every invalid case in the acceptance criteria, plus settings rules and the public view. - `test/checkPlugins.test.mjs` runs 11 broken fixture manifests (in `test/fixtures/plugins-check/`), each expecting a specific message. It also tests missing files, registry mismatches, duplicate commands across plugins, the command-line exit codes, and `--online` against a fake registry. - `packages/plugins/test/registry.test.ts` covers the development flag and asset paths. Docs: `docs/docs/features/plugins.md` explains the concept, the folder layout, the manifest and the check, with a link from the agent extensions page. It isn't in the sidebar, which matches the existing agent extensions page. Two choices the issue left open: - Development flag: the development marking lives in `registry.json`, not in the manifest. - Folder references: skills and automation templates are referenced by folder path inside the plugin. Templates use the existing `template.json` + `prompt.md` layout. No previews were made, since nothing here is visible in the UI.
Contributor
Author
Epic progress: Plugin system: manifest, handlers, commands and official plugins9 out of 21 PRs merged ⏳
Merged child PRs: #3562, #3564, #3565, #3567, #3569, #3571, #3572, #3573, #3578 In progress: #3579 (issue #3550, Plugin inbound webhooks: verified endpoints that call a handler) - review 6/10, Ultrafix cycle 3 of 10 Up next: #3551 (Plugin slash commands: prompt commands on issues and pull requests); #3552 (Plugin slash commands: write authorization, handler commands and shared command ownership); #3553 (Official plugin: Outbound webhooks); #3554 (Official plugins: Slack (incoming webhook), Discord and Microsoft Teams notifications); #3555 (Official plugin: Slack app mode (threads per pull request, mention to start a task)); #3556 (Official plugin: Linear (start work from a ticket, keep the ticket in sync)); #3557 (Official plugin: Jira Cloud (start work from a ticket, keep the ticket in sync)); #3558 (Official plugin: Sentry (turn an error into an issue, give the agent the error context)); #3559 (Official plugins: Cloudflare, Vercel and Netlify (deploy from a pull request)); #3560 (Official plugins: Notion and Figma (read-only context for the agent)); #3561 (Plugin authoring guide, scaffold command and compatibility rules for official plugins) This comment is updated automatically by ProPR as child PRs open, change and merge. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
Author
|
CI failed: CodeQL Please investigate and fix this CI failure.
Failure evidence
|
Contributor
Author
|
✅ Applied the requested follow-up changes in commit 7691cf4 Summary of ChangesI fixed the CodeQL warning in For real npm package names the result is the same, since a scoped name like All 20 tests in 🤖 Implementation Details
Undo Changes • View Task Execution 💡 ProPR Slash Commands
🤖 Generated by ProPR |
![propr-dev[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
…6`. When the script builds the npm registry URL for a package server, it now swaps every `/` in the package name for `%2f` (`replaceAll`), not just the first one. I fixed the CodeQL warning in `scripts/check-plugins.mjs:136`. When the script builds the npm registry URL for a package server, it now swaps every `/` in the package name for `%2f` (`replaceAll`), not just the first one. For real npm package names the result is the same, since a scoped name like `@scope/pkg` has only one `/`. The change makes the encoding correct for any input and stops CodeQL flagging it. All 20 tests in `test/checkPlugins.test.mjs` pass, and `npm run plugins:check` passes. This isn't a visible change, so there are no previews. PR: #3563 Comment by: @github-advanced-security[bot] (ID: 4571542316) Model: Claude Opus 5.5
Contributor
Author
|
ℹ️ Analyzed the follow-up request by @propr-dev[bot] Analysis SummaryI made no further changes because the fix is already on this branch. CodeQL flagged commit const url = `${registry.replace(/\/+$/, '')}/${server.package.replaceAll('/', '%2f')}/${encodeURIComponent(server.version)}`;The other All 20 tests in The CodeQL alert should close once CodeQL runs on There are no previews because nothing visible changed. No code changes were necessary based on the current state of the branch. 🤖 Analysis Details
💡 ProPR Slash Commands
🤖 Generated by ProPR |
… manage its state through `/api/instance/plugins/v1`, and MCP has read-only `list_plugins` and `get_plugin` tools. As the issue asked, nothing is applied to runs yet.
Tests: the new Core test (10 cases) and the API route test pass, run directly and through the suite runner. So do the affected existing suites: OpenAPI coverage and conformance, MCP observable surface, docs, input budget, operator surface and integration, route authorization, database export, extension OAuth routes and the plugin manifest tests. `gen:openapi:check`, the typechecks for shared, core, api, client and plugins, and zero-warning lint on core and api also pass. I didn't run the full repository suite.
What it does
- State: a new migration adds `plugin_installations`, `plugin_secrets` and an append-only `plugin_audit`. The store is `packages/core/src/plugins/pluginStore.ts`.
- A plugin installs disabled, with its default settings and the manifest's permissions granted.
- Disabling keeps settings and connections. Uninstalling deletes them, but the audit trail stays.
- Settings: checked against the manifest schema on save (unknown keys, types, limits, repository maps). Required settings can't be removed while the plugin is enabled.
- Connections: `token` and `signing_secret` values are sealed with the existing private-secret mechanism, tied to this instance, plugin and connection. Reads only say connected or not connected, who set it and when. Native exports erase them. `oauth` connections report `not_connected`.
- Enable refused: with a 409 and the reasons (also returned as `enableBlockers`) while a required setting, a required token connection or a package-server secret acknowledgement is missing.
- Upgrades: a newer manifest that adds a permission flags the plugin Needs approval, and it keeps its old grant until approved. Changing a package server's package, version or secret variable names also asks for the acknowledgement again. Install requires that acknowledgement up front.
- Access and audit: every route, reads included, needs `instance.manage_settings` and a current durable administrator. That is checked before the request body is read, and again inside each write. Every change is recorded in the audit trail.
- API wiring: routes are registered from a new `packages/api/pluginRouteRegistry.ts`, so `routeRegistry.ts` doesn't grow. OpenAPI schemas and route docs are added, and the spec and client types are regenerated.
- MCP: `list_plugins` returns compact rows and `get_plugin` the full read. Both are documented in `docs/mcp.md` and `docs/mcp-coverage.md`.
- Docs: `docs/docs/features/plugins.md` has a new "Installing a plugin" section covering the install flow, connections, permissions and upgrades.
Choices the issue left open
- Required token connection: a `token` connection counts as required when a contributed MCP server uses it (a bearer server's connection or a package server's secret variable). The manifest has no "required" flag for connections, so I needed a rule.
- Mutations: they need JSON plus an `X-ProPR-Plugins: 1` header, like the extension library. Clearing a connection is a `PUT` with `{ "value": null }`. Uninstalling is a `POST` that must repeat the plugin id as confirmation.
- Errors: routes return `{ error }` like the extension routes, not the newer error envelope. The shared permission guard already answers in that shape.
- Installed version: after an upgrade that needs no approval, the installed version moves to the new manifest on the next change.
Decision for you: the production Docker images don't include the `packages/plugins` folder, and I didn't add `@propr/plugins` as a declared dependency, to avoid lockfile churn. Core loads the registry at runtime instead, and where it's missing the list is empty with one warning. To ship real plugins, add `packages/plugins` to the images and declare the dependency; that can be this PR or the next issue.
No previews, since nothing here shows in the UI.
…d skill to the extension library as managed items, and its template to the template catalog. Its OAuth connection signs in through the extension OAuth module. Runs pick these items up through the normal library resolution.
Tests: I ran the new Core test (5 cases), the updated plugin store and plugin API route tests, and the extension, OAuth, automation, MCP, OpenAPI and settings-UI suites that touch this code; all pass. `gen:openapi:check`, `plugins:check`, the typechecks and zero-warning lint for core and api also pass. I didn't run the full repository suite. Most suites need `PROPR_DEMO_MODE=true` locally. `extensionServerMount`, `mcpObservableSurface` and `mcpOperatorSurface` fail under that setting and pass with a placeholder GitHub relay configured.
How it works
- Managed items: library items carry a `managedBy` marker (plugin id, manifest name, `active`). An item applies to runs only while its plugin is enabled and nothing awaits an acknowledgement. They default to automatic apply on, limited to the plugin's repository limit.
- Edit guard: a library save may change only enabled, the repository and agent limits, and automatic apply on a managed item. Changing anything else, removing it, adding a forged one or replacing its credential is refused. For a package server that covers package, version, integrity value, arguments and variables.
- Sync: every plugin change brings the library in line inside the same database transaction (new code in `packages/core/src/plugins/pluginContributions.ts`). Credentials come from the plugin's connections; nothing is written when nothing changed.
- Disabling deactivates the items and keeps their settings and connections, so re-enabling restores them.
- Uninstalling removes the items. It is refused while an automation adds one of them explicitly, and the message names the automation.
- A name clash with an existing library item gets the `<plugin id>-<name>` prefix.
- Package servers: they use the existing install path. The version and integrity value come from the manifest, the package is installed once without lifecycle scripts, verified at launch, mounted read-only and run through the stdio bridge. With its secret missing the server is skipped with `secret_not_configured` and the run continues. Without a current acknowledgement it is never installed or launched.
- OAuth: three new routes start, complete and disconnect an OAuth connection: `POST /api/instance/plugins/v1/<id>/connections/<name>/oauth/{start,complete,disconnect}`. The extension callback also completes the sign-in. A plugin read now reports `connected`, `not_connected` or `needs_sign_in`.
- Templates: `GET /api/automation-definitions/templates` and the `list_automation_templates` MCP tool include enabled plugins' templates, each with a `plugin` field naming it.
- Read model: a plugin read now includes `contributions`, which links each server and skill to its library item. The OpenAPI spec and client types are regenerated.
- UI: managed rows in Settings show a "Plugin: <id>" badge and an "Inactive" badge when they don't apply, and have no Remove button.
- Docs: `plugins.md` has a new "What an enabled plugin adds" section, and `agent-extensions.md` and `docs/mcp.md` are updated.
Choices the issue left open
- Created at install: managed items exist from install, inactive, so the OAuth connection can be signed in before enabling. They still apply only while the plugin is enabled.
- Repository limit: changing the plugin's limit resets its managed items' limit to it, overriding per-item edits.
- Shared OAuth connection: when several servers use one connection, it shows connected only when all of them are signed in.
- Changed skill files: after an upgrade, a changed skill becomes a new bundle. An automation that added the old bundle then reports it as not found.
- Template ids: a template keeps its id unless a shipped template or another plugin uses it; then it gets the `<plugin id>-` prefix.
Decision for you: the new-automation form in the web UI still uses the template list built into the app, so plugin templates show up only through the API and MCP. Making them appear in the form means having the UI fetch the catalog; I left that out to keep this change focused.
Two preview images of the Settings extension list with managed items are attached; they use mocked API data.
GitHub moved ubuntu-latest to Ubuntu 26.04. rpmbuild there fails the desktop RPM build (%install cannot find the staged usr/ directory), so rpm-maker.test.ts and rpm-signing.test.mjs fail in every full test suite run. Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
propr-dev
Bot
changed the title
CI: pin hosted Linux jobs to Ubuntu 24.04 (plugins epic branch)
propr-dev
Bot
changed the title
… plugin goes through install, connect, limit to a repository, enable, disable and uninstall entirely from the page, and the state badge changes after each step. Tests: typecheck and zero-warning lint pass for `propr-ui`. The full UI suite passes (349 files, 3213 tests), and so does the new Playwright spec `e2e/plugins.pw.ts` (2 tests). The browser runs use a mocked plugin API that keeps state between steps, not a real server. No backend code changed. What's on the page (`/plugins` and `/plugins/<id>`) - Access: only administrators with `instance.manage_settings` see it. It's in the sidebar, under More on mobile, and there's an "Open Plugins" link above Skills & MCP servers in Settings. Anyone else who opens the address gets "Administrator access required". - Catalog: one card per plugin with icon, name, description, category and state badge, a category filter and search by name. The badges are Not installed, Off, Needs connection, Enabled, Needs approval and Degraded. - Detail: four steps in fixed order, each marked Done or To do: 1. Install lists what the plugin adds in plain words from the manifest and each permission with its explanation. When a package server gets secret variables, it shows the package, exact version and variable names, and Install stays disabled until that acknowledgement is ticked. After an upgrade that needs approval, this step shows what's new and an Approve button. 2. Connect has one row per connection with the manifest's help text and link. OAuth reuses the existing Connect / Reconnect / Disconnect component from Skills & MCP servers. Tokens and signing secrets have a write-only field; once saved, the page only says who stored it and when. 3. Configure only offers Limit to repositories for now, as the issue asked. 4. Enable is one checkbox. While something is missing it can't be ticked, and the step lists the API's reasons, for example the missing required connection. - What it added lists the managed servers and skill, linking to Skills & MCP servers, and the template. - Uninstall is a separate action. Its confirmation lists what will be removed, and if the API refuses, the page shows the API's message word for word. - Safety: manifest text is shown as plain text, only `https` links are clickable, and nothing supplied by the plugin runs in the browser. Code: the API client is `propr-ui/src/api/pluginsApi.ts`, typed from the generated OpenAPI types. The page is in `propr-ui/src/pages/PluginsPage/`, split into small components that stay under the line limit. Component tests cover the catalog, each step and the error states. `docs/docs/features/plugins.md` now has a Plugins page section with a walkthrough, and `docs/docs/features/web-ui.md` has a Plugins section. Changes outside the brief: - I put the `/mcp-logs` route on one line in `App.tsx`, written the same way as the new Plugins route, so the file stays under the 400-line limit. - The Enabled checkbox shows the new state while the request is in progress. Without that, a click looked like it did nothing until the API answered. Decision for you: - Plugin icons: the API doesn't serve a plugin's icon file, so each card shows an icon chosen by category instead. Showing the real icon needs an icon route on the API. Previews of the catalog, the install acknowledgement, the blocked Enable step, uninstall with the API refusal, and two phone-width views are attached; they use mocked API data.
propr-dev
Bot
changed the title
…generated from the manifest, upgrades show what changed with an Approve action, and managed items are badged and read-only in Skills & MCP servers. Typecheck and zero-warning lint pass, the full UI suite passes (352 files, 3236 tests), and `e2e/plugins.pw.ts` passes (3 tests). The browser tests run against a mocked plugin API that keeps state between steps, not a real server. No backend code changed.
Configure step
- `PluginSettingsForm.tsx` and `PluginSettingFields.tsx` build one field per setting:
- text → text box
- choice list → select with Not set
- number → number box, with its range shown
- true/false → checkbox
- list → text area, one entry per line
- repository map → rows of repository and value, with this instance's repositories suggested
- The form starts from the saved settings. Save settings sends every setting at once, and an emptied field removes that setting. The client added `setPluginSettings` for the existing `PUT …/settings` route.
- Values are checked before sending with the API's own validation function from `@propr/shared`, so the messages are worded the same.
- The API returns one combined error string. The page splits it by setting name and shows each part under its field; anything that names no field appears above the button.
- Required settings are marked Required to enable. Configure counts as Done only once they all have values, and the Enable checkbox stays blocked with the API's reason, e.g. "Required setting Verbosity (verbosity) is missing".
Needs approval (`PluginApproval.tsx`)
- It shows the new permissions and, for each package server whose package, exact version or secret variables changed, the change compared with the last acknowledgement: "Version changes from 1.0.0 to 1.1.0", "Secret variables added: …", or that the server is new.
- Approve stays disabled until the acknowledgement checkbox is ticked. Approving clears the badge.
Managed items in Skills & MCP servers
- Rows show a From plugin: Example pack badge linking to the plugin page. The plugin's display name comes from the plugin list, and the plugin id is shown if that list can't be read.
- Managed rows have no Remove button, and their Edit button reads Edit limits. For servers it opens `ManagedServerForm`, which shows the definition read-only (endpoint and authentication, or registry, package, exact version, command, arguments, variables and secret variable names). Only the limits and automatic apply can be changed there; the enabled checkbox stays in the list.
- A managed skill's form now carries the same read-only note.
Activity: a placeholder section shown once the plugin is installed, saying there is no activity yet.
Changes outside the brief
- I replaced the example plugin's settings in the shared test fixture with the real manifest's six settings.
- `PluginInstallStep.tsx` was split into small modules (`PluginAcknowledgements.tsx`, `pluginApprovalUi.ts`) to stay within the lint rules.
- Besides `plugins.md`, I updated `web-ui.md` and the managed-items note in `agent-extensions.md` to match.
Limitations
- The browser test shows a client-side field error, not an API refusal: the page runs the same checks as the API first, so a bad value never reaches it. The API-error path is covered by component tests.
- The repository-map field only suggests this instance's repositories; you can still type any `owner/name`.
Previews of the form with a field error, the saved settings, Needs approval, the managed-item form and the phone view are attached; they use mocked API data.
Acceptance criteria
- AC1: met — `PluginSettingsForm.tsx`, `PluginSettingFields.tsx` and `pluginSettingsUi.ts` cover all five types; `PluginSettingsForm.test.tsx` tests rendering, an unchanged save returning the same values, edits and API errors by field; `PluginSettingsAndApproval.test.tsx` covers an API refusal on the page; `e2e/plugins.pw.ts` saves every type and checks them after a reload
- AC2: met — `PluginSettingsAndApproval.test.tsx` ("blocks the enabled checkbox while a required setting is missing") and `e2e/plugins.pw.ts` (empties Verbosity, Enable is blocked with the reason)
- AC3: met — `PluginApproval.tsx`, `pluginApprovalUi.ts`; `PluginSettingsAndApproval.test.tsx` and the e2e "shows what an upgrade asks for and approving it clears Needs approval"
- AC4: met — `ExtensionManagedItem.tsx` and `ExtensionItemRow.tsx`; `ExtensionManagedItem.test.tsx` (badge link, no Remove, read-only definition, save changes only limits and automatic apply) and the e2e managed-items steps
- AC5: met — `PluginSettingsForm.test.tsx`, `PluginSettingsAndApproval.test.tsx`, `ExtensionManagedItem.test.tsx`; `e2e/plugins.pw.ts` passes (3/3)
- AC6: met — `docs/docs/features/plugins.md` (new Settings form, Needs approval and Managed items in Skills & MCP servers sections, updated walkthrough and Activity)
propr-dev
Bot
changed the title
… the issue lists records `queued`, `started` and one terminal event (`completed`, `failed` or `cancelled`), and a failed append doesn't change how the task ends. All the new tests and the related existing suites pass, along with lint (zero warnings), typecheck and the OpenAPI check. Where events are written. All task status changes already go through one state manager in core, so I added the appends there rather than in each job file. `queued` is written when the task is created; `started` and the terminal events are written right after the state change is saved to the task history. This covers issue tasks, pull request follow-ups, reviews, merge-conflict tasks and automation runs. The event-building code is in `packages/core/src/services/taskJournalEvents.ts`. Behaviour you might not assume: - Not journaled: goal tasks, task imports and agent runs with no repository. They don't fit the issue's five kinds. - Queue handoffs: when a job is requeued or rescheduled, the task is briefly marked `cancelled` but continues later. That appends no `cancelled` event, and resuming it appends no second `started`. - Retries: a retried task appends `started` and a new terminal event again, so one task can have several runs in the journal. - Failure category: this is a short code such as `timed_out`, `git_operation` or `worker`. Anything that looks like free text becomes `unknown`. - Pull request outcome: `opened` when the task opened a pull request (including a continuation one), `updated` when it pushed a commit to an existing one, otherwise empty. - Realtime: task events publish no extra activity update, because task state changes already send their own. Migration you should know about. The journal table only allowed the three existing domains, and SQLite can't change that rule in place. The new migration (`20261114120000_add_task_repository_event_domain.js`) rebuilds the table. It keeps every row, its sequence number and the sequence counter, so existing cursors stay valid and deleted numbers are never reused. Rolling it back deletes the `task` events. Readers. The API, MCP tools and CLI accept the new domain because they build their domain list from the shared one. I updated the MCP tool description and the API docs text, and regenerated the OpenAPI spec and client types. Automations have no event triggers built from the journal's domains, so I left them unchanged, as the issue says to. Acceptance criteria - AC1: met — `packages/core/test/taskJournalEvents.test.ts` checks `queued` → `started` → `completed` with the full payload for an implementation task and a pull request follow-up; `test/taskJournalFinalizers.test.ts` checks the same through the follow-up job completion path in `src/jobs`. - AC2: met — cancelled tasks (user-cancelled and pull-request-closed) append `cancelled`; failed tasks append `failed` with `git_operation`, `timed_out` or `worker` as the category, in both of those test files. - AC3: met — the sentinel test in `packages/core/test/taskJournalEvents.test.ts` puts marker strings into the task's prompt fields, reasons, tool output, results, error messages and logs, then checks no stored `task` event contains them and only the documented payload fields appear; `test/taskJournalFinalizers.test.ts` checks the same for follow-ups. - AC4: met — with the journal table removed, the task still finishes `completed` with its full history and the failure is logged (`packages/core/test/taskJournalEvents.test.ts`), and the follow-up finalizer still reports `finalized` (`test/taskJournalFinalizers.test.ts`). - AC5: met — `packages/api/test/repositoryEventTaskDomain.test.ts` covers list, filters, wait and 30-day retention; `packages/api/test/mcpRepositoryEvents.test.ts` covers the MCP tool and REST route; `packages/shared/test/repositoryEvents.test.ts` covers the domain list; docs are in `docs/docs/architecture/repository-event-journal.md` (domain, payloads, dedupe keys, retention) and `docs/docs/operations/api-reference.md`.
propr-dev
Bot
changed the title
added 2 commits
November 14, 2026 19:58
…it its time limit and was stopped, for the same reason. I won't restart it. The test results were already read straight from the suite log, so the implementation summary and acceptance criteria above still stand.
… each. To check the tests, I ran the new ones against the original source files: all 7 failed there, and all 48 plugin tests pass with the fix. Core typecheck and zero-warning lint on `src/plugins` pass too. I've fixed all four findings and added regression tests for each. To check the tests, I ran the new ones against the original source files: all 7 failed there, and all 48 plugin tests pass with the fix. Core typecheck and zero-warning lint on `src/plugins` pass too. **F1 – comment permission vs. target type.** GitHub's issue endpoints accept pull-request numbers too, so the method name alone didn't prove what a number was. The backend's `comment` now takes the target kind (`'issue'` or `'pull_request'`). It reads the number first and throws `not_found` without posting if the kind doesn't match. `commentOnIssue` and `commentOnPullRequest` each pass their own kind. - **Same defect elsewhere:** `getIssue` had the same gap, letting `issues.read` read a pull request, and now refuses one too. - **Tests:** both substitutions are covered, directly and through a handler, and nothing is posted in either case. One existing test changed: it now expects the extra lookup request before the comment is posted. **F2 – JSON-escaped credentials.** The list of credential spellings to scrub now includes the JSON-escaped form, plus a variant with `/` escaped as `\/`. A JSON body is also checked after decoding, so an upstream can't spell the credential back with `\uXXXX` or `\"`. If a decoded string held it, `text()` and `json()` return the body re-serialised with it replaced; a clean body is returned unchanged. A body nested too deeply to check is refused rather than returned. - **Tests:** a credential containing a quote, a backslash and a `/`, echoed four ways: `JSON.stringify`, all-`\u` escapes, `\/` escapes, and as an object key. A non-JSON page with the escaped credential embedded is covered too. **F3 – HTTP requests after timeout.** The handler's abort signal now reaches `ctx.http`. The DNS lookup, credential read and transport call all stop as soon as the run ends, and the signal is checked again before sending. The default transport cancels an in-flight request on abort. - **Same defect elsewhere:** the default `ctx.propr` backend had the same gap between fetching the GitHub App token and sending. It now gets the signal too and checks it after the token arrives and before queueing a task. - **Tests:** DNS held until after the timeout, the credential held until after the timeout, and an in-flight request getting the abort. In each case nothing is sent. **F4 – state recreated after uninstall.** Inside its transaction, a `ctx.state.set` now requires the installation to still exist with the same `installed_at` the run started with, so a reinstall under the same id doesn't count. Uninstall now deletes the installation row before the state rows. Either the write commits first and uninstall deletes its row, or the write finds the installation gone and throws `unavailable`. - On Postgres the write holds a `FOR SHARE` lock on that row until it commits. Our tests use SQLite, which runs one write transaction at a time, so the Postgres locking path itself hasn't been run. - **Tests:** a handler suspended mid-run resumes after uninstall, and after uninstall plus reinstall. Its write fails and no state row exists. - One existing test ran example-pack from a record that was never installed. It now inserts a matching installation row first. I updated `docs/docs/architecture/plugin-runtime.md` for each change. Nothing here is visible in the UI, so there are no previews. The new tests are in `packages/core/test/pluginHandlerLifecycle.test.ts`, plus additions to `pluginProprClient.test.ts`. Two things I noticed but didn't change, since they're outside these findings: - `createTask(repository, issueNumber)` doesn't check that the number is an issue rather than a pull request. It only needs one permission, so it can't be used to get around one. - A handler that is still running when its plugin is disabled, but not uninstalled, can still write state until it finishes or times out. PR: #3573 Comment by: @propr-ultrafix (ID: 0) Model: Claude Opus 5.5
Owner
|
/merge |
Contributor
Author
|
🔀 Resolved merge conflicts from Conflicting Files
Resolution SummaryAll five conflicts are resolved and no conflict markers are left anywhere in the repo. Both YAML files parse, the core package typechecks, the edited TypeScript file passes lint with zero warnings, and
🤖 Resolution Details
System-triggered merge conflict resolution |
Automatically resolved merge conflicts after merging target branch into PR branch. Resolved conflicts in: - .github/workflows/desktop-release-guard.yml - CHANGELOG.md - docs/mcp.md - docs/static/openapi/propr-api.yaml - packages/core/src/utils/workerStateManager.ts PR: #3563 Model: Claude Opus 5.5
Contributor
Author
|
CI failed: CodeQL Please investigate and fix this CI failure.
Failure evidence
|
Contributor
Author
|
✅ Applied the requested follow-up changes in commit 8c53262 Summary of ChangesI fixed the CodeQL alert. The plugin store no longer hashes the OAuth What changed
There is one small difference in behaviour: a Checks
I haven't run CodeQL, so it's not confirmed the alert closes. The store already hashes There's no visible change, so I made no previews. Review digestRisk-tiered paths
Validation evidence
Diff size: 2 files changed, +9/−4 lines 🤖 Implementation Details
Undo Changes • View Task Execution 💡 ProPR Slash Commands
🤖 Generated by ProPR |
added 2 commits
November 14, 2026 21:06
…sion test. When I ran the new tests against the original source files, all three failed; with the fix, the plugin test files pass. Core typecheck passes, and zero-warning lint passes on `src/plugins`. Lint only reported "file ignored" for the test files, because core lint covers `src/` only. I fixed all three findings (F5, F6, F7), each with a regression test. When I ran the new tests against the original source files, all three failed; with the fix, the plugin test files pass. Core typecheck passes, and zero-warning lint passes on `src/plugins`. Lint only reported "file ignored" for the test files, because core lint covers `src/` only. **F5: every state operation is now tied to the installation** (`pluginStateStore.ts`) - `get`, `set`, `delete` and `keys` now all run inside one transaction that first checks the installation is still the same one, by comparing `installed_at`. On Postgres it also holds a shared lock on the installation row until commit, so an uninstall can't overtake an operation in progress. - An old handler that resumes after an uninstall and reinstall now gets `unavailable` from all four methods. It can no longer read, list or delete the new installation's state. - Test: for each of `get`, `keys` and `delete`, a held handler resumes after uninstall plus reinstall, gets `unavailable`, and the new installation's value is still there. The `suspendThenSet` fixture handler gained an optional `op` input so this test could reuse it. **F7: state cancellation is checked again before and after each change** - `set` checks the signal again after the count/size query and before the insert. - Every operation also checks the signal just before its transaction commits. If the run ended during the insert or delete, the change is rolled back. - Test: I abort the signal at the moment the count/size query is sent, then do the same during the insert and during the delete. Each call is refused, and the stored value and key list stay as they were. **F6: no task is queued after cancellation** (`pluginProprBackend.ts`) - `createTask` now passes the run's signal to the enqueue function. - The default enqueue loads the queue, checks the signal, and only then calls `queue.add`. - To make the test possible, there is a new optional `issueQueue` setting so a test can supply the queue. A custom `enqueueIssueTask` now also receives the signal as a third argument. - Test: queue initialization is held until after the abort; no job is added and the call is refused with `unavailable`. A live run still queues its job. I checked the backend's other GitHub write calls for the same gap and found none: every request already rechecks the signal after the App token is fetched. I found nothing separate to report outside these three findings, and I made no previews because nothing here is visible to a user. PR: #3573 Comment by: @propr-ultrafix (ID: 0) Model: Claude Opus 5.5
… the OAuth `state` itself. That alert is a false positive in substance: `state` is a random 256-bit single-use token created in `startSignIn`, not a password. A slow password hash would only make lookups slower without adding any protection. I fixed the CodeQL alert. The plugin store no longer hashes the OAuth `state` itself. That alert is a false positive in substance: `state` is a random 256-bit single-use token created in `startSignIn`, not a password. A slow password hash would only make lookups slower without adding any protection. **What changed** - I added `pendingSignInServer(state)` to the extension OAuth store (`packages/core/src/extensions/extensionOAuthStore.ts`). It returns the server a sign-in is pending for, without claiming the sign-in. Before looking anything up it checks that `state` has the 43-character base64url format, the same check `completeSignIn` already does. It uses the store's existing `sha256` helper. - In `packages/core/src/plugins/pluginStore.ts`, `completeOAuth` now calls that method. The `node:crypto` import and the hashing code that CodeQL flagged are gone. There is one small difference in behaviour: a `state` with the wrong format is now refused before the database is queried. The response is the same 400 "Unknown, used or expired sign-in request" as before. **Checks** - Zero-warning lint passes on both files, and the core typecheck passes. - `test/pluginContributions.test.ts` and `test/extensionOAuth.test.ts` pass (33 tests). They cover the plugin OAuth sign-in start to finish and the refusal of an unknown `state`. I haven't run CodeQL, so it's not confirmed the alert closes. The store already hashes `state` the same way elsewhere and that hasn't been flagged, which suggests it will. There's no visible change, so I made no previews. PR: #3563 Comment by: @github-advanced-security[bot] (ID: 4575640667) Model: Claude Opus 5.5
Contributor
Author
|
✅ Applied the requested follow-up changes in commit 20a2652 Summary of ChangesThe alert is already gone from the current code. CodeQL failed on The remaining risk: the earlier fix added What I changed:
Checks: zero-warning lint on both source files and the core typecheck pass. I can't run CodeQL here, and I couldn't read the alert list (the code-scanning API returns 403), so the alert closing is only confirmed once CodeQL finishes on the next push. There's no visible change, so I made no previews. Review digestRisk-tiered paths
Validation evidence
Diff size: 3 files changed, +13/−12 lines 🤖 Implementation Details
Undo Changes • View Task Execution 💡 ProPR Slash Commands
🤖 Generated by ProPR |
added 5 commits
November 14, 2026 21:15
…led on `f107020a9`, and the next commit (`8c5326217`) removed the hashing it flagged at `pluginStore.ts:371`. But CodeQL never scanned that fix: its run on `8c5326217` was cancelled after about 40 seconds and reports "Error when processing the SARIF file". I've also closed off the one way the same alert could come back. The alert is already gone from the current code. CodeQL failed on `f107020a9`, and the next commit (`8c5326217`) removed the hashing it flagged at `pluginStore.ts:371`. But CodeQL never scanned that fix: its run on `8c5326217` was cancelled after about 40 seconds and reports "Error when processing the SARIF file". I've also closed off the one way the same alert could come back. **The remaining risk:** the earlier fix added `pendingSignInServer(query.state)`, which sends the OAuth `state` straight into the shared `sha256` helper. That is the same data flow, starting from `startOAuth`, that CodeQL flagged. It could have reappeared as a new alert at `extensionOAuthStore.ts:38`. **What I changed:** - **`extensionOAuthStore.ts`:** `completeSignIn` now takes an optional list of allowed servers. With the list, it only claims a pending sign-in for one of those servers. Any other `state` is refused with the usual 400 and is not used up. I removed `pendingSignInServer`. - **`pluginStore.ts`:** `completeOAuth` passes the connection's server names into `completeSignIn`. The separate lookup before it is gone. The `state` now only reaches a hash through the path CodeQL already scanned on `f107020a9` without flagging it, and the sign-in takes one database query instead of two. - **Behaviour:** a sign-in pending for a different server is still refused before it can be used, as before. If the OAuth tables are missing, the plugin route now returns 503 instead of 400, the same as the extension callback already does. - **Test:** `extensionOAuth.test.ts` now checks that a restricted completion refuses a `state` pending for another server, stores nothing, and that the same `state` still completes afterwards. **Checks:** zero-warning lint on both source files and the core typecheck pass. `pluginContributions.test.ts` and `extensionOAuth.test.ts` pass, 18 of them in the OAuth file including the new case. I can't run CodeQL here, and I couldn't read the alert list (the code-scanning API returns 403), so the alert closing is only confirmed once CodeQL finishes on the next push. There's no visible change, so I made no previews. PR: #3563 Comment by: @propr-dev[bot] (ID: 6884174538) Model: Claude Opus 5.5
… the fixes, the touched core plugin test files pass (pluginHandlerLifecycle 8/8, pluginHandlerRunner 6/6), and so does `packages/plugins/test/testing.test.ts`. When I swapped the original source files back in, both new tests failed, so they catch the defects. Core typecheck, the plugins package typecheck and zero-warning lint on `src/plugins` pass. I made one last small refactor, to clear a lint warning about too many parameters; lint, typecheck and those two suites were re-run after it. I fixed F8 and F9 and added regression tests for both. With the fixes, the touched core plugin test files pass (pluginHandlerLifecycle 8/8, pluginHandlerRunner 6/6), and so does `packages/plugins/test/testing.test.ts`. When I swapped the original source files back in, both new tests failed, so they catch the defects. Core typecheck, the plugins package typecheck and zero-warning lint on `src/plugins` pass. I made one last small refactor, to clear a lint warning about too many parameters; lint, typecheck and those two suites were re-run after it. **F8: credentials hidden in escaped response headers** (`pluginHttp.ts`) - Headers now go through a new `scrubbedHeader` step instead of the literal `scrub` alone. After literal scrubbing, it decodes JSON escapes (`\uXXXX`, `\"`, `\\`, `\/`, `\n` and so on) anywhere in the value, up to 8 times so double-escaped values are caught too. If the decoded text contains any spelling of the credential, the whole header value is replaced with `[REDACTED]`. - This goes further than only handling headers that are entirely JSON. A value like `"\u0042…", other`, where several headers were joined with `, `, is not valid JSON as a whole, but a handler could split it and parse one part. - Headers that don't contain the credential are passed through byte-for-byte; a new test checks this. - `fetchProbe` now also reports `parsedHeadersHaveProbe`: it parses each header value, and each `, `-separated part, as JSON. The test covers a fully `\u`-escaped string, a JSON object, a joined value and a double-encoded value. **F9: old runs using a reinstalled plugin's credentials** (`pluginContext.ts`, `pluginHttp.ts`, `pluginStateStore.ts`) - The default credential lookup now checks the run's own installation (matched by `installed_at`) and reads the secret in the same transaction. It uses the same `requireInstallation` check as `ctx.state`, which is now exported for this. - `createPluginHttp` has a new `authorize` option. For authenticated requests the context sets it to re-check the installation after DNS and the credential lookup, just before sending. This also covers a custom `connectionSecret` source and a credential cached from an earlier request. - The new lifecycle test holds DNS and then tries three cases: uninstall only, uninstall and reinstall with a replacement token, and the same reinstall using a lookup that knows only the plugin id. In every case the handler gets `unavailable`, nothing reaches the transport, and the replacement token never appears in the run's result. - One race remains and can't be closed: an uninstall that commits after the final check but before the HTTPS request goes out. In that window the request can only carry the old installation's own secret, never the replacement's. - I added a short "Installation binding" paragraph and the header rule to `docs/docs/architecture/plugin-runtime.md`. **Found along the way, not fixed (outside F8/F9):** - Response header *names* are never scrubbed. An upstream that sends a header named after a lowercase credential would show it to the handler. - The body check only works when the whole body is JSON. A non-JSON body, such as HTML, that contains a `\u`-escaped JSON string of the credential is only scrubbed literally, so the handler could pull out that string and decode it. PR: #3573 Comment by: @propr-ultrafix (ID: 0) Model: Claude Opus 5.5
…th the fixes, all the plugin tests pass: 25 in the handler, `ctx.propr` and runner suites, and 30 more in the store, contributions, lint and `packages/plugins` testing suites. When I swapped the original source files back in, all three new tests failed. Core typecheck and zero-warning lint on `src/plugins` pass.
I fixed F10 and F11 and added regression tests for both. With the fixes, all the plugin tests pass: 25 in the handler, `ctx.propr` and runner suites, and 30 more in the store, contributions, lint and `packages/plugins` testing suites. When I swapped the original source files back in, all three new tests failed. Core typecheck and zero-warning lint on `src/plugins` pass.
**F10: credentials escaped in response bodies** (`pluginHttp.ts`)
- There is now one shared check, `revealsCredential`. It decodes the text's JSON escapes one round at a time, up to 32 rounds, and looks for any spelling of the credential after each round. Text that is still decoding after 32 rounds counts as holding the credential, so the check fails closed.
- **Bodies:** after the literal scrub, any body (HTML, plain text or JSON) that still decodes to the credential is replaced with `[REDACTED]`. That covers `\uXXXX` escapes inside an HTML script.
- **Nested JSON:** inside a JSON body, a string that would reveal the credential when parsed again is replaced with `[REDACTED]` on its own. The rest of the document stays readable.
- **Headers:** header filtering had the same weakness, so it now uses the same check. It used to stop after 8 decode rounds and pass the header through unchanged. An escape like `\u005c` turns into the backslash of the next escape, so about 2 KB was enough to hide a 40-character token 10 rounds deep.
- The new test covers the HTML script case and the nested JSON case. It also checks 10-round and 40-round escapes in both a body and a header, and that escapes with no credential in them come through unchanged.
**F11: uninstalled handlers keeping `ctx.propr` access**
- `buildPluginContext` now has one installation check, also used by authenticated `ctx.http`. It confirms the run's installation (same `installed_at`) still exists and the run hasn't ended.
- `scopedProprClient` runs this check before every call reaches the backend, after its permission and repository checks. That covers every operation and any backend.
- The default GitHub backend runs it again right before each GitHub request, after the App client is ready. It also runs it before adding a task job, after the queue starts up or before a custom `enqueueIssueTask`.
- A refused call returns `unavailable` ("The plugin is no longer installed") and is recorded as `handler_refused`. Audit rows don't depend on the installation, so recording still works after uninstall.
- The new tests suspend a handler, then uninstall (or uninstall and reinstall) before its next `createIssue`, `createTask` or `commentOnIssue`; nothing reaches the backend. A second test uninstalls while the App token and the queue are still loading; nothing is sent or queued.
One race remains and can't be closed: the check commits before the GitHub request goes out, so uninstall doesn't wait for a request that already passed it. `ctx.http` behaves the same way.
Docs: `docs/docs/architecture/plugin-runtime.md` now describes the new body and nested-string handling and the new installation rule for `ctx.propr`.
**Separate findings, not changed:**
- Filtering only covers JSON-style escapes. A plugin author who controls an allowed host can still return the credential in any encoding they make up (reversed, XOR'd, and so on).
- `ctx.propr` still uses the permissions and repository list from when the run started. A change to the grant or repository limit, or disabling the plugin, during a run isn't rechecked; only removal or reinstall is.
- An existing type error in the `fetchUrl` fixture handler (`handlers.ts:40`, TS2352) shows up when that file is typechecked. Normal typecheck doesn't cover it.
There are no visual changes, so I didn't create previews.
PR: #3573
Comment by: @propr-ultrafix (ID: 0)
Model: Claude Opus 5.5
…attempt` audit entry before it sends any change to GitHub or the task queue. If that write fails, nothing is sent. I fixed F12: `ctx.propr` now writes an attributed `handler_attempt` audit entry before it sends any change to GitHub or the task queue. If that write fails, nothing is sent. **How a mutating call works now** (`createIssue`, `createTask`, `commentOnIssue`, `commentOnPullRequest`, `addLabel`, `removeLabel`), in `packages/core/src/plugins/pluginProprClient.ts`: 1. The existing permission, repository-limit and installation checks run first. 2. A `handler_attempt` entry is written, with `plugin:<id>` as the actor. If that write fails, the call is refused as `unavailable` and the backend is never called. 3. The backend runs. 4. The outcome is recorded as a separate entry: `handler_call` on success, `handler_refused` with the reason on failure. All entries for one call share a new `callId`, so the attempt and its outcome can be matched. Once the attempt is recorded, a failure to record the outcome is logged on the server (`[plugins] Could not record the outcome of an attempted handler call`) and the call still returns its result. Rejecting it would only invite a retry that creates a second issue or comment, and the attempt entry already credits the mutation to the plugin. Read operations are unchanged: they get one entry when they finish, and a failure to write it still fails the call. That is safe because nothing outside ProPR changes. **Checking related code:** the GitHub backend still re-checks the installation right before it sends anything. A call refused at that point leaves an attempt entry followed by a refusal. The "uninstalled mid-run" lifecycle test still shows only refusals, because the installation check happens before the attempt is written. The other parts of `ctx` (`ctx.http`, `ctx.state`) make no audit-trail promise. **Other files changed:** - `handler_attempt` is added to the list of audit actions in `packages/shared/src/pluginInstallations.ts`, the OpenAPI spec and the generated client types. `gen:openapi:check` passes. - `pluginContext.ts` maps each outcome to its audit action and does the server-side logging. - Both docs pages (`plugins.md` and `plugin-runtime.md`) describe the attempt entry and what happens when audit writes fail. **Tests** (in `packages/core/test/pluginProprClient.test.ts`): - **Recorded before sending:** for each mutation run through a handler, the backend checks the real `plugin_audit` table at the moment it is called and finds the `handler_attempt` entry already there. - **Attempt can't be written:** a SQLite trigger rejects attempt entries. Every mutation is then refused as `unavailable` and nothing reaches the backend. The same is checked with a record function that throws. - **Outcome can't be written:** `createIssue` still returns the created issue and the attempt entry stays. A backend failure after the attempt leaves an attempt entry and a refusal entry with the same `callId`, and the logging hook is called. - I updated two existing tests to the new entries. **Results:** all 56 plugin tests pass across the six plugin suites. With the original source files swapped back in, all three new tests and the two updated ones fail. Typecheck passes for `shared`, `core`, `client` and `plugins`, and zero-warning lint passes on `src/plugins`. I also ran `taskJournalEvents`, an existing suite this change doesn't touch. It didn't run properly because my command left out the `--experimental-test-module-mocks` flag it needs, so that is no evidence either way. **Separate issue, not changed:** when an audit write fails and no attempt entry exists to fall back on (a refusal, or a read finishing), the handler gets the raw database error instead of `unavailable`. One of the new tests depends on this. The behaviour was already there before this PR's F12 work and wasn't selected, so I left it alone. There are no visual changes, so I made no previews. PR: #3573 Comment by: @propr-ultrafix (ID: 0) Model: Claude Opus 5.5
…test for each. When I put the original source files back, the new tests failed; with the fix, all 60 tests in the plugin suites pass.
I fixed both findings, F13 and F14, and added a regression test for each. When I put the original source files back, the new tests failed; with the fix, all 60 tests in the plugin suites pass.
**F13: credentials in response header names** (`packages/core/src/plugins/pluginHttp.ts`)
- `bufferedResponse` now drops any response header whose name contains the credential in any spelling it already checks for. I dropped the header rather than rewriting the name, because rewritten names could collide.
- Header names arrive in lowercase, so the check ignores case. Without that, the base64 form or the URL-encoded form (`%2F` arrives as `%2f`) of a mixed-case secret would get through. When I made the check case-sensitive, the new test failed, so the test catches this.
- The same check also catches a credential hidden in a name with JSON escapes. Other headers reach the handler as before.
**F14: `getTask` in the test kit** (`packages/plugins/testing/index.ts`)
- The kit now waits for the `getTask` answer and checks the task's repository against the configured limit, ignoring case. A task from another repository is refused with `repository_not_allowed` and is left out of `kit.calls`.
- Two follow-on changes were needed to keep the kit matching production:
- **Placeholder task:** an unanswered `getTask` returned a placeholder task in `owner/repo`, which the new check would refuse. It now uses the first repository in the limit.
- **Missing task:** a null answer is refused with `not_found`, as in production, instead of being misreported as `repository_not_allowed`.
**Tests**
- **Handler-visible sentinel test:** the fixture's echo transport now also returns the credential as a header name, and the test checks the header names the handler sees. The `fetchProbe` handler reports a new `headerNamesHaveProbe` flag.
- **New test for header names:** a mixed-case secret containing `/` is echoed in seven header-name spellings. None reaches the handler, and `content-type` and `x-request-id` arrive unchanged.
- **New test-kit test:** an out-of-limit `getTask` answer is refused and not recorded; an in-limit one passes; the placeholder is in the limit; a null answer gives `not_found`.
**Checks:** typecheck passes for core and for `@propr/plugins` (including the handlers config), and zero-warning lint passes on `pluginHttp.ts` and the whole plugins package. The test files aren't covered by any typecheck, so I checked the edited ones separately: the only errors are on lines I didn't touch (untyped migration imports and an existing cast in `fetchUrl`).
**Docs:** `docs/docs/architecture/plugin-runtime.md` now says credential-named headers are dropped, and describes the kit's `getTask` check and placeholder.
**Separate issue, not fixed:** if a call's repository argument isn't a string, for example `listEvents({})` or `getIssue(undefined, 1)`, the test kit skips the limit check and returns an answer, while production refuses it with `invalid_input`. That's missing input validation, which the kit doesn't do at all, rather than the defect in these findings, so I didn't change it.
There's no visible UI change, so I made no previews.
PR: #3573
Comment by: @propr-ultrafix (ID: 0)
Model: Claude Opus 5.5
propr-dev
Bot
changed the title
…ther hex case, are now redacted before the handler sees them, and the same gap in the handler's log and result redaction is closed. Both new regression tests fail against the original source files and pass with the fix. All 62 plugin tests pass (the 60 from before plus the 2 new ones), and core typecheck and zero-warning lint on `src/plugins` pass. I fixed F15. Credentials echoed with percent escapes, in either hex case, are now redacted before the handler sees them, and the same gap in the handler's log and result redaction is closed. Both new regression tests fail against the original source files and pass with the fix. All 62 plugin tests pass (the 60 from before plus the 2 new ones), and core typecheck and zero-warning lint on `src/plugins` pass. **The fix, in `packages/core/src/plugins/pluginHttp.ts`** - **Redaction in place:** every credential spelling is now also matched where some of its characters are percent-escaped, with lowercase or uppercase hex (`%2f`, `%2F`, or an escaped letter). The match is replaced with `[REDACTED]` and the rest of the value stays, so `token=plugin%2fcred…&page=2` becomes `token=[REDACTED]&page=2`. Stored secrets are printable ASCII (`PLUGIN_SECRET_PATTERN`), so only escapes of ASCII characters are needed. - **Withholding:** the existing check for text that decodes back to the credential now undoes percent escapes (`%2f`, `%u002f`) as well as JSON escapes, in any order. So a value encoded twice, or a `%` hidden inside a JSON escape (`\u00252f`), is withheld whole, in bodies, header values and header names. - **Cost:** decoding is still capped at 32 rounds. A round that produces more than 8 different decoded texts also counts as holding the credential. On ordinary JSON, the check now takes about 40 ms for a 1 MiB body, up from 15 ms. On deliberately nasty bodies of about 700 KB it takes up to 125 ms, up from 27 ms. **The same gap in logs and results** The handler's log lines, log fields, error message and result were redacted by the shared extension redactor, which only catches the exact `encodeURIComponent` spelling. A credential that reached the handler some other way, such as in its input payload, could be logged with `%2f` and survive. `pluginContext.ts` now runs the same scrubbing as above before that redactor, through a new exported helper, `scrubCredentialValues`. **Tests, in `pluginHandlerLifecycle.test.ts`** - **Responses:** the upstream echoes the token in a JSON body, a form body and header values. The encodings are lowercase hex, the full `Bearer …` value, every character escaped (both hex cases), encoded twice, `%u` escapes, and a percent escape inside a JSON escape. The handler-side probe decodes everything it can see with `decodeURIComponent`, falling back to `unescape`-style decoding, repeatedly, and must never recover the token. The test also checks that text which only happens to contain escapes reaches the handler unchanged. I checked separately that each of those encodings leaks on the original code, so none of the cases is a no-op. - **Logs and results:** a handler logs and returns a lowercase-encoded token taken from its input. The token is replaced in place, and the other forms can't be recovered by URL-decoding the run. - **Fixture changes:** to support these, `fetchProbe` now reports `urlDecodedHasProbe` and tolerates a body that isn't JSON. I also added a `fetchThenLog` fixture handler. The existing JSON-escape test's expected result now includes the new key. I also updated `docs/docs/architecture/plugin-runtime.md` to describe the percent-escape handling for responses and logs. Nothing visible in the UI changed, so there are no previews. **Found separately, not changed:** the shared extension redaction in `packages/core/src/extensions/extensionOutput.ts`, and the similar spelling list at `operatorCatalog.ts:40`, also match only the exact uppercase `encodeURIComponent` spelling. That code predates this PR and also serves the MCP gateway and extension subprocess output. I left it alone because changing it would fix an older problem outside F15. PR: #3573 Comment by: @propr-ultrafix (ID: 0) Model: Claude Opus 5.5
…ed log line with the full set of credentials acquired during the run when it builds the final result, so a token logged before the handler's first authenticated request no longer survives in the returned logs. I fixed F16. The handler runner now re-redacts every captured log line with the full set of credentials acquired during the run when it builds the final result, so a token logged before the handler's first authenticated request no longer survives in the returned logs. **What changed** - `packages/core/src/plugins/pluginHandlerRunner.ts`: in `finish`, the logs are mapped through the context's `redact` instead of being copied as is. The result and error were already redacted there; the logs were the gap. - `packages/core/test/fixtures/plugins/fixture-handlers/handlers.ts`: new `logThenFetch` fixture handler, the reverse of the existing `fetchThenLog`. It logs a value from the input first, then makes the authenticated request. - `packages/core/test/pluginHandlerRunner.test.ts`: new regression test. The input carries the sentinel in plain, base64 and percent-encoded spellings. The test asserts the credential was acquired only after the line was logged, that the message and fields come back redacted, that no spelling appears anywhere in the returned run, and that the audit table never holds it. **Sibling check** Every log line goes through the single `write` closure in `pluginContext.ts`, including the `ctx.http` decision lines and the `ctx.propr` refusal lines, and the logs array is only exposed to the caller through `finish`. So one re-redaction at that point covers all paths. Audit rows written by `recordCall` hold operation metadata only, not handler input, and the existing sentinel assertion on `plugin_audit` still passes. I found no other place where captured output escapes before the credential set is complete. **Verification** - The four plugin handler suites (runner, lifecycle, `ctx.propr` client, lint) pass: 34 tests, 0 failures. - With the original runner swapped back in, the new test fails and the other six in that file still pass, so it catches the defect. - Core typecheck passes. Zero-warning lint on `src/plugins` passes. The fixture handler is linted by the existing plugin-lint test, which passes. No visual preview was created: the change is not visually perceptible. PR: #3573 Comment by: @propr-ultrafix (ID: 0) Model: Claude Fable 5.1
… the original sources and pass with the fix.
Both findings are fixed, with regression tests that fail on the original sources and pass with the fix.
**F17, revoked installation authority.** `requireInstallation` in `packages/core/src/plugins/pluginStateStore.ts` now returns the installation's current enabled state, grants and repository limit from the same transaction. A new exported `installationAuthorizer` in `pluginContext.ts` uses it in two forms. The `request` form backs authenticated `ctx.http` requests and refuses a disabled installation. The `call` form backs `ctx.propr` and the default GitHub backend: it also requires the call's permission to still be granted through the run's manifest, and the named repository to still be inside the current limit. The scoped client and the backend now pass the permission and repository of each dispatch, so the recheck right before each GitHub request and queued task covers authority, not only identity. The default connection-secret read refuses a disabled installation too. Refusals map to `unavailable` ("The plugin is disabled"), `repository_not_allowed` and `permission_denied`, and are recorded as `handler_refused` with that code.
**F18, uppercase hex echoes.** Spellings in `pluginHttp.ts` are now typed with an `anyCase` flag set only for the hex encoding. Matching folds ASCII letters for that spelling alone, in literal replacement, percent-escaped replacement, the decode-and-recheck pass, header names, and the log and result redaction. Base64, URL-encoded and plain spellings stay case-sensitive, and a value differing from the secret only in case is left untouched.
**Tests added** to `packages/core/test/pluginHandlerLifecycle.test.ts`:
- Uppercase and mixed-case hex of the token and of the full header value, in JSON and text bodies, header values, `json()`, logs and results, plus negative cases for case-sensitive ordinary spellings.
- A suspended handler whose installation is disabled, limited to another repository, or has a grant revoked cannot create an issue or task. Nothing reaches the backend and the audit trail records the refusal code.
- An authenticated request suspended in the DNS lookup or the credential read is never sent after a disable. An unauthenticated request is unaffected.
- The GitHub backend, held during App token fetch and queue initialisation, refuses the excluded repository while still serving an allowed one, refuses everything when disabled, and refuses a comment whose grant was revoked while still adding a label.
**Verification.** Core typecheck and zero-warning lint on `src/plugins` pass. All 63 tests across the six plugin suites pass, and the 4 tests in the plugins package pass. With the original source files swapped back in, the hex test fails with the uppercase hex visible verbatim, and the two handler-level F17 tests fail with the dispatch going through. The backend-level F17 test imports the new helper and so cannot run against the old code.
**Docs** in `plugin-runtime.md` and `plugins.md` now describe the live recheck and the any-case hex matching. No visual preview was produced because the change has no user-visible UI.
One note outside the selected scope: `ctx.state` still operates while an installation is disabled. The finding named `ctx.propr` and authenticated HTTP, and the feature docs say state is kept across disable, so I left it unchanged.
PR: #3573
Comment by: @propr-ultrafix (ID: 0)
Model: Claude Fable 5.1
… repository against the current installation once the task is read, so a repository restriction made while the handler was suspended refuses the read instead of returning the task. F19 is fixed. `ctx.propr.getTask` now authorizes the task's repository against the current installation once the task is read, so a repository restriction made while the handler was suspended refuses the read instead of returning the task. **What changed** - `packages/core/src/plugins/pluginProprClient.ts`: after the backend returns the task and the initial-limit check passes, the scoped client calls the `authorize` option again with `tasks.read` and the task's repository. The initial-limit check is retained. A repository refusal from that recheck is rethrown as `repository_not_allowed` with a message that does not name the repository, matching the existing initial-limit refusal, since the handler never supplied one. Disabled, uninstalled and revoked-grant refusals pass through unchanged. The option's doc comment describes the second call. - `packages/core/test/pluginHandlerLifecycle.test.ts`: a new regression runs a handler that suspends on a held read, applies each administrative change from the existing restriction table, then calls `getTask` for a task in the repository the run started with. The repository restriction case asserts the task was read but withheld, the refusal code and message, and the audit record. The disabled and revoked cases assert the refusal happens before the backend. An unchanged installation still returns the task. The test fails on the original source and passes with the fix. - `docs/docs/architecture/plugin-runtime.md`: one sentence added to the Installation bullet describing when `getTask` checks the task's repository. **Sibling check** `getTask` is the only `ctx.propr` operation whose repository is resolved after the backend call. Every other operation names its repository up front, and the existing per-call authorization already covers it. The default database-backed `getTask` in the backend is a local row read with no outbound dispatch, so the recheck belongs in the scoped client, which covers every backend. The remaining window between the recheck and the return is a race with an external administrative write and cannot be closed further. **Verification** - The three plugin suites pass through the repository runner: lifecycle, scoped client and handler runner, 11 files' worth of tests in the lifecycle file alone and 30 tests across the two directly affected files. - `tsc --noEmit` in `packages/core` passes. - Zero-warning ESLint passes on the changed source file. The test file is outside the core lint configuration, as before. No visual preview was created. The change has no user-visible UI. PR: #3573 Comment by: @propr-ultrafix (ID: 0) Model: Claude Fable 5.1
…w redacted before they are cut to length. Before the fix, a credential sitting across the cut point came back with only its last character missing. I fixed F20. Log messages and handler error messages are now redacted before they are cut to length. Before the fix, a credential sitting across the cut point came back with only its last character missing. **What changed** - **Logs** (`packages/core/src/plugins/pluginContext.ts`): when a line is written, it is redacted and stored with 32 KiB of extra text past the 2,000-character limit. When the run ends, each line is redacted again with every credential the run acquired, and only then cut to 2,000 characters. This also covers a line logged before the credential was acquired. - **Error messages** (`packages/core/src/plugins/pluginHandlerRunner.ts`): same order. The message keeps the extra text, and `finish` cuts it to 1,000 characters only after redacting it. - **Fallback for very long credentials:** stored connection secrets are at most 4,096 characters, and the longest form a matcher looks for is about 24,600 characters, so the 32 KiB margin covers every storable secret. Only a custom `connectionSecret` source could supply a credential longer than the margin. If one is cut at the limit, the trailing characters that match its start (raw, base64, base64url, URL-encoded, hex or with `Bearer `) are dropped. - **Docs:** `docs/docs/architecture/plugin-runtime.md` now describes the order. **Regression tests** (`packages/core/test/pluginHandlerRunner.test.ts`) - A credential straddling the 2,000-character log limit, logged before (`logThenFetch`) and after (`fetchThenLog`) the credential is acquired. Also a credential straddling the 1,000-character error limit, via a new fixture `fetchThenThrow`. Each asserts that no 8-character prefix of the credential appears anywhere in the returned run. - A 40 KiB credential cut at the log limit: the returned line ends at the ordinary text, with no part of the credential. **Verification** - Both new tests fail when I put back the original two source files. The log-before-acquisition case reports "a prefix of the credential leaked". The suite stops at its first failing case, so that run didn't check the after-acquisition log case and the error case separately. - With the fix, all 70 tests in the plugin suites pass, including `packages/plugins/test/testing.test.ts`. Core typecheck passes, and zero-warning lint on `src/plugins` passes. **Found separately (not changed):** the same cut-then-redact order exists in a few short echoes of handler-supplied names inside SDK errors. These are the connection name (48 characters, `pluginContext.ts:194`), a header name (64, `pluginHttp.ts:320`) and an HTTP method (16, `pluginHttp.ts:407`). A handler would have to pass a credential as one of those names for it to show up partly. That is far-fetched, so I left them out of this fix. PR: #3573 Comment by: @integry (ID: 6893002429) Model: Claude Opus 5.5
…est this PR doesn't touch. I fixed the Shard 2/4 failure. It was a timing flake in a test this PR doesn't touch. **What failed:** one test in `packages/api/test/mcpTunnelMode.test.ts`, "a 409 keeps forcing registration through transient failures until one succeeds". It expected 5 Connect registrations and saw 4. Every other test in the shard passed. Neither this test nor the code it exercises (`packages/api/mcp/connectRegistration.ts`) has changed on this branch. **Cause:** after firing the manager's retry timer by hand, the test waited exactly 100 event-loop turns for the extra registration to arrive. That registration goes through async database and crypto work that runs on Node's background thread pool, so it doesn't finish in a fixed number of turns. On a slow or busy machine the count ran out first. I reproduced it locally: 1 failure in 15 runs of the original test. **Fix:** only that wait changed, at `packages/api/test/mcpTunnelMode.test.ts:321`. It now checks every 5 ms until the fifth registration arrives, giving up after 10 seconds. The assertions are the same, so a registration that never happens still fails the test. **Verification:** - The changed test passed 25 of 25 isolated runs, against 14 of 15 before. - The full `mcpTunnelMode.test.ts` file passes 17 of 17. - Zero-warning ESLint on the file, using the `packages/api` config, is clean. I didn't re-run the whole Shard 2/4 suite locally; the CI re-run on the next commit will confirm the shard. The "Node.js 20 is deprecated" message in the failure evidence is a warning about `actions/checkout`. It didn't cause the failure. No preview was created because nothing in this change is visible to a user. PR: #3573 Comment by: @propr-dev[bot] (ID: 6893300396) Model: Claude Opus 5.5
…it failed because an earlier job hung, which caused the whole unsigned packaging matrix to be skipped. Neither the hang nor the fix involves this PR's plugin code. I fixed the CI failure. The finalize step itself was fine: it failed because an earlier job hung, which caused the whole unsigned packaging matrix to be skipped. Neither the hang nor the fix involves this PR's plugin code. **What happened in run 43707247942** - The `Prove stock Chromium existing-target axe boundary` job has to pass before the packaging matrix runs. It hit its 10-minute job timeout in the step `Require exact CSP observer regression across real Chromium navigation`. That step normally takes about 7 seconds and passed on the previous commit. - The log stops after subtest 36. The next test is the only one that drives real Chromium, and when the job was cleaned up, only a single main `chrome-headless-shell` process was still running. - So the packaging matrix finished as `skipped`, and `Finalize unsigned validation checksums` failed because of it. **Cause:** in `apps/desktop/scripts/packaged-acceptance-csp-reload.test.mjs`, the test cleans up with `t.after(() => browser.close())`. The 30-second test timeout doesn't cover cleanup, so a stuck `browser.close()` waits forever, and the open connection to Chromium stops the test process from exiting. I couldn't make the hang happen naturally: 15 local runs against the same Chromium as CI (151.0.7922.34) all passed in about a second. I did check the paths separately: - A hang inside the test body fails cleanly at 30 seconds. - A hung cleanup, simulated on the original code, kept the process alive until my outer timeout killed it. **Fix (that file only):** - The test now starts Chromium as a separate browser server that it owns, and connects to it. - Cleanup gives `close()` 10 seconds; if that runs out, it kills the Chromium process outright (SIGKILL). - The two local fixture servers are closed with `closeAllConnections()`, so a held-open connection can't keep them alive either. **Verification:** - The real-Chromium test passes in 5 of 5 runs. - The full file passes: 52 of 52 with the real-Chromium flag set as CI does it, and 51 passed with 1 skipped without it. - ESLint is clean. - With `close()` forced to hang, the new code exits after 10 seconds instead of hanging. The other failing check, `Prove stock Chromium existing-target axe boundary`, was this same hang, so this change should clear both. PR: #3573 Comment by: @propr-dev[bot] (ID: 6893504078) Model: Claude Opus 5.5
…e when a handler throws a value that can't be turned into a string, instead of rejecting.
I fixed F21: `runPluginHandler` now returns a failed outcome when a handler throws a value that can't be turned into a string, instead of rejecting.
**Fix** (`packages/core/src/plugins/pluginHandlerRunner.ts`): getting the error message is now exception-safe. If converting the thrown value throws, or doesn't produce a non-empty string, the error is the fixed text `'The handler failed'`. While checking the rest of that catch block, I found `error instanceof HandlerTimeout` has the same problem: a proxy whose `getPrototypeOf` trap throws makes `instanceof` throw too. So that check is now inside a `try` as well. With both guarded, nothing in the catch block can throw any more. That means `finish` always runs and always aborts the run's context.
**Regression test** (`packages/core/test/pluginHandlerRunner.test.ts`): a new test throws five values from a handler:
- the finding's `{ message: 'Upstream failed', toString: 'upstream field' }`
- `Object.create(null)`
- an `Error` whose `message` isn't a string
- an `Error` whose `message` getter throws
- a proxy whose `getPrototypeOf` trap throws
For each one it checks that the runner resolves with `outcome: 'failed'`, `timedOut: false` and the fixed message, and that the handler's `ctx.signal` is aborted. It also checks that a thrown string still comes back as its own message.
**Verification:**
- `pluginHandlerRunner.test.ts` passes (10/10).
- With the original runner source put back, the new test fails with the `TypeError` from the finding.
- ESLint (zero warnings allowed) passes on the changed source file, and `tsc --noEmit` passes for `packages/core`. ESLint ignores the test file, so it wasn't linted.
**Separate, not changed:** a few other spots call `String()` on a value the handler passes in:
- `pluginContext.ts:171`: the `ctx.log` message
- `pluginContext.ts:197`: the connection name in an error
- `pluginHttp.ts:407`: `init.method` in an error
- `packages/plugins/testing/index.ts:147`: the test harness's log capture
These aren't the same defect. They throw a `TypeError` back to the handler during its own call, and the runner, now fixed, still turns that into a failed outcome. At most, the handler gets a `TypeError` where it would have got a `PluginSdkError`. `pluginContext.ts:229` converts a database error from recording a call, not something the handler throws.
PR: #3573
Comment by: @integry (ID: 6893865246)
Model: Claude Opus 5.5
…dlers-sdk-types-20261114-1908-nxn ✅⑧ [3548 by Claude Opus 5.5] Plugin handlers: SDK types, scoped ProPR client, outbound HTTP, plugin state and the core-import lint rule
propr-dev
Bot
changed the title
…-side dispatcher reads the event journal, retries failed handlers, tracks plugin health, and records every delivery in an Activity list on the Plugins page. All the acceptance criteria are covered by new tests, and everything I ran passes: 265 server-side tests, 53 UI component tests and 4 Playwright end-to-end tests. Typecheck and the zero-warning lint checks are clean too. I didn't run the full repository test suite or a docs site build, so docs links are unchecked.
Subscriptions. The manifest format is now `{ on: '<domain>.<type>', handler, filter? }`. `on` must be a real journal event, and `filter` matches event fields or `payload.<key>` values. This replaces the old placeholder `{ event }` shape, so I updated the 12 plugin-check test manifests that still used it. Each handler call now carries a stable `deliveryId` (`evt-<seq>-<handler>`) and the `attempt` number.
Delivery.
- Cursors and order: each plugin keeps a cursor per repository. Events for one repository reach a plugin in order, one at a time; a delivery waiting on a retry holds back only its own repository.
- Retries: a failed or timed-out (30 s) handler is retried after 1 min, 5 min, 30 min and 2 h. After the fifth attempt it is marked failed and the next event follows.
- Lease: only one worker delivers to a plugin at a time. If a worker stalls past its lease, the event is redelivered with the same id. That is why delivery is at least once.
- Gaps: events that left the journal before delivery are skipped, and the gap is recorded once.
- Health: after 3 failures in a row the plugin shows as Degraded, and administrators get one Inbox notification. After 10 (set by `PROPR_PLUGIN_PAUSE_AFTER_FAILURES`), deliveries pause until someone presses Resume.
The dispatcher starts from `src/daemon.ts`, and the hourly cleanup job that prunes the journal also deletes delivery records after 14 days.
Behaviour you might not expect:
- Enabling a plugin again starts its subscriptions fresh, the same as installing it. Events that happened while it was disabled are never delivered.
- A manual Retry gets exactly one attempt, outside the repository's normal order. While deliveries are paused, the retry waits for Resume.
- Retry and Resume are recorded in the plugin's audit trail, using two new audit actions.
API and UI. Three new admin-only routes live in `packages/api/routes/pluginActivityRoutes.ts`: list activity, retry a delivery, and resume. I regenerated the OpenAPI spec and client types. The plugin detail page now has a working Activity list with Retry and a Refresh button, plus Degraded or Paused badges and a Resume button. To keep the end-to-end spec under the 400-line limit, I moved its shared mock API into `propr-ui/e2e/plugins.fixture.ts`. Preview screenshots of the Degraded and Paused states, desktop and phone, are attached separately.
Acceptance criteria
- AC1: met — `packages/core/src/plugins/pluginDispatcher.ts`; `packages/core/test/pluginDispatcher.test.ts` covers: runs once per matching event, in order per repository, with filters and the repository limit; the retry schedule then failed; the cursor surviving a restart, and a crashed attempt redelivered with the same id; history not replayed.
- AC2: met — `packages/core/test/pluginDispatcher.test.ts` ("a handler on pull_request.merged that throws, hangs or exhausts its retries never affects the merge step") drives the real `recordPullRequestOutcome` merge step.
- AC3: met — `packages/core/test/pluginDispatcher.test.ts` ("two dispatchers running at once deliver each event once (lease)"). Breaking the lease makes this test and the restart test fail.
- AC4: met — `packages/core/src/plugins/pluginDeliveries.ts`, `pluginDispatcher.ts`; `packages/core/test/pluginDispatcher.test.ts` ("repeated failures make the plugin Degraded with one notification, then pause it until Resume").
- AC5: met — `packages/api/routes/pluginActivityRoutes.ts`, `packages/api/test/pluginActivityRoutes.test.ts`, `propr-ui/src/pages/PluginsPage/PluginActivity.tsx`, `PluginActivity.test.tsx`, `propr-ui/e2e/plugin-activity.pw.ts`.
- AC6: met — `docs/docs/features/plugins.md` (Event subscriptions: events and payloads, delivery guarantees and limits, health, Activity) and `docs/docs/architecture/plugin-runtime.md` (Event dispatcher); the pause setting is also listed in `docs/docs/operations/configuration-reference.md`.
propr-dev
Bot
changed the title
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ProPR 🔨 9 of 21 issues merged · #3550 in progress (Ultrafix cycle 3 of 10, last review 6/10) · updated 19:44 UTC
Plugin system: manifest, handlers, commands and official plugins
This Epic PR aggregates all changes for the plan: Plugin system: manifest, handlers, commands and official plugins
Issues in this Epic
Auto-close
When this PR is merged, the following issues will be automatically closed:
Fixes #3542
Fixes #3543
Fixes #3544
Fixes #3545
Fixes #3546
Fixes #3547
Fixes #3548
Fixes #3549
Fixes #3550
Fixes #3551
Fixes #3552
Fixes #3553
Fixes #3554
Fixes #3555
Fixes #3556
Fixes #3557
Fixes #3558
Fixes #3559
Fixes #3560
Fixes #3561
Created automatically by ProPR