fix(copilot): share context selection across plugin processes - #86
Conversation
…e started in `/neatcontext:use` reported a context connected while `get_context` kept answering from a different one. No error anywhere: both halves were telling the truth about different files. The Copilot adapter derived a session by hashing `process.cwd()`, on the premise that every plugin process is handed the workspace. That premise is false. Copilot spawns the MCP bridge with the plugin installation directory and the slash-command CLI with the user's workspace, so the two halves hashed different paths and scoped to different selection files. Copilot does publish a session identity to both -- COPILOT_AGENT_SESSION_ID, and COPILOT_LOADER_PID for the host process -- so a session here is now a session, as on every other host, with the workspace digest kept as the fallback for a build that publishes neither. The mechanism for carrying a session across a process that outlives it already existed for two hosts and was copied into each. It is promoted into `shared/core/` and given `configureHostPid()`, so each adapter names its own host's pid instead of core knowing every host's variable -- those variables are inherited by child processes, and a shared list would let one host key on another. Codex keeps `process.ppid` unchanged. Where the two halves can still fail to agree, they say so: a drift warning when a live bridge is serving another session, an upgrade hint naming the connection a workspace had before selections were per session, and a warning when the host publishes no identity at all rather than degrading in silence. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ving one Review follow-ups. `sessionIdentityIsShared()` accepted a NEATCONTEXT_HOST_KEY that `hostKey()` rejects, so `..` or a path-shaped value suppressed the warning while no pointer file was ever written -- claiming a channel that was never opened. The rule now lives in one exported `normalizeHostKey()` and both callers ask it. The upgrade hint is retired once the user acts on it. Left in place, the record behind it survived a deliberate `/neatcontext:disconnect` and went on telling them to reconnect what they had just disconnected. It is removed on the first connection made in a session of its own, and never when this session is the workspace fallback -- that file is the live selection then, not a leftover. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove unreachable guards and exercise the remaining host identity, migration, drift, and atomic-write branches required by diff coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Thanks. But can you share what was the bug with more details? |
|
Here is a minimal reproduction from the version before this PR: User-visible reproduction
Actual: Expected: slash commands and MCP tools use the same context selection for the current session. Deterministic developer reproductionStart the long-lived MCP bridge from the plugin directory, then run the command CLI from a different working directory and connect a context. Before this fix, each process derived its selection scope from its own working directory, so the CLI wrote one selection file while the bridge read another. Running both processes from the same directory hides the bug. This is a scope mismatch, not a cache or timing issue. |
tanglearncode
left a comment
There was a problem hiding this comment.
Reviewed the full diff, ran the suite (66/66 green, npm run check clean), and then went and checked the PR's two factual premises against a real GitHub Copilot CLI (1.0.79-6, Windows) rather than taking them from the commit message. Summary up front, because the result cuts both ways:
The bug is real, and correctly diagnosed ✅
~/.neatcontext/plugin-sessions/ on my machine contains copilot-ws-7819cfbe611eb1b9.json, and that digest is exactly sha256(lowercase(<plugin installation directory>)). The bridge really was hashing the plugin directory while the CLI hashed the workspace. This is not a theoretical fix.
COPILOT_AGENT_SESSION_ID is also real and genuinely shared — identical value in a Copilot-spawned MCP server and in a shell-tool child, matching the id Copilot prints for --resume. The core of the fix works.
COPILOT_LOADER_PID does not exist ❌
It is in neither process. The complete COPILOT* set on 1.0.79-6 is COPILOT_AGENT_SESSION_ID, COPILOT_CLI, COPILOT_CLI_BINARY_VERSION.
So hostKey() falls back to process.ppid, which differs between the two halves (bridge → the Copilot process; CLI → a throwaway per-command shell). The whole pointer-file / drift-detection half of this PR is inert on Copilot CLI: nothing reads what the CLI writes, nothing writes what the bridge reads, and no drift warning can fire. The mid-session case the PR is named after is therefore not actually fixed on this host — and, by design, nothing reports that.
That is the one item I would not merge without resolving, or at least documenting honestly. Details inline on session.mjs:147.
Also worth fixing
sessionIdentityIsShared()still claims a channelhostKey()will not open, and a new test pins that behaviour (session.mjs:100,copilot-session-drift.test.mjs:369).- The 0.3.3 upgrade hint outlives the context it names, permanently — reproduced (
neatcontext-cli.mjs:112). /neatcontext:statuscarries a second, non-waiting copy of the drift check that will produce false positives once host keys line up (neatcontext-cli.mjs:261).pruneHostPointers()is never called on Copilot, and the sweep skips the new.tmpfiles (neatcontext-cli.mjs:82,shared/core/host-session.mjs:337).- Two README claims the code does not deliver (
README.md:53,README.md:60).
The design — one pointer file per host process, promoted into shared/core/ with a per-adapter pid provider, atomic temp-write + rename — is the right shape, and the reasoning about not keeping a global list of every host's pid variable is correct and worth having written down. The problem is that the specific variable the Copilot adapter was built around isn't there.
Probe method, if you want to reproduce: register a stub MCP server via --additional-mcp-config that dumps process.cwd()/ppid/env on startup, and have the prompt run a shell command that does the same. Note that a server registered that way gets the workspace as its cwd — only plugin-provided servers get the plugin directory, which is why the on-disk digest above is the better evidence.
Narrow the fix to the session identity verified on Copilot CLI. Keep workspace scoping as the fallback and cover command-to-MCP agreement across different working directories. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Thanks for the detailed review and for verifying the behavior on a current Copilot CLI build. I narrowed the PR to the behavior we can verify:
I removed the host-pointer and drift-detection design entirely. That also removes the unsupported Current validation: full suite passes (456 tests), |
tanglearncode
left a comment
There was a problem hiding this comment.
Re-reviewed at 49151be. This is a much better PR, and it resolves the blocker from my previous review.
Reverting the pointer-file machinery was the right call. What is left is the part that was actually verified against a real Copilot CLI — COPILOT_AGENT_SESSION_ID exists, is published to both the MCP server and the slash-command process, and matches the id Copilot prints for --resume — and none of it now depends on COPILOT_LOADER_PID, which does not exist on Copilot CLI 1.0.79-6. The title and description were updated to match the narrowed scope, which I appreciate: the PR no longer promises to fix the mid-session case it cannot.
Verification on this branch: npm run check clean; tests/copilot-plugin.test.mjs 14/14; full suite 456/456 across three consecutive runs. (My first full run reported 455/456, but I had not captured the reporter output and three subsequent runs were clean, so I cannot name the test — most likely contention with a npm run check I had just run. Mentioning it only so a CI hiccup is not a surprise.)
One item before merge
Existing Copilot users lose their connected context with no signal. Reproduced; details inline on session.mjs. The old copilot-ws-<digest> selection is not deleted, it simply stops being read — and revision 1's upgrade hint was removed along with machinery it did not depend on. Restoring the hint, or at minimum a README line, is the difference between a silent disappearance and "reconnect it once."
Smaller points
explicitIdno longer validates, and a host-supplied value now becomes a path segment (session.mjs:39).- The mid-session limitation is undocumented, and the suite no longer covers it at all (
session.mjs:9,copilot-plugin.test.mjs:593). - The README fallback sentence promises more than the fallback delivers on the host shape that caused this bug (
README.md:54).
Everything above is small. With the upgrade note added I would be happy to see this merged.
Validate host-provided session ids, surface existing workspace selections after upgrade, and document fallback and long-lived bridge limitations. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Clarify that the legacy workspace hint is per session and verify it disappears after the session reconnects. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Thanks for the follow-up review. I addressed the remaining items without reintroducing the pointer-file design:
Validation remains green: 457 full-suite tests, |
|
LGTM. Thanks! |
Summary
/neatcontext:status.Testing
npm testnpm run check