Skip to content
PR Quorum
Sign inStart free
← Review gallery
microsoft/SkillOpt #148openreviewed 5h ago

feat(devin): add SessionEnd hook + install script

by ichoosetoaccept+128 157 filesreviewed in 1m 36sView PR on GitHub
needs attentionThe panel surfaced issues that deserve attention before merge.3 findings · 2 high+

Reviewer panel

Correctness
deepseek/deepseek-v4-pro
errored
Security
deepseek/deepseek-v4-pro
1 found
Architecture
deepseek/deepseek-v4-pro
2 found

Findings

01Hhighconf 95%

jq merge overwrites existing SessionEnd hooks instead of merging arrays

plugins/devin/install.sh:35

The install script uses jq -s '.[0] * .[1]' to merge the new SessionEnd hook into an existing hooks.v1.json. The * operator recursively merges objects, but SessionEnd is an array. When the existing config already contains a SessionEnd key (populated by another plugin), the merge will silently replace its array with the new one, discarding existing hooks. This contradicts the stated goal of preserving "other project hooks" and can lead to lost functionality. A safe merge should concatenate the SessionEnd arrays while still deep-merging other keys.

02Hhighconf 80%

Command injection via DEVIN_PROJECT_DIR environment variable

plugins/devin/hooks/hooks.v1.json:9

The hook command embeds the environment variable ${DEVIN_PROJECT_DIR} without any sanitisation. If this variable contains shell metacharacters (e.g. "; rm -rf /; ") and the hook subsystem evaluates the command string through a shell, an attacker who can influence DEVIN_PROJECT_DIR could execute arbitrary commands with the privileges of the Devin process. Even if the variable is normally set by Devin, external configuration or other hooks might allow contamination.

03Mmedconf 85%

sys.path modified without tearDown restoration risks test isolation

tests/test_devin_plugin.py:67

The test test_env_tilde_is_expanded inserts the devin plugin path at index 0 via sys.path.insert(0, PLUGIN) but never removes it. If other tests in the same module (or order-dependent runs) import mcp_server afterwards, they may unintentionally load the devin version where the copilot version was expected, causing flaky failures or hard-to-debug import issues. The change should either restore the original sys.path in a tearDown or limit the path alteration to a context manager.

Want this on your own pull requests?
The same reviewer panel runs on every PR — one focused review, no noise.
Start freeLive demo