fix(ci): repair source-shape checks - #2864
Merged
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughTest suite enhancements add a stubbed shell function injection into startup diagnostics and replace static regex-based tests with behavioral tests that validate runtime permission enforcement and chmod command execution on configuration directories. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/nemoclaw-start.test.ts`:
- Line 1433: The test currently stubs normalize_mutable_config_perms() with a
log string but never asserts it ran; update the startup-order expectation set to
include "ORDER:normalize" (the output from normalize_mutable_config_perms) at
the correct position before the gateway/configure expectations so the test fails
if the pre-gateway block stops invoking normalize_mutable_config_perms; locate
the stubbed function name normalize_mutable_config_perms and add
"ORDER:normalize" into the sequence of expected log entries/assertions that
verify startup order.
In `@test/repro-2681-group-writable.test.ts`:
- Around line 57-69: The test currently fakes root uid but still calls the real
stat, making normalize_mutable_config_perms() take the shields-up fast path on
runners that created the temp tree as root; update the spawnSync bash script
(the array passed to spawnSync in the test) to also stub stat to report a
non-root owner (similar to how the shields-up case stubs stat to "root") so
normalize_mutable_config_perms() exercises the chmod/find path. Specifically,
inside the command sequence used by the test (the block that defines id() and
calls normalize_mutable_config_perms), add a stat() shim that intercepts the
same stat invocation used by the code under test (stat -c '%U' "$config_dir")
and returns a non-root username (e.g., "notroot") while delegating other stat
calls to command stat "$@"; keep references to normalize_mutable_config_perms
and normalizeMutableConfigPermsFor so the change is made in the same spawnSync
command payload.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 87e50d53-d671-4659-bce7-e01cf89a273e
📒 Files selected for processing (2)
test/nemoclaw-start.test.tstest/repro-2681-group-writable.test.ts
| : 'id() { if [ "${1:-}" = "-u" ]; then printf "0"; elif [ "${1:-}" = "-g" ]; then printf "0"; else command id "$@"; fi; }', | ||
| 'emit_sandbox_sourced_file() { local target="$1"; cat > "$target"; chmod 444 "$target"; }', | ||
| 'verify_config_integrity_if_locked() { echo "ORDER:verify"; }', | ||
| 'normalize_mutable_config_perms() { echo "ORDER:normalize"; }', |
Contributor
There was a problem hiding this comment.
Assert the normalization step instead of only stubbing it.
Adding the stub fixes the 127 path, but the harness still never checks that normalize_mutable_config_perms actually ran. If the pre-gateway block stops invoking it, this test will keep passing. Please assert ORDER:normalize in the later startup-order expectation set, ideally before the gateway/configure steps.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/nemoclaw-start.test.ts` at line 1433, The test currently stubs
normalize_mutable_config_perms() with a log string but never asserts it ran;
update the startup-order expectation set to include "ORDER:normalize" (the
output from normalize_mutable_config_perms) at the correct position before the
gateway/configure expectations so the test fails if the pre-gateway block stops
invoking normalize_mutable_config_perms; locate the stubbed function name
normalize_mutable_config_perms and add "ORDER:normalize" into the sequence of
expected log entries/assertions that verify startup order.
Comment on lines
+57
to
+69
| const result = spawnSync( | ||
| "bash", | ||
| [ | ||
| "-c", | ||
| [ | ||
| "set -euo pipefail", | ||
| 'id() { if [ "${1:-}" = "-u" ]; then printf "0"; else command id "$@"; fi; }', | ||
| normalizeMutableConfigPermsFor(configDir), | ||
| "normalize_mutable_config_perms", | ||
| ].join("\n"), | ||
| ], | ||
| { encoding: "utf-8", timeout: 5000 }, | ||
| ); |
Contributor
There was a problem hiding this comment.
Make the “restores group-write” test independent of the runner’s file ownership.
This harness fakes id -u=0 but still uses the real stat -c '%U' "$config_dir". On any CI/container that creates the temp tree as root, normalize_mutable_config_perms() will take the shields-up fast path and skip the chmod/find calls, so this test becomes environment-dependent. Stub stat here to report a non-root owner, the same way the shields-up case stubs it to root.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/repro-2681-group-writable.test.ts` around lines 57 - 69, The test
currently fakes root uid but still calls the real stat, making
normalize_mutable_config_perms() take the shields-up fast path on runners that
created the temp tree as root; update the spawnSync bash script (the array
passed to spawnSync in the test) to also stub stat to report a non-root owner
(similar to how the shields-up case stubs stat to "root") so
normalize_mutable_config_perms() exercises the chmod/find path. Specifically,
inside the command sequence used by the test (the block that defines id() and
calls normalize_mutable_config_perms), add a stat() shim that intercepts the
same stat invocation used by the code under test (stat -c '%U' "$config_dir")
and returns a non-root username (e.g., "notroot") while delegating other stat
calls to command stat "$@"; keep references to normalize_mutable_config_perms
and normalizeMutableConfigPermsFor so the change is made in the same spawnSync
command payload.
cv
enabled auto-merge (squash)
May 1, 2026 22:57
cjagwani
approved these changes
May 1, 2026
21 tasks
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
3 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.
Summary
Updates the CI regression tests that were failing on main by stubbing the new startup permission-normalization call in the Telegram diagnostics harness and replacing source-text assertions with behavioral coverage. This keeps the source-shape budget at zero while preserving coverage for the #2681 mutable-permissions contract.
Changes
normalize_mutable_config_permsin the extracted pre-gateway startup test so the root entrypoint path no longer exits 127.test/repro-2681-group-writable.test.tsto exercise permission behavior through temporary shell execution and mockedunlockAgentConfigcalls instead of asserting on Dockerfile/source strings.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesmake docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
Tests