feat(plugin-sdk): add LLM completion API to plugin#64294
Conversation
Greptile SummaryThis PR adds Confidence Score: 5/5Safe to merge — the new LLM and sandbox APIs are correctly gated by existing policy checks and use proven lazy-loading patterns; all remaining findings are P2 style suggestions. Both APIs correctly delegate policy enforcement to existing mechanisms (isToolAllowed for sandbox, agent auth for LLM). The process-wide capability caching is sound because the factories are stateless (config is read inside each call closure). No correctness, data-integrity, or security defects were found. The two flagged items are a codebase-style rule violation (dynamic vs. static import mixing) and a missing test isolation mock — neither affects runtime behavior. src/agents/pi-embedded-runner/compact.ts (dynamic import inconsistency) and src/agents/pi-embedded-runner/run/attempt.test.ts (missing capabilities mock). Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/agents/pi-embedded-runner/compact.ts
Line: 1329-1331
Comment:
**Dynamic import mixed with static imports of the same module**
`context-engine-capabilities.js` is imported statically in `context-engine-maintenance.ts` and `attempt.prompt-helpers.ts`, but dynamically here in `compact.ts`. The repo's dynamic-import guardrail in `CLAUDE.md` prohibits mixing `await import("x")` and `import ... from "x"` for the same module in production code paths.
Since the static imports elsewhere already pull this module into the bundle at load time, the dynamic import here buys no laziness. Convert to a static import at the top of `compact.ts` to stay consistent:
```suggestion
...(await resolveContextEngineCapabilities()),
```
(With a corresponding static `import { resolveContextEngineCapabilities } from "./context-engine-capabilities.js";` added at the top of the file.)
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/agents/pi-embedded-runner/run/attempt.test.ts
Line: 1895
Comment:
**Missing mock for `context-engine-capabilities.js`**
`buildAfterTurnRuntimeContext` now calls `resolveContextEngineCapabilities()`, which dynamically imports the real `runtime-llm.runtime.ts` and `runtime-sandbox.runtime.ts` modules on its first invocation during these tests. `context-engine-maintenance.test.ts` correctly guards against this with a `vi.mock` stub, but this file does not. Add the same mock near the top of this file so tests stay isolated from those runtime surfaces:
```ts
vi.mock("../context-engine-capabilities.js", () => ({
resolveContextEngineCapabilities: async () => ({ llm: undefined, sandbox: undefined }),
}));
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat(plugin-sdk): add LLM completion and..." | Re-trigger Greptile |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d68dc4563d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc1c91ee1d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Codex review: needs real behavior proof before merge. Summary Reproducibility: not applicable. this is a feature PR, not a bug report. Source inspection shows current main lacks the LLM runtime API while the PR head adds the SDK, runtime, context-engine, docs, config, and test surfaces. Real behavior proof Next step before merge Security Review detailsBest possible solution: Land this PR or a maintainer-owned replacement once live proof and API approval are in place, preserving the request-scoped identity and config-gated override policy. Do we have a high-confidence way to reproduce the issue? Not applicable: this is a feature PR, not a bug report. Source inspection shows current main lacks the LLM runtime API while the PR head adds the SDK, runtime, context-engine, docs, config, and test surfaces. Is this the best way to solve the issue? Unclear as a merge decision: the implementation direction is plausible and latest checks are green, but the PR still needs real behavior proof and maintainer approval for the public API/config shape. Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 6cfb08680e71. |
78509a3 to
3042195
Compare
…runtime Signed-off-by: DaevMithran <daevmithran1999@gmail.com>
Signed-off-by: DaevMithran <daevmithran1999@gmail.com>
Signed-off-by: DaevMithran <daevmithran1999@gmail.com>
|
Merged via squash.
Thanks @DaevMithran! |
Summary
api.runtime.llm.complete()for single-shot LLM inference onPluginRuntimeCore. ExtendedContextEngineRuntimeContextso context engines receive these capabilities incompact(),afterTurn(), andmaintain().Change Type
Scope (select all touched areas)
User-visible / Behavior Changes
api.runtime.llm.complete()— single-shot LLM completion using the agent's auth and model config.runtimeContext.llmincompact(),afterTurn(), andmaintain()hooks.PluginLlmCompleteParams,PluginLlmCompleteResult,PluginLlmCompleteMessage.Diagram (if applicable)
N/A
Security Impact (required)
Yes— plugins can now invoke LLM completions.No— reuses existing agent auth profiles.No— uses the same model inference path.NoCompatibility / Migration
YesNoNo