Conversation
yansigit
force-pushed
the
feat/provider-extension-system
branch
from
September 15, 2026 06:50
9dfc155 to
8e72099
Compare
yansigit
marked this pull request as ready for review
September 15, 2026 06:52
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+378
to
+390
| let len = u32::from_be_bytes([buf[1], buf[2], buf[3], buf[4]]) as usize; | ||
| let end = 5 + len; | ||
| if buf.len() < end { | ||
| return None; | ||
| return Ok(None); | ||
| } | ||
| let mut payload = buf[5..end].to_vec(); | ||
| if flag & 0x01 != 0 { | ||
| // gzip-compressed payload | ||
| let mut decoded = Vec::new(); | ||
| if GzDecoder::new(&payload[..]) | ||
| GzDecoder::new(&payload[..]) | ||
| .read_to_end(&mut decoded) | ||
| .is_ok() | ||
| { | ||
| payload = decoded; | ||
| } | ||
| .context("Invalid gzip payload in Cursor agent stream")?; | ||
| payload = decoded; |
There was a problem hiding this comment.
Cursor response frames accept any declared u32 length, and gzip payloads expand into an unbounded buffer. A remote endpoint can make jcode retain a huge incomplete frame or expand a small compressed response until the process runs out of memory before the turn timeout applies. Reject oversized wire frames and stop decompression after a fixed decoded-size limit.
How this was verified: A response fixture expanded 16,329 compressed bytes into 16 MiB without any frame or decoded-output limit.
Knowledge Base Used: Provider selection and runtime adapters
Artifacts
- Authored executable harness copies the reviewed parser body, builds a compressed response frame and an oversized length header, then runs both cases; it provides the narrow runtime reproduction.
- Captured command output shows 16,329 compressed bytes expanding to 16,777,216 bytes and no rejection of a maximum u32 header, confirming unbounded response handling.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/jcode-provider-cursor-runtime/src/agent_transport.rs
Line: 378-390
Comment:
**Bound Cursor response frames**
Cursor response frames accept any declared `u32` length, and gzip payloads expand into an unbounded buffer. A remote endpoint can make jcode retain a huge incomplete frame or expand a small compressed response until the process runs out of memory before the turn timeout applies. Reject oversized wire frames and stop decompression after a fixed decoded-size limit.
**How this was verified:** A response fixture expanded 16,329 compressed bytes into 16 MiB without any frame or decoded-output limit.
**Knowledge Base Used:** [Provider selection and runtime adapters](https://app.greptile.com/solo-systems/-/custom-context/knowledge-base/1jehuang/jcode/-/docs/provider-selection-and-runtime-adapters.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+925
to
+943
| 3 if field.wire == 2 => { | ||
| let mut id = None; | ||
| let mut blob = None; | ||
| for f in crate::wire::iter_fields(field.data) { | ||
| if f.field == 1 && f.wire == 2 { | ||
| id = Some(f.data.to_vec()); | ||
| } else if f.field == 2 && f.wire == 2 { | ||
| blob = Some(f.data.to_vec()); | ||
| } | ||
| } | ||
| if let (Some(id), Some(blob)) = (id, blob) { | ||
| set_blob = Some((id, blob)); | ||
| } | ||
| } | ||
| _ => {} | ||
| } | ||
| } | ||
| if let Some((id, blob)) = set_blob { | ||
| blob_store.insert(id, blob); |
There was a problem hiding this comment.
Each remote SetBlob message copies its caller-controlled identifier and body into a per-turn map without limits on blob size, entry count, or total retained bytes. A remote endpoint can send unique blobs until jcode exhausts memory. Enforce a bounded blob-store budget and reject or evict data that exceeds it.
How this was verified: A focused wire fixture retained 128 unique 4 KiB remote blobs, while the storage path has no size, count, or total-memory check.
Knowledge Base Used: Provider selection and runtime adapters
Artifacts
- Focused harness reads the active storage branch and sends nested remote SetBlob wire messages with duplicate and unique identifiers.
Duplicate blob retention output
- Execution with repeated identifiers retained one 4 KiB blob, establishing the duplicate-key comparison.
- Execution with 128 unique identifiers retained 128 bodies totaling 524,288 bytes, demonstrating unbounded growth with unique keys.
- Captured source assertions show the active SetBlob branch inserts remote identifiers and bodies without a configured bound or eviction path.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/jcode-provider-cursor-runtime/src/agent_transport.rs
Line: 925-943
Comment:
**Bound retained Cursor blobs**
Each remote `SetBlob` message copies its caller-controlled identifier and body into a per-turn map without limits on blob size, entry count, or total retained bytes. A remote endpoint can send unique blobs until jcode exhausts memory. Enforce a bounded blob-store budget and reject or evict data that exceeds it.
**How this was verified:** A focused wire fixture retained 128 unique 4 KiB remote blobs, while the storage path has no size, count, or total-memory check.
**Knowledge Base Used:** [Provider selection and runtime adapters](https://app.greptile.com/solo-systems/-/custom-context/knowledge-base/1jehuang/jcode/-/docs/provider-selection-and-runtime-adapters.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+143
to
+161
| record.manifest.validate()?; | ||
| for permission in &record.manifest.permissions { | ||
| if !policy.allows(permission) { | ||
| return Err(ExtensionError::PermissionDenied { | ||
| provider: record.manifest.id.clone(), | ||
| permission: permission.clone(), | ||
| }); | ||
| } | ||
| } | ||
| let transport = SubprocessProvider::spawn_with_config( | ||
| &record.manifest.executable, | ||
| &record.manifest.args, | ||
| client, | ||
| record.manifest.capabilities.clone(), | ||
| jcode_provider_subprocess::SubprocessConfig { | ||
| environment: Some(minimal_environment()), | ||
| ..Default::default() | ||
| }, | ||
| ) |
There was a problem hiding this comment.
The permission check only evaluates capabilities that an extension declares. An extension that omits filesystem starts with ordinary host process authority, so it can still access files despite the --allow-* boundary. Enforce granted capabilities at process launch, or do not present these flags as a security boundary.
How this was verified: An extension with no filesystem declaration successfully wrote a host file, while the same extension declaring filesystem access was rejected.
Knowledge Base Used: Provider selection and runtime adapters
Artifacts
Undeclared filesystem authority fixture
- Authored Rust fixture starts real provider subprocesses and makes the omitted-permission provider write a host file, demonstrating the authority comparison.
- Authored shell command materializes the standalone fixture, executes it from the repository, and captures its actual output.
Declared filesystem permission output
- Executed baseline shows the default policy rejected the filesystem-declaring provider and the target file remained absent, establishing the intended boundary.
Omitted filesystem permission output
- Executed comparison shows the empty-permission manifest started and wrote the host file through the real provider request path, confirming the bypass.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/jcode-provider-extensions/src/lib.rs
Line: 143-161
Comment:
**Enforce extension permissions**
The permission check only evaluates capabilities that an extension declares. An extension that omits `filesystem` starts with ordinary host process authority, so it can still access files despite the `--allow-*` boundary. Enforce granted capabilities at process launch, or do not present these flags as a security boundary.
**How this was verified:** An extension with no filesystem declaration successfully wrote a host file, while the same extension declaring filesystem access was rejected.
**Knowledge Base Used:** [Provider selection and runtime adapters](https://app.greptile.com/solo-systems/-/custom-context/knowledge-base/1jehuang/jcode/-/docs/provider-selection-and-runtime-adapters.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+196
to
+209
| let manifest_candidates = [ | ||
| root.join("plugin.json"), | ||
| root.join(".codex-plugin").join("plugin.json"), | ||
| root.join(".claude-plugin").join("plugin.json"), | ||
| ] | ||
| .into_iter() | ||
| .filter(|path| path.is_file()) | ||
| .collect::<Vec<_>>(); | ||
| let manifest_path = manifest_candidates | ||
| .first() | ||
| .cloned() | ||
| .ok_or(BundleError::MissingManifest)?; | ||
|
|
||
| let manifest_bytes = read_bounded(&manifest_path, MAX_PLUGIN_MANIFEST_BYTES).map_err( |
There was a problem hiding this comment.
Bundle inspection canonicalizes the bundle root but follows symlinks in repository-controlled metadata and skill paths without checking that their resolved targets remain inside that root. A crafted plugin bundle can make inspection read host files outside quarantine and include their parseable metadata in output or installation decisions. Canonicalize every inspected child path and reject targets outside the bundle root.
How this was verified: Inspection of bundles with symlinked metadata and skills returned sentinel names and descriptions stored outside each bundle root.
Artifacts
Bundle symlink fixture command
- Creates an in-root control bundle plus manifest and skills child-symlink bundles, then executes the real jcode CLI inspector; it demonstrates the reproducible validation procedure.
- Archive of the executed fixture containing child symlinks and their external metadata targets, showing the inspected files were outside the individual bundle roots.
Control bundle inspection output
- Real CLI output from inspecting the control bundle reports only in-root manifest and skill sentinel values, establishing the comparison baseline.
Symlinked bundle inspection output
- Real CLI output from inspecting the attack bundles reports external manifest and skill sentinel metadata, confirming inspection read beyond the bundle root.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/jcode-provider-extensions/src/bundle.rs
Line: 196-209
Comment:
**Contain bundle inspection paths**
Bundle inspection canonicalizes the bundle root but follows symlinks in repository-controlled metadata and skill paths without checking that their resolved targets remain inside that root. A crafted plugin bundle can make inspection read host files outside quarantine and include their parseable metadata in output or installation decisions. Canonicalize every inspected child path and reject targets outside the bundle root.
**How this was verified:** Inspection of bundles with symlinked metadata and skills returned sentinel names and descriptions stored outside each bundle root.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+235
to
+264
| self.request(id.clone(), method, params).await?; | ||
| let deadline = tokio::time::Instant::now() + self.config.request_timeout; | ||
| let mut frames = Vec::new(); | ||
| loop { | ||
| let remaining = deadline.saturating_duration_since(tokio::time::Instant::now()); | ||
| if remaining.is_zero() { | ||
| return Err(AdapterError::Timeout); | ||
| } | ||
| let frame = self.next_with_timeout(Some(remaining)).await?; | ||
| match &frame { | ||
| Frame::Event { request_id, .. } if request_id == &id => frames.push(frame), | ||
| Frame::Response { | ||
| id: response_id, .. | ||
| } if response_id == &id => { | ||
| frames.push(frame); | ||
| return Ok(frames); | ||
| } | ||
| Frame::HelloOk { .. } => { | ||
| return Err(AdapterError::UnexpectedFrame( | ||
| "received hello_ok after handshake".into(), | ||
| )); | ||
| } | ||
| Frame::Event { request_id, .. } => { | ||
| return Err(AdapterError::UnexpectedFrame(format!( | ||
| "event for another request while waiting for {id}: {request_id}" | ||
| ))); | ||
| } | ||
| other => { | ||
| return Err(AdapterError::UnexpectedFrame(format!( | ||
| "unexpected frame while waiting for {id}: {other:?}" |
There was a problem hiding this comment.
Concurrent requests against one persistent provider independently read its shared stdout stream. A response intended for one request is treated as an unexpected frame by the other caller, so both valid requests can fail and leave the provider stream out of sync. Serialize complete request collection or route frames through one reader keyed by request ID.
Knowledge Base Used: Provider selection and runtime adapters
Artifacts
Concurrent provider routing harness
- Authored shell harness creates a temporary Rust integration test, runs sequential and concurrent two-ID flows against a persistent Python JSONL provider, and removes the temporary fixture.
Sequential provider routing output
- Sequential requests each returned a successful response carrying their own ID, establishing the same-process baseline.
Concurrent provider routing output
- Concurrent requests consumed one another's responses and both returned `UnexpectedFrame`, confirming broken response routing.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/jcode-provider-subprocess/src/lib.rs
Line: 235-264
Comment:
**Route concurrent provider responses**
Concurrent requests against one persistent provider independently read its shared stdout stream. A response intended for one request is treated as an unexpected frame by the other caller, so both valid requests can fail and leave the provider stream out of sync. Serialize complete request collection or route frames through one reader keyed by request ID.
**Knowledge Base Used:** [Provider selection and runtime adapters](https://app.greptile.com/solo-systems/-/custom-context/knowledge-base/1jehuang/jcode/-/docs/provider-selection-and-runtime-adapters.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+567
to
+591
| pub fn save(&self) -> Result<(), ExtensionError> { | ||
| if let Some(parent) = self.path.parent() { | ||
| fs::create_dir_all(parent).map_err(|source| ExtensionError::Write { | ||
| path: parent.to_path_buf(), | ||
| source, | ||
| })?; | ||
| } | ||
| let bytes = serde_json::to_vec_pretty(&RegistryFile { | ||
| version: REGISTRY_VERSION, | ||
| providers: self.providers.clone(), | ||
| })?; | ||
| let mut encoded = bytes; | ||
| encoded.push(b'\n'); | ||
|
|
||
| let nonce = SystemTime::now() | ||
| .duration_since(UNIX_EPOCH) | ||
| .map_or(0, |duration| duration.as_nanos()); | ||
| let temp = self | ||
| .path | ||
| .with_extension(format!("json.tmp.{}.{}", std::process::id(), nonce)); | ||
| fs::write(&temp, encoded).map_err(|source| ExtensionError::Write { | ||
| path: temp.clone(), | ||
| source, | ||
| })?; | ||
| if let Err(source) = fs::rename(&temp, &self.path) { |
There was a problem hiding this comment.
Provider registry updates use independent in-memory snapshots with no cross-process transaction lock. Two CLI or TUI operations can both load the old registry and save different mutations, causing the later save to silently erase the earlier change. Lock the complete open, mutate, and save transaction, or use conflict-aware transactional storage.
Artifacts
Provider registry concurrency harness
- Authored Rust harness synchronizes two child processes after both have opened the same registry, then records the final provider states to demonstrate a lost update.
Provider registry concurrency command
- Authored shell command builds the harness against the checked-out provider extensions crate and captures sequential and concurrent runs.
Sequential registry update output
- Sequential control shows both alpha and beta changed from enabled to disabled, establishing the expected non-racing behavior.
Concurrent registry update output
- Synchronized processes show alpha remained enabled while beta became disabled, proving one independent update was overwritten.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/jcode-provider-extensions/src/lib.rs
Line: 567-591
Comment:
**Lock registry updates**
Provider registry updates use independent in-memory snapshots with no cross-process transaction lock. Two CLI or TUI operations can both load the old registry and save different mutations, causing the later save to silently erase the earlier change. Lock the complete open, mutate, and save transaction, or use conflict-aware transactional storage.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+347
to
+351
| store.remove(name)?; | ||
| if let Some(provider_id) = provider_id { | ||
| if owned { | ||
| registry.remove(&provider_id)?; | ||
| registry.save()?; |
There was a problem hiding this comment.
Plugin removal deletes the installed plugin before removing and saving its owned provider registration. If registry persistence fails, the plugin is already gone while its registration remains; a later retry cannot discover the deleted plugin's provider and reports success without cleaning it up. Make the store and registry transition recoverable or persist the registry change before deleting the plugin.
Artifacts
Plugin registry save failure reproduction
- The executed script creates isolated installed-plugin and provider-registry fixtures, injects the registry rename failure, compares normal and failed removal, and retries removal.
- Normal plugin removal exited successfully and its captured postcondition shows both the plugin directory and provider registration absent.
Failed registry save removal output
- The injected registry rename failure deleted the plugin while retaining the provider registration, and the recorded retry left that registration behind.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/cli/commands/plugins.rs
Line: 347-351
Comment:
**Make plugin removal atomic**
Plugin removal deletes the installed plugin before removing and saving its owned provider registration. If registry persistence fails, the plugin is already gone while its registration remains; a later retry cannot discover the deleted plugin's provider and reports success without cleaning it up. Make the store and registry transition recoverable or persist the registry change before deleting the plugin.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+160
to
+161
| if matches.first().is_some_and(|command| command.start > 0) || matches.len() > 1 { | ||
| return dispatch_command_sequence(app, trimmed, &matches); |
There was a problem hiding this comment.
Submitting /btw explain /help output is split into two local commands rather than sent as one prompt. The first command forks a session using only explain, the second reports Unknown command 'output', and no normal user turn starts. This loses the intended request and unexpectedly changes session state; only explicit command-sequence syntax should dispatch commands after the first token.
Knowledge Base Used: Terminal user interface
Artifacts
- The executed shell script runs the narrow TUI submission test and writes the command, working directory, exit code, and complete output; it provides the exact reproducible invocation.
- The uploaded executed test source submits both supplied examples through the real App input path and asserts their user-visible state consequences; it shows `/quit` is preserved while the `/btw` plus `/help` input is consumed.
- The full observed Cargo output records the command, `/home/user/repo` working directory, exit code 0, and the single passing test; it confirms the asserted dispatch behaviors executed successfully.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/jcode-tui/src/tui/app/commands_dispatch.rs
Line: 160-161
Comment:
**Keep prompt text intact**
Submitting `/btw explain /help output` is split into two local commands rather than sent as one prompt. The first command forks a session using only `explain`, the second reports `Unknown command 'output'`, and no normal user turn starts. This loses the intended request and unexpectedly changes session state; only explicit command-sequence syntax should dispatch commands after the first token.
**Knowledge Base Used:** [Terminal user interface](https://app.greptile.com/solo-systems/-/custom-context/knowledge-base/1jehuang/jcode/-/docs/terminal-user-interface.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
yansigit
force-pushed
the
feat/provider-extension-system
branch
from
September 16, 2026 01:56
eb7f4b0 to
07a7cca
Compare
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
1 participant
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.
Summary
Add an upgrade-safe two-tier extension system for jcode, including external provider execution, explicit lifecycle controls, portable bundle inspection, an embedded Rust tier, and metadata-only GitHub plugin installation.
Refs #745
Depends on #1260
Design
/plugin inspect,add,update,list,doctor,trust, andremovelifecycle.Review structure
This branch now contains only 12 focused protocol/extension/plugin commits. The earlier 69-commit assembled branch, which also contained Cursor transport, slash commands, account imports, model-picker changes, and storage work, is preserved separately as
yansigit:integration/provider-extension-system-alland is not proposed for merge.The two protocol foundation commits are included to keep this branch buildable before #1260 merges. Once #1260 lands, they can be dropped with a narrow rebase so the review shows only extension registry, bundle, embedded-tier, and GitHub lifecycle changes.
Validation
upstream/masterusing only the intended protocol and extension commits.scripts/dev_cargo.sh check -p jcode --bin jcodepassed on the focused 12-commit branch.obra/superpowersmetadata-only inspect/add/list/doctor/remove lifecycle previously passed in an isolated home.