docs: add AGENTS.md with release, optional-peer, and repo-state notes#1736
Conversation
Records the barrel/optional-peer trap found while releasing v0.21.39: a module-scope import of an optional peer in any barrel-reachable file breaks clean-install consumers, and CI cannot observe it because the repo always has its own devDependencies installed. Points at the existing cut-release runbook rather than restating it, and notes the pre-existing prettier/tsc failures so the next contributor does not think they caused them. Signed-off-by: Ahmed Jamil <197998703+CodeAhmedJamil@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds root-level agent guidance for release procedures, package compatibility verification, optional peer constraints, repository health notes, and document maintenance. Replaces ChangesAgent guidance
Estimated code review effort: 1 (Trivial) | ~3 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@AGENTS.md`:
- Around line 28-30: Update the build validation command in AGENTS.md to create
a package tarball after building, install or require that tarball from a
temporary consumer directory, and run the check there without the repository’s
optional peer dependencies. Replace the direct repo-tree require while
preserving the intended module-load verification.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
|
Heads-up: #1737 merged and shipped in
Separately, the "repo state that looks broken but is pre-existing" section is accurate and was genuinely useful while working #1737 - the ~82 prettier failures are still there, and Also filed #1739 for a related but distinct trap in the same family: |
The documented command required ./dist/index.js from inside the repo tree, so Node resolved the optional peers out of the repo's own node_modules - the check passed unconditionally and proved nothing, which is the exact failure mode the section warns about. Pack the build and require it from a throwaway consumer instead: npm installs required peers but not optional ones, reproducing a downstream clean install. Also record how to verify a published release by content rather than by the version number moving - the extension contract export and the absence of a module-scope optional-peer require are what the meshery-extensions chain depends on - and note that current-version should be read from the npm latest dist-tag rather than inferred from semver ordering. Command block verified verbatim against the published 0.21.40 tarball: optional peers absent, barrel loads. Signed-off-by: Ahmed Jamil <197998703+CodeAhmedJamil@users.noreply.github.com>
|
Addressed CodeRabbit's finding in 0df0e4c — it was correct and the impact was larger than Also folded in two release-verification notes learned while cutting this release: verify a published version by content (extension contract export present, no module-scope optional-peer require) rather than by the version number moving, and resolve "what is current" from the npm Still docs-only — @copilot review |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@AGENTS.md`:
- Around line 44-45: Update the optional-peer validation commands in AGENTS.md
to enumerate every optional peer declared in package.json, including date-fns
and `@mui/x-date-pickers`, and exit nonzero when any are installed before running
the `@sistent/sistent` require check. Preserve the require test only after
confirming all optional peers are absent.
- Around line 35-37: Update the AGENTS.md guidance around the throwaway consumer
check to distinguish it from the CI guard in optionalPeerDependencies.test.ts.
State that the CI test detects eager optional-peer imports at source level,
while the packed build loaded from a clean consumer validates built/published
behavior; present them as complementary checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Two corrections to the optional-peer section. 'CI cannot see this' went stale when #1737 landed src/__testing__/optionalPeerDependencies.test.ts, which scans src/ for load-time imports of an enforced optional peer and fails jest. Describe the two guards as complementary instead: the test covers source, the packed-consumer check covers what the bundler emitted and what is actually on npm - neither subsumes the other. Point at the test as the source of truth for which peers are enforced, since it is where the react / react-dom exemption is recorded. The absence check only looked for date-fns and only warned, so it could print a warning and then report a passing load with @mui/x-date-pickers still installed - a false pass in exactly the situation the check exists to catch. It now walks the enforced peers and exits nonzero. Deliberately not enumerating every peer under peerDependenciesMeta: react and react-dom are marked optional there but are imported at module scope by every component, so requiring their absence could never pass. That exemption is documented in the test rather than invented here. Verified verbatim against the published 0.21.40 tarball: clean consumer loads (exit 0); with date-fns force-installed it aborts (exit 1). Signed-off-by: Ahmed Jamil <197998703+CodeAhmedJamil@users.noreply.github.com>
Adds
AGENTS.md(withCLAUDE.mdsymlinked to it) capturing knowledge that cost real time to rediscover while cutting v0.21.39.The substantive entry is the barrel/optional-peer trap.
src/index.tsxre-exports nearly everything, so a module-scope import of an optional peer in any reachable file makesimport { anything } from '@sistent/sistent'throw for consumers who did not install it - naming sistent rather than the missing peer. #1732 fixed this for@mui/x-date-pickers; #1735 is the remainingdate-fnsinstance.The reason it is worth writing down: CI cannot catch it, because this repo always has its own devDependencies installed. It only appears in a downstream clean install. The note includes the one-line check that does catch it.
Also records:
.claude/skills/cut-release/SKILL.mdrather than restating itprettier --check(~82 files) andtsc --noEmitfail repo-wide today and are not CI gates, so contributors do not assume they broke something or mass-reformat to "fix" itNo code or config change.
Signed-off-by: Ahmed Jamil 197998703+CodeAhmedJamil@users.noreply.github.com
Summary by CodeRabbit