refactor(cli): share the banner renderer through a shared .cts boundary - #8589
Open
harjothkhara wants to merge 5 commits into
Open
refactor(cli): share the banner renderer through a shared .cts boundary#8589harjothkhara wants to merge 5 commits into
harjothkhara wants to merge 5 commits into
Conversation
The renderBox terminal-banner renderer was duplicated byte-for-byte in src/lib/cli/banner.ts (CLI) and nemoclaw/src/banner.ts (plugin). The two copies had already drifted in a comment, and a change to one was invisible to the other package's tests. Move the one implementation into nemoclaw/src/shared/banner-boundary.cts, following the existing shared-boundary pattern (sandbox-name.cts, openshell-policy-boundary.cts, snapshot-sanitizer-boundary.cts). Both banner.ts files become thin re-export wrappers, so the ../cli/banner and ./banner.js import paths stay stable for their consumers. Register the .cjs -> .cts source alias in the three required places (vitest.config.ts, nemoclaw/vitest.project.ts, and the alias contract test test/plugin-vitest-project.test.ts) and add the boundary to nemoclaw/tsconfig.shared.json. Refs: NVIDIA#8291 Signed-off-by: harjoth <harjoth.khara@gmail.com>
Contributor
|
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 (4)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe pull request moves banner rendering and its public types into a shared canonical boundary. Existing wrappers delegate to that implementation. TypeScript and Vitest include and alias the boundary. Tests validate rendering behavior, wrapper identity, and generated artifacts. ChangesBanner boundary consolidation
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
…rappers The source banner tests alias the .cjs boundary back to its .cts source, so they cannot catch a missing or stale compiled nemoclaw/dist/shared/banner-boundary.cjs. Add a package-contract test that imports the built CLI and plugin wrappers by file URL, renders through the real generated boundary, and asserts both wrappers render identically and that the .cjs and .d.cts artifacts ship. Mirrors the openshell-policy-boundary package contract. Refs: NVIDIA#8291 Signed-off-by: harjoth <harjoth.khara@gmail.com>
Contributor
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
8 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. 3 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 against this exact revision. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
harjothkhara
marked this pull request as ready for review
August 7, 2026 22:53
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/package-contract/banner-boundary.test.ts (1)
29-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winProve that the plugin wrapper loads the canonical boundary.
Lines 43-45 only compare output for one input. A stale or reintroduced plugin-local renderer can produce the same output and pass this test.
Import
nemoclaw/dist/shared/banner-boundary.cjsand assert thatplugin.renderBoxis its exportedrenderBoxfunction. This proves that the superseded plugin path cannot execute.Proposed test change
const cli = await importBuilt("dist", "lib", "cli", "banner.js"); const plugin = await importBuilt("nemoclaw", "dist", "banner.js"); + const pluginBoundary = await importBuilt( + "nemoclaw", + "dist", + "shared", + "banner-boundary.cjs", + ); + + expect(plugin.renderBox).toBe(pluginBoundary.renderBox);As per path instructions, “Migration tests must prove the superseded path is unreachable or removed, not merely prove that the new path also works.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/package-contract/banner-boundary.test.ts` around lines 29 - 45, Update the test around the built plugin wrapper import to also import the canonical boundary module from nemoclaw/dist/shared/banner-boundary.cjs and assert that plugin.renderBox is exactly its exported renderBox function. Keep the existing output comparison, using the identity assertion to prove the superseded plugin-local renderer cannot be used.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/package-contract/banner-boundary.test.ts`:
- Around line 29-45: Update the test around the built plugin wrapper import to
also import the canonical boundary module from
nemoclaw/dist/shared/banner-boundary.cjs and assert that plugin.renderBox is
exactly its exported renderBox function. Keep the existing output comparison,
using the identity assertion to prove the superseded plugin-local renderer
cannot be used.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8dbbeb32-c91c-41d9-83bc-4f93d607b72b
📒 Files selected for processing (8)
nemoclaw/src/banner.tsnemoclaw/src/shared/banner-boundary.ctsnemoclaw/tsconfig.shared.jsonnemoclaw/vitest.project.tssrc/lib/cli/banner.tstest/package-contract/banner-boundary.test.tstest/plugin-vitest-project.test.tsvitest.config.ts
…ction Comparing rendered output alone would still pass if a plugin-local renderer were reintroduced. Assert plugin.renderBox is the boundary's exported function, so the superseded path cannot execute. Refs: NVIDIA#8291 Signed-off-by: harjoth <harjoth.khara@gmail.com>
harjothkhara
marked this pull request as draft
August 8, 2026 00:55
…t test Address epic NVIDIA#8281's negative-net-LOC rule and NVIDIA#8291's requirement that existing parity tests become direct shared-core contract tests. - Replace src/lib/cli/banner.test.ts and nemoclaw/src/banner.test.ts (two copies testing two wrappers) with one nemoclaw/src/shared/banner-boundary.test.ts that imports the boundary directly. - Rewrite the package-contract test to compare the real compiled dist artifacts in a native Node subprocess. Under Vitest the source alias maps the .cjs boundary to its .cts source, so an in-process compare proves source==source, not that the shipped wrappers load the generated boundary; the subprocess bypasses the alias and asserts both built wrappers share one function. - Add the sourceBoundary tag and point regressionTest at the direct test. Net delta is now negative. Refs: NVIDIA#8291 Signed-off-by: harjoth <harjoth.khara@gmail.com>
Assert the built wrappers' exports are callable and render correctly, not just reference-equal, so the identity check cannot pass vacuously. Add a subprocess timeout. Refs: NVIDIA#8291 Signed-off-by: harjoth <harjoth.khara@gmail.com>
harjothkhara
marked this pull request as ready for review
August 8, 2026 01:39
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
1 participant
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
The
renderBoxterminal-banner renderer was duplicated byte-for-byte insrc/lib/cli/banner.ts(CLI) andnemoclaw/src/banner.ts(plugin). Move the one implementation into a shared.ctsboundary and reduce both files to thin re-export wrappers, so the two packages render the banner from one source. No behavior changes.Related Issue
Refs #8291 — one slice of "Use shared .cts boundaries for pure CLI and plugin parity code": the third bullet, "Share the banner renderer while retaining package entry wrappers." The other bullets are separate PRs.
Changes
nemoclaw/src/shared/banner-boundary.ctswithrenderBox,BannerLine, andRenderBoxOptions, following the existing boundary pattern (sandbox-name.cts,openshell-policy-boundary.cts,snapshot-sanitizer-boundary.cts).src/lib/cli/banner.tsandnemoclaw/src/banner.tsto re-export wrappers. Their import paths (../cli/banner,./banner.js) stay stable forsrc/lib/tunnel/services.tsandnemoclaw/src/index.ts.nemoclaw/tsconfig.shared.jsonsobuild:clicompiles it before both packages..cjs→.ctssource alias in the three required places:vitest.config.ts,nemoclaw/vitest.project.ts, and the alias contract testtest/plugin-vitest-project.test.ts.src/lib/cli/banner.test.ts,nemoclaw/src/banner.test.ts) into one direct shared-core contract testnemoclaw/src/shared/banner-boundary.test.ts— the change Use shared .cts boundaries for pure CLI and plugin parity code #8291 asks for.test/package-contract/banner-boundary.test.ts. Under Vitest the source alias maps everybanner-boundary.cjsspecifier to the.ctssource, so an in-process compare would prove source-equals-source. This test runs a native Node subprocess to compare the real compiled dist artifacts and assert both shipped wrappers resolve to the one generatedrenderBox, plus that the.cjs/.d.ctsartifacts ship.Net delta is −2 LOC, per epic #8281's negative-net-LOC rule. The two former copies had already drifted in a comment; the consolidated boundary keeps the more general wording ("long URLs or endpoints"). The boundary header carries a
sourceBoundary:tag noting the renderer only sizes and truncates — callers own content safety.Type of Change
Quality Gates
renderBoxoutput is byte-identical and no public API, CLI, config, or output changes. A docs review found no stale references.Documentation Writer Review
no-docs-neededdocs/,*.md, and*.mdxforrenderBox,banner-boundary, the boundary list, andsandbox-name.ctsreturned zero stale references. The onlybannerhits in docs describe the runtime registration banner, which is unchanged. No contributor or architecture doc enumerates the shared boundaries.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/tunnel/services.test.ts --project plugin nemoclaw/src/shared/banner-boundary.test.ts nemoclaw/src/register.test.ts --project integration test/plugin-vitest-project.test.ts --project e2e-supportandvitest run --project package-contract test/package-contract/banner-boundary.test.ts→ all pass.typecheck:cliand plugin typecheck pass.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: Not run in full. The change adds one additive test alias mirroring three existing ones; the affected lanes (cli, plugin, integration alias contract, e2e-support) and both typechecks pass. Twopackage-contractfailures (onboard-stdin-eof,credentials-cli-command) are pre-existing on cleanorigin/mainand unrelated to this change.Proof
Drift hazard removed — before, on
origin/main(two copies), mutating only the CLI copy (drop the two-space safety gap) failed just the CLI suite; the plugin suite was blind. Now there is one source.nemoclaw/src/shared/banner-boundary.ctsfails the shared-core test (Tests 1 failed | 4 passed), then passes once reverted.renderBoxfunction; removingnemoclaw/dist/shared/banner-boundary.cjsfails it (Cannot find module ...), and it passes once built.Limitations
e2e.yaml, the restore action). Extending those tobanner-boundary.cjsrequires bumping the restore action's immutable content-pin, an integrity anchor. That is left to the maintainers; the boundary still ships in the artifact tar (the wholenemoclaw/dist/shareddirectory is packaged).Signed-off-by: Harjoth Khara harjoth.khara@gmail.com
Summary by CodeRabbit