fix(onboard): accept managed runtime aliases - #9408
Conversation
d300b5c to
a69d907
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan includes up to 12 reviews per rolling hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe managed startup profile validator recognizes canonical runtime aliases under ChangesManaged startup runtime aliases
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🟡 Moderate · up to This change enables canonical managed runtime aliases during onboarding and targeted tests pass, but required sensitive-path review and quality-gate attestations are still incomplete, so the PR is not merge-ready until those checks are completed or explicitly accepted. Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
a69d907 to
0ba97a8
Compare
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
3 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 2 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite for the commit under review. Recommended E2E: None Manual-only E2E: 2 optional E2E recommendations
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
jyaunches
left a comment
There was a problem hiding this comment.
LOC Reduction / Codebase Simplicity Review
Why this blocks
managed-startup-runtime-alias.test.ts:12-22 copies both Slack alias definitions already owned by messaging/channels/slack/manifest.ts:16-31. The positive test at lines 45-49 therefore does not actually exercise the shipped stock Slack runtime aliases: it can remain green if the manifest changes, and its copied objects already omit the manifest's message field.
This leaves two owners for the accepted alias contract even though the new production validator is correctly generic.
Refactor direction
Derive the positive fixture from slackManifest.runtime.hermes.envAliases, then spread those same entries to construct the invalid variants. Remove the copied alias constants rather than maintaining a test-local mirror.
Expected result
The manifest remains the single alias owner, the positive test covers the real shipping contract, and the new validator stays generic with fewer fixture lines and no synchronization risk.
Signed-off-by: Deepak Jain <deepujain@gmail.com>
0ba97a8 to
ae992f4
Compare
|
Updated in ae992f4. The positive fixture now uses slackManifest.runtime.hermes.envAliases, and every invalid case is derived from the same shipping manifest entry. Focused regression: 8/8 passed; build, typecheck, and lint also passed. |
Resolved at ae992f4: the test now derives its aliases from the shipping Slack manifest and removes the copied fixture authority.
jyaunches
left a comment
There was a problem hiding this comment.
LOC Reduction / Codebase Simplicity Review
Resolved at ae992f43d56ecb81d2c0d04887f284df79f47464.
managed-startup-runtime-alias.test.ts now imports the shipping slackManifest, passes its runtime aliases directly to the positive case, and derives every invalid variant from the same manifest entry. The copied Slack alias objects are gone, and the test is 11 lines smaller.
I found no new blocking LOC-reduction or codebase-simplicity issue in the updated complete diff. This closes only the prior simplicity review; it is not an approval or a correctness, security, or CI review.
prekshivyas
left a comment
There was a problem hiding this comment.
Sensitive-path review completed at this head: all nine security areas pass. The alias exemption remains limited to the exact schema path, canonical envKey, exact match, and recognized placeholder; raw credential values are still rejected. I found no code-level issues, and 128 focused tests passed locally. I’m holding approval only because PR review advisor (Nemotron 3 Ultra) is still failing.
|
The rerun is complete at ae992f4: both Advisor lanes and the publish job now pass. |
prekshivyas
left a comment
There was a problem hiding this comment.
Approved after the Nemotron infrastructure rerun completed successfully. The exact reviewed head is unchanged, all reported checks are now passing or intentionally skipped, and the sensitive-path review remains valid.
<!-- markdownlint-disable MD041 --> ## Summary Managed onboarding rejected the stock Slack runtime aliases before sandbox startup. The validator now accepts only canonical runtime alias triples whose environment key, anchored OpenShell resolver expression, and approved placeholder all identify the same credential environment variable. ## Related Issue Fixes #9397 ## Changes - Recognize canonical entries only at `messaging.plan.runtimeSetup.envAliases[*]`. - Keep raw credentials, invalid environment names, malformed expressions, mismatched aliases, and placeholders at other paths rejected. - Add focused positive coverage for both stock Slack aliases and denial coverage for each neighboring failure mode. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [ ] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: Not applicable; `scripts/prepare-dgx-station-host.sh` is unchanged. - Station profile/scenario: Not applicable. - Result: Not applicable. - Supporting evidence: Not applicable. ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [ ] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run validate:pr` passed after refreshing `origin/main` when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — `vitest run --project cli src/lib/onboard/managed-startup-runtime-alias.test.ts src/lib/onboard/managed-startup-profile.test.ts` (128 passed) - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [ ] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Deepak Jain <deepujain@gmail.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added support for validating canonical messaging runtime environment aliases. * Valid aliases now support Slack bot and app credentials with matching placeholders. * Alias fields are accepted only in the expected runtime configuration paths. * **Bug Fixes** * Invalid alias formats, mismatched placeholders, raw credentials, and misplaced credential-shaped data are now rejected. * Improved validation helps prevent incorrectly configured messaging runtime aliases. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Signed-off-by: Deepak Jain <deepujain@gmail.com>
Summary
Managed onboarding rejected the stock Slack runtime aliases before sandbox startup. The validator now accepts only canonical runtime alias triples whose environment key, anchored OpenShell resolver expression, and approved placeholder all identify the same credential environment variable.
Related Issue
Fixes #9397
Changes
messaging.plan.runtimeSetup.envAliases[*].Type of Change
Quality Gates
DGX Station Hardware Evidence
scripts/prepare-dgx-station-host.shis unchanged.Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablevitest run --project cli src/lib/onboard/managed-startup-runtime-alias.test.ts src/lib/onboard/managed-startup-profile.test.ts(128 passed)npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: Deepak Jain deepujain@gmail.com
Summary by CodeRabbit
New Features
Bug Fixes