Notice Suppression & Persistent Exclude Management#157
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a persistent Changesintent exclude command, notice suppression, and exclude flag consolidation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
View your CI Pipeline Execution ↗ for commit ded082f
☁️ Nx Cloud last updated this comment at |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/intent/tests/cli.test.ts (2)
883-936: ⚡ Quick winAdd install-path notice suppression tests to match the new CLI contract.
Line 883 and Line 909 cover suppression for
list, but this PR also wires suppression intoinstall. Add at least oneinstall --mapcase for--no-noticesand one forINTENT_NO_NOTICESto protect that path from regressions.🤖 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 `@packages/intent/tests/cli.test.ts` around lines 883 - 936, The test file currently only covers notice suppression for the list command (the two it() blocks shown). You need to add two more test cases to cover the install command path with notice suppression. Add a new it() test that calls main() with install --map and --no-notices flags, verifying that stderr does not contain 'Notices:' and 'intent.skills is not set'. Add another new it() test that sets INTENT_NO_NOTICES=1 and calls main() with install --map (without --no-notices), verifying the same stderr expectations. Both new tests should follow the same setup pattern as the existing list tests: create temp directories, write an installed intent package using writeInstalledIntentPackage, set process.env.INTENT_GLOBAL_NODE_MODULES, and verify the exit code is 0 and appropriate output is captured.
909-936: ⚡ Quick winExpand env-var suppression coverage to all documented true-like values.
Line 909 currently validates only
INTENT_NO_NOTICES='1'. The suppression contract also includestrue,yes, andon(with normalization), so this leaves a regression gap for documented behavior.Suggested test shape
+ it.each(['1', 'true', 'yes', 'on', ' TRUE ', 'Yes'])( + 'suppresses notices when INTENT_NO_NOTICES=%s', + async (value) => { + const root = mkdtempSync( + join(realTmpdir, 'intent-cli-list-env-no-notices-matrix-'), + ) + const isolatedGlobalRoot = mkdtempSync( + join(realTmpdir, 'intent-cli-list-env-no-notices-matrix-empty-global-'), + ) + tempDirs.push(root, isolatedGlobalRoot) + writeInstalledIntentPackage(root, { + name: '`@tanstack/query`', + version: '5.0.0', + skillName: 'fetching', + description: 'Query data fetching patterns', + }) + + process.env.INTENT_GLOBAL_NODE_MODULES = isolatedGlobalRoot + process.env.INTENT_NO_NOTICES = value + process.chdir(root) + + const exitCode = await main(['list']) + const stderr = errorSpy.mock.calls.flat().join('\n') + + expect(exitCode).toBe(0) + expect(stderr).not.toContain('intent.skills is not set') + expect(stderr).not.toContain('Notices:') + }, + )🤖 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 `@packages/intent/tests/cli.test.ts` around lines 909 - 936, The test at line 909 in packages/intent/tests/cli.test.ts only validates INTENT_NO_NOTICES suppression for the value '1', but the documented behavior also supports 'true', 'yes', and 'on'. Expand the test coverage by parameterizing the existing test case to run with all four documented true-like values, ensuring each value properly suppresses notices by verifying that stderr does not contain the notice-related output. This can be done by either creating separate test cases for each value or using a parameterized test approach (such as test.each() if supported by the test framework), maintaining the same test structure and assertions for each iteration.
🤖 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 `@packages/intent/tests/cli.test.ts`:
- Around line 883-936: The test file currently only covers notice suppression
for the list command (the two it() blocks shown). You need to add two more test
cases to cover the install command path with notice suppression. Add a new it()
test that calls main() with install --map and --no-notices flags, verifying that
stderr does not contain 'Notices:' and 'intent.skills is not set'. Add another
new it() test that sets INTENT_NO_NOTICES=1 and calls main() with install --map
(without --no-notices), verifying the same stderr expectations. Both new tests
should follow the same setup pattern as the existing list tests: create temp
directories, write an installed intent package using
writeInstalledIntentPackage, set process.env.INTENT_GLOBAL_NODE_MODULES, and
verify the exit code is 0 and appropriate output is captured.
- Around line 909-936: The test at line 909 in packages/intent/tests/cli.test.ts
only validates INTENT_NO_NOTICES suppression for the value '1', but the
documented behavior also supports 'true', 'yes', and 'on'. Expand the test
coverage by parameterizing the existing test case to run with all four
documented true-like values, ensuring each value properly suppresses notices by
verifying that stderr does not contain the notice-related output. This can be
done by either creating separate test cases for each value or using a
parameterized test approach (such as test.each() if supported by the test
framework), maintaining the same test structure and assertions for each
iteration.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e68c9060-5dd6-4d32-8318-8e18e888322a
📒 Files selected for processing (15)
.changeset/crisp-flies-mate.md.changeset/green-dingos-refuse.mddocs/cli/intent-exclude.mddocs/cli/intent-install.mddocs/cli/intent-list.mddocs/cli/intent-load.mddocs/concepts/configuration.mddocs/config.jsonpackages/intent/src/cli-output.tspackages/intent/src/cli-support.tspackages/intent/src/cli.tspackages/intent/src/commands/exclude.tspackages/intent/src/commands/install.tspackages/intent/src/commands/list.tspackages/intent/tests/cli.test.ts
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
Adds two user-facing capabilities to the
intentCLI and removes the legacy--excludeflag in favor of a persistent command.1. Notice suppression
Migration/risk notices (printed to stderr when
intent.skillsis unset) can now be silenced:--no-noticesflag onlistandinstall— suppresses notices for a single run.INTENT_NO_NOTICES=1env var — suppresses notices for the whole shell session (also acceptstrue,yes,on).Notices remain on stderr, separate from warnings (stdout), so
loadoutput stays machine-safe.2.
intent excludecommand (replaces--excludeflag)Excludes are now a persistent, first-class command that edits
package.json#intent.exclude:intent exclude list— show current patterns.intent exclude add <pattern>— add a pattern (validated viacompileExcludePatterns).intent exclude remove <pattern>— remove a pattern.--jsonfor machine-readable output.The old runtime
--excludeflag onlist/loadhas been removed, along with theexcludefield fromGlobalScanFlagsand its mapping incoreOptionsFromGlobalFlags. Discovery/filtering behavior is unchanged — the change is purely how excludes are configured (persistent vs. per-invocation).Changes
New files
packages/intent/src/commands/exclude.ts—intent excludecommand implementation.docs/cli/intent-exclude.md— CLI reference for the new command.Modified
packages/intent/src/cli-output.ts— addedNoticeOutputOptions, notice-suppression logic (INTENT_NO_NOTICES+noNotices), gatedprintNotices.packages/intent/src/cli-support.ts— addednoNotices/noticesflags andnoticeOptionsFromGlobalFlags; removedexcludefield and mapping.packages/intent/src/cli.ts— registeredexclude; added--no-noticestolist/install; removed--excludefromlist/load.packages/intent/src/commands/list.ts,install.ts— thread notice options intoprintNotices.docs/cli/intent-list.md,docs/cli/intent-load.md,docs/concepts/configuration.md,docs/overview.md,docs/config.json— documented suppression +intent exclude; removed--excludereferences.Breaking change
The
--excludeflag onlistandloadis removed. Migrate tointent exclude add <pattern>, which persists the pattern inpackage.json#intent.exclude.Summary by CodeRabbit
New Features
intent excludecommand withlist,add, andremoveactions to persistently manage excluded packages and skills in your configuration.--no-noticesflag tointent listandintent installto suppress non-critical notices.INTENT_NO_NOTICESenvironment variable for CI/automation workflows.Documentation