feat(anchor-pnpm): new CLI for managing pnpm-workspace.yaml anchors#293
feat(anchor-pnpm): new CLI for managing pnpm-workspace.yaml anchors#293stephansama wants to merge 3 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR introduces Changesanchor-pnpm package
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
core/anchor-pnpm/__snapshots__/tsnapi/cli.snapshot.d.tsOops! Something went wrong! :( ESLint: 10.2.1 Error: Cannot find module '/node_modules/@stephansama/eslint-config/dist/index.mjs' ... [truncated 540 characters] ... 265:11) core/anchor-pnpm/src/anchors.tsOops! Something went wrong! :( ESLint: 10.2.1 Error: Cannot find module '/node_modules/@stephansama/eslint-config/dist/index.mjs' ... [truncated 540 characters] ... 265:11) core/anchor-pnpm/src/cli.tsOops! Something went wrong! :( ESLint: 10.2.1 Error: Cannot find module '/node_modules/@stephansama/eslint-config/dist/index.mjs' ... [truncated 540 characters] ... 265:11)
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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
@stephansama/ai-commit-msg
@stephansama/alfred-kaomoji
@stephansama/anchor-pnpm
@stephansama/astro-iconify-svgmap
@stephansama/auto-readme
@stephansama/catppuccin-jsonresume-theme
@stephansama/catppuccin-opml
@stephansama/catppuccin-rss
@stephansama/catppuccin-typedoc
@stephansama/catppuccin-xsl
@stephansama/eslint-config
create-stephansama-example
@stephansama/find-makefile-targets
@stephansama/github-env
@stephansama/multipublish
@stephansama/pnpm-hooks
@stephansama/prettier-plugin-handlebars
@stephansama/remark-asciinema
@stephansama/single-file
@stephansama/svelte-social-share-links
@stephansama/typed-env
@stephansama/typed-events
@stephansama/typed-nocodb-api
@stephansama/typed-templates
@stephansama/types-github-action-env
@stephansama/types-lhci
commit: |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces @stephansama/anchor-pnpm, a CLI tool for managing YAML anchor declarations in pnpm-workspace.yaml to streamline version management across catalogs. The implementation includes core logic for adding, listing, updating, removing, and syncing anchors, along with a command-line interface and unit tests. Feedback highlights several critical logic issues: applySyncProposal needs to verify anchor versions to prevent data corruption, the sync function requires logic to handle duplicate or existing anchor names, and the remove function should check for an anchor's existence before applying side effects. Additionally, the README should be updated to reflect that the sync command is already implemented.
| export function applySyncProposal( | ||
| document: Document, | ||
| proposal: SyncProposal, | ||
| ): void { | ||
| add(document, proposal.anchorName, proposal.version); | ||
| const catalogs = document.get(CATALOGS_KEY); | ||
| if (!isMap(catalogs)) return; | ||
| const group = catalogs.get(proposal.catalog); | ||
| if (!isMap(group)) return; | ||
|
|
||
| for (const entry of group.items) { | ||
| if (!isPair(entry) || !isScalar(entry.key)) continue; | ||
| if (!proposal.keys.includes(String(entry.key.value))) continue; | ||
| entry.value = new Alias(proposal.anchorName); | ||
| } | ||
| } |
There was a problem hiding this comment.
In applySyncProposal, the result of add is ignored. If add fails because an anchor with the same name already exists but has a different version, the function will proceed to alias catalog entries to that incorrect version. This can cause silent data corruption where packages are pinned to the wrong version.
It's safer to verify that the anchor exists and has the expected version before applying aliases.
export function applySyncProposal(
document: Document,
proposal: SyncProposal,
): void {
const { added } = add(document, proposal.anchorName, proposal.version);
if (!added) {
const existing = list(document).find((e) => e.name === proposal.anchorName);
if (existing?.value !== proposal.version) return;
}
const catalogs = document.get(CATALOGS_KEY);
if (!isMap(catalogs)) return;
const group = catalogs.get(proposal.catalog);
if (!isMap(group)) return;
for (const entry of group.items) {
if (!isPair(entry) || !isScalar(entry.key)) continue;
if (!proposal.keys.includes(String(entry.key.value))) continue;
entry.value = new Alias(proposal.anchorName);
}
}| export function sync(document: Document): SyncProposal[] { | ||
| const catalogs = document.get(CATALOGS_KEY); | ||
| if (!isMap(catalogs)) return []; | ||
|
|
||
| const proposals: SyncProposal[] = []; | ||
|
|
||
| for (const pair of catalogs.items) { | ||
| if (!isPair(pair) || !isScalar(pair.key) || !isMap(pair.value)) | ||
| continue; | ||
| const catalogName = String(pair.key.value); | ||
| const versionToKeys = new Map<string, string[]>(); | ||
|
|
||
| for (const entry of pair.value.items) { | ||
| if (!isPair(entry) || !isScalar(entry.key)) continue; | ||
| const value = entry.value; | ||
| if (!isScalar(value) || value.anchor) continue; | ||
| const versionString = String(value.value); | ||
| const keyString = String(entry.key.value); | ||
| versionToKeys.set(versionString, [ | ||
| ...(versionToKeys.get(versionString) ?? []), | ||
| keyString, | ||
| ]); | ||
| } | ||
|
|
||
| for (const [version, keys] of versionToKeys) { | ||
| if (keys.length < 2) continue; | ||
| proposals.push({ | ||
| anchorName: catalogName, | ||
| catalog: catalogName, | ||
| keys, | ||
| version, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| return proposals; | ||
| } |
There was a problem hiding this comment.
The sync function has a bug where it generates duplicate anchorName values if a single catalog contains multiple distinct versions that are each repeated. It also fails to account for existing anchor names in the __versions section. This can lead to data corruption when applying proposals, as multiple versions might end up aliased to the same (first) anchor.
I suggest tracking existing anchor names and ensuring each proposal gets a unique name.
export function sync(document: Document): SyncProposal[] {
const catalogs = document.get(CATALOGS_KEY);
if (!isMap(catalogs)) return [];
const proposals: SyncProposal[] = [];
const existingAnchors = new Set(list(document).map((e) => e.name));
for (const pair of catalogs.items) {
if (!isPair(pair) || !isScalar(pair.key) || !isMap(pair.value))
continue;
const catalogName = String(pair.key.value);
const versionToKeys = new Map<string, string[]>();
for (const entry of pair.value.items) {
if (!isPair(entry) || !isScalar(entry.key)) continue;
const value = entry.value;
if (!isScalar(value) || value.anchor) continue;
const versionString = String(value.value);
const keyString = String(entry.key.value);
versionToKeys.set(versionString, [
...(versionToKeys.get(versionString) ?? []),
keyString,
]);
}
for (const [version, keys] of versionToKeys) {
if (keys.length < 2) continue;
let anchorName = catalogName;
let counter = 1;
while (existingAnchors.has(anchorName)) {
anchorName = `${catalogName}-${counter++}`;
}
existingAnchors.add(anchorName);
proposals.push({
anchorName,
catalog: catalogName,
keys,
version,
});
}
}
return proposals;
}| - `anchor-pnpm add <name> <version>` — append a new `&<name> <version>` entry. | ||
| - `anchor-pnpm update <name> <version>` — set the scalar value of an existing anchor (all `*<name>` aliases follow automatically). | ||
| - `anchor-pnpm remove <name>` — remove the anchor; errors if aliases still reference it (pass `--force` to inline them first). | ||
| - `anchor-pnpm sync` — _scheduled, not yet implemented._ Auto-detect catalog groups with repeated versions and convert them to anchor+alias pairs. |
There was a problem hiding this comment.
The README states that anchor-pnpm sync is "not yet implemented", but the implementation is included in this pull request. This should be updated to reflect the current status.
| - `anchor-pnpm sync` — _scheduled, not yet implemented._ Auto-detect catalog groups with repeated versions and convert them to anchor+alias pairs. | |
| - `anchor-pnpm sync` — Auto-detect catalog groups with repeated versions and convert them to anchor+alias pairs (use `--apply` to write changes). |
| export function remove( | ||
| document: Document, | ||
| name: string, | ||
| options: { force?: boolean } = {}, | ||
| ): { dangling: string[]; removed: boolean } { | ||
| const seq = getVersionsSeq(document); | ||
| if (!seq) return { dangling: [], removed: false }; | ||
|
|
||
| const dangling = collectAliasPaths(document, name); | ||
| if (dangling.length > 0 && !options.force) { | ||
| return { dangling, removed: false }; | ||
| } | ||
|
|
||
| if (dangling.length > 0 && options.force) { | ||
| const scalar = findAnchor(seq, name); | ||
| replaceAliasesWithLiteral(document, name, scalar?.value); | ||
| } | ||
|
|
||
| const index = seq.items.findIndex( | ||
| (item) => isScalar(item) && item.anchor === name, | ||
| ); | ||
| if (index === -1) return { dangling: [], removed: false }; | ||
| seq.items.splice(index, 1); | ||
| return { dangling: [], removed: true }; | ||
| } |
There was a problem hiding this comment.
The remove function should verify the anchor's existence in the __versions section before performing any side effects like replacing aliases. Currently, if force is true but the anchor is missing from __versions, it replaces aliases with undefined (resulting in null in YAML) and then returns removed: false, leaving the document in an inconsistent state.
export function remove(
document: Document,
name: string,
options: { force?: boolean } = {},
): { dangling: string[]; removed: boolean } {
const seq = getVersionsSeq(document);
if (!seq) return { dangling: [], removed: false };
const index = seq.items.findIndex(
(item) => isScalar(item) && item.anchor === name,
);
if (index === -1) return { dangling: [], removed: false };
const scalar = seq.items[index] as Scalar;
const dangling = collectAliasPaths(document, name);
if (dangling.length > 0 && !options.force) {
return { dangling, removed: false };
}
if (dangling.length > 0 && options.force) {
replaceAliasesWithLiteral(document, name, scalar.value);
}
seq.items.splice(index, 1);
return { dangling: [], removed: true };
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@core/anchor-pnpm/README.md`:
- Line 34: Update the README entry for the anchor-pnpm CLI: change the
`anchor-pnpm sync` line to reflect that the command is implemented (remove
"scheduled, not yet implemented") and briefly describe its behavior (auto-detect
catalog groups with repeated versions and convert them to anchor+alias pairs),
so users are not misled; locate the `anchor-pnpm sync` text in
core/anchor-pnpm/README.md and replace the outdated status text with a short,
accurate description of the shipped functionality.
- Around line 38-42: The fenced code block showing pnpm flags is missing a
language specifier; update the opening fence from ``` to use a language label
(e.g., ```text or ```console) so the block is lint-safe (MD040) and preserves
plaintext formatting—edit the fenced block that contains the lines starting with
"--workspace, -w", "--dry-run, -d", and "--verbose, -v" in the README.
In `@core/anchor-pnpm/src/anchors.ts`:
- Around line 116-123: sync() currently assigns the same anchorName (anchorName:
catalogName) for every repeated-version group, causing collisions when multiple
duplicated versions exist; change sync() to generate deterministic,
non-conflicting anchor names for each proposal (e.g., include a stable suffix
derived from the version or an incremental index: use catalogName + ":" +
versionHash or catalogName + "#" + groupIndex) and populate proposals with that
unique anchorName (refer to versionToKeys, proposals, catalogName, version).
Additionally add a collision guard in applySyncProposal that, before rewriting
values, resolves any existing anchorName and aborts with an error if it points
to a different version than the proposal (i.e., check resolvedVersion vs
proposal.version), so applySyncProposal will not silently overwrite a mismatched
anchor.
- Around line 79-89: The current logic calls replaceAliasesWithLiteral(document,
name, scalar?.value) when options.force is true before verifying the anchor
actually exists, which can rewrite aliases even if no anchor is removed; change
the ordering so you first locate the anchor (use seq.items.findIndex / isScalar
with anchor === name) and only if index !== -1 proceed to call
replaceAliasesWithLiteral with the real scalar value and then splice seq.items
and return removed: true; if the anchor is not found, return removed: false
without touching aliases.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7fdf174c-8962-4bd1-a6f4-a76cc51787f3
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (15)
README.mdcore/anchor-pnpm/README.mdcore/anchor-pnpm/__snapshots__/tsnapi/cli.snapshot.d.tscore/anchor-pnpm/__snapshots__/tsnapi/cli.snapshot.jscore/anchor-pnpm/__snapshots__/tsnapi/index.snapshot.d.tscore/anchor-pnpm/__snapshots__/tsnapi/index.snapshot.jscore/anchor-pnpm/package.jsoncore/anchor-pnpm/src/anchors.tscore/anchor-pnpm/src/cli.tscore/anchor-pnpm/src/index.tscore/anchor-pnpm/src/workspace.tscore/anchor-pnpm/test/anchors.test.tscore/anchor-pnpm/tsconfig.jsoncore/anchor-pnpm/tsdown.config.tscore/anchor-pnpm/vitest.config.ts
| - `anchor-pnpm add <name> <version>` — append a new `&<name> <version>` entry. | ||
| - `anchor-pnpm update <name> <version>` — set the scalar value of an existing anchor (all `*<name>` aliases follow automatically). | ||
| - `anchor-pnpm remove <name>` — remove the anchor; errors if aliases still reference it (pass `--force` to inline them first). | ||
| - `anchor-pnpm sync` — _scheduled, not yet implemented._ Auto-detect catalog groups with repeated versions and convert them to anchor+alias pairs. |
There was a problem hiding this comment.
Fix outdated sync command status in docs.
Line 34 says sync is not implemented, but this PR ships it. This will mislead users about available functionality.
Suggested doc correction
-- `anchor-pnpm sync` — _scheduled, not yet implemented._ Auto-detect catalog groups with repeated versions and convert them to anchor+alias pairs.
+- `anchor-pnpm sync` — detect catalog groups with repeated versions and convert them to anchor+alias pairs (`--apply` writes the proposal).🤖 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 `@core/anchor-pnpm/README.md` at line 34, Update the README entry for the
anchor-pnpm CLI: change the `anchor-pnpm sync` line to reflect that the command
is implemented (remove "scheduled, not yet implemented") and briefly describe
its behavior (auto-detect catalog groups with repeated versions and convert them
to anchor+alias pairs), so users are not misled; locate the `anchor-pnpm sync`
text in core/anchor-pnpm/README.md and replace the outdated status text with a
short, accurate description of the shipped functionality.
| ``` | ||
| --workspace, -w Path to pnpm-workspace.yaml (default: auto-detect via @manypkg/find-root) | ||
| --dry-run, -d Print the rewritten YAML to stdout instead of writing | ||
| --verbose, -v Show detailed output | ||
| ``` |
There was a problem hiding this comment.
Add a language to the fenced code block.
Line 38 opens a fence without a language (MD040), which will keep markdown lint warnings active.
Suggested lint-safe fence
-```
+```text
--workspace, -w Path to pnpm-workspace.yaml (default: auto-detect via `@manypkg/find-root`)
--dry-run, -d Print the rewritten YAML to stdout instead of writing
--verbose, -v Show detailed output</details>
<!-- suggestion_start -->
<details>
<summary>📝 Committable suggestion</summary>
> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
```suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 38-38: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@core/anchor-pnpm/README.md` around lines 38 - 42, The fenced code block
showing pnpm flags is missing a language specifier; update the opening fence
from ``` to use a language label (e.g., ```text or ```console) so the block is
lint-safe (MD040) and preserves plaintext formatting—edit the fenced block that
contains the lines starting with "--workspace, -w", "--dry-run, -d", and
"--verbose, -v" in the README.
| if (dangling.length > 0 && options.force) { | ||
| const scalar = findAnchor(seq, name); | ||
| replaceAliasesWithLiteral(document, name, scalar?.value); | ||
| } | ||
|
|
||
| const index = seq.items.findIndex( | ||
| (item) => isScalar(item) && item.anchor === name, | ||
| ); | ||
| if (index === -1) return { dangling: [], removed: false }; | ||
| seq.items.splice(index, 1); | ||
| return { dangling: [], removed: true }; |
There was a problem hiding this comment.
Prevent forced removal from mutating aliases when the anchor is missing.
On Line 79–82, aliases are inlined before confirming the anchor exists. If the anchor is absent, Line 87 returns removed: false after already rewriting aliases (potentially to null), which is data corruption.
Safe ordering fix
export function remove(
document: Document,
name: string,
options: { force?: boolean } = {},
): { dangling: string[]; removed: boolean } {
const seq = getVersionsSeq(document);
if (!seq) return { dangling: [], removed: false };
const dangling = collectAliasPaths(document, name);
if (dangling.length > 0 && !options.force) {
return { dangling, removed: false };
}
+ const scalar = findAnchor(seq, name);
+ if (!scalar) return { dangling, removed: false };
+
if (dangling.length > 0 && options.force) {
- const scalar = findAnchor(seq, name);
- replaceAliasesWithLiteral(document, name, scalar?.value);
+ replaceAliasesWithLiteral(document, name, scalar.value);
}
const index = seq.items.findIndex(
(item) => isScalar(item) && item.anchor === name,
);
if (index === -1) return { dangling: [], removed: false };
seq.items.splice(index, 1);
return { dangling: [], removed: true };
}🤖 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 `@core/anchor-pnpm/src/anchors.ts` around lines 79 - 89, The current logic
calls replaceAliasesWithLiteral(document, name, scalar?.value) when
options.force is true before verifying the anchor actually exists, which can
rewrite aliases even if no anchor is removed; change the ordering so you first
locate the anchor (use seq.items.findIndex / isScalar with anchor === name) and
only if index !== -1 proceed to call replaceAliasesWithLiteral with the real
scalar value and then splice seq.items and return removed: true; if the anchor
is not found, return removed: false without touching aliases.
| for (const [version, keys] of versionToKeys) { | ||
| if (keys.length < 2) continue; | ||
| proposals.push({ | ||
| anchorName: catalogName, | ||
| catalog: catalogName, | ||
| keys, | ||
| version, | ||
| }); |
There was a problem hiding this comment.
sync() can generate conflicting anchor names for different versions.
Line 119 uses anchorName: catalogName for every repeated-version group in that catalog. If one catalog has multiple duplicated versions, proposals collide on the same anchor and --apply can rewrite entries to the wrong version.
Direction for deterministic non-conflicting names
proposals.push({
- anchorName: catalogName,
+ anchorName: `${catalogName}_${version.replace(/[^A-Za-z0-9_-]/g, "_")}`,
catalog: catalogName,
keys,
version,
});🤖 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 `@core/anchor-pnpm/src/anchors.ts` around lines 116 - 123, sync() currently
assigns the same anchorName (anchorName: catalogName) for every repeated-version
group, causing collisions when multiple duplicated versions exist; change sync()
to generate deterministic, non-conflicting anchor names for each proposal (e.g.,
include a stable suffix derived from the version or an incremental index: use
catalogName + ":" + versionHash or catalogName + "#" + groupIndex) and populate
proposals with that unique anchorName (refer to versionToKeys, proposals,
catalogName, version). Additionally add a collision guard in applySyncProposal
that, before rewriting values, resolves any existing anchorName and aborts with
an error if it points to a different version than the proposal (i.e., check
resolvedVersion vs proposal.version), so applySyncProposal will not silently
overwrite a mismatched anchor.
1 participant
Closes STE-187
New
@stephansama/anchor-pnpmpackage atcore/anchor-pnpm/. CLI for managing YAML anchor declarations inpnpm-workspace.yamlwhile preserving anchors / aliases / comments via theyamlpackage's Document API.Commands shipped
anchor-pnpm list— print every&<name>entry with its resolved versionanchor-pnpm add <name> <version>— append to__versionsanchor-pnpm update <name> <version>— change a scalar value; aliases follow automaticallyanchor-pnpm remove <name>— delete, with--forceto inline any*<name>aliases firstanchor-pnpm sync— scan catalogs for repeated versions and print proposed anchor+alias pairs (--applyto commit them)Global flags:
--workspace/-w(override path),--dry-run/-d(print rewritten YAML, don't write),--verbose/-v.Tests:
test/anchors.test.ts— 9 cases covering list / add / update / remove (with dangling-alias detection) / sync / applySyncProposal. All passing.Acceptance criteria
listprints all anchor name → version pairsaddinserts a new entry into__versionsupdatechanges the scalar value of an existing anchorremoveerrors on dangling aliases unless--forceis passedsyncdetects catalog groups with shared versions and writes anchors + aliases--dry-run