Repository navigation
[3548 by Claude Opus 5.5] Plugin handlers: SDK types, scoped ProPR client, outbound HTTP, plugin state and the core-import lint rule - #3573
Conversation
![propr-dev[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
…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.
|
/ultrafix goal=8 max=10 |
|
🔄 Ultrafix loop started (goal: 8/10, max cycles: 10) First action:
|
|
✅ AI Code Review Complete requested by @propr-ultrafix Posted 1 review: |
🔍 AI Code Review — codex:gpt-6-astraOverall EvaluationThis PR adds the handler SDK, runtime, scoped clients, persistent state, fixtures, and documentation. It needs changes before merge: permission enforcement, credential redaction, and handler lifecycle cleanup have concrete gaps. ✅ Repository scoping — Explicit repository arguments are checked before backend calls, and ✅ Pinned HTTP destinations — The transport dials the vetted public address, preserves the original TLS identity, and does not follow redirects. ✅ Focused coverage — Added fixtures exercise success, exceptions, asynchronous hangs, permission refusals, state limits, and basic credential echoes. This review uses static analysis of the supplied complete diff; no commands were run. The authoritative checks report 30 passed, 32 neutral/skipped, four pending, and no failures. Merge blockersEvery finding below was introduced by this PR and must be resolved before merging. F1: 🔴 Comment permissions do not enforce target type
F2: 🔴 JSON escaping bypasses credential scrubbing
F3: 🔴 HTTP requests can start after timeout
F4: 🔴 Active handlers can recreate uninstalled state
SuggestionsThese are optional follow-ups and are not sent to S1: 🟢 Align test-kit repository checks
S2: 🟢 Exercise compiled handler loadingThe registry test explicitly exercises the source ScoreThe implementation covers substantial functionality, but permission bypasses, credential exposure, and lifecycle gaps require correction. Current checks show no failures, although four remain pending. Score: 5/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
|
✅ Applied the requested follow-up changes in commit 85d63e8
Summary of ChangesI'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 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
F2 – JSON-escaped credentials. The list of credential spellings to scrub now includes the JSON-escaped form, plus a variant with
F3 – HTTP requests after timeout. The handler's abort signal now reaches
F4 – state recreated after uninstall. Inside its transaction, a
I updated Two things I noticed but didn't change, since they're outside these findings:
Review digestRisk-tiered paths
Validation evidence
Diff size: 11 files changed, +379/−27 lines 🤖 Implementation Details
💡 ProPR Slash Commands
🤖 Generated by ProPR |
… 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
|
✅ AI Code Review Complete requested by @propr-ultrafix Posted 1 review: |
🔍 AI Code Review — codex:gpt-6-astraOverall EvaluationThis PR adds the handler SDK, runtime, scoped ProPR client, HTTP transport, persistent state, documentation, and fixtures. It needs changes before merge: installation isolation and timeout fencing remain incomplete in several mutation paths. ✅ Explicit permission checks — Operations enforce granted permissions and repository limits; GitHub issue/comment operations also verify the target kind. ✅ Constrained HTTP transport — Requests resolve and validate addresses before dialing, retain the original TLS identity, and cancel through the handler signal. ✅ Focused regression coverage — Tests cover handler failures, HTTP credential echoes, permission refusals, and state writes after uninstall. The authoritative checks report 34 passed, none failed, and one pending. This review uses static analysis of the supplied code; no commands were run. Merge blockersEvery finding below was introduced by this PR and must be resolved before merging. F5: 🔴 Fence every state operation by installation
F6: 🔴 Recheck cancellation before queue insertion
F7: 🔴 Recheck state cancellation before mutation
SuggestionsThese are optional follow-ups and are not sent to S3: 🟢 Match test-kit task scoping
ScoreThe implementation has substantial coverage and clear boundaries, but the remaining lifecycle defects can delete replacement state or perform mutations after timeout. These require localized fixes before merge. Score: 6/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
|
✅ Applied the requested follow-up changes in commit d30665f
Summary of ChangesI 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 F5: every state operation is now tied to the installation (
F7: state cancellation is checked again before and after each change
F6: no task is queued after cancellation (
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. Review digestRisk-tiered paths
Validation evidence
Diff size: 6 files changed, +149/−28 lines 🤖 Implementation Details
💡 ProPR Slash Commands
🤖 Generated by ProPR |
…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
|
✅ AI Code Review Complete requested by @propr-ultrafix Posted 1 review: |
🔍 AI Code Review — codex:gpt-6-astraOverall EvaluationThe PR adds the handler SDK, runner, scoped clients, persistent state, documentation, and substantial fixture coverage. It needs changes before merge because credential isolation has two remaining gaps. ✅ Permission enforcement — ✅ Cancellation coverage — HTTP and GitHub operations recheck cancellation after asynchronous setup, and state operations check cancellation before committing. ✅ State lifecycle protection — State access checks installation identity, preventing an old handler from accessing a later installation’s state. This review is based on static analysis of the supplied code; no commands were run. Current checks show no failures and five pending. Merge blockersEvery finding below was introduced by this PR and must be resolved before merging. F8: 🔴 Escaped response headers expose credentials
F9: 🔴 Old runs can use reinstalled credentials
SuggestionsThese are optional follow-ups and are not sent to S4: 🟢 Match test-kit task scoping
ScoreThe implementation has useful coverage and clear separation of responsibilities, but the two credential-boundary defects require correction. Pending checks should also complete before merging. Score: 6/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
|
✅ Applied the requested follow-up changes in commit 3c22120
Summary of ChangesI 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 F8: credentials hidden in escaped response headers (
F9: old runs using a reinstalled plugin's credentials (
Found along the way, not fixed (outside F8/F9):
Review digestRisk-tiered paths
Validation evidence
Diff size: 6 files changed, +112/−10 lines 🤖 Implementation Details
💡 ProPR Slash Commands
🤖 Generated by ProPR |
… 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
|
✅ AI Code Review Complete requested by @propr-ultrafix Posted 1 review: |
🔍 AI Code Review — codex:gpt-6-astraOverall EvaluationThis PR adds the handler SDK, runner, scoped ProPR client, outbound HTTP, persistent state, documentation, and fixtures. It is not ready to merge: credential-response filtering and authorization after uninstall need correction. ✅ Network destination checks — HTTP requests validate HTTPS, allowed hosts, and public DNS results, then dial the vetted address without following redirects. ✅ State lifecycle protection — State operations check installation identity inside transactions, and the added tests cover uninstall/reinstall and cancellation rollback. ✅ Permission separation — Issue and pull-request comment operations verify the target kind before posting. This review is based on static tracing of the supplied code; no commands were run. The authoritative checks report 32 passed, 2 pending, and no failures. Merge blockersEvery finding below was introduced by this PR and must be resolved before merging. F10: 🔴 Escaped credentials survive response-body filtering
F11: 🔴 Uninstalled handlers retain ProPR authority
SuggestionsThese are optional follow-ups and are not sent to S5: 🟢 Clarify test-kit fidelity
ScoreThe implementation has substantial coverage and useful lifecycle protections, but two security-boundary defects remain. Pending checks alone do not affect this assessment. Score: 5/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
|
✅ Applied the requested follow-up changes in commit ca5eb57
Summary of ChangesI fixed F10 and F11 and added regression tests for both. With the fixes, all the plugin tests pass: 25 in the handler, F10: credentials escaped in response bodies (
F11: uninstalled handlers keeping
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. Docs: Separate findings, not changed:
There are no visual changes, so I didn't create previews. Review digestRisk-tiered paths
Validation evidence
Diff size: 7 files changed, +235/−34 lines 🤖 Implementation Details
💡 ProPR Slash Commands
🤖 Generated by ProPR |
…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
|
✅ AI Code Review Complete requested by @propr-ultrafix Posted 1 review: |
🔍 AI Code Review — codex:gpt-6-astraOverall EvaluationThis PR adds the handler SDK, scoped clients, HTTP transport, installation state, test helpers, and documentation. It needs changes before merge because external mutations can occur without a durable audit record. ✅ Destination checks — HTTP requests validate every resolved address and dial the vetted address while preserving the original TLS hostname. ✅ Installation fencing — State operations check installation identity inside their transactions, and lifecycle tests cover uninstall and reinstall. ✅ Permission separation — The GitHub backend checks issue versus pull-request targets before posting comments. Review was static; no commands were run. The supplied current-head status reports 29 passed, five pending, and no failed checks. Merge blockersEvery finding below was introduced by this PR and must be resolved before merging. F12: 🔴 Record authorization before external mutations
SuggestionsThese are optional follow-ups and are not sent to S6: 🟢 Match task scoping in the test kit
S7: 🟢 Verify concurrent state limits
ScoreThe implementation has substantial coverage and clear boundaries, but the audit ordering leaves a required correctness gap. Pending CI checks should also finish before merge. Score: 6/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
|
✅ Applied the requested follow-up changes in commit 9adc1ac
Summary of ChangesI fixed F12: How a mutating call works now (
Once the attempt is recorded, a failure to record the outcome is logged on the server ( 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 Other files changed:
Tests (in
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 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 There are no visual changes, so I made no previews. Review digestRisk-tiered paths
Validation evidence
Diff size: 8 files changed, +176/−30 lines 🤖 Implementation Details
💡 ProPR Slash Commands
🤖 Generated by ProPR |
…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
|
✅ AI Code Review Complete requested by @propr-ultrafix Posted 1 review: |
…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
propr-dev
Bot
changed the title
|
CI failed: Finalize unsigned validation checksums Please investigate and fix this CI failure.
Failure evidence
|
propr-dev
Bot
changed the title
|
✅ Applied the requested follow-up changes in commit 35afbab Summary of ChangesI 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
Cause: in
Fix (that file only):
Verification:
The other failing check, Review digestRisk-tiered paths
Validation evidence
Diff size: 1 file changed, +17/−5 lines 🤖 Implementation Details
Undo Changes • View Task Execution 💡 ProPR Slash Commands
🤖 Generated by ProPR |
…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
propr-dev
Bot
changed the title
|
CI failed: Packaged Connect (linux-arm64) Please investigate and fix this CI failure.
Failure evidence
|
propr-dev
Bot
changed the title
|
ℹ️ Analyzed the follow-up request by @propr-dev[bot] Analysis SummaryI looked into it. This failure isn't caused by the PR's code, and I didn't change any files. What failed: the job died during So the headers download on that runner was incomplete or got corrupted, and the native compile had nothing to work with. Why it isn't this PR:
What to do: re-run the job, or let the next push run it again. The headers download should succeed on a fresh runner. If the same No code changes were necessary based on the current state of the branch. 🤖 Analysis Details
💡 ProPR Slash Commands
🤖 Generated by ProPR |
propr-dev
Bot
changed the title
|
/review |
propr-dev
Bot
changed the title
|
✅ AI Code Review Complete requested by @integry Posted 1 review: |
🔍 AI Code Review — codex:gpt-6-astraOverall EvaluationThe PR implements the handler SDK, runtime, scoped clients, state storage, and documentation with substantial regression coverage. It needs a localized correction before merge: some thrown values escape the runner instead of producing the promised failed outcome. ✅ Mutation attribution — Mutations require a durable ✅ Authority rechecks — The default backend rechecks installation status, permissions, and repository scope after initializing GitHub or the queue. ✅ Lifecycle coverage — Added tests exercise timeout cancellation, uninstall/reinstall boundaries, and credential redaction. This review used the supplied code only; no commands were run. Current checks report 22 passed, 10 pending, 10 neutral/skipped, and no failures. Merge blockersEvery finding below was introduced by this PR and must be resolved before merging. F21: 🔴 Safely format non-Error throws
SuggestionsThese are optional follow-ups and are not sent to S18: 🟢 Clarify test-kit parityThe documentation says the fake applies the same host and state checks as production, but S19: 🟢 Extend import-rule coverageThe dynamic-import selectors in ScoreThe implementation is thorough, but the runner’s failure-handling contract has a concrete escape path that also bypasses context shutdown. The correction is localized; pending checks remain outstanding. Score: 6/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
propr-dev
Bot
changed the title
|
/fix |
propr-dev
Bot
changed the title
|
✅ Applied the requested follow-up changes in commit c259678
Summary of ChangesI fixed F21: Fix ( Regression test (
For each one it checks that the runner resolves with Verification:
Separate, not changed: a few other spots call
These aren't the same defect. They throw a Review digestRisk-tiered paths
Validation evidence
Diff size: 2 files changed, +50/−3 lines 🤖 Implementation Details
Undo Changes • View Task Execution 💡 ProPR Slash Commands
🤖 Generated by ProPR |
…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
propr-dev
Bot
changed the title
|
/review astra fable |
propr-dev
Bot
changed the title
|
✅ AI Code Review Complete requested by @integry Posted 2 reviews: |
🔍 AI Code Review — astraOverall EvaluationThe PR adds the handler SDK, runner, scoped ProPR operations, outbound HTTP, persistent state, and documentation. It is ready to merge within the supplied scope; this review found no verified merge blocker. ✅ Audited mutations — Mutations require a durable attempt record before dispatch, while completion-record failures do not incorrectly turn successful mutations into retryable failures. ✅ Lifecycle authorization — The default backend rechecks installation identity, enabled state, permissions, and repository limits after client or queue initialization. ✅ Focused regression coverage — Tests exercise timeout cancellation, uninstall/reinstall isolation, permission boundaries, and credential redaction across multiple encodings. Review was static, using the supplied diff and context; no commands were run. Current checks report 35 passed, 12 neutral/skipped, and no failures or pending checks. Merge blockersNo merge blockers. SuggestionsThese are optional follow-ups and are not sent to S20: 🟢 Clarify test-kit validation parity
S21: 🟢 Document cooperative timeout limits
ScoreThe implementation addresses the stated acceptance criteria with substantial regression coverage and passing current checks. Static review found no actionable defect; the remaining suggestions concern documentation and test-helper fidelity. Score: 8/10 🤖 Review Details
Copy and edit to choose records from this review: 🤖 Review by ProPR |
🔍 AI Code Review — fable🤖 Review Details
🤖 Review by ProPR |
propr-dev
Bot
changed the title
propr-dev
Bot
changed the title
AI Implementation Summary
Closes #3548
Branch:
3548/claude-opus-5-5-plugin-handlers-sdk-types-20261114-1908-nxnCommits: ✅ Changes committed (c7aad41)
AI Processing Completed
Execution Details:
Summary:
Nothing changes here either: the second copy of that wait loop also hit 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.
Review digest
Acceptance criteria (0 of 6 met, as stated by the agent)
With a fixture plugin: `runPluginHandler` runs a handler, returns its logs and outcome; a throwing handler and a handler that hangs past the timeout return a failed outcome without throwing.`ctx.propr` refuses operations outside the granted permissions and outside the repository limit; allowed operations are attributed to the plugin in the audit trail and task events.`ctx.http` refuses hosts not in `outboundHosts`, private addresses and plain HTTP; credentials are injected and never visible to the handler (sentinel test on handler-visible values and on logs).`ctx.state` enforces its size and key-count limits and is deleted on uninstall.The restricted-imports lint rule fails a fixture plugin that imports a core module.Docs: handler interface in `docs/docs/features/plugins.md` and a new `docs/docs/architecture/plugin-runtime.md`.Risk-tiered paths
packages/core/src/db/migrations/20261114180000_plugin_state.js(added, +20/−0): database migrationpackages/plugins/package.json(+7/−4): dependency manifesteslint.config.js(+37/−0)packages/client/src/generated/apiTypes.ts(+7/−7)packages/core/src/extensions/endpointPolicy.ts(+5/−1)packages/core/src/plugins/pluginContext.ts(added, +145/−0)packages/core/src/plugins/pluginHandlerRunner.ts(added, +117/−0)packages/core/src/plugins/pluginHttp.ts(added, +177/−0)packages/core/src/plugins/pluginProprBackend.ts(added, +100/−0)packages/core/src/plugins/pluginProprClient.ts(added, +143/−0)packages/core/src/plugins/pluginStateStore.ts(added, +70/−0)packages/core/src/plugins/pluginStore.ts(+4/−2)packages/core/src/queue/taskQueue.types.ts(+2/−0)packages/core/src/services/taskIntent.ts(+4/−20)packages/core/src/services/taskIntentIssueRef.ts(added, +25/−0)packages/core/src/utils/workerStateManager.ts(+3/−3)packages/core/src/utils/workerStateManager.types.ts(+5/−0)packages/plugins/example-pack/handlers.ts(added, +22/−0)packages/plugins/src/index.ts(+43/−2)packages/plugins/testing/index.ts(added, +142/−0)packages/plugins/tsconfig.handlers.json(added, +11/−0)packages/shared/src/index.ts(+1/−0)packages/shared/src/pluginInstallations.ts(+5/−1)packages/shared/src/pluginSdk.ts(added, +165/−0)packages/shared/src/plugins.ts(+3/−1)docs/docs/architecture/plugin-runtime.md(added, +226/−0)docs/docs/features/plugins.md(+112/−5)docs/sidebars.ts(+1/−0)docs/static/openapi/propr-api.yaml(+8/−0)packages/api/test/pluginRoutes.test.ts(+2/−1)packages/core/test/fixtures/plugins/fixture-handlers/handlers.ts(added, +68/−0)packages/core/test/fixtures/plugins/fixture-handlers/manifest.json(added, +24/−0)packages/core/test/fixtures/plugins/fixture-imports-core/handlers.ts(added, +12/−0)packages/core/test/fixtures/plugins/registry.json(added, +6/−0)packages/core/test/helpers/pluginHandlerFixture.ts(added, +92/−0)packages/core/test/pluginContributions.test.ts(+2/−1)packages/core/test/pluginHandlerLint.test.ts(added, +57/−0)packages/core/test/pluginHandlerRunner.test.ts(added, +165/−0)packages/core/test/pluginProprClient.test.ts(added, +151/−0)packages/core/test/pluginStore.test.ts(+2/−1)packages/core/test/taskJournalEvents.test.ts(+13/−0)packages/plugins/example-pack/README.md(+3/−0)packages/plugins/test/testing.test.ts(added, +61/−0)Validation evidence
Diff size: 43 files changed, +2,268/−49 lines
Detailed Logs:
2fc9a78c-6209-42be-bed8-82719e83f81eLog files stored at:
/usr/src/app/logs/executions/issue-3548-2026-11-14T19-58-35-980Z-conversation.json/usr/src/app/logs/executions/issue-3548-2026-11-14T19-58-35-980Z-output.txtLatest Conversation Messages
This PR was created automatically by ProPR after processing issue #3548.
💡 Need changes?
Comment on this PR to request refinements — the AI agent monitors comments and will update the implementation based on your feedback. Keep iterating until you're satisfied!