PR review log

Scheduled AI code reviews posted on behalf of Engel Nyst.

Pull request Repository Review Head Reviewed at (UTC) Summary
Security: Unvalidated Workspace Directory in VSCode URL Endpoint OpenHands/software-agent-sdk #4282 Review permalink fa03653 The reviewed head is tree-identical to the previously reviewed code and still accesses a nonexistent VSCodeService.config attribute. Direct reproduction raised AttributeError, and the focused VSCode router test suite failed two tests. The review requested explicit workspace-root ownership plus real route-level coverage before a human security or agent-server maintainer considers merging.
Enable ruff's line-reducing rules and apply the safe fixes OpenHands/software-agent-sdk #4281 Review permalink 259aaf4 The review found the selected Ruff rules pragmatic and the sampled safe rewrites structurally sound, including the immutable-value dict.fromkeys conversion. The migration nevertheless spans 98 files across agent, tool-execution, conversation, and evaluation paths, creating meaningful aggregate blast radius. It asks a human maintainer to satisfy the repository evaluation gate before merging despite the local simplicity of each rewrite.
fix(file_editor): stop insert gluing text onto a newline-less last line OpenHands/software-agent-sdk #4163 Review permalink 3ab0b42 The review found the newline-less EOF fix minimal and correctly placed at the boundary between preserved lines and inserted text. Existing history remains untouched, writes stay atomic, and focused tests exercise the real file round trip and view fidelity. Because file editing is benchmark-sensitive tool behavior, it asks a human maintainer to perform lightweight evaluation validation before merging.
fix(grep): honor brace alternation in the include filter OpenHands/software-agent-sdk #4164 Review permalink ae68403 The review found that a comma-less brace group before an alternation causes the new parser to stop scanning and leave the whole pattern unexpanded. A mixed pattern such as x{literal}*.{ts,tsx} therefore still matches nothing even though later brace alternation is valid. It asks the author to distinguish a literal current group from the absence of later groups and add mixed-order regression cases.
fix(conversation): persist secrets added via update_secrets OpenHands/software-agent-sdk #4166 Review permalink b94cb02 The review found that deep-copying the registry recursively clones caller-owned dynamic secret providers merely to trigger autosave. Stateful providers can silently fork their counters or caches, while a valid lock-backed provider can make update_secrets fail with a deep-copy error. It asks the author to copy the registry mapping without cloning provider objects and to add a persistence round-trip regression test that proves identity and state continuity.
fix(llm): clamp thinking budget and sibling key in joint-budget clamp OpenHands/software-agent-sdk #4168 Review permalink 0ca2cda The review found the joint-budget clamp coherent and verified that both output-budget keys stay aligned after dynamic input accounting. It confirmed that thinking remains strictly below max_tokens and is removed when no legal minimum budget can fit. Because this changes core LLM request construction, it asks a human maintainer to perform lightweight evaluation validation before merging.
feat(hooks): discover hooks.json in .agents/ as well as .openhands/ OpenHands/software-agent-sdk #4327 Review permalink 305f73b The review found that centralizing hooks.json discovery across .agents and .openhands is coherent and backed by focused precedence tests. It requires a linked OpenHands docs pull request because this changes automatic SDK behavior and exports new public discovery symbols. It also calls for human behavior-level validation because the new precedence determines which command hooks run around agent actions.
Security: Unvalidated Workspace Directory in VSCode URL Endpoint OpenHands/software-agent-sdk #4282 Review permalink f76b710 The review confirmed that head f76b710b is tree-identical to the previously reviewed code, so the reported follow-up did not change the implementation. The router still dereferences a nonexistent VSCodeService.config attribute before its try block, causing every enabled URL request to fail. It asks for explicit workspace-root ownership and real route tests covering valid paths, escapes, symlinks, and URL encoding.
feat: drive the automation UI from the interface manifest OpenHands/OpenHands #16222 Review permalink c022f15 The review found that the production interface manifest replaces existing localized automation copy with English literals for every supported non-English locale. It also found that timeout admission permits maxima beyond the backend ceiling while lower maxima can block unrelated edits and display a fixed, inaccurate 1800-second error. It asks the author to preserve locale-aware rendering and enforce a timeout contract that matches both the server and PATCH semantics.
fix(acp): surface Claude Opus 5 in Claude Code model picker OpenHands/software-agent-sdk #4326 Review permalink 9653690 The review confirmed that the explicit Claude Opus 5 pin and unchanged default are compatible, with all 45 focused provider tests passing. It found that the versioned labels on the rolling opus[1m] and sonnet aliases misrepresent providers where those aliases still resolve to older models. It recommends stable generic alias labels while reserving version promises for explicit model pins.
feat(notifications): desktop notifications when the agent needs you OpenHands/OpenHands #16175 Review permalink 0c24ab8 The review found that notification clicks capture only the conversation ID, even though conversation identity is scoped by backend and organization. If the user switches backends before clicking a still-visible notification, the app can navigate to the ID under the wrong backend and show a 404 or an unrelated colliding conversation. The review asked the author to capture and restore the originating backend context and add a regression test before merging.
feat(transcript-export): bounded head+tail export for very large conversations OpenHands/OpenHands #16174 Review permalink 9848ab9 The review found the bounded head-and-tail export design coherent and well covered by focused loader and renderer tests. Cursor handling, deduplication, ordering, and omission notices preserve the existing full-export behavior for smaller conversations. The review classified the change as medium risk because it alters non-trivial pagination and large-history behavior, but found it worth merging.
Security: Unvalidated Workspace Directory in VSCode URL Endpoint OpenHands/software-agent-sdk #4282 Review permalink e8c7ab2 The review confirmed that head e8c7ab23 is tree-identical to the previously reviewed code, so the reported fix did not change the implementation. The router still dereferences a nonexistent VSCodeService.config attribute before its try block, causing every enabled URL request to fail. It asks for explicit workspace-root ownership and real route tests covering valid paths, escapes, symlinks, and URL encoding.
fix: make sidebar "Load more" discover folders, not folder contents OpenHands/OpenHands #16179 Review permalink 13f00a5 The review found that grouped Load more still stops after a page adds only hidden rows to an existing folder. Because the stable collapsed preview intentionally excludes those rows, the click produces no visible change and still requires another click to reveal the next folder. It asks the pagination driver and regression test to continue until a folder appears, pagination ends, or the existing safety cap is reached.
feat(conversations): add client-side conversation archive OpenHands/OpenHands #16100 Review permalink 2773b5e The review found that this dependent branch still contains the older destructive grouped-pagination implementation from #16179. Conversations arriving on later backend pages for an existing folder are removed from the group model and remain unreachable even after expansion. It asks the author to rebase onto the dependency’s latest complete-group and preview-only fix before merging.
Bound agent-server webhook delivery memory OpenHands/software-agent-sdk #4323 Review permalink 56eadb6 The review confirmed that the merge from main did not materially change the webhook implementation. Delivery remains single-flight with explicit serialized-byte and event-count limits for requests and queued work. Concurrency, retry, overflow, and packaged-image evidence support the bounded design.
fix: make overflow menus usable on touch devices OpenHands/OpenHands #16101 Review permalink c3c89fe The review verified that complete Tailwind candidates now compile into the required fine-hover CSS. Pointer-specific handling fixes hybrid touch and mouse behavior without device-wide assumptions. Focused tests cover generated styles, menu toggling, hover, and outside-click interactions.
feat: add conversation search via command menu OpenHands/OpenHands #16102 Review permalink e4eb62e The review found that the server narrows results to title matches before the client attempts multi-field filtering. This makes repository, branch, workspace, and model-only matches impossible despite the feature claims. The tests mask the defect by returning a mocked row that the real title-filtered backend would exclude.
fix(agent-server): bound webhook delivery memory with serialized batched draining OpenHands/software-agent-sdk #4323 Review permalink 3390901 The review found the bounded single-flight webhook queue simple and well matched to the memory-amplification failure. It preserves FIFO delivery and retry behavior while adding count and byte backpressure with focused concurrency, batching, and eviction coverage. No actionable correctness or security issue was found, though shared delivery behavior makes rollout medium risk.
feat(settings): add Pi ACP provider configuration UI OpenHands/OpenHands #16229 Review permalink 9d9d3e1 The review found that Pi is serialized as a custom ACP server without persisting its required launch command. The backend rejects that combination, so saved settings and profiles cannot reliably launch Pi, while the test currently encodes the invalid null command. It also flags the unpinned pi-acp runtime package because the latest release was only about one day old and inside the seven-day supply-chain waiting window.
Security: Unvalidated Workspace Directory in VSCode URL Endpoint OpenHands/software-agent-sdk #4282 Review permalink 74bb5e4 This review found that the route reads vscode_service.config.workspace_path even though VSCodeService has no config attribute. Because the access occurs before the try block, every enabled URL request fails before path validation can run. It asks for an explicit authoritative workspace root and API-level traversal tests instead of production assertions.
fix(acp): surface a 'credentials configured' banner state on Docker/cloud (#15643) OpenHands/OpenHands #16145 Review permalink 5ed8b5c This review found the new four-state ACP banner model simple and internally consistent. It preserves the distinction between stored credentials and a verified host login while sharing behavior across onboarding and Settings. Backend-aware secret queries, cache invalidation, cloud coverage, and an end-to-end regression test provide strong evidence.
docs: clarify quickstart URLs and next steps OpenHands/OpenHands #16227 Review permalink 5b251f1 The review verified that the Quickstart now pairs each launch path with its actual readiness signal and destination URL. The npm/source and Docker instructions match current launcher output, Docker's /canvas default, and live documentation links. It found no actionable defect and classified the docs-only change as low risk.
fix: reconcile non-native tool-call streamed <function=...> XML delta OpenHands/OpenHands #16220 Review permalink cc41ff1 The review confirmed that the latest commit scopes raw <function=...> XML reconciliation by sender before any streamed delta can be removed. A focused regression test preserves planning-agent output when a main-agent action arrives, while the original duplicate-rendering case remains covered. It found the prior blocker resolved and classified the shared streaming change as medium risk.
fix: launch automation with its installed uvicorn OpenHands/OpenHands #16216 Review permalink f0ef9ec The review confirmed that automation and Uvicorn now run inside the same isolated uv environment across development, npm, and Electron launch paths. Focused cross-platform tests and a real health-endpoint run verify the command shape and runtime behavior. It found no actionable defect and classified the shared launcher change as medium risk.
feat: report accumulated LLM cost in the automation completion callback OpenHands/software-agent-sdk #4311 Review permalink 495f2a4 The callback plumbing and focused tests are substantial, but the implementation sends the conversation’s lifetime accumulated cost rather than the current automation run’s delta. That overreports resumed or attached conversations because persisted stats retain prior spend, and the local close path also reads mutable state without the required lock. The review asks for a baseline-and-delta design, lock-safe snapshotting, and regression tests for resumed and concurrent cases.
Security: Unvalidated Workspace Directory in VSCode URL Endpoint OpenHands/software-agent-sdk #4282 Review permalink d8d8bb8 The changed router is not valid Python because reviewer prose and Markdown fences were committed into the module. Even after removing that text, the route reads vscode_service.config, but VSCodeService has no config attribute, so requests would fail. The review asks for a clean implementation using an authoritative workspace boundary plus route-level traversal tests and runtime evidence.
feat: add local Agent Canvas Planner support OpenHands/OpenHands #16178 Review permalink 121ea08 The review found that the latest slash-command commit does not fix the local planner’s generic-child identity assumption. The UI still treats the first sub-conversation as the planner and can route prompts, WebSocket state, and PLAN.md handling to the wrong child. It classified the integration as high risk and requested explicit planner identity, multi-child regression coverage, and a real local-stack run.
feat: link chat file paths to the Files drawer OpenHands/OpenHands #16181 Review permalink 0786454 The review confirmed that nested workspace normalization now removes the full active working directory before using the generic workspace fallback. Markdown links and existing navigate-to-file actions share the corrected path logic, backed by focused tests and concrete video evidence. It classified the shared navigation change as medium risk and found the prior blocker resolved.
feat(sdk): classify conversation errors OpenHands/software-agent-sdk #4316 Review permalink 52f70a7 The review confirmed that bare agent errors now remain unknown diagnostics instead of being mislabeled as recoverable outcomes. Knowledgeable producers explicitly classify tool validation, unexpected executor failures, and crash recovery, with telemetry regressions preserving those distinctions. It classified the additive public event contract as medium risk and found the earlier blocker resolved.
fix: PATCH /api/settings loads the profile's LLM when setting active_profile OpenHands/software-agent-sdk #4319 Review permalink a215ffc The review rechecked the latest comment-only cleanup and found no semantic change to the profile activation fix. Profile loading still precedes the locked transaction, invalid or unknown names fail before persistence, and explicit LLM diffs retain precedence. It classified the public secret-bearing settings change as medium risk and found it worth merging.
fix: reconcile non-native tool-call streamed <function=...> XML delta OpenHands/OpenHands #16220 Review permalink 3d61554 The review found that the new marker fallback ignores sender identity while main and planning agent streams share the same event store. A marker-bearing trailing delta from one agent can therefore be deleted when an action from the other agent arrives. It classified the live-stream reconciliation change as medium risk and requested sender scoping plus an opposite-sender regression test.
fix(config-service): surface OpenRouter and other model-derived providers in Basic picker OpenHands/OpenHands #16096 Review permalink ba34160 The review confirmed that the revised provider extraction surfaces OpenRouter while rejecting bare model IDs and unknown or region-like prefixes. The focused negative regression coverage and full green mock-LLM suite address the earlier malformed-provider concern. It found no remaining actionable defect and classified the local discovery change as low risk.
feat: add MiniMax M3 model selection OpenHands/OpenHands #16223 Review permalink d8f04f4 The review found that changing the canonical frontend model migrates every fresh or defaulted conversation to MiniMax M3. Unit tests and deterministic mock-LLM runs prove wiring but not real-provider availability, agent quality, latency, or cost, while repository guidance still names the old default. It classified the rollout as high risk and requested real-path evidence plus an explicit human maintainer decision.
fix(sdk): exclude boolean false from anyOf non-null type selection OpenHands/software-agent-sdk #4322 Review permalink f35eb1e The review found that filtering false only during type selection leaves a contradictory schema combining a concrete string type with an anyOf union that accepts no instances. The added regression test proves only that conversion does not throw and never checks the emitted schema semantics. It classified the MCP tool-registration boundary as medium risk and requested semantics-preserving normalization with stronger assertions.
fix(chat): allow dismissing failed pending user messages OpenHands/OpenHands #16142 Review permalink c990c19 The review found the failed-message dismissal flow small, direct, and consistent with the existing optimistic queue model. The new control removes the actual Zustand store entry while preserving retry and stop behavior, and focused tests exercise the rendered path. It found no actionable issue and classified the isolated UI change as low risk.
feat: link chat file paths to the Files drawer OpenHands/OpenHands #16181 Review permalink a09170f The review found that path normalization strips a generic /workspace prefix before the active working directory. Nested workspace roots can therefore resolve to a nonexistent Files drawer path and also regress existing navigate-to-file actions. It requested working-directory-first normalization and a nested-root regression test.
feat:上下文压缩模块+task list OpenHands/software-agent-sdk #4296 Review permalink d82a9e5 The review found that the context-compaction title masks a 334-file downstream product fork with more than 52,000 added lines and 213 commits. The branch adds PyroMind services, dependencies, deployment images, workflows, and knowledge assets while conflicting with upstream and providing no evidence. It classified the PR as high risk and requested a fresh, current, narrowly isolated context-compaction patch.
refactor(types): use client error events OpenHands/OpenHands #16214 Review permalink bfe118d The review confirmed that Canvas now consumes the published TypeScript client’s canonical agent and conversation error-event contracts instead of maintaining local duplicates. The client definitions and updated fixtures agree on their discriminated shapes, while the latest main-branch merge did not change the feature. It found no actionable defect and classified the type-ownership cleanup as low risk.
feat: drive the automation UI from the interface manifest OpenHands/OpenHands #16222 Review permalink 2182b3d The review found that interface-manifest admission accepts required flags, field types, and numeric minimums that the edit form does not enforce. The current counterpart manifest preserves existing behavior, but a future admitted manifest could promise constraints that the host silently violates. It requested either enforcing those semantics in the form or narrowing admission to exactly what the host implements.
feat: mock the setup contract from the published fixtures OpenHands/OpenHands #16221 Review permalink bbf20e4 The review found the fixture refactor simpler and correctly grounded in the pinned published automation contract. Canonical client error-event types remove local structural drift, while focused handlers cover success and failure exchanges. It found no actionable defect and classified the test-only change as low risk.
fix(config-service): surface OpenRouter and other model-derived providers in Basic picker OpenHands/OpenHands #16096 Review permalink bcc949d The review found that splitting every model identifier at a slash turns valid bare model names into provider entries. The agent-server contract explicitly returns bare identifiers, while its backend extraction logic is deliberately conservative to prevent bogus providers from reaching user interfaces. It requested authoritative provider validation and regression cases for bare and unknown model forms while preserving OpenRouter discovery.
Security: Unvalidated Workspace Directory in VSCode URL Endpoint OpenHands/software-agent-sdk #4282 Review permalink e96ebca The review found that the VSCode router tests still assert the old relative workspace behavior and now contradict the canonical absolute path passed by the route. It also identified duplicated manual URL reconstruction in the router despite the service already owning URL creation. It requested updated traversal and encoding tests plus a single owner for URL construction before relying on the security boundary.
feat(sdk): classify conversation errors OpenHands/software-agent-sdk #4316 Review permalink 0db6c2b The review found that AgentErrorEvent is used for both recoverable tool validation failures and unexpected executor or crash-recovery failures. Defaulting every instance to a retryable agent outcome would hide real first-party diagnostics from telemetry and downstream consumers. It requested conservative defaults, explicit classifications at knowledgeable producers, and telemetry regression tests.
Handle OpenAI device auth rate limits OpenHands/software-agent-sdk #3969 Review permalink 2b406e5 The review found that the final OpenAI token exchange still raises a plain runtime error for a provider rate limit. A 429 at that hop bypasses the new typed route boundary and can still surface as an internal server error. The review requested status-preserving exchange errors and a retry-state regression test before merge.
fix(settings): disable save when edits are reverted OpenHands/OpenHands #16120 Review permalink 2c04d86 The review confirmed that the pull-request-specific settings code is unchanged after merging the latest main branch. Its normalized authoritative baseline still disables no-op saves while preserving real edits, with focused unit and end-to-end coverage. The review found no actionable defect and retained a medium risk rating for the shared settings behavior.
fix: launch automation with its installed uvicorn OpenHands/OpenHands #16216 Review permalink 65eda94 The review found that the launcher now runs the automation package and Uvicorn inside one isolated uv environment. Focused command tests and a real health-endpoint run support the new release, version, and Git-reference paths. The review found no actionable defect and classified the shared launcher change as medium risk.
feat: report accumulated LLM cost in the automation completion callback OpenHands/software-agent-sdk #4311 Review permalink b676136 The review found that submitting stats publication to an executor does not guarantee it enters the FIFO lock queue before terminal error handling. Remote close can therefore report an earlier cached cost, while the required automation receiver change remains unmerged. The review classified this billing and completion-callback path as high risk and requested authoritative ordering plus receiver-first rollout enforcement.
fix(agent-server): handle non-ASCII filenames in Git diff endpoints OpenHands/OpenHands #16219 Review permalink d61d86f The review found that the current head does not contain the promised Unicode Git path fix. Its twelve-file diff instead adds unrelated credential encryption, enterprise MCP OAuth, observability, and persistence changes. The review classified this as high risk and asked the author to restore the intended branch before another review.
feat: add local Agent Canvas Planner support OpenHands/OpenHands #16178 Review permalink 374b91e The review found that the local planner infers planner identity from the first generic child conversation. A pre-existing delegated child can therefore be adopted as the planner and prevent the real planner from being created. The review classified this as high risk and requested an explicit typed planner identity plus multi-child regression coverage.
fix(agent-server): handle non-ASCII filenames in Git endpoints OpenHands/software-agent-sdk #4321 Review permalink e456771 The review found that the new Git path decoder only works for non-ASCII names at the repository root. Splitting before decoding loses Git's whole-path quote framing and leaves nested paths escaped. The review requested full-path decoding and a nested regression test before merge.
feat(security): AST-backed shell command-name resolution (#2721 Phase 2b) OpenHands/software-agent-sdk #3944 Review permalink a39a199 The review found the AST-based shell detector structurally stronger than extending regex special cases. It noted broad focused coverage and a passing local security suite. The change remains high risk because it directly controls destructive-command classification and needs human security review.
Security: Unvalidated Workspace Directory in VSCode URL Endpoint OpenHands/software-agent-sdk #4282 Review permalink 17ed2d6 The review found that the existing VSCode router test suite fails on the proposed path-validation changes. It asked for the contract assertions to be updated and for traversal-focused regression cases to be added. The review classified the change as medium risk because it modifies a workspace security boundary without passing focused tests.
fix(profiles): repair v1 skills migration OpenHands/software-agent-sdk #4320 Review permalink 7d40076 The review found the persisted profile schema change explicit and backward-compatible. It highlighted that the compatibility checker now follows the real model graph and that focused profile tests passed. The remaining risk is moderate because persisted configuration shapes can affect existing users.
fix: render the VSCode button on self-hosted (local) backends OpenHands/OpenHands #16106 Review permalink d477beb The review found a high-risk browser isolation regression in proxying OpenVSCode under the Canvas origin. Because Canvas persists all configured backend API keys in origin-wide localStorage, any editor-side script compromise can now read credentials unrelated to the current editor session, while a path prefix and Referrer-Policy do not isolate JavaScript storage. It recommended retaining a distinct editor origin or first removing browser-readable credentials from the shared origin and obtaining human security review.
feat(settings): add public from_persisted entry point OpenHands/software-agent-sdk #3503 Review permalink fc4dd4c The review found the loader implementation focused and well tested, including preservation of encrypted secret-bearing models. It asked the author to add and cross-link the required OpenHands/docs pull request because this introduces a public SDK API. Overall implementation risk is low once the required user documentation is supplied.
fix(security): redact git remote URL credentials in terminal output OpenHands/software-agent-sdk #4175 Review permalink cb0f0b0 The review found that the redactor treats any URL userinfo containing a masking placeholder as fully safe. A composite credential can therefore retain and expose a second secret after the GitHub token is replaced with a placeholder. The review asks for whole-userinfo masking unless it is exactly one placeholder, plus a regression test for the composite case.
fix: PATCH /api/settings loads the profile's LLM when setting active_profile OpenHands/software-agent-sdk #4319 Review permalink 93d363a The review traced active-profile resolution through profile loading, secret serialization, validation, file locking, and persisted settings updates. The implementation returns 404 before persistence for unknown profiles, preserves explicit LLM overrides, and passed all 151 targeted settings and profiles tests locally. No actionable defects were found, though the API semantic change and secret-bearing settings path make the overall risk medium.
refactor(types): use client error events OpenHands/OpenHands #16214 Review permalink 5143f0c The review confirmed that Canvas now consumes the published TypeScript client’s canonical AgentErrorEvent and ConversationErrorEvent contracts instead of retaining local duplicates. The published client definitions were compared with the SDK Pydantic models, while updated fixtures, build checks, and both 60-test E2E suites align with the discriminated shapes. No actionable defects were found, and the change was assessed as low risk because it is a type-ownership cleanup without runtime or dependency changes.
fix: render the VSCode button on self-hosted (local) backends OpenHands/OpenHands #16106 Review permalink 83e2271 The review found that full-stack development advertises the editor on the direct Vite origin without configuring the Vite proxy target, so the button falls through to the Canvas SPA on port 3001. It also reproduced a Docker validation bypass where an unnormalized Canvas base path collides with the normalized editor prefix and lets the editor route take over the application mount. The medium-risk routing change needs matched advertisement and route configuration plus normalization-aligned regression tests before merge.
fix: handle automation terminal statuses and card layout OpenHands/OpenHands #16213 Review permalink 7513bd5 The review confirmed that the frontend now models all six backend automation states and uses a defensive fallback for future status values. The badge, activity log, card layout, translations, and focused regression tests are updated consistently without new dependencies or sensitive behavior. The isolated change was assessed as low risk and worth merging.
fix: render the VSCode button on self-hosted (local) backends OpenHands/OpenHands #16106 Review permalink a206164 The review found that dev-extra-backend keeps the editor enabled while omitting the base-path route, so the new control can open another stack’s canvas or editor instead of its own workspace. It also found that Docker public mode deliberately omits the editor route but still receives an available capability from the shared agent-server, leaving a visible button that falls through to the SPA. Because these mismatches cross an API-key-bearing editor URL and shared launcher routing, the review classified the change as high risk and requiring an origin-aware capability fix plus runtime regression coverage.
fix(security): redact git remote URL credentials in terminal output OpenHands/software-agent-sdk #4175 Review permalink 3184927 The review confirmed that the fail-open registry exception path now applies deterministic token and URL masking, with all 44 focused tests passing. It found that successful registry masking is followed by the same two generic regex redactors again, causing duplicate full scans of every terminal observation and blurring ownership. The security fix is worth merging, with a focused cleanup recommended to reserve generic fallback redaction for absent or failing registries.
Security: Unvalidated Workspace Directory in VSCode URL Endpoint OpenHands/software-agent-sdk #4282 Review permalink 083f9a0 The review reproduced a Python syntax error caused by committed assistant prose and Markdown fences around vscode_router.py, preventing normal agent-server import. It also confirmed that the route still reads vscode_service.config even though the production VSCodeService has no config attribute, so the endpoint remains broken after parsing is restored. The high-risk security change needs clean Python, explicit workspace-root ownership, and production-shaped traversal tests before merge.
fix(settings): reject malformed LLM base URLs instead of saving them OpenHands/OpenHands #16192 Review permalink 58a5d73 The review found the URL validation centralized cleanly in the shared settings coercion path, covering both the standard settings save and embedded profile editor. Tests exercise malformed, valid, blank, whitespace-padded, and non-URL fields, while the pull request includes runtime user-interface evidence. No actionable defects were found; risk is medium because the shared coercion helper intentionally tightens accepted URL formats across URL-named settings.
feat(conversation): add conversation fork popouts OpenHands/OpenHands #16207 Review permalink 3d045e8 The review found that popout conversations still consume primary-conversation execution state, causing status and confirmation controls to reflect the wrong runtime. It also found global attachment and composer-command state shared by every mounted chat, allowing drafts or files to leak into another conversation or be cleared remotely. Because the tests mock away the real multi-composer boundary, the review classified the change as high risk and requiring state isolation plus integration coverage.
fix(sdk): reset parent_id when appending to rerun log OpenHands/software-agent-sdk #4315 Review permalink 71e080b The review found that resetting copied rerun actions to the rerun log root is the correct ownership boundary for an independent event tree. The focused regression test exercises realistic message-to-action ancestry, preserves the original event, and passes locally. With no public API or dependency changes, the review classified the fix as low risk and worth merging.
fix: preserve MCP credentials during Canvas mutations OpenHands/OpenHands #16144 Review permalink 61f499b The review found that non-OAuth authentication patches are discarded wholesale whenever any nested credential leaf remains redacted. It explained that adding a new header beside an unchanged redacted header can therefore report success while silently losing the new value, and requested leaf-wise header patches plus a regression test. Because the PR changes credential persistence across local and cloud backends, the review classified it as high risk and needing rework.
fix: handle terminal automation run statuses OpenHands/OpenHands #16180 Review permalink f92d8d3 The review found that the frontend now models both valid terminal automation statuses and renders each explicitly. It highlighted the unknown-status fallback as the right defense against future backend additions crashing the route. The review classified the isolated, tested presentation change as low risk and worth merging.
fix(settings): use "Default" label for Agent Profile badge OpenHands/OpenHands #16206 Review permalink 23ac532 The review found a narrowly scoped terminology fix that reuses the existing Default translation key. It noted that the focused test verifies the visible badge while leaving profile activation behavior untouched. The review classified the change as low risk and worth merging.
fix: make sidebar "Load more" discover folders, not folder contents OpenHands/OpenHands #16179 Review permalink 126ad25 The review found that grouped pagination discards conversations that arrive after a folder first appears. The folder expansion control can only reveal retained rows, and the added test codifies two loaded conversations remaining inaccessible. The implementation should separate stable collapsed previews from the complete loaded conversation collection.
feat: report accumulated LLM cost in the automation completion callback OpenHands/software-agent-sdk #4311 Review permalink 20ad653 The review found that fire-and-forget stats publication allows terminal errors to reach the client before the latest cost snapshot. A cached prior stats object can then be reported as authoritative, while the current automation receiver also rejects the new cost field until its coordinated schema change is deployed. The SDK needs authoritative completion ordering, a deterministic race regression, and a receiver-first rollout.
Security: Unvalidated Workspace Directory in VSCode URL Endpoint OpenHands/software-agent-sdk #4282 Review permalink 955e24f The review found that the VSCode router reads a config attribute that VSCodeService does not define. Every enabled URL request therefore raises before path validation, leaving the endpoint unusable. The review asks for explicit workspace-root ownership and real route tests covering default, allowed, traversal, and out-of-root paths.
feat: add a domain-neutral extension-manifest host OpenHands/OpenHands #16127 Review permalink c821b79 The review found the extension-manifest host coherent and appropriately fail-closed. Validation, request derivation, capability checks, preflight behavior, setup routing, and focused tests align around clear ownership boundaries. The first-party extension package bump is pinned with matching registry integrity and repository provenance.
ci: enable freeze-on-ready and update sdk-release skill OpenHands/software-agent-sdk #4313 Review permalink d0431ed The review found that none of the three updated reusable workflow callers ran on the exact head, so the new freeze path lacks runtime evidence. The first-consumer release state machine can create branches, mutate draft state, trigger publishing, and require a later merge-back, making syntax checks insufficient. The review asks for a disposable end-to-end canary and human release-infrastructure validation before merge.
fix(security): redact git remote URL credentials in terminal output OpenHands/software-agent-sdk #4175 Review permalink 0d2894a The review found that terminal redaction still fails open when the conversation secret registry masker raises. In that exception path, raw credential-bearing output bypasses both deterministic generic redactors and can reach the model. The PR needs a fail-closed fallback and a regression test before merging.
fix: identify PostHog users with Cloud identity OpenHands/OpenHands #16087 Review permalink 30fae19 The review confirmed that the previous stale cross-account telemetry attribution bug is fixed. Backend, organization, credential, and removal transitions now establish explicit identity boundaries with focused regression coverage. The change remains medium risk because it affects privacy-sensitive shared telemetry state, but it is worth merging.
feat: add a domain-neutral extension-manifest host OpenHands/OpenHands #16127 Review permalink 526948c The manifest-driven automation setup is well-structured, validates extension-authored data at a clear trust boundary, and preserves a fallback for catalog entries without setup manifests. Its local and cloud routing, preflight behavior, and form derivation are covered by focused tests, while the same-organization package update has npm provenance. No actionable defect was found, and overall risk is medium because the feature creates automations from extension-published manifests and depends on backend validation.
feat:上下文压缩模块+task list OpenHands/software-agent-sdk #4296 Review permalink 368d9a4 The fork merge adds broad Pyromind platform functionality but introduces multiple authentication and secret-management blockers. Default development mode trusts client-supplied identity headers, callback endpoints mutate run state without authentication, and deployment and JWT secrets are committed in source. The review recommends splitting the change, rotating secrets, enforcing callback authentication, and adding end-to-end security evidence.
fix(sdk): report remote conversation cost in automation callback OpenHands/software-agent-sdk #4311 Review permalink 45a1a12 The change wires cost into callbacks, but failed remote runs can close after an execution-status event before final stats arrive. Because cached stats from the initial snapshot still exist, the callback may report stale or zero cost rather than the current run’s accumulated cost. The review asks for lifecycle synchronization and a regression test covering real failure ordering.
fix(automations): render CANCELLED and SKIPPED run statuses OpenHands/OpenHands #16198 Review permalink fd3c87b The review found that CANCELLED and SKIPPED are represented consistently across the frontend status type, badge configuration, translations, and mock data. A neutral fallback prevents future backend statuses from crashing the automation detail page, and focused tests cover every known label and icon plus the unknown-status path. The isolated rendering correction was assessed as low risk and worth merging.
fix(home): don't gate the composer on the seeded default profile's LLM ref OpenHands/OpenHands #16200 Review permalink fa0167b The review found that the LLM preflight now uses the same local OpenHands default-profile rule as the conversation launch path. Focused regressions cover both the keyed active profile with an unauthenticated stale reference and the case where no usable active profile exists. The narrowly scoped decision change was assessed as low risk and worth merging.
fix: render the VSCode button on self-hosted (local) backends OpenHands/OpenHands #16106 Review permalink 086991e The review found that self-hosted VSCode is routed through the existing authenticated agent-server boundary rather than a newly exposed service. Launcher configuration, ingress ordering, proxy targeting, and user-interface gating are kept consistent with focused tests. The network-sensitive change was assessed as high risk and needs human end-to-end validation of unauthorized access, upgrade behavior, and the pinned agent-server image.
fix(settings): disable save when edits are reverted OpenHands/OpenHands #16120 Review permalink 404c9b4 The review found that save eligibility is derived from normalized form values compared with a refreshed persisted baseline. Hydration, successful saves, and revert-to-original behavior keep the baseline coherent without accumulating per-field dirty flags. The shared settings-state change was assessed as medium risk and worth merging.
Regenerate favicon PNG/ICO from favicon.svg for consistent design OpenHands/OpenHands #16202 Review permalink 0ea4fa2 The review verified that the regenerated PNG files have the expected dimensions and that the ICO contains the intended three image sizes. The raster assets match the SVG source without introducing code or dependency churn. The isolated static-asset update was assessed as low risk and worth merging.
feat: link chat file paths to the Files drawer OpenHands/OpenHands #16181 Review permalink 16ec2a4 The review found that workspace-path recognition and file-drawer navigation are centralized in one reusable component. The guarded call sites avoid nested interactive markup, while focused tests cover extraction, click behavior, and non-file text. The shared chat-rendering change was assessed as medium risk and worth merging.
fix: preserve MCP credentials during Canvas mutations OpenHands/OpenHands #16144 Review permalink 4a6b900 The review found that editing a canonical remote server drops its top-level protocol headers before the mandatory local connectivity test. A server that requires those headers can therefore fail the probe and never reach the sparse patch that would have preserved them. The credential-sensitive change was assessed as high risk and needs a regression test covering header-preserving edit, test, and save behavior.
Security: Unvalidated Workspace Directory in VSCode URL Endpoint OpenHands/software-agent-sdk #4282 Review permalink a57f820 The review found that the router still reads a config attribute that VSCodeService never defines, so every enabled URL request fails before validation. It also identified redundant URL reconstruction and assertions whose behavior is hidden by unrestricted mocks. The security-sensitive change was assessed as high risk and needs explicit workspace-root ownership plus real route and traversal tests.
Feat: structured output OpenHands/software-agent-sdk #4207 Review permalink d4b16e4 The review found no concrete implementation defect in schema persistence, repair, parsing, or compatibility paths. It noted that the change alters central agent decision behavior while the repository-required integration-test and benchmark gate has not run. The structured-output feature was assessed as medium risk and needs that behavioral evaluation recorded before merge.
Security: Unvalidated Workspace Directory in VSCode URL Endpoint OpenHands/software-agent-sdk #4282 Review permalink 37dccee The reviewed head accidentally commits assistant reasoning and Markdown fences into vscode_router.py, so the module fails Python compilation and can prevent the agent server from importing. Even after removing that wrapper, the route still reads vscode_service.config although VSCodeService exposes no config attribute, leaving the endpoint broken. The review asks for clean Python, explicit workspace-root ownership, and real route coverage before merge.
fix: render the VSCode button on self-hosted (local) backends OpenHands/OpenHands #16106 Review permalink fbf38d7 The review found that the Docker launcher can use different VSCode path and port overrides for agent-server and the proxy. Existing OH_VSCODE_BASE_PATH or OH_VSCODE_PORT settings can therefore make the new button open an unreachable route. The medium-risk fix needs one normalized configuration pair and a regression test covering overridden values.
fix(sdk): respect subscription validator composition OpenHands/software-agent-sdk #3953 Review permalink 6c21c17 The review confirmed that the current revision removes the duplicate public subscription state flagged previously. A scoped ContextVar now carries serialized input through Pydantic validator ordering while the private flag remains the single runtime source of truth. Focused regressions and green SDK, API compatibility, and persisted-settings checks support the medium-risk merge recommendation.
feat:上下文压缩模块+task list OpenHands/software-agent-sdk #4296 Review permalink 5dd1ccf The review found that a value shaped like a live OpenAI API key remains committed and must be revoked, removed, and purged from reachable history. It also found that the default development authentication mode accepts caller-controlled identity headers and verifies tokens with a source-controlled signing secret. The cross-cutting platform fork was assessed as high risk and needs security-owner review, runtime evidence, and decomposition before merging.
fix(settings): reject malformed LLM base URLs instead of saving them OpenHands/OpenHands #16192 Review permalink 1cc9a26 The review confirmed that malformed provider URLs are rejected at the shared schema boundary before settings persistence. Optional blank values and unrelated string settings retain their existing behavior, while the route now gives immediate field feedback. Focused tests and green end-to-end CI support the low-risk assessment and merge recommendation.
Support workspace image files in vision inspection OpenHands/software-agent-sdk #3948 Review permalink f69efca The review found that default relative remote workspace paths are still sent to an endpoint that requires absolute paths, so the primary remote workflow remains broken. It also identified a symlink-based containment bypass that can expose files outside the workspace to the vision model, plus inconsistent traversal normalization and a post-download size check. Focused tests passed, but their mocked download boundary does not exercise these failures.
docs: refresh AGENTS.md guidance OpenHands/OpenHands #16121 Review permalink 285789a The review confirmed that TelemetryProvider mounts the sole executable internal useTelemetry lifecycle, matching the new repository guidance. The one-line documentation change clearly prevents duplicate install and session telemetry without altering the public hook API. No actionable issue was found, and the documentation-only change was assessed as low risk.
fix(mcp): pin fetch server runtime dependencies OpenHands/software-agent-sdk #4303 Review permalink eecaf44 The review confirmed that sub-agent responses now use the canonical context-aware MCP serializer, keeping secrets redacted by default while permitting explicit plaintext output. The sparse patch model also includes enabled, and 128 focused tests passed locally across response serialization, OpenAPI, and settings behavior. The small but credential-sensitive API change was assessed as medium risk and worth merging.
fix: preserve MCP credentials during Canvas mutations OpenHands/OpenHands #16144 Review permalink b431656 The review found that same-strategy header-auth edits omit removed keys even though the backend recursively merge-patches nested maps. A header the user deletes can therefore remain stored and continue being sent to the MCP server, and the current patch schema does not accept null header tombstones. The credential-sensitive migration was assessed as high risk and needs atomic deletion semantics plus a real merge-path regression test.
Security: Unvalidated Workspace Directory in VSCode URL Endpoint OpenHands/software-agent-sdk #4282 Review permalink 05a5d2b The review found that the router accesses a config attribute that VSCodeService does not define, so every enabled URL request fails before validation runs. It also found that unconstrained mocks hide the service-contract mismatch and that no real route tests cover valid, default, or traversal paths. The security-sensitive change was assessed as high risk and needs corrected configuration ownership plus focused endpoint tests before merging.
ci: defer mock E2E to main and release PRs OpenHands/OpenHands #16088 Review permalink 35368aa The review confirmed that ordinary pull requests retain skipped required contexts while release pull requests and main still run the full suites. Removing change detection and comment plumbing makes the policy easier to reason about, and the evaluator covers the intended event matrix. The change was assessed as medium risk because full-stack failures move later, so maintainers should watch the first real main and release runs.
fix: route runtime services through server_info OpenHands/OpenHands #16090 Review permalink 0d496c2 The review found the backend-owned runtime metadata design coherent and compatible with older static bundles. Typed client access, host-keyed caching, proxy handling, and focused tests cover the new data flow without unnecessary abstractions. The prompt and shared-proxy changes were assessed as medium risk and still merit final human evaluation judgment.
fix: preserve MCP credentials during Canvas mutations OpenHands/OpenHands #16144 Review permalink d13d219 The review confirmed that the latest rename implementation preserves non-editor metadata and that auth-strategy changes are canonicalized by the released SDK. It found that switching between remote and stdio transports leaves fields from the outgoing variant persisted, including credentials hidden by the new form state. The credential-bearing migration was assessed as high risk and needs explicit variant cleanup plus end-to-end transition tests before merging.
fix: preserve MCP credentials during Canvas mutations OpenHands/OpenHands #16144 Review permalink 326f882 The review found that changing an MCP authentication strategy emits a nested merge patch that retains fields from the previous discriminated variant, causing the agent server to reject the save. It also found that renaming reconstructs a server from a lossy editor model and silently drops canonical fields such as description, icon, cwd, SSE read timeout, and keep-alive. The credential-bearing settings migration was assessed as high risk and needs atomic auth replacement plus lossless rename tests before merging.
fix: preserve MCP credentials during Canvas mutations OpenHands/OpenHands #16144 Review permalink 8c39d33 The review verified that the latest OAuth patch now preserves edited metadata and replacement secrets while omitting redacted leaves. The name-keyed sparse mutation design removes destructive full-catalog rewrites and uses the released first-party client package with green end-to-end coverage. No blocking code issue was found, though credential-bearing settings changes remain high risk and merit human review.
feat(security): supply-chain typosquat analyzer on shared shell parser OpenHands/software-agent-sdk #3563 Review permalink dc2f3eb The review found that GNU timeout, a common executable wrapper, is absent from the parser's wrapper model. As a result, timeout 30 npm install lodahs is treated as low risk even though it executes the typosquat install. The review recommends adding bounded timeout option parsing and regressions before relying on the analyzer.
fix(backend-registry): pick a healthy local backend as the fallback OpenHands/OpenHands #16173 Review permalink 2e50372 The review found the local-first fallback to be compact and deterministic while preserving explicit cloud selections. Focused regression tests cover cloud-first ordering, unhealthy local backends, all-unhealthy degradation, and registries without local entries. The isolated selection change was assessed as low risk and worth merging.
chore(ci): remove QA Changes workflows OpenHands/software-agent-sdk #4299 Review permalink 7b55f41 The review confirmed that the QA producer and its dedicated evaluator were removed together. No dangling workflow, trigger-label, or plugin references remained in the repository. The deletion was assessed as medium risk because it intentionally removes repository-wide automated QA coverage.
feat(llm): verify kimi-for-coding (Kimi Code membership) OpenHands/software-agent-sdk #4150 Review permalink 80fc7cf The review found the one-line model-list addition to be the smallest coherent SDK change. The PR supplied concrete live-endpoint evidence while leaving endpoint configuration to the companion client change. The discoverability-only update was assessed as low risk and worth merging.
chore(deps): bump nefrob/pr-description from 1.2.0 to 1.3.0 OpenHands/software-agent-sdk #4129 Review permalink b479e23 The review verified that the SHA pin matches the upstream release commit. The exact PR-description update step succeeded and rewrote the image block on the reviewed head. The established and directly exercised action bump was assessed as low risk and worth merging.
fix(task): propagate interruptions to active subagents OpenHands/software-agent-sdk #4108 Review permalink 7e9e507 The review found a startup race that can lose an interrupt before the child async task registers itself. It also found that forcing all subagents through the async LLM path breaks existing sync-only custom subclasses. The orchestration change was assessed as high risk and needs compatibility fixes, a deterministic race test, and human-confirmed lightweight evals.
chore(deps): bump lewagon/wait-on-check-action from 1.7.0 to 1.8.1 OpenHands/software-agent-sdk #4044 Review permalink 025046b The review verified that the SHA pin matches the established upstream release. The only workflow using the updated wait action was skipped, so current-head CI did not exercise the changed dependency. The bump was assessed as medium risk and needs a successful examples-workflow run before merging.
Feat: structured output OpenHands/software-agent-sdk #4207 Review permalink a5d6e71 The review found that the advertised schema drops object-level JSON Schema constraints such as dependentRequired even though runtime validation still enforces them. It also found that dynamic keys allowed by additionalProperties are misclassified as action arguments and rejected. The change was assessed as medium risk and needs one end-to-end schema contract plus human-confirmed lightweight evals.
docs: fix runtime modes table in architecture doc OpenHands/OpenHands #16163 Review permalink f4799d7 The review confirmed that all three replacement commands exist in package.json. Their descriptions match the launcher implementations and repository runtime guidance. No actionable issue was found, and the documentation-only change was assessed as low risk.
Security: Unvalidated Workspace Directory in VSCode URL Endpoint OpenHands/software-agent-sdk #4282 Review permalink 5c71c10 The review found that the router reads vscode_service.config even though VSCodeService has no config attribute, causing enabled URL requests to fail. It also called out unnecessary manual query reconstruction and the absence of a regression test using the real service boundary. The change was assessed as high risk and needs corrected configuration wiring plus focused path-validation tests.