Skip to content

fix(agent-runtime): keep project instructions for out-of-root paths - #991

Merged
vastsa merged 823 commits into
vastsa:mainfrom
525300887039:fix/968-instruction-root-fallback-17942650
Oct 5, 2026
Merged

vastsa merged 823 commits into
vastsa:mainfrom
525300887039:fix/968-instruction-root-fallback-17942650

Conversation

@525300887039

@525300887039 525300887039 commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Problem

In a project-bound Desktop session, a Read / Write / Edit / BrowserPreview call on an attachment or another path outside the project root resolves to an empty project instruction chain. The runtime then replaces the active chain with that result, silently removing the project's AGENTS.md from the system prompt. Passing the project root itself as a path has the same effect.

Fix

Resolve out-of-root targets and the root path itself to the project's root instruction chain. In-project paths still receive their root-to-nested chain; instruction files outside the project are never read. No-project and empty-root behavior remain unchanged, and the runtime still applies genuinely empty results rather than retaining stale nested instructions.

Add resolver tests and a Read regression that connects the real resolver to the runtime: after a nested read and an out-of-project read, the prompt keeps root rules but not the previous nested rules. Update the English and Chinese runtime specification and E2E-AGENTS-001 expectations.

Verification

Task candidate: 6ab074e9a702868efea984613852a34ebafc9f53
Base main: e23cc9362f574e3c7eeafa46416ee1b041177994

  • pnpm check:pr-base — passed; current origin/main is an ancestor.
  • pnpm --filter @pi-desktop/shared build — passed.
  • pnpm --filter @pi-desktop/agent-runtime build and typecheck — passed.
  • pnpm --filter @pi-desktop/agent-runtime test — 1,288 passed across 93 files.
  • Resolver/runtime targeted tests — 296 passed across 2 files.
  • pnpm test:e2e:hosted-search — 8/8 passed. The new fixture scenario runs the production instruction resolver through the real sidecar Read flow: a nested read adds nested rules; reading an attachment outside the project restores the root chain and never loads an outside AGENTS.md. It uses a deterministic local provider and temporary files; artifact SHA-256: 9b81d968e5f4177df3c75fd5f3908aab786035a2e92d2f7f02cf88f221e86eb4.
  • pnpm lint, pnpm docs:check (557 pages, 85 English/Chinese pairs), and git diff --check — passed.

Limitations

  • The targeted sidecar E2E does not launch the full Electron UI. No Electron, IPC, or RPC contract changes are part of this fix; the broader provider/UI journey remains marked Draft in E2E-AGENTS-001.
  • No real provider credentials or paid services were used.

Fixes #968

@muzimu217

muzimu217 commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

This matches the root cause chain documented in #968 and my review there: an out-of-root tool target made loadProjectInstructions return undefined, and the runtime replaced the active chain with that empty result — silently dropping the project's AGENTS.md for the rest of the turn. Your fix implements exactly the fallback shape suggested back then (out-of-root keeps the root's own chain instead of dropping everything), and since it lands inside loadProjectInstructions it covers both call sites (the agent sidecar path and the host app path) at once.

The logic reads correctly to me: targetDirectory is either root itself (out-of-root or the root path, whose parent lies outside) or a dirname already verified within-root, so dropping the in-loop isWithinRoot check is safe — the walk can only terminate at root. Outside-instruction files are never read, in-project paths keep the root-to-nested chain, and the runtime still honors genuinely empty results rather than retaining stale nested rules.

Correction (edited): an earlier version of this comment described the empty-chain overwrite as something I had validated locally — that was overstated; the behavior is from the #968 root-cause analysis and code reading, not a fresh run on my machine. I was not able to assemble this branch's toolchain locally, so verification rests on your resolver and runtime regressions (and CI once a maintainer triggers the workflows). My contribution here is the #968 linkage and the code review above.

One observation, not a blocker: isWithinRoot resolves paths lexically, so a symlink inside the project pointing outside would still count as within-root for the instruction walk — same as before this change, and consistent with how tool-path resolution treats symlinks elsewhere. Fine to leave as-is.

vastsa and others added 26 commits October 2, 2026 17:42
…landing-20261002

fix(desktop): preserve clickable Windows paths in inline code
Rollup emits Main as out/main/index.js plus shared chunks under
out/main/chunks/. getModuleDirectory returned the chunk directory,
so renderer/preload/plugin-host paths resolved one level too deep
and the window loaded a missing index.html (black screen).
Add a regression test proving getModuleDirectory() resolves to
out/main from both the entry bundle and out/main/chunks/*.js, so
renderer/preload/plugin-host paths cannot drift one level deep
again. Document the shared-chunk layout in E2E-001.
Reconcile renderer pending queues with successful Host reads so requests resolved outside the renderer disappear without dropping requests delivered during a read. Guard overlapping restores by generation and cover the lifecycle with regression tests.\n\nRefs: mocode vastsa#484
…-blackscreen

fix(electron): resolve main output root from inside Rollup chunks
When an OpenAI-compatible provider configures a canonical model name
such as gemini-3.8-flash, relay catalog resolution matches the official
Google model entry and attaches api: "google-generative-ai" to its
modelConfig. Previously, providerRequestTransport allowed any model-level
api to override the provider apiStyle unconditionally, forcing the runtime
to route requests through the Google Generative AI adapter against the
OpenAI proxy endpoint, causing immediate HTTP 404 errors.

Scope model-level wire API overrides to wire-compatible protocols,
ensuring that foreign catalog protocols (e.g. Google Generative AI or
Anthropic Messages) do not overwrite an OpenAI-compatible provider's
configured chat_completions transport.

fixes vastsa#1310
The Command-K recents view and the tray still offered a scheduled run's
transcript, because only the SessionList and search hits applied the ownership
rule: the palette's empty-query list reads the store directly, and the tray
builds its recents from `session.list`. Both now go through the same rule — one
`listableSessions` helper in the renderer and its main-process twin for the tray
— so no session list offers a conversation that belongs to the Scheduled page.

Also repairs the spec text an earlier change left truncated (a duplicated English
fragment, the Chinese UI/IA paragraph and the config_json field list), records
the retention boundary a fast cadence reaches — a pruned run takes its session's
ownership marker with it and that transcript returns to the ordinary lists — and
indexes `task_runs(session_id)` for the ownership probe that summaries and
search run per row.

Reported by @vastsa in review of vastsa#1298.

refs vastsa#1291
Use the published catalog for chat model limits and capabilities so
provider model lists no longer inherit one repeated pi-ai context value.
Keep endpoint discovery and explicit per-account user overrides intact. Add
regression coverage and update the catalog decision and runtime specs.

Fixes vastsa#1304
The reordering left the previous three lines in place, so the step list repeated
"fields and verify its first occurrence is one hour away; pause/resume; Run now"
and trailed off at "conversation, use model tool calls…". Restores the original
"observe automatic completion; delete the settled task." step, which the Chinese
plan still states.
Pin the issue's one-million-token Astra configuration through live
model discovery, metadata caching, settings selection, and runtime budget
resolution. This locks down the distinction between published catalog data
and an account's explicit context window.
Keep the Chinese decisions log aligned with the new models.dev chat
metadata authority and preserve the superseded catalog decisions as
historical context. Match the source decision table and timeline.
…etail

feat(scheduled): 定时任务页改造为「任务 + 运行记录」主从视图(vastsa#1291 阶段一)
Update source-contract and lookup fixtures that still encode the former
Pi metadata authority. Keep the assertions focused on bundled models.dev
metadata and explicit provider-binding resolution.
fix(models): use models.dev as metadata authority
…mini-relay

fix(runtime): preserve provider apiStyle for foreign catalog models
MCP tools were classified as low risk and auto-allowed in every mode,
so ask/accept-edits never showed an approval card. Classify mcp_ tools
as medium and ignore server-declared risk. auto still runs them; Plan
and Goal still deny them.

Fixes vastsa#1320
Keep provider choices authoritative and show catalog gaps clearly while preserving safe runtime fallbacks. Restrict stacked panes to narrow viewports so short windows remain navigable.

fixes vastsa#1313
fixes vastsa#1301
fixes vastsa#1295
fixes vastsa#1260
Keep the Chinese decisions log synchronized with D640 so the
repository locale validation accepts the API routing change.
…ompt

fix(permissions): require approval for user MCP tools
Integrate the latest main changes without rewriting the published
request branch. Keep the upstream MCP decision at D640 and move the
custom endpoint decision to D641 in both specification locales.
…weep

fix(models): honor custom limits and API styles
fix(asktool): reconcile stale pending cards
@vastsa

vastsa commented Oct 2, 2026

Copy link
Copy Markdown
Owner

Thanks for the focused fix. I verified the reported failure against the current implementation: resolving an out-of-root target can yield an empty chain, which replaces the active project instructions. The root fallback closes that path without reading instruction files outside the project. On an isolated candidate based on 8906c88, the resolver/runtime tests passed (278/278) and agent-runtime typecheck passed.

I’m holding the merge because E2E-AGENTS-001 is still Draft. The hosted-search E2E is adjacent and does not exercise out-of-root instruction resolution; the Electron → main → sidecar user journey for this change remains unverified. Please add or run a relevant fixture-backed user-path E2E on a candidate containing the latest main. Thanks for documenting the behavior and its boundaries clearly.

zszz3 added 2 commits October 3, 2026 02:04
Persist provider-neutral instruction and tool updates before dispatch so
skill refresh, session restoration, and compaction preserve their order.
Keep execution permissions authoritative and gate native updates on the
original model, API, and endpoint binding.

Related to vastsa#1285
Keep Pi's exact transport capabilities independent of models.dev-owned
limits and prices so catalog enrichment cannot silently disable updates.
Normalize compaction checkpoints to the durable journal shape, including
extension-provided text blocks, and cover recovery cleanup boundaries.

Related to vastsa#1285
AR307 and others added 23 commits October 5, 2026 09:17
Coordinate screenshot capture with the pane that owns native bounds so
Chromium cannot restore a stale viewport after a concurrent resize.
Pin queued captures to their originating page and preserve caller errors.
fix(work-panel): restore window dragging in unused tab-strip space
…ure-resize

fix(browser): preserve the latest viewport after screenshot capture
projectSessionGetResult projected only the newest `session.compaction` record.
`session.compactions` is the unbounded compaction history and every entry keeps
its own `summary` / `retainedTail` / `details.modifiedFiles`, so a session that
has been compacted several times still overflowed the 512 KiB `MAX_RESULT_CHARS`
limit and the whole answer was replaced by a `{truncated, reason:
"MCP_RESULT_LIMIT", preview}` envelope. External clients (`pi_session_get`) then
never reached `messages`, and reducing the transcript page could not help
because the overflow is independent of `messageLimit` / `contentLimit`.

Project every history entry to the same compact identity the tools contract
promises (`createdAt` and `details.generation`), and keep the projection working
for sessions that carry history but no newest record. Answers already under the
limit, sessions without any compaction record, and the desktop's own session
detail are unchanged.

Reported from the MoCode client (mocode issue 506).
…on-history

fix(mcp): bound the session/get compaction history before truncating
The legacy Edit shape (`old_string` / `new_string` without `tag` / `ops`)
located the unique match by byte offset but then emitted a whole-line
`PUT first.=last:` with `new_string` as the body. `PUT N.=M` replaces whole
lines, so when `old_string` matched only part of a line the unmatched prefix
of the first line and suffix of the last line were silently deleted while the
tool still reported success (`let x = foo;` with `= foo;` -> `= bar;` became
`= bar;`). Counting `old_string.split('\n')` lines also overshot by one when
`old_string` ended in a newline, so deleting `foo\n` wiped the next line too.

Build the exact substring result instead, trim the unchanged leading and
trailing lines, and lower the difference to a single line-anchored op
(`PUT a.=b:`, `CUT a.=b`, or a `PUT >N:` insertion) that still goes through
`hashline::apply_edit` with the live tag. Only changed lines are anchored, so
the provenance gate sees exactly what changed. Exact whole-line matches produce
the same file as before; an identical replacement still fails with
`EDIT_NO_CHANGE`, and not-found / ambiguous matches still fail with
`EDIT_LEGACY_MATCH_FAILED`.

Refs vastsa#1106
Give skill invocations an explicit namespace so they can be distinguished
from ordinary slash commands while retaining the existing skill IDs and
send-time resolution.
The new Skill namespace left runtime specs and a provider acceptance scenario describing the old syntax. Align those contracts and exercise catalog generation, completion text, multiple mentions, and send-time ID resolution with a permanent regression test.
…refix

feat(composer): Skill 命令统一使用 /skill: 前缀
The OpenAI Realtime adapter required an HTTPS base URL and the Live
WebSocket transport accepted only wss, so a Realtime-compatible server
on the user's own machine (http://127.0.0.1:8010/v1) could not be used.
ADR 0304 already lets endpoints the user typed reach loopback and LAN
over plain http under the default relaxed network mode; the Realtime
path was the one user-supplied endpoint still hard-coded to TLS.

Map an http base URL to ws and let the existing public-network guard
decide, from networkPolicy.mode, whether that plaintext hop is allowed.
Third-party Live endpoints (Gemini) keep wss only, base URLs with
credentials, a query or a fragment stay refused, and a plain ws
endpoint on a proxied route fails closed rather than being tunneled in
the clear. The scheme rules move to a small Electron-free module so
they can be tested directly.

Refs vastsa#1318
The line-anchored edit format preserves terminal newline state, so the
legacy substring adapter cannot express a terminal-newline-only change.
Pin the result in a regression test and make the compatibility contract
explicit.
…al-line

fix(tools): preserve unmatched text in partial-line legacy edits
Exercise the production transport against a loopback WebSocket and keep the proxied plaintext refusal covered. Add a plain-HTTP fixture mode so the full isolated call journey can validate the user-endpoint policy.
…k-http

fix(live-voice): let Realtime use a plain-HTTP user endpoint
Transcript content is the source of truth for indexed messages. Sync a line before committing its SQLite row so a failed flush cannot leave a retry with a phantom index.

This integration keeps the contributor commits and merges the latest main, including both migration index changes.
…-hot-paths

perf(host-core): reduce history costs in search and session reads
Exercise the production instruction resolver through the real sidecar
Read flow so nested rules are replaced by the project root chain for
an external attachment. Record the fixture-backed coverage in both
E2E plans.
@vastsa
vastsa marked this pull request as ready for review October 5, 2026 16:04
Copilot AI balanced review requested due to automatic review settings October 5, 2026 16:04

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@vastsa vastsa left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing this. The linked issue is confirmed: an out-of-project target and the project root itself could resolve to a successful empty chain, which replaced the active project rules. The resolver now falls back to the root chain while continuing to read instruction files only inside that root; no-project and genuinely empty-root behavior remain unchanged.

I added fixture-backed coverage that runs the production resolver through the real sidecar Read flow. It verifies a nested read adds nested rules and a following attachment read restores the root rules without loading an outside AGENTS.md. The PR candidate integration tree matches the tested task candidate.

Validation passed: agent-runtime tests 1,288/1,288, targeted tests 296/296, pnpm test:e2e:hosted-search 8/8, package build/typecheck, lint, docs checks, and pnpm check:pr-base against e23cc93. GitHub reports no configured checks for this branch. The broader Desktop UI journey remains Draft, but this fix does not change Electron or IPC behavior and its sidecar/resolver path is now exercised. Approving and merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Project instruction file (AGENTS.md) is silently dropped from the system prompt when a tool call targets a path outside the project root