`lsp regressions > detects pyright and pylsp in Windows virtualenv Scripts for
Python-only roots` fails on any machine that has ~/.omp/agent/lsp.json:
expect(config.servers[server]?.resolvedCommand).toBe(localBin)
Expected: ".../.venv/Scripts/pyright-langserver.exe"
Received: undefined
The test was not asserting against the packaged defaults at all. loadConfig
walks the user config dirs (~/.omp/agent, ~/.pi/agent, ~/.claude) via
getConfigDirPaths, which resolves from os.homedir(). Any user lsp.json with a
`servers` block sets hasOverrides, which takes loadConfig off its auto-detect
branch and onto the override branch, where the user's rootMarkers replace the
packaged ones.
On this machine that file overrides pyright with
rootMarkers: ["pyproject.toml", "uv.lock", "requirements.txt", "setup.py"]
which contains neither `pyrightconfig.json` nor `setup.cfg` — precisely the
two markers the test creates. loadConfig therefore returned zero servers.
`ruff` is absent from that file, keeps the packaged rootMarkers, and its
sibling tests pass, which is why only this one failed.
Confirmed by probing inside the test: hasRootMarkers() true and
resolveCommand() returning the .exe, while loadConfig().servers was {} — the
detection helpers were fine, the branch was not.
Point os.homedir() at an empty directory for every test in the file so they
see a pristine environment. Bun's os.homedir() reads the passwd entry rather
than $HOME, so the env var alone does not redirect the walk; both are set.
Verification: 76 pass / 0 fail (was 75 / 1) on a machine with a user lsp.json.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review noted that the option whitelist still let generic short flags
escape: `rm -rf -v /` and `rm -rf -i /` were not classified critical,
which is the same separator class the change set out to close.
Pin only the recursive/force flag and skip any other options on either
side of it, which also removes the need to enumerate long options. An
absolute target is still required, so `rm -rf -- ./build`,
`rm --recursive --force ./dist` and `rm -v /tmp/scratch` remain benign
and are asserted.
`CRITICAL_BASH_PATTERNS` required the target to follow one short flag
cluster directly, so anything in between escaped the check:
rm -rf / matched
rm -rf -- / missed
rm --recursive --force / missed
rm -rf --no-preserve-root / missed
rm --no-preserve-root -rf / missed
The last two matter most. GNU coreutils already refuses `rm -rf /` with
"it is dangerous to operate recursively on '/'" and names
`--no-preserve-root` as the override, so the pattern matched the form
that fails safe and missed the form that does not.
Repeat the option separator instead of assuming the path follows one
cluster, and treat `--no-preserve-root` as critical wherever it appears.
Absolute targets are still required for the first pattern, so
`rm -rf -- ./build` and `rm --recursive --force ./dist` stay benign;
both are asserted in the test.
PR #5270 reported the `--` and long-option forms in July and was closed
unmerged by the contributor-vouch bot rather than on merit. This keeps
its cases, credits them in the tests, and adds the `--no-preserve-root`
forms that patch did not cover.
- Added live tracking and stale status warnings for agent activity snapshots.
- Fixed text wrapping with ANSI escape sequences to defer style open sequences after whitespace.
- Added VirtualRenderScheduler for deterministic virtual-clock rendering tests.
On macOS the shared headless browser daemon launched from the system Google Chrome app bundle, running as a com.google.Chrome instance. macOS LaunchServices could then deliver the user's open-URL Apple Events to the daemon instead of their own Chrome, silently swallowing link clicks.
ensureChromiumExecutable now prefers the isolated Chrome for Testing binary (com.google.chrome.for.testing) on macOS, falling back to system Chrome only when Chrome for Testing cannot be obtained. Other platforms keep the download-avoiding system Chrome preference.
Fixes#8673
Mirrored registry status from pre-wire session run-state transitions and required live session corroboration before a peer can sustain bare hub waits.
Fixes#8634
Added Chromium no-startup-window to the broker-owned browser launch so no unowned foreground page survives session tab cleanup.
Covered the resolved Chromium argv and documented the Windows regression.
Fixes#8615
reloadServer() sent the rust-analyzer-specific rust-analyzer/reloadWorkspace
request to every server before falling back to workspace/didChangeConfiguration.
Servers that crash on an unknown method instead of replying -32601 (Roslyn,
dotnet/roslyn#84890) were killed by `lsp reload` rather than reloaded.
Gate the request on isRustAnalyzerClient (exported from client.ts) or a
"rust-analyzer" server name; every other server reloads via
workspace/didChangeConfiguration directly. Consolidate tool.ts's inline
rust-analyzer detection onto the same helper.
Fixes#8571
The shared-browser CDP liveness probes (probeEndpoint, waitForCdp, probeCdpAt) used a bare fetch() against the loopback DevTools endpoint. Bun's fetch honors HTTP_PROXY/HTTPS_PROXY and forwards even 127.0.0.1 requests to the proxy unless NO_PROXY covers them, so a local proxy (e.g. Clash) that 502s internal addresses made a healthy daemon look dead and ensureSharedBrowser tore it down.
Replace the three probes with probeCdpStatus(), a raw-TCP HTTP/1.1 GET that never routes through a proxy and resolves to the response status (or null on unreachable/aborted/timeout).
Fixes#8567
The test bounds two 15s marker waits and a 5s shutdown probe, so 35s of
legitimate waiting sat inside a 30s budget. Adding test files reshuffled
the fixed 10-file CI chunks and put a second daemon-spawning file in the
same process, at which point a loaded runner killed the test mid-wait and
reported only its own timeout instead of the marker assertion that failed.
Review follow-up on #8052. A batched LSP write recorded only the
destination, and the flush reread each entry from disk before
post-processing. That reread tolerated `ENOENT` and rethrew everything
else, so a destination a sandbox denies for reading as well as writing
failed the flush after every write in the batch had already succeeded,
the brokered one included. The tool call owning the flush then reported
failure for bytes that were on disk, contradicting the seam's promise
that a native tool continues as if its own write had worked.
The pending entry now carries the content it committed. The flush still
prefers a fresh read, so an external change made before the flush wins
and a file deleted before it is still not recreated; the remembered
bytes stand in only when the read is denied. Recovered content flows
into `runLspWritethrough`, whose `writeContent` already routes through
`writeFileWithFallback`, so a formatter rewrite of it is brokered too.
This also fixes a shape that predates the seam: a plain write-only file
(mode `0o200`) failed the same flush with nothing registered at all.
Covered by a real-permission test in `test/tools/lsp-batching.test.ts`
that brokers a write to a `0o000` file inside a batch and then flushes;
rethrowing instead of substituting the remembered bytes fails it with
`EACCES` from `flushWritethroughBatch`.
Review raised that `apply_patch`'s non-overwrite contract for `create` and
a rename destination is decided with `Bun.file(dst).exists()`, which
reports `false` when the parent hides the target's metadata rather than
distinguishing "absent" from "unknown" — so a privileged handler could
be asked to write over a protected file it was told not to touch.
Reproduced all three shapes: no handler is consulted in any of them. A
hidden-metadata destination is refused by the path resolver, because the
same denied `lstat` that fools the existence check also leaves the final
component unproven, and an unverifiable destination is never brokered. A
destination that is a symlink onto a protected file is caught earlier —
`exists()` follows the link and reports `true`. A plainly visible
existing file is caught by the same check.
So the contract holds, but it holds through two independent guards in two
files. Pinned that with a regression test asserting the premise (the
existence check cannot see the file), that no handler is consulted, and
that the file is intact; deleting the resolver's symlink proof makes it
fail. Recorded the coupling in the module header too, since relaxing the
refusal to broker unverifiable paths would silently break exclusivity
and needs an explicit intent field first.
Review on #8052 found three problems in the write/delete fallback seam.
A symlink guard that only `lstat`'d the final component let the same
escape through a symlinked ancestor: `ws/link/file` under a
`ws/link -> /outside` link reached a handler as a lexically innocent
path, so a helper's prefix allowlist passed while the bytes landed
outside. Refusing every symlinked component is not available, since
`/var` and `/tmp` are links on macOS and every path under `os.tmpdir()`
traverses one. So `req.dst` is now the path the failed syscall itself
acted on, via `resolveSyscallTarget` beside `confineToWorkspace`: fully
resolved for a write, resolved up to the last component for a delete,
because `unlink` removes a link rather than following it. Resolving also
closes the TOCTOU window a refusal left open. A path that cannot be
canonicalized — a dangling final link, or an ancestor whose own
resolution is denied — is not brokered at all.
Per-handler throw isolation lived outside the per-extension trampoline,
so a throw from one extension's first handler advanced the registry to
the next extension and skipped every later handler that one had
registered. Each handler call is now wrapped individually.
The registry stays process-wide. A subagent spawned with restricted
tools gets `preloadedExtensionPaths: []` and loads no extensions of its
own, so scoping resolution to the originating session would turn its
brokered writes into hard failures, and a host that registers once in
its top-level session expects its subagents covered. The request names
its origin instead: `req.sessionId` against the handler's own
`ctx.sessionManager.getSessionId()`, entered by `ExtensionToolWrapper`,
which `sdk.ts` already puts around the whole tool registry whenever a
runner exists. Both registries are also walked over a snapshot, so a
concurrent session shutdown cannot make another session's walk skip
whichever handler shifted into the hole.
Unit tests go 30 -> 35 and integration 6 -> 7, covering the resolved
target, a symlinked ancestor on both seams, a dangling link, a target
whose own metadata is behind the boundary, same-extension handler
ordering after a throw, and `req.sessionId` matching the handler's own
session end to end.
A host that runs omp inside an OS sandbox can grant a path mid-session but cannot
apply that grant to an in-process write: `write` and `edit` do their I/O in the
agent process, so an out-of-workspace write fails and stays failed until the
process restarts under a wider profile.
Nothing available today closes that. A `tool_call` handler can block and a
`tool_result` handler can rewrite content, but neither can re-run a tool.
`ctx.invokeTool` delegates execution, but the delegated native tool runs in the
same process under the same restrictions. And the failure lands AT the write
syscall - after the tool computed the final content, before it returned - so the
bytes are gone with the throw, and reconstructing them means reimplementing
`edit`'s hashline protocol and the snapshot bookkeeping.
The byte-write that `write`, `edit` and `apply_patch` perform on an ordinary file
path already funnels through one two-line primitive
(`file ? file.write(content) : Bun.write(dst, content)`) at four call sites.
Routing that primitive through `writeFileWithFallback` gives an embedder a single
seam to intercept a permission-denied write: the native tool still records its own
snapshot under the real destination path once a handler reports success, so a
follow-up hashline `edit` on that path keeps working.
Only a permission boundary diverts - `EPERM`/`EACCES`/`EROFS`. Two cases needed
more than that:
- `Bun.write` creates missing parents itself, and when that `mkdir` is the denied
operation it reports the subsequent `open()`'s `ENOENT` instead of the denial -
making a sandboxed write into a new out-of-tree directory indistinguishable from
an ordinary bad path. Redoing the `mkdir` explicitly recovers the real errno, and
because it runs through the same enforcement path as the write it also sees
kernel-level denials (Seatbelt, LSM) that a `stat`/`access` probe reports as
writable. If no handler takes the write, the original `ENOENT` is still what
propagates, with the recovered denial attached as its `cause`.
- `apply_patch` creates the parent as a separate step before writing, so a denial
there threw before the seam was ever reached. That `mkdir` now tolerates a
permission denial when a fallback is registered, letting the write report it.
A denial reached through a SYMLINK is never brokered. The in-process write follows
the link, so the kernel denied the link's TARGET, but a handler receives `dst` and
a privileged helper opening it with ordinary follow semantics would land the bytes
wherever the link points. That also defeats the obvious helper-side defence, since
a prefix allowlist passes when the link sits inside the allowed root while its
target does not. omp cannot vouch for the destination, so it refuses rather than
hand the ambiguity to a privileged writer - the same answer `confineToWorkspace`
already gives an unresolvable link.
Removing a file is a different primitive, so it gets its own seam
(`deleteFileWithFallback`, `registerFileDeleteFallback`) covering `edit`'s `REM`,
a hashline `MV`'s source unlink, and `apply_patch`'s delete op. Two differences
from the write path: `ENOENT` is never diverted, since nothing is created on the
way to an unlink and `REM` needs it to become a not-found error; and the seam
refuses a target it can confirm is a directory, because `unlink` on a directory
reports `EPERM` on Darwin and is otherwise indistinguishable from a sandbox
denial. That check cannot always run - a sandbox denying the unlink usually denies
the target's metadata too - so the request carries `confirmedFile`, and a handler
is required to use a plain unlink rather than resolving or recursing.
The two registries are deliberately separate. A write handler brokers `content` to
`dst`, so a delete request reaching it with no content invites brokering an empty
write and truncating the file it was asked to remove.
With nothing registered both seams are inert: the primitives run exactly as
before, a failure rethrows from the same place, and no extra syscalls are
performed.
Scope is deliberately narrow. Archive-member and SQLite writes are unchanged -
neither is a byte-write to a path, so brokering them needs a different request
shape - along with the ACP bridge's `writeTextFile`, the `lsp` tool's own
workspace-edit and formatter writes, and directory removal.
- Add `renderPdfPageScreenshot` to render PDF pages via headless Chromium.
- Update `ReadTool` to intercept legacy PDF image paths and return page screenshots.
- Ensure daemon clients are closed on read command exit.
- Replaced the custom MuPDF-WASM PDF extraction and rendering pipeline with the new `pdfToMarkdown` native function from `@oh-my-pi/pi-natives`.
- Removed legacy MuPDF extraction modules, WASM embedding scripts, and PDF image extraction tools.
- Added OCR warnings and browser/text redirection for unsupported PDF image reads.
- Updated native package definitions, documentation, and test suites for the new PDF inspection capability.
- Replaced child_process spawn with Bun.spawn in packages/coding-agent/src/utils/external-editor.ts.
- Removed the browser tab evaluation test suite from packages/coding-agent/test/tools/browser-tab-evaluate.test.ts.
- Replaced time-based sleeps and polling loops with event-driven promise resolvers and fake timers across agent and tool tests.
- Migrated test suites to share in-memory auth storage and fixtures using lifecycle hooks.
- Updated catalog model definitions, metadata, and configurations.
The executable version probe added in ecb22957 ("validate Linux browser
executables") replaced the file-only check in resolveSystemChromium with
isChromiumExecutable, which spawns the candidate `--version` for every
platform. On Windows chrome.exe is a GUI-subsystem binary: `--version`
does not print to a detached stdout and can hand off to a running
instance, opening/activating the user's normal browser window, after
which the probe rejects the candidate and falls back to cached Chrome
for Testing.
Gate the spawn probe on process.platform === "linux" (its intended
platform, where non-Chromium PATH wrappers are the real risk) and trust
the executable-file check on Windows and macOS.
Fixes#8445
- Remove redundant definedness, null, and type checks across test suites in multiple packages.
- Clean up unused assertions, metadata tests, and obsolete test cases.
- Add good versus bad test filter guidelines and requirements to project documentation.
Updates test assertions and fixtures to reflect that V4 Pro now exposes the full [low, high, max] effort ladder, switches the incompatible-fallback test to the openrouter deepseek-v4-pro entry, and adds a shared browser lease in the evaluation suite to avoid relaunching Chromium per test under load.
- Displace overwritten destination into a temporary sibling directory during workspace renames.
- Restore the displaced file and clean up the temp directory if the main rename operation fails.
- applyWorkspaceEdit takes an onExecuted callback fired after each
filesystem mutation, so callers hold the executed prefix even when a
later op throws.
- applyWorkspaceEditWithLsp reconciles overlays/watchers for that prefix
best-effort before rethrowing the original apply error.
- applyWorkspaceEdit now returns { applied, executed }; ops skipped via
ignoreIfExists/ignoreIfNotExists are excluded from executed.
- applyWorkspaceEditWithLsp derives didClose/refresh/watched-file
notifications from executed ops, so a skipped rename no longer closes
the old URI overlay or emits phantom Deleted/Created events.
can1357