iframe-proxy

eljese · 2026-10-21T05:58:30Z

discoverPiModelsCached forked one pi --list-models spawn per concurrent caller, used a fixed 20s timeout and 60s TTL, keyed the cache by cwd even though listing output is cwd-independent, and failed callers hard on any spawn error.

Single-flight: concurrent same-key discoveries share one in-flight spawn; failures are never cached (entry dropped in finally).
Configurable timeout/TTL via PAPERCLIP_PI_MODELS_TIMEOUT_MS (clamped 5s..300s, default 45s) and PAPERCLIP_PI_MODELS_CACHE_TTL_MS (0 disables caching, default 60s, max 1h); typo-safe fallbacks to defaults.
Cache key excludes cwd (verified identical output across cwds).
Opt-in stale fallback (allowStaleOnFailure): serve the last expired entry instead of throwing; default stays fail-closed; listPiModels stays best-effort. Covered by hermetic mock-spawn cases plus the real-spawn catalogue-wording case.

Verified on exact tree: packages/adapters/pi-local src/server/models.test.ts 18/18 green.

…ack) discoverPiModelsCached forked one pi --list-models spawn per concurrent caller, used a fixed 20s timeout and 60s TTL, keyed the cache by cwd even though listing output is cwd-independent, and failed callers hard on any spawn error. - Single-flight: concurrent same-key discoveries share one in-flight spawn; failures are never cached (entry dropped in finally). - Configurable timeout/TTL via PAPERCLIP_PI_MODELS_TIMEOUT_MS (clamped 5s..300s, default 45s) and PAPERCLIP_PI_MODELS_CACHE_TTL_MS (0 disables caching, default 60s, max 1h); typo-safe fallbacks to defaults. - Cache key excludes cwd (verified identical output across cwds). - Opt-in stale fallback (allowStaleOnFailure): serve the last expired entry instead of throwing; default stays fail-closed; listPiModels stays best-effort. Covered by hermetic mock-spawn cases plus the real-spawn catalogue-wording case.

commitperclip · 2026-10-21T05:59:39Z

greptile-apps · 2026-10-21T06:02:56Z

Confidence Score: 4/5

[Medium impact] The changes affect model discovery in the pi-local workflow.

Fix the shared key for workspace-relative commands and satisfy the contribution requirements before merging.

Findings

Workspaces share the wrong models ▶
Required PR sections are missing ▶
New comments violate contribution rules ▶

Fix with agent prompt

### Issue 1
packages/adapters/pi-local/src/server/models.ts:134
If two agents use `command: "./node_modules/.bin/pi"` with the same environment but different workspaces, removing `cwd` from the key makes them share cached results and one in-flight discovery. `runChildProcess` resolves that command against each workspace, so these can be different executables. The second agent can then reject a valid model or accept one its executable cannot use. Keep `cwd` in the key for relative commands, or key by the resolved executable.

### Issue 2
packages/adapters/pi-local/src/server/models.ts:15
The description explains the changes and reports a test result, but omits Thinking Path, Risks, Model Used, and Checklist. `CONTRIBUTING.md` requires the full PR template. Please complete these sections before merging so reviewers can see the reasoning and remaining risks.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

### Issue 3
packages/adapters/pi-local/src/server/models.ts:11-13
This PR adds explanatory code comments, but `CONTRIBUTING.md` says: “Do not submit a PR that includes code comments.” Remove the new comments before merging and put the explanation in the PR description. The same pattern appears above `discoveryInFlight` and in the new stale-fallback block.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

Adds shared model discovery, configurable timeouts and cache duration, and optional stale results after failures.

Pi model lookups share one discovery and can opt into stale results.
Pi model discovery uses a configurable timeout.

_{Reviews (1) · Last reviewed commit: "Harden pi-local model discovery (single-..." · Reviewed by Greptile}

greptile-apps · 2026-10-21T06:03:00Z

    .map(([key, value]) => `${key}=${hashValue(value)}`)
    .join("\n");
-  return `${command}\n${cwd}\n${envKey}`;
+  return `${command}\n${envKey}`;


Workspaces share the wrong models

If two agents use command: "./node_modules/.bin/pi" with the same environment but different workspaces, removing cwd from the key makes them share cached results and one in-flight discovery. runChildProcess resolves that command against each workspace, so these can be different executables. The second agent can then reject a valid model or accept one its executable cannot use. Keep cwd in the key for relative commands, or key by the resolved executable.

Prompt To Fix With AI

This is a comment left during a code review. Path: packages/adapters/pi-local/src/server/models.ts Line: 134 Comment: **Workspaces share the wrong models** If two agents use `command: "./node_modules/.bin/pi"` with the same environment but different workspaces, removing `cwd` from the key makes them share cached results and one in-flight discovery. `runChildProcess` resolves that command against each workspace, so these can be different executables. The second agent can then reject a valid model or accept one its executable cannot use. Keep `cwd` in the key for relative commands, or key by the resolved executable. --- For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

greptile-apps · 2026-10-21T06:03:00Z

+// from /tmp and /), so the discovery key intentionally excludes cwd. Callers
+// still pass cwd through to the spawned process.
+
+export function resolvePiModelsTimeoutMs(): number {


Required PR sections are missing

The description explains the changes and reports a test result, but omits Thinking Path, Risks, Model Used, and Checklist. CONTRIBUTING.md requires the full PR template. Please complete these sections before merging so reviewers can see the reasoning and remaining risks.

Context Used: CONTRIBUTING.md has a guide for a good PR message including a "thinking path". If the PR doesn't include a "thinking path" and "details about what you did, why you did it, why it matters & the benefits, how we can verify it works, and any risks", the... (source)

Prompt To Fix With AI

This is a comment left during a code review. Path: packages/adapters/pi-local/src/server/models.ts Line: 15 Comment: **Required PR sections are missing** The description explains the changes and reports a test result, but omits Thinking Path, Risks, Model Used, and Checklist. `CONTRIBUTING.md` requires the full PR template. Please complete these sections before merging so reviewers can see the reasoning and remaining risks. **Context Used:** CONTRIBUTING.md has a guide for a good PR message including a "thinking path". If the PR doesn't include a "thinking path" and "details about what you did, why you did it, why it matters & the benefits, how we can verify it works, and any risks", the... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0)) --- For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

greptile-apps · 2026-10-21T06:03:00Z

+// `pi --list-models` output does not depend on cwd (verified: identical output
+// from /tmp and /), so the discovery key intentionally excludes cwd. Callers
+// still pass cwd through to the spawned process.


New comments violate contribution rules

This PR adds explanatory code comments, but CONTRIBUTING.md says: “Do not submit a PR that includes code comments.” Remove the new comments before merging and put the explanation in the PR description. The same pattern appears above discoveryInFlight and in the new stale-fallback block.

Context Used: CONTRIBUTING.md has a guide for a good PR message including a "thinking path". If the PR doesn't include a "thinking path" and "details about what you did, why you did it, why it matters & the benefits, how we can verify it works, and any risks", the... (source)

Prompt To Fix With AI

This is a comment left during a code review. Path: packages/adapters/pi-local/src/server/models.ts Line: 11-13 Comment: **New comments violate contribution rules** This PR adds explanatory code comments, but `CONTRIBUTING.md` says: “Do not submit a PR that includes code comments.” Remove the new comments before merging and put the explanation in the PR description. The same pattern appears above `discoveryInFlight` and in the new stale-fallback block. **Context Used:** CONTRIBUTING.md has a guide for a good PR message including a "thinking path". If the PR doesn't include a "thinking path" and "details about what you did, why you did it, why it matters & the benefits, how we can verify it works, and any risks", the... ([source](https://app.greptile.com/review/custom-context?memory=instruction-0)) --- For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

superagent-security

Superagent found 1 security concern(s).

superagent-security · 2026-10-21T06:05:48Z

+  // Failures are still never written — only successful discoveries land here.
  for (const [key, value] of discoveryCache.entries()) {
-    if (value.expiresAt <= now) discoveryCache.delete(key);
+    if (value.expiresAt + STALE_RETENTION_MS <= now) discoveryCache.delete(key);


P2: Expired model-discovery entries are retained for 30 minutes with no size cap, allowing unbounded cache growth

Retaining expired entries 30 minutes with no size cap lets a company actor accumulate large attacker-chosen cache keys despite TTL eviction.

Cap cache entries/bytes and retain stale values only for keys whose callers explicitly need fallback.

AI prompt

Check if this security scanner issue is valid. If so, understand the root cause and fix it. If appropriate, update or add tests. Keep the change focused and preserve intended behavior. <file name="packages/adapters/pi-local/src/server/models.ts"> <violation number="1" location="packages/adapters/pi-local/src/server/models.ts:142"> <priority>P2</priority> <title>Expired model-discovery entries are retained for 30 minutes with no size cap, allowing unbounded cache growth</title> <evidence>The PR changes pruning from `if (value.expiresAt <= now) discoveryCache.delete(key)` to `if (value.expiresAt + STALE_RETENTION_MS <= now) discoveryCache.delete(key)` (line 142) and adds `STALE_RETENTION_MS = 30 * 60_000` (line 10). The cache still has no maximum entry count (line 111), and the key is built from the full command plus every non-volatile environment key name and a SHA-256 of each value (lines 128-134). That key space is caller-controlled through the Pi adapter test endpoint: `testEnvironment` passes `config.cwd` and `config.env` into discovery (test.ts:112-118, 173), and every distinct environment key/value produces a different key. Successful discoveries are inserted at line 248. Thus a company actor can submit many requests with unique env values/names; each successful request performs one Pi spawn and leaves a new entry in the process-wide map for 30 minutes after its TTL expires, instead of the base behavior where the next discovery pruned entries immediately after the 60-second TTL. There is no LRU, max-entry bound, or size accounting. The JSON body limit is 10 MB (server/src/http/body-limits.ts:1), and environment key names are included verbatim in the cache key, so a single entry can retain a large attacker-influenced string. The single-flight map only deduplicates identical keys and does not limit distinct keys.</evidence> <recommendation>Bound the cache by entry count and total retained bytes (for example an LRU with a hard cap) and prune expired entries eagerly. Only retain a stale entry for keys/callers that have opted into allowStaleOnFailure, and cap the size of command/env components used in the key. Add a regression test that inserts many distinct keys and asserts a hard memory/entry bound.</recommendation> </violation> </file>

greptile-apps Bot reviewed Oct 21, 2026

View reviewed changes

superagent-security Bot reviewed Oct 21, 2026

View reviewed changes

Sunbelt Computer Software

PL/B Language Development and Support

Navigation Menu

Repository navigation

fix(pi-local): harden model discovery (single-flight, timeouts, stale fallback) - #17064

fix(pi-local): harden model discovery (single-flight, timeouts, stale fallback)#17064
eljese wants to merge 1 commit into
paperclipai:masterfrom
eljese:pr-upstream/pi-models-hardening

eljese commented Oct 21, 2026

Uh oh!

commitperclip Bot commented Oct 21, 2026

Uh oh!

greptile-apps Bot commented Oct 21, 2026 •

edited

Loading

Uh oh!

greptile-apps Bot Oct 21, 2026

Uh oh!

greptile-apps Bot Oct 21, 2026

Uh oh!

greptile-apps Bot Oct 21, 2026

Uh oh!

superagent-security Bot left a comment

Uh oh!

superagent-security Bot Oct 21, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Sunbelt Computer Software

PL/B Language Development and Support

Conversation

eljese commented Oct 21, 2026

Uh oh!

commitperclip Bot commented Oct 21, 2026

Uh oh!

greptile-apps Bot commented Oct 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Confidence Score: 4/5

Findings

Summary

Uh oh!

greptile-apps Bot Oct 21, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Oct 21, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Oct 21, 2026

Choose a reason for hiding this comment

Uh oh!

superagent-security Bot left a comment

Choose a reason for hiding this comment

Uh oh!

superagent-security Bot Oct 21, 2026

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

greptile-apps Bot commented Oct 21, 2026 •

edited

Loading