pr-review: harden JS/TS code intelligence#252
Draft
gnguralnick wants to merge 145 commits into
Draft
Conversation
Reimplements the DI/test-seam refactor against the FastAPI->Flask migration that landed on main (PR #175 merged it in). The prior approach used FastAPI Depends/dependency_overrides, neither of which exists under flask-sock; this version achieves the same goal with the Flask state container. - create_application drops the agent_manager / claude_auth_service / welcome_resender test-only params. It unconditionally builds and starts the live AgentManager (conftest's AgentManager.start no-op keeps that cheap and observe-free under test). Tests that need a seeded manager build the app, then set state_of(app).agent_manager; auth tests set state_of(app).claude_auth_service / .welcome_resender. - SystemInterfaceState.broadcaster is now derived from agent_manager (a property) instead of a stored field, so a single manager seed repoints the broadcaster too and the two can never diverge. The is_agent_manager_owned flag is gone; the app always owns the manager it builds, so shutdown() always stops it. - Agent messaging: the discover/send collaborators move off the private _send_message_to_agent's defaulted params onto a MngrMessenger FrozenModel that AgentManager owns. The send_message free function and _send_message_to_agent are gone; tests construct MngrMessenger(discover=, send=) directly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add a [create_templates.modal] block so minds can launch a workspace on a Modal sandbox (the "modal" compute provider), mirroring the lima template (no Dockerfile build; toolchain provisioned over SSH; provider flipped on per-create; no autostart since sandboxes are ephemeral). Re-grounded off fresh main, replacing the earlier orphan/squash branch. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Delete the libs/telegram_bot package and the send-telegram-message / read-telegram-history skills; drop the TELEGRAM_* pass-env vars, the telegram-bot dependency / workspace member / source, and the Dockerfile copy. send-user-message now uses its inline fallback as the sole channel (probe-and-dispatch contract unchanged). Scrub telegram from assorted docs/docstrings (README, CLAUDE.md, settings, runtime_backup, cloudflare_tunnel, system_interface, dealing-with-the-unexpected, build-web-service reserved names). Regenerated uv.lock. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Teaches the workspace agent how to reach the Minds API through the latchkey minds-api-proxy (latchkey curl http://latchkey-self.invalid/minds-api-proxy/...), to discover routes/types from GET /api/schema, and to use the core capabilities: list/inspect workspaces, create a fresh workspace (with operation polling), SSH into another workspace (keypair + POST .../ssh, incl. the local reverse-tunnel case), read/export backups, and recover/lifecycle/destroy. The headline workflow strings these together to migrate content out of an old or broken workspace into a clean new one, then hands cleanup to the new workspace's own agent via `mngr message`. Includes the latchkey permission flow for a specific workspace: the `minds-workspaces` scope gates each verb (only the schema is allowed by default), and a per-workspace grant is requested via a `type: "workspace"` permission request carrying the verb list + `target_workspace_id`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…retry form A submitted OAuth code consumes the single-use sign-in session end-to-end (the backend terminates its 'claude auth login' subprocess unconditionally, and the modal clears its local session id). The failure path returned the user to the same code-entry form, but the session was already gone -- so the 'Verify & finish' button silently no-opped (no request, no error, no feedback). Route OAuth code failures to the full error screen instead, and make that screen's only action 'Start over', which restarts sign-in from the beginning (spawning a fresh OAuth session). The header close button / backdrop still dismiss the modal. API-key submit failures are unchanged (retryable in place). Adds a regression test and a changelog entry.
…unsc
When the docker daemon's runsc runtime is registered with --overlay2=none (now
required so a workspace's rootfs -- and mngr's SSH provisioning written to it --
survives a container restart), the container's /run lives on gVisor's
gofer-backed filesystem, which returns EOPNOTSUPP for os.link() of a socket
inode. supervisord installs its control socket via a hard link, so it wedges
("Unlinking stale socket") and never starts system_interface, leaving the
workspace stuck on the "Loading workspace" loader.
Add default_start_args=["--tmpfs","/run"] to [providers.docker] so /run is a
tmpfs where the hard link works -- mirroring the tmpfs /run the aws-<region>
provider start args and the ovh/vultr/imbue_cloud host setup already apply.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Changes for new Minds V1 API
Content-addressed guard (scripts/_provision_guard.sh): setup_system.sh skips when the workspace tree was already provisioned (marker under /var/lib/minds/provision keyed by git tree hash), so a Lima create booting a pre-baked image for the same tree skips ~25-30s of redundant system setup. Never skips without a matching marker. install_dependencies/build_workspace are deliberately NOT guarded -- they emit in-repo outputs the create re-materializes from git, so they must run every create.
Satisfies test_meta_ratchets::test_prevent_bash_without_strict_mode. The file is sourced by setup_system.sh (already strict), so this is a no-op for callers and keeps the lib safe to source anywhere; guard logic re-validated under set -e.
lima: skip setup_system provisioning when already baked (issue 2306)
Dockview is configured with defaultRenderer: "always", so an inactive chat tab stays mounted while an ancestor is hidden with display:none. mithril's m.redraw() is global, so the hidden ChatPanel keeps running its scroll logic against a scroll element that reports scrollTop/scrollHeight/clientHeight all as 0. maybePage() then mapped that zero position to event 0 and fired a JUMP that replaced the loaded window with the start of the conversation, and applyScrollPosition() clobbered the retained scrollTop to 0. Guard the scroll-management hooks (maybePage, applyScrollPosition, the viewport measure, and the ResizeObserver) so they skip while the element is not measurable, and force a redraw when the tab becomes visible again so the position is restored. The measurability predicate is extracted into a pure, unit-tested helper (scrollVisibility.ts) following the scrollFollow.ts / virtualWindow.ts pattern, plus a Playwright regression test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> (cherry picked from commit 2659502) (cherry picked from commit 338b4102c602d124e95797c9005999cea67c5457)
…g scenario (#193) * Harden update-system-interface: ground UI fixes in the real motivating scenario; close preview tab on teardown A UI fix delegated through this skill validated against a fixture the worker invented rather than the real conversation that motivated the change, producing a CSS selector that never matched the real DOM plus a passing-but-meaningless test. Close the process gaps at the three points where the real scenario should have entered, and fix a teardown gap: - Delegate: the lead must capture the real motivating DOM/measurements and pass them in the brief under a new `## Real scenario to reproduce` section, as the worker's reproduction fixture and acceptance target. - Preview: the preview opens on the worker's own empty agent, not the motivating conversation; the lead must open the real agent, re-measure, and confirm the number actually moved before asking the user. - Worker sub-skill: the fixture must reproduce the lead-provided DOM shape (not a guess), assert that shape, and the regression test must fail-before/pass-after. - Teardown: `unpreview` tears down the preview servers/services but leaves the `si-preview` dockview tab open; add an explicit step to close it (layout panel vs. service lifecycle are separate concerns). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Revise real-scenario hardening: point the worker at the live conversation instead of transcribing the DOM The worker is not cut off from the motivating conversation -- the system interface discovers agents from the shared MNGR_HOST_DIR, so an instance the worker boots renders the same real conversations the user sees. So the lead names the motivating agent and the worker looks at it firsthand, rather than the lead measuring the DOM and the worker reconstructing it from prose. --------- Co-authored-by: runtime-backup <runtime-backup@mindsbackup.local> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Gabriel Guralnick <gabriel@imbue.com>
The Lima provisioning-skip-guard change made setup_system.sh source its sibling scripts/_provision_guard.sh via "$(dirname "$0")", but the Dockerfile copies only setup_system.sh into /usr/local/bin (renamed to fct-setup-system). At build time dirname "$0" resolves to /usr/local/bin, so the source target /usr/local/bin/_provision_guard.sh does not exist and the fct-setup-system RUN fails with 'No such file or directory'. Copy _provision_guard.sh alongside setup_system.sh so the source resolves. The Lima path was unaffected because it runs the script in place from scripts/, where the guard already sits beside it.
…ngr #2310) Set 2 CPU / 4 GB and the 24h sandbox + idle timeouts on the [create_templates.modal] block via setting__extend, so the create template owns these values rather than the mngr_modal provider defaults (the provider side reverts them to its minimal defaults in imbue-ai/mngr#2310). Reorganized the block to mirror the vultr/aws/imbue_cloud cloud-template layout. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Tracks 4d3c69a342064e1ad4f5dfead6f6a77b7262d28f in mngr.
vendor/mngr: refresh from mngr main (4d3c69a34)
…ted state
Push dependency injection to a single composition root and remove the last
test monkeypatches, so production code reads as if no tests existed and every
collaborator is injected (not constructed-then-patched) under test.
- main is the composition root: `build_production_state` wires the real object
graph; `main` starts the agent manager (the sole `AgentManager.start` caller)
and serves. `build_application` only constructs, so it never spawns observe.
- create_application(state: SystemInterfaceState) is now a pure assembler: it
wires routes/plugins/error-handling onto the app and attaches the injected
state. It constructs nothing and starts nothing. The config / http-client /
filter parameters move up into the composition root.
- AgentManager.build gains an injectable `messenger` (defaults to the real
MngrMessenger), so the message-send path is fakeable without patching.
- testing.build_test_state is the test-side composition root: it builds a
SystemInterfaceState with fakes for whichever collaborators a test overrides
and cheap real instances for the rest, never starting the manager.
testing.RecordingMngrMessenger records sends for the one test that asserts on
the send path. Tests now inject via build_test_state instead of post-build
`state_of(app).x = ...` mutation, create_application test params, or
`patch("...MngrMessenger.send_to_agent")`.
- conftest no longer monkeypatches AgentManager.start (nothing starts the
manager in tests), so PREVENT_MONKEYPATCH_SETATTR drops 1 -> 0.
get_state() is kept as the deliberate Flask-boundary locator: Flask owns
handler invocation, so handlers read the injected state from it but never
construct or patch -- the composition-root pattern applied to a web framework.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
vendor/mngr was only refreshed at release time and drifted hundreds of commits behind mngr main between releases, so the minds-launch-to-msg health check (binary from mngr main, agent from this repo's committed vendor) risked a vendor-skew wedge. Add sync-vendor-mngr.yml: twice daily, an hour before each launch-to-msg cron, clone mngr main, re-archive it into vendor/mngr (same git archive flow as mngr's just sync-vendor-mngr), and push to main. No-ops when mngr main has not moved. mngr is public so reading needs no credential, and the push to this repo's own main uses the built-in GITHUB_TOKEN. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ot a None sentinel `messenger: MngrMessenger | None = None` plus an `if messenger is not None else MngrMessenger()` branch read as a test-only seam: `None` was never a real state, only "tests override, production doesn't." Replace it with a module-level `_DEFAULT_MESSENGER` constant (a shared frozen, stateless MngrMessenger with the real discover/send) used directly as the default, mirroring the sibling `mngr_binary: str = _DEFAULT_MNGR_BINARY` in the same signature. No None, no branch; the default is the production value, and tests pass a fake. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add daily workflow to sync vendor/mngr to mngr main
Tracks ccb62268d4f4006f7b741a9bb1c12f21cdf9807d in mngr.
…#196) * docker provider: default to runc, add docker_runsc overlay for gVisor [providers.docker] no longer hardcodes docker_runtime="runsc", so it uses Docker's default runtime (runc), which is available everywhere -- notably macOS, where gVisor's runsc is not. This drops the need for the MNGR__PROVIDERS__DOCKER__DOCKER_RUNTIME=runc workaround on hosts without gVisor. Add a docker_runsc create-template overlay that opts a create into the gVisor runtime: `--template docker --template docker_runsc` reuses the entire docker template body and only switches the container runtime to runsc, so the runc/runsc choice is the single difference with no duplicated body to keep in sync. The minds desktop app stacks the overlay by default on Linux and omits it on macOS. Co-authored-by: Sculptor <sculptor@imbue.com> * docker template test: update stale gVisor-selection docstring PR #196 moves the gVisor (runsc) runtime selection out of the [providers.docker] block and into the opt-in `docker_runsc` create-template overlay. Update the parenthetical in test_docker_template_hardens_start_args_and_drops_sys_ptrace, which still claimed the runtime is selected via `docker_runtime` in [providers.docker] and "not a create-template setting" -- both now false. The assertions are unchanged (this test never asserted the runtime). Co-authored-by: Sculptor <sculptor@imbue.com> --------- Co-authored-by: Gabriel Guralnick <gabriel@imbue.com> Co-authored-by: Sculptor <sculptor@imbue.com>
`build_test_state` exposed nine override parameters but only five are ever passed by a test (config, agent_manager, claude_auth_service, welcome_resender, latchkey_http_client). The other four -- provider_names, include_filters, exclude_filters, and the service-proxy http_client -- were never substituted, so their None-defaulting was dead flexibility. Drop them as parameters and fix those state fields to their production defaults inline. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add chat file attachments (with image previews) to the system interface
Tracks cd558f298a00041a60bb0ee21192eaccb8e46d3f in mngr.
Add Modal create template (paired with mngr #2310)
Tracks f10b20c5081bf3c8acee27eb08cd26c4af044e7b in mngr.
…to gabriel/knowing-to-refresh
The recurring failure mode when editing a service is verifying a change by writing test data into the live store and then deleting it as cleanup, with a too-broad delete taking real records with it. - Scaffold now generates a DATA_DIR constant that defaults to runtime/<name>/ but honors a per-service <PACKAGE_UPPER>_DATA_DIR env var, so a service's data location can be redirected without code edits. - build-web-service documents routing all persistent state through DATA_DIR and why it keeps later edits safe. - update-service adds a 'protect the user's data' section: verify read-only or against a disposable copy pointed at by the override, never delete from the live store to clean up, snapshot before any in-place change, and leave the data dir in place on teardown.
Tracks 8440a5d45199c7287d4a20cdf8d37fb1bf147e0b in mngr.
Tracks c55d2b225c360b12d3a7a8396c98eff65b502301 in mngr.
vendor/mngr: refresh from mngr c55d2b225 (minds v0.3.6)
…plate into mngr/pr-review-ux-actions
Extract the "boot a throwaway instance of a service on a spare port" motion -- previously hand-rolled in the update-service data-isolation prose and baked into the system-interface preview -- into one shared, unopinionated script, .agents/shared/scripts/serve_isolated_instance.py (up/down). It takes the launch command, cwd, and env overrides as parameters, picks a free port, injects it, waits for health, and either returns the loopback URL (for the agent's own data-safe testing) or, given the preview flags, registers it and wraps it in the labeled "preview" frame as a tab. The frame wrapper (preview_wrapper_server.py) moves into shared alongside it. update-system-interface's preview/unpreview become thin adapters that delegate boot/teardown to the shared script, passing the system-interface specifics (neuter layout persistence, probe /api/agents, register inner app + wrapper). The merge and reveal/auto-rollback machinery -- the part whose failure strands the user with no UI -- stays owned by reveal_system_interface.py and was intentionally not generalized. Scaffolded Flask services now read their listen port from a <PKG>_PORT env override (mirroring <PKG>_DATA_DIR), so a throwaway can bind a spare port beside the live one. update-service documents the shared-script invocation for data-safe testing and the optional user-facing preview, and tells agents to retrofit both overrides onto older services.
…-ai/forever-claude-template into mngr/pr-review-ux-actions
Mirror the Jedi-backed Python intelligence for JavaScript/TypeScript files: declaration-aware hover (signature + doc comment) and go-to-definition that resolves locals, parameters, and relative imports across the cached repo tree. Covers .js/.jsx/.ts/.tsx and their .mjs/.cjs/.mts/.cts variants. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The doc-comment hover only picked up the single comment node immediately above a declaration. Two gaps: a const/let/var comment is a sibling of the outer lexical declaration (not the inner declarator), so it was missed entirely; and a run of // lines is one node per line, so only the last was shown. Climb to the statement node and gather the whole contiguous comment block (stopping at a blank line), joined in source order. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two issues surfaced while reviewing a real JS file: - Monaco bundles a TS/JS language service that auto-registers its own hover and go-to-definition providers, producing a duplicate 'Go to Definition' context-menu entry whose navigation fails (it only sees the single open model, no repo context). Disable the built-in providers via setModeConfiguration so our backend-driven ones are the only ones; syntax highlighting (a separate grammar) is unaffected. - Hovering a const showed only 'const NAME: Type', omitting the value. Include the initializer when it is short and single-line, so a constant's value is visible on hover. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Names bound by a destructuring pattern (const { session } = ...) or a
require() call produced no hover/definition, which is most of a CommonJS
file's top-level names. Now:
- expand object/array destructuring patterns into their bound identifiers
when collecting scope bindings, so each name resolves to its binding;
- recognize require('m') like an import -- relative specifiers follow to the
file, external ones fall back to the local binding so hover still works;
- hover shows the binding shape and source, e.g.
const { session } = require('electron').
Member/property access (obj.method) still resolves to nothing: that needs
type inference, which a syntactic parser cannot provide.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…e server For repos the user explicitly prepares, JS/TS hover + go-to-definition come from a real TypeScript language service (member + inferred types, library .d.ts) instead of tree-sitter. A subtle 'Enable rich types' pill launches a headless claude -p agent inside the cached tree that installs the repo's dependencies (npm/pnpm/...) and a pinned typescript@5 language server under .pr-review-prep/; jshover/jsdef then use tsintel for prepared trees, falling back to tree-sitter on any error. node_modules and .pr-review-prep are excluded from file listing and search. typescript@5 is pinned deliberately: bare 'npm install typescript' now resolves to 7.x, whose npm package lacks the classic language service API. - prepare.py/tsintel.py + tsintel_server.mjs (Node helper), with injected launcher/transport seams so tests never spawn a real agent or process. - Verified live end to end: a prepared tree resolves a member (method) hover + go-to-def and an inferred variable type that tree-sitter cannot. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The app runs in a sandboxed iframe without allow-modals, so window.confirm() was silently ignored and the 'Enable rich types' button did nothing. Switch to the existing confirmDialog() in-app modal, as the merge/close actions already do. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The prepare agent ran with --output-format json, so prepare.log only appeared at the end -- a multi-minute black box. Switch to stream-json and parse events incrementally, writing a readable line per shell command / output / narration to the log as it happens. The detail header now shows a live, auto-scrolling log panel while installing (and on failure, so the error is visible), and the pill pulses; polling tightened to 2.5s. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Hang fix: the prepare agent ran in the extracted tree (no .git, carries the repo's .claude hooks), so a Stop hook blocked the headless agent from ever exiting -- it finished its work then hung. Set MNGR_CLAUDE_SUBAGENT_PROXY_CHILD so those hooks skip, as they do for proxied subagents. - Bar host-level changes: the agent had apt-installed Node to satisfy an engine-strict lockfile; the system prompt now forbids system/global installs and steers to --engine-strict=false instead. - Model picker: the Enable-rich-types dialog now offers Opus/Sonnet/Haiku (default Sonnet, remembered in localStorage); threaded through start_prepare. - Live log: tag each streamed line by kind and color agent narration / shell commands / tool calls / output distinctly in the panel. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
tsserver's quickInfo documentation only includes JSDoc (/** */), so once a repo was prepared, functions documented with plain // comment blocks lost their descriptions on hover (a regression vs the tree-sitter engine). When tsserver returns no documentation, extract the leading comment block at the symbol's definition via ts.getLeadingCommentRanges (which covers // and /* */), clean the markers, and stop at a blank-line gap. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-ai/forever-claude-template into mngr/pr-review-ux-actions
…guards - Frontend: map the .mjs/.cjs/.mts/.cts extensions to javascript/typescript so hover and go-to-definition actually fire for them (the backend jsintel already handled these grammars; the UI rendered them as plaintext and never sent a request). - Replace the prefix-unsafe str.startswith path-containment checks in jsintel and prepare with Path.is_relative_to, so a sibling directory sharing the tree root's name as a prefix can't be read. - Bound the tsintel Node-helper startup handshake with a kill timer so a wedged spawn can't block the global registry lock and freeze JS intel for every tree. - Tests: cover the .mjs/.mts/.cjs variants and the prefix-sibling escape. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Cosmetic import grouping (isort style); no behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Background harden pass for the JavaScript/TypeScript code-intelligence update to the pr-review service. Verified the committed change (tree-sitter baseline + opt-in rich types) against its design and drove the real TypeScript language service to confirm inferred types, cross-import go-to-definition, and doc-comment surfacing.
Hardening fixes
.mjs/.cjs/.mts/.ctsnow get intelligence. The backend already parsed these grammars, but the frontendEXT_LANGmap omitted the extensions, so the UI rendered them as plaintext and never fired hover/go-to-def. Added a test.str.startswithcontainment checks injsintel/preparewithPath.is_relative_to, so a cache dir sharing the tree root's name as a prefix can't be read. Added a test.tsintelNode-helper startup handshake with a kill timer (it otherwise ran under the shared registry lock).Tests
uv run pytestfromlibs/pr_review→ 149 passed (+2 new), coverage and all 14 ratchets green;ruff checkclean; no new type-checker errors. The realclaude -pprepare agent and Node language service are not exercised in CI (covered by injected seams + manual verification).🤖 Generated with Claude Code