fix(installer): scope agent process conflicts by user - #10689
fix(installer): scope agent process conflicts by user#10689prekshivyas wants to merge 2 commits into
Conversation
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueComment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall line coverage in commit 9bed1d7 in the TypeScript / code-coverage/cliThe overall line coverage in commit 9bed1d7 in the Show a line coverage summary of the most impacted files.
Updated |
| vllm_port="$((10#$vllm_port))" | ||
| processes="$(ps -eo pid=,ppid=,comm=,args=)" | ||
| agent_matches="$(awk -v self="$$" -v parent="$PPID" ' | ||
| processes="$(ps -eo euid=,pid=,ppid=,comm=,args=)" |
There was a problem hiding this comment.
[P1][Test migration] This changes the mocked ps contract to euid pid ppid comm args, but test/install/install-station-container-coexistence.test.ts:209 and :361 still emit the old pid ppid comm args rows; three other local ps fixtures in that file use the same stale shape. Impact: the tests now interpret the helper PID as owner_uid and skip the row for the wrong reason, so Docker mutation quiescence coverage can stay green without exercising the self/parent exclusion or current parser layout. Smallest safe fix: prefix the affected rows with $EUID and shift the remaining fields to match this five-column schema. Regression: run the coexistence suite and retain its Docker-boundary assertions with correctly shaped current-user process data.
| ` | ||
| MODE=--check | ||
| ps() { printf '999 1 openshell openshell gateway\n'; } | ||
| ps() { printf '%s 999 1 openshell openshell gateway\n%s 1000 1 node node /home/other/.nemoclaw/openshell gateway\n' "$EUID" "$((EUID + 1))"; } |
There was a problem hiding this comment.
[P1][Security regression coverage] This fixture combines same-user PID 999 with cross-user PID 1000, then expects the same-user fatal path. Impact: it never proves the changed allow case; a separate cross-user blocking path or state could remain and this test would still pass as long as PID 1000 is omitted from the first error. Smallest safe fix: add a separate behavior test with only a different-EUID openshell or nemoclaw row, an empty ss, and a direct check_agent_and_inference_conflicts call; assert exit 0, agent_inference_workloads=none, and no agent-workload error. Regression: keep this same-user denial case and make the cross-user-only success path a checked-in test.
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Requesting changes at exact head 65b78f3. The effective-UID production filter is direct and preserves host-wide vLLM and Docker checks, but two current-head P1 test blockers leave the safety contract unproven: (1) the changed test combines same-user and cross-user agents and therefore never proves that a cross-user-only agent permits progress; (2) Docker-mutation coexistence fixtures still emit the old four-column process rows, so they skip helper rows for the wrong reason under the new five-column parser. Add the separate successful cross-user-only regression and migrate the affected process fixtures. Exact-head evidence: 72 focused installer and coexistence tests pass, a direct synthetic cross-user-only probe exits 0 with agent_inference_workloads=none, diff hygiene is clean, the commit is GitHub Verified, and DCO, required CI, CodeQL, all nine Advisor specialists, and the full automated matrix are green. CodeRabbit is paused because this is a draft. Candidate-specific GB300 validation or explicit maintainer acceptance of the recorded exception is also still required before the draft becomes ready for review or merge.
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Re-reviewing exact head 9bed1d74c17ade9108874d997aedab9e77c52ace. This is a merge-from-main head; the two PR files are byte-for-byte unchanged from the previously reviewed head, so both P1 blockers remain:
- [P1] The current test combines a same-user agent with a cross-user agent, then exits on the same-user blocker. It never proves the accepted #10649 behavior that a cross-user agent by itself permits preparation. Please add a separate cross-user-only success regression. Existing thread: #10689 (comment)
- [P1] Five coexistence fixtures still emit the old four-column
psrows while the production parser now requireseuid,pid,ppid,comm,args. Those fixtures bypasscheck_agent_and_inference_conflictsand can let Docker-mutation tests pass for the wrong reason. Update every affected row and assert the intended mutation/baseline gate is reached. Existing thread: #10689 (comment)
Exact-head local evidence: build:cli passed; 53 host-preparation tests passed; 19 container-coexistence tests passed; Vitest project membership is exact; diff hygiene passed. DCO is green and both commits are GitHub Verified. All nine current-head Advisor specialists failed before analysis because the shared inference endpoint returned HTTP 429; I safely reran only those infrastructure failures. CodeRabbit is paused while this is draft. Candidate-specific GB300 validation, or explicit maintainer acceptance of that validation exception, also remains required before this can leave draft or merge.
|
PR Review Advisor finished for commit |
2 participants
Outcome
Station Express now ignores NemoClaw and OpenShell process signatures owned by another effective user. It still blocks the invoking user's agent processes and host-wide vLLM conflicts.
Reason
The host-preparation process scan did not include ownership. On a shared host, another account's unrelated NemoClaw or OpenShell process could therefore block installation.
Related issues
Fixes #10649
Changes
Verification
npm run validate:pr- passed against67d6abeb811296f22cf5c00df5c0fd5536f27bf1for candidate65b78f3a1961028ff0556d6fe168f59d62c91b98.agent_inference_workloads=none.Review notes
65b78f3a1961028ff0556d6fe168f59d62c91b98.Signed-off-by: Prekshi Vyas prekshiv@nvidia.com