Repository navigation
Harden agent UX, Sentry gating, and bundle rebuilds - #95
Conversation
Track the June 1 package ecosystem/SDK audit in pm-z0ip and apply the concrete fixes found by the local and sub-agent review lanes. Agent UX: make pm get --depth brief omit body, linked shells, and claim-state history; compact validate resolution diagnostics by default while preserving --verbose-diagnostics. Correctness: use stable object equality for linked-test JSON assertions and runtime metadata comparisons so key insertion order does not create false mismatches. Reliability: stop bundle-cli from deleting the live bundle before rebuilds, preventing transient missing chunk failures in shared checkouts; update dogfood coverage for the new brief contract. Observability: keep expected handled CLI failures out of Sentry exception capture by default and have the Sentry gate ignore/report existing expected CLI error issues separately from crash issues. Docs: clarify standalone extension entrypoints must be import-free unless packaged with SDK dependencies. Verification: focused regression suite (7 files, 164 tests), full coverage (135 files, 2092 tests, 100% thresholds), typecheck, static quality, docs/skills, secret scan, changelog check, pm health, telemetry stack health, and linked pm tests passed locally.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
@codex review /gemini review @gemini-code-assist review |
|
Warning Review limit reached
More reviews will be available in 2 minutes and 2 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
📝 WalkthroughWalkthroughThis PR applies audit findings to compact brief-get output, add deterministic stable equality, reduce default diagnostic verbosity, refine Sentry gating/handled-error capture, harden concurrent bundle builds, update extension docs, and extend tests and audit metadata. ChangesCLI UX and output hardening
Build safety and extension documentation
Audit task tracking and metadata
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 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. 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 |
![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 several optimizations and bug fixes to the CLI and SDK ecosystem. Key changes include optimizing get --depth brief to omit body, linked, and claim_state to reduce token usage, replacing order-sensitive JSON.stringify comparisons with stableStringify across filters and assertions, suppressing expected handled CLI errors from Sentry telemetry, and preventing bundle-cli.mjs from deleting the live bundle directory during concurrent rebuilds. Additionally, default validation diagnostics are now more compact. Review feedback suggests importing readdir and unlink in scripts/bundle-cli.mjs to clean up stale chunks after the build completes.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| #!/usr/bin/env node | ||
|
|
||
| import { readFile, rm, writeFile } from "node:fs/promises"; | ||
| import { readFile, writeFile } from "node:fs/promises"; |
There was a problem hiding this comment.
To support cleaning up stale chunks after the build completes, import readdir and unlink from node:fs/promises.
import { readdir, readFile, unlink, writeFile } from "node:fs/promises";
There was a problem hiding this comment.
Code Review
This pull request hardens the package ecosystem SDK and CLI agent UX by optimizing token usage in get --depth brief, ensuring order-insensitive object equality with stableStringify, making default validation diagnostics more compact, and preventing transient runtime errors during concurrent builds. The feedback highlights a few key areas for improvement: first, the reduced diagnostic limit permanently truncates lifecycle diagnostics because buildLifecycleCheck does not respect verboseDiagnostics; second, removing the directory cleanup in bundle-cli.mjs will accumulate stale chunk files over time; and third, performance can be optimized by adding fast-path strict equality checks before falling back to stableStringify in comparison helpers.
| const GIT_LS_FILES_MAX_BUFFER = 32 * 1024 * 1024; | ||
| const FILE_LIST_SUMMARY_LIMIT = 40; | ||
| const DIAGNOSTIC_LIST_SUMMARY_LIMIT = 40; | ||
| const DIAGNOSTIC_LIST_SUMMARY_LIMIT = 5; |
There was a problem hiding this comment.
Reducing DIAGNOSTIC_LIST_SUMMARY_LIMIT to 5 is a great way to keep the default output compact. However, buildLifecycleCheck (unlike buildMetadataCheck and buildResolutionCheck) does not accept or respect the verboseDiagnostics option.
As a result, all lifecycle diagnostics (such as active_closure_like_metadata_rows, active_terminal_parent_rows, stale_blocker_rows, dependency_cycle_item_ids, and dependency_cycle_sample_paths) will now be permanently truncated to a maximum of 5 items, even when the user explicitly runs the command with --verbose-diagnostics.
To fix this, we should update buildLifecycleCheck to accept verboseDiagnostics: boolean and pass the appropriate limit to summarizeList calls inside it, similar to how it is handled in the other checks.
| // Do not delete the live bundle before rebuilding. Agents often run docs, | ||
| // dogfood, and build gates concurrently in one checkout; removing this folder | ||
| // creates a transient broken `dist/cli.js` runtime. | ||
| await build({ |
There was a problem hiding this comment.
By removing await rm(outputDir, { recursive: true, force: true }); to prevent transient broken runtimes during concurrent builds, esbuild will now overwrite existing files but will not clean up old chunks.
Since splitting: true is enabled and chunk names contain hashes (chunks/[name]-[hash]), every code change will generate new chunk files with different hashes. Over time, the dist/cli-bundle/chunks/ directory will accumulate stale chunk files indefinitely. This can significantly bloat the packaged size when publishing to npm or running local checkouts.
Consider enabling metafile: true in the esbuild options, and then performing a post-build cleanup step to delete any files in outputDir that are not listed in the generated metafile. This safely prunes stale chunks without causing transient runtime failures during the build.
| function valuesEqual(left: unknown, right: unknown): boolean { | ||
| return JSON.stringify(left) === JSON.stringify(right); | ||
| return stableStringify(left) === stableStringify(right); | ||
| } |
There was a problem hiding this comment.
Calling stableStringify on every comparison has non-trivial serialization overhead, especially when processing large datasets of items with primitive field values.
Adding a fast-path reference/primitive equality check (left === right) before falling back to stableStringify will significantly improve performance for identical values.
| function valuesEqual(left: unknown, right: unknown): boolean { | |
| return JSON.stringify(left) === JSON.stringify(right); | |
| return stableStringify(left) === stableStringify(right); | |
| } | |
| function valuesEqual(left: unknown, right: unknown): boolean { | |
| if (left === right) return true; | |
| return stableStringify(left) === stableStringify(right); | |
| } |
| function areValuesEqual(left: unknown, right: unknown): boolean { | ||
| return JSON.stringify(left) === JSON.stringify(right); | ||
| return stableStringify(left) === stableStringify(right); | ||
| } |
There was a problem hiding this comment.
Calling stableStringify on every comparison introduces unnecessary serialization overhead for primitive values.
Adding a fast-path equality check (left === right) before falling back to stableStringify will optimize performance when comparing identical primitives or references.
| function areValuesEqual(left: unknown, right: unknown): boolean { | |
| return JSON.stringify(left) === JSON.stringify(right); | |
| return stableStringify(left) === stableStringify(right); | |
| } | |
| function areValuesEqual(left: unknown, right: unknown): boolean { | |
| if (left === right) return true; | |
| return stableStringify(left) === stableStringify(right); | |
| } |
| if (stableStringify(metadataRecord[fieldKey]) === stableStringify(fieldValue)) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
We can avoid the serialization overhead of stableStringify for identical primitive values by adding a fast-path strict equality check first.
| if (stableStringify(metadataRecord[fieldKey]) === stableStringify(fieldValue)) { | |
| continue; | |
| } | |
| if (metadataRecord[fieldKey] === fieldValue || stableStringify(metadataRecord[fieldKey]) === stableStringify(fieldValue)) { | |
| continue; | |
| } |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b84f1cfca9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| function valuesEqual(left: unknown, right: unknown): boolean { | ||
| return JSON.stringify(left) === JSON.stringify(right); | ||
| return stableStringify(left) === stableStringify(right); |
There was a problem hiding this comment.
When a runtime string filter is set to the literal value undefined, items that do not have that metadata key now pass the filter because stableStringify(undefined) and stableStringify("undefined") both produce the same serialized string. This affects list/search/calendar runtime filters for sparse metadata: a query for that real string value will include items where the field is absent, whereas the previous JSON.stringify comparison kept those cases distinct.
Useful? React with 👍 / 👎.
| const GIT_LS_FILES_MAX_BUFFER = 32 * 1024 * 1024; | ||
| const FILE_LIST_SUMMARY_LIMIT = 40; | ||
| const DIAGNOSTIC_LIST_SUMMARY_LIMIT = 40; | ||
| const DIAGNOSTIC_LIST_SUMMARY_LIMIT = 5; |
There was a problem hiding this comment.
When --check-command-references or --check-history-drift finds more than five affected IDs, this shared limit now truncates those diagnostics to five entries, but --verbose-diagnostics is only passed to metadata/resolution checks and these two checks still call summarizeList with the default limit. The validate help promises full diagnostic ID lists with --verbose-diagnostics, so users in those contexts cannot recover the omitted stale references or drifted item IDs from validate output.
Useful? React with 👍 / 👎.
| // Do not delete the live bundle before rebuilding. Agents often run docs, | ||
| // dogfood, and build gates concurrently in one checkout; removing this folder | ||
| // creates a transient broken `dist/cli.js` runtime. |
There was a problem hiding this comment.
When pnpm build runs repeatedly in the same checkout after code-splitting output changes, esbuild writes new hashed chunks but does not remove old ones from outdir. Because package.json publishes dist/** and the Sentry upload scans dist/, leaving dist/cli-bundle in place can ship or upload orphaned chunks/maps from prior builds; use a temp directory/swap or post-success cleanup of unreferenced chunks instead of leaving stale files behind.
Useful? React with 👍 / 👎.
| function isExpectedHandledCliIssue(issue) { | ||
| const metadata = issue && typeof issue === "object" ? issue.metadata : null; | ||
| const type = metadata && typeof metadata.type === "string" ? metadata.type : ""; | ||
| return type === "PmCliError" || type === "CommandError"; |
There was a problem hiding this comment.
For any unresolved fatal/error Sentry issue whose metadata.type is PmCliError or CommandError, the release gate now drops it before tallying severity, without checking whether it was an expected usage/not-found/conflict outcome. The repo also throws PmCliError for internal/generic failures, so a real production regression reported under that type can make the gate pass even though the queried issue is still unresolved at error/fatal level.
Useful? React with 👍 / 👎.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.agents/pm/history/pm-z0ip.jsonl (1)
1-39:⚠️ Potential issue | 🟠 Major | ⚡ Quick winProtected system-of-record file is being edited directly.
This PR directly modifies
.agents/pm/history/pm-z0ip.jsonl, which is disallowed for this repository path policy. Please remove direct.agents/pmedits from this PR and apply updates through thepmworkflow/tooling that owns these records.As per coding guidelines
.agents/pm/**:pmis the system of record. Do not edit.agents/pmfiles directly.🤖 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 @.agents/pm/history/pm-z0ip.jsonl around lines 1 - 39, The PR illegally modifies the system-of-record audit entry "pm-z0ip" (metadata/id pm-z0ip) by adding/updating metadata, comments, files, tests and test_runs; revert all direct edits to that record in this branch and remove any changes touching the pm-z0ip JSONL entry from the patch, then re-apply the intended updates using the authorized pm workflow/tooling (use the pm CLI or service to claim/update the task, add comments, files, tests or test_runs through its commands rather than editing metadata/*, comments, files or test_runs fields directly), and confirm the repo no longer contains any direct modifications to the pm-z0ip record before rebasing and re-submitting the PR.
🤖 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 `@src/cli/commands/validate.ts`:
- Around line 127-128: The DIAGNOSTIC_LIST_SUMMARY_LIMIT was lowered to 5 which
unintentionally truncates arrays in checks that never receive the
verboseDiagnostics flag (notably the lifecycle, history_drift, and
command_references builders); either pass the verboseDiagnostics boolean into
those builders (e.g., thread the verboseDiagnostics param from the validate
command into lifecycle, history_drift, and command_references functions) so they
can bypass the shared DIAGNOSTIC_LIST_SUMMARY_LIMIT when verbose is true, or
change those builders to use a separate cap (or conditional that checks
verboseDiagnostics) instead of the global DIAGNOSTIC_LIST_SUMMARY_LIMIT so pm
validate --verbose-diagnostics restores full diagnostics for fields like
active_closure_like_metadata_rows, drifted_items, and
stale_pm_id_reference_rows.
---
Outside diff comments:
In @.agents/pm/history/pm-z0ip.jsonl:
- Around line 1-39: The PR illegally modifies the system-of-record audit entry
"pm-z0ip" (metadata/id pm-z0ip) by adding/updating metadata, comments, files,
tests and test_runs; revert all direct edits to that record in this branch and
remove any changes touching the pm-z0ip JSONL entry from the patch, then
re-apply the intended updates using the authorized pm workflow/tooling (use the
pm CLI or service to claim/update the task, add comments, files, tests or
test_runs through its commands rather than editing metadata/*, comments, files
or test_runs fields directly), and confirm the repo no longer contains any
direct modifications to the pm-z0ip record before rebasing and re-submitting the
PR.
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 5707b5da-3183-4602-aa55-c0c28f8e12ab
📒 Files selected for processing (19)
.agents/pm/history/pm-z0ip.jsonl.agents/pm/tasks/pm-z0ip.toondocs/EXTENSIONS.mdscripts/bundle-cli.mjsscripts/dogfood-package-first.mjsscripts/release/sentry-telemetry-gate.mjssrc/cli/commands/get.tssrc/cli/commands/test.tssrc/cli/commands/update-many.tssrc/cli/commands/update.tssrc/cli/commands/validate.tssrc/cli/main.tssrc/core/schema/runtime-field-filters.tstests/integration/help-runtime.spec.tstests/integration/release-automation-contract.spec.tstests/unit/get-append-command.spec.tstests/unit/test-command.spec.tstests/unit/update-many-command.spec.tstests/unit/validate-command.spec.ts
|
@codex review /gemini review @gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request hardens the package ecosystem and CLI agent UX. Key changes include optimizing get --depth brief to omit body, linked, and claim state data; introducing a primitive-safe stableValueEquals helper to perform order-insensitive object comparisons; limiting default validation diagnostics to reduce token usage; and updating the CLI build script to avoid deleting the live bundle during rebuilds. Feedback on the changes suggests updating the file collection helper in the build script to also clean up stale symbolic links by checking entry.isSymbolicLink().
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if (entry.isFile()) { | ||
| files.push(absolutePath); | ||
| } |
There was a problem hiding this comment.
Stale symbolic links in the output directory won't be collected or cleaned up because entry.isFile() is false for symlinks. Including entry.isSymbolicLink() ensures that any stale symlinks are also cleaned up correctly.
if (entry.isFile() || entry.isSymbolicLink()) {
files.push(absolutePath);
}
|
@codex review /gemini review @gemini-code-assist review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6dbd4e038f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| function isExpectedHandledCliIssue(issue) { | ||
| const metadata = issue && typeof issue === "object" ? issue.metadata : null; | ||
| const type = metadata && typeof metadata.type === "string" ? metadata.type : ""; | ||
| return (type === "PmCliError" || type === "CommandError") && issue?.isUnhandled === false; |
There was a problem hiding this comment.
When the release gate has a Sentry token set, fetchSentryIssues uses the REST API path rather than the CLI fallback; Sentry's List a Project's Issues response schema includes metadata but not isUnhandled, and buildSentryIssuesUrl does not request any event field that would add it. In that normal required-gate context, issue?.isUnhandled is undefined, so handled PmCliError/CommandError groups are still counted as high instead of reported under ignored_expected_cli_error_total, leaving the gate blocked on the expected CLI errors this change is meant to ignore.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Code Review
This pull request hardens the package ecosystem SDK and optimizes the agent UX. Key changes include: updating get --depth brief to omit body, linked, and claim_state fields to reduce token usage; introducing a primitive-safe stableValueEquals helper to replace order-sensitive JSON.stringify comparisons across several commands; making default validation diagnostics more compact unless verbose diagnostics are requested; preventing the deletion of the live bundle during rebuilds to avoid breaking concurrent runs; and suppressing expected handled CLI errors from Sentry reporting. Documentation and extensive test coverage have been updated accordingly. There are no review comments, and I have no additional feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ce3f5f9c3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| title: 2026-06-01 package ecosystem SDK agent UX audit and hardening | ||
| description: "Detailed manual audit, temp-project dogfood, SDK/ecosystem review, agent-optimized UX review, telemetry/Sentry validation, CI/PR review loop, and targeted hardening on latest main. Duplicate check: pm search found prior closed broad-audit lineage pm-59gj/pm-kd9n/pm-etxf/pm-qmx3 and no in-progress audit items; this task records the new June 1 requested pass." | ||
| type: Task | ||
| status: in_progress |
There was a problem hiding this comment.
This task records final verification and links the changed files/tests, but the committed tracker state still leaves it as status: in_progress with an assignee, so it will continue to appear in pm list-in-progress and look actively owned by this agent. AGENTS.md requires the close/release step for completed work; please close and release this item through pm so the repository’s system-of-record state matches the shipped change.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ce3f5f9c3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| chunkNames: "chunks/[name]-[hash]", | ||
| logLevel: "warning", | ||
| }); | ||
| await removeStaleBundleFiles(buildResult.metafile.outputs); |
There was a problem hiding this comment.
When two pnpm build/bundle-cli runs overlap in the same checkout and the second build produces different chunk hashes, this post-build stale cleanup can delete files that the other build has just written because its metafile outputs are not in this process's expectedFiles set. That can leave dist/cli-bundle/main.js pointing at a now-missing chunk, recreating the transient broken bundle state this change is meant to prevent; stale cleanup needs serialization/locking or a safer generation swap strategy.
Useful? React with 👍 / 👎.
|
@codex review /gemini review @gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request hardens the CLI and optimizes agent UX by introducing a build lock mechanism to serialize concurrent bundle builds, making get --depth brief more compact to reduce token usage, and limiting default validation diagnostics unless verbose mode is requested. It also replaces order-sensitive JSON.stringify comparisons with a primitive-safe stableValueEquals helper and suppresses handled CLI errors from Sentry. The review feedback correctly identifies a race condition in the stale lock cleanup logic in scripts/bundle-cli.mjs and suggests using an atomic rename operation to safely clean up stale locks.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| #!/usr/bin/env node | ||
|
|
||
| import { readFile, rm, writeFile } from "node:fs/promises"; | ||
| import { mkdir, readdir, readFile, rm, stat, unlink, writeFile } from "node:fs/promises"; |
There was a problem hiding this comment.
Add rename to the imports from node:fs/promises to support atomic stale lock cleanup.
import { mkdir, readdir, readFile, rename, rm, stat, unlink, writeFile } from "node:fs/promises";| async function acquireBundleBuildLock() { | ||
| await mkdir(path.dirname(lockDir), { recursive: true }); | ||
| const startedAt = Date.now(); | ||
| while (true) { | ||
| try { | ||
| await mkdir(lockDir); | ||
| return async () => { | ||
| await rm(lockDir, { recursive: true, force: true }); | ||
| }; | ||
| } catch (error) { | ||
| if (!error || typeof error !== "object" || !("code" in error) || error.code !== "EEXIST") { | ||
| throw error; | ||
| } | ||
| const lockStats = await stat(lockDir).catch(() => null); | ||
| if (lockStats && Date.now() - lockStats.mtimeMs > staleLockMs) { | ||
| await rm(lockDir, { recursive: true, force: true }); | ||
| continue; | ||
| } | ||
| if (Date.now() - startedAt > lockTimeoutMs) { | ||
| throw new Error(`Timed out waiting for bundle build lock at ${lockDir}`); | ||
| } | ||
| await sleep(lockRetryMs); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
There is a race condition in the stale lock cleanup logic. If multiple concurrent processes find that the lock is stale, they will all attempt to delete it using rm(lockDir). Once one process successfully deletes and recreates the lock directory via mkdir(lockDir), another concurrent process's pending rm(lockDir) can delete the newly acquired lock, allowing a third process to also acquire it. This violates mutual exclusion.
To prevent this race condition, we can atomically rename the stale lock directory to a unique temporary name using rename before deleting it. If the rename operation succeeds, that process has safely claimed the right to clean up the stale lock without risk of deleting a newly created lock.
async function acquireBundleBuildLock() {
await mkdir(path.dirname(lockDir), { recursive: true });
const startedAt = Date.now();
while (true) {
try {
await mkdir(lockDir);
return async () => {
await rm(lockDir, { recursive: true, force: true });
};
} catch (error) {
if (!error || typeof error !== "object" || !("code" in error) || error.code !== "EEXIST") {
throw error;
}
const lockStats = await stat(lockDir).catch(() => null);
if (lockStats && Date.now() - lockStats.mtimeMs > staleLockMs) {
const tempLockDir = lockDir + ".stale." + Date.now() + "." + Math.random().toString(36).slice(2);
try {
await rename(lockDir, tempLockDir);
await rm(tempLockDir, { recursive: true, force: true });
} catch (renameError) {
// If rename fails, another process already claimed or deleted it
}
continue;
}
if (Date.now() - startedAt > lockTimeoutMs) {
throw new Error(`Timed out waiting for bundle build lock at ${lockDir}`);
}
await sleep(lockRetryMs);
}
}
}
|
Codex Review: Didn't find any major issues. Can't wait for the next one! ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
@codex review /gemini review @gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces several optimizations and hardening improvements to the CLI package ecosystem. Key changes include implementing a file-system-based build lock in bundle-cli.mjs to serialize concurrent builds safely, reducing token usage for agent reads by omitting body, linked, and claim_state in get --depth brief, and compacting default validation diagnostics. Additionally, it introduces a primitive-safe stableValueEquals helper for order-insensitive object comparisons and suppresses expected handled CLI errors from Sentry reporting. Feedback on these changes suggests optimizing the build lock retry loop when a lock is immediately released, and handling non-plain objects like RegExp in stableValueEquals to prevent false-positive equality matches.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const lockStats = await stat(lockDir).catch(() => null); | ||
| if (lockStats && Date.now() - lockStats.mtimeMs > staleLockMs) { |
There was a problem hiding this comment.
When stat(lockDir) fails (e.g., because the lock was just released and deleted by another process), lockStats resolves to null. Currently, the loop continues to check the timeout and then sleeps for lockRetryMs (250ms) before retrying.
Since a null stats object indicates the lock is now free, we can immediately continue the loop to attempt to acquire the lock without any delay. This optimizes responsiveness during concurrent builds.
const lockStats = await stat(lockDir).catch(() => null);
if (!lockStats) {
continue;
}
if (Date.now() - lockStats.mtimeMs > staleLockMs) {| export function stableValueEquals(left: unknown, right: unknown): boolean { | ||
| if (Object.is(left, right)) { | ||
| return true; | ||
| } | ||
| if (left === null || right === null || typeof left !== "object" || typeof right !== "object") { | ||
| return false; | ||
| } | ||
| return stableStringify(left) === stableStringify(right); | ||
| } |
There was a problem hiding this comment.
The new stableValueEquals helper relies on stableStringify for object comparison. However, stableStringify (via sortObjectKeys) does not support non-plain objects such as RegExp, serializing them all to empty objects ("{}"). This can lead to false positives where semantically different regular expressions (e.g., /abc/ and /def/) are evaluated as equal.
Consider adding an explicit check for RegExp instances to ensure correctness.
export function stableValueEquals(left: unknown, right: unknown): boolean {
if (Object.is(left, right)) {
return true;
}
if (left === null || right === null || typeof left !== "object" || typeof right !== "object") {
return false;
}
if (left instanceof RegExp && right instanceof RegExp) {
return left.toString() === right.toString();
}
return stableStringify(left) === stableStringify(right);
}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eee78ebcbd
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| await Promise.all( | ||
| existingFiles | ||
| .filter((filePath) => !expectedFiles.has(filePath)) | ||
| .map((filePath) => unlink(filePath).catch((error) => { |
There was a problem hiding this comment.
When a bundle rebuild overlaps with an already-running dist/cli.js, this cleanup can still delete the old hashed chunks that the running process may import later; src/cli/main.ts lazy-loads command registrars with dynamic import(...), so a process that loaded the old main.js before this rebuild can fail with the same missing-module error when it reaches one of those lazy imports. The lock only serializes rebuilds, not CLI readers, so stale chunks need a grace/retention strategy instead of immediate unlinking.
Useful? React with 👍 / 👎.
| function isExpectedHandledCliIssue(issue) { | ||
| const metadata = issue && typeof issue === "object" ? issue.metadata : null; | ||
| const type = metadata && typeof metadata.type === "string" ? metadata.type : ""; | ||
| return (type === "PmCliError" || type === "CommandError") && issue?.isUnhandled === false; |
There was a problem hiding this comment.
This treats every handled PmCliError/CommandError issue as expected, but the CLI still intentionally reports handled errors whose exit code is not in the expected set (shouldLogHandledErrorToSentry only suppresses usage/not-found/conflict), and there are real PmCliError(..., EXIT_CODE.GENERIC_FAILURE) paths such as invalid update-many checkpoints. Those high/critical handled generic failures will now be removed from relevant and can no longer block the release gate, so the filter should distinguish expected exit codes/error categories rather than only type plus isUnhandled === false.
Useful? React with 👍 / 👎.
|
@codex review /gemini review @gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces several robustness and performance improvements, including a file-locking mechanism for concurrent CLI bundle builds, stable object equality comparisons (stableValueEquals) to prevent order-sensitive failures, and a verboseDiagnostics option for compact validation outputs. It also refines the get --depth brief command to omit unnecessary payload fields and adds Sentry telemetry filtering for expected handled CLI errors. The review feedback highlights a potential infinite loop in the bundle build lock acquisition if unexpected errors occur, suggests using lstat instead of stat to correctly handle symbolic links during stale file cleanup, and recommends relaxing the isUnhandled check in the Sentry telemetry gate to safely handle missing or undefined fields.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const lockStats = await stat(lockDir).catch(() => null); | ||
| if (!lockStats) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
If stat(lockDir) fails with an error other than ENOENT (for example, permission denied EACCES or EPERM), the catch block currently silences the error and returns null. This causes the loop to immediately continue without sleeping, leading to an infinite, CPU-burning tight loop.
We should only ignore ENOENT errors and propagate any other unexpected errors to fail the build gracefully.
const lockStats = await stat(lockDir).catch((err) => {
if (err && err.code === "ENOENT") {
return null;
}
throw err;
});
if (!lockStats) {
continue;
}| #!/usr/bin/env node | ||
|
|
||
| import { readFile, rm, writeFile } from "node:fs/promises"; | ||
| import { mkdir, readdir, readFile, rename, rm, stat, unlink, writeFile } from "node:fs/promises"; |
There was a problem hiding this comment.
Import lstat from node:fs/promises to safely inspect symbolic links without following them during stale file cleanup.
import { lstat, mkdir, readdir, readFile, rename, rm, stat, unlink, writeFile } from "node:fs/promises";| existingFiles | ||
| .filter((filePath) => !expectedFiles.has(filePath)) | ||
| .map(async (filePath) => { | ||
| const fileStats = await stat(filePath).catch(() => null); |
There was a problem hiding this comment.
Using stat on symbolic links follows them to their target. If a symlink is broken, stat will fail with ENOENT and return null, meaning broken symlinks will never be cleaned up. Additionally, for valid symlinks, stat checks the target's modification time rather than the symlink's own creation/modification time.
Using lstat instead of stat ensures we inspect the symlink itself, allowing broken symlinks to be cleaned up and correctly checking the symlink's own age.
const fileStats = await lstat(filePath).catch(() => null);| if ((type !== "PmCliError" && type !== "CommandError") || issue?.isUnhandled !== false) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
If isUnhandled is missing or undefined in the Sentry API response (which is common for issue group endpoints), issue?.isUnhandled !== false will evaluate to true, causing the function to return false and fail to ignore expected handled CLI errors.
Changing the check to issue?.isUnhandled === true ensures we only reject explicitly unhandled errors (crashes), while safely treating missing/undefined values as handled CLI errors.
| if ((type !== "PmCliError" && type !== "CommandError") || issue?.isUnhandled !== false) { | |
| return false; | |
| } | |
| if ((type !== "PmCliError" && type !== "CommandError") || issue?.isUnhandled === true) { | |
| return false; | |
| } |
| const gateSource = await readFile(path.join(repoRoot, "scripts/release/sentry-telemetry-gate.mjs"), "utf8"); | ||
| expect(gateSource).toContain('commandFor("sentry")'); | ||
| expect(gateSource).toContain("function isExpectedHandledCliIssue"); | ||
| expect(gateSource).toContain('issue?.isUnhandled !== false'); |
There was a problem hiding this comment.
Update the test assertion to match the relaxed isUnhandled check in sentry-telemetry-gate.mjs.
| expect(gateSource).toContain('issue?.isUnhandled !== false'); | |
| expect(gateSource).toContain('issue?.isUnhandled === true'); |
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/pm/history/pm-z0ip.jsonl:
- Around line 39-49: Summary: Direct edits were made to the pm system-of-record
JSONL (.agents/pm/history/pm-z0ip.jsonl); remove them and regenerate via the
supported PM workflow. Fix: remove the file .agents/pm/history/pm-z0ip.jsonl
from the PR (undo the changes/patches that add/replace metadata entries), revert
any commits that modified .agents/pm/** so the repository returns to the
canonical pm state, and re-create the intended history/tasks using the official
PM workflow/tool (do not hand-edit .agents/pm files); ensure future changes use
the PM system (refer to the .agents/pm/** pattern and the specific file
pm-z0ip.jsonl when locating the edits).
🪄 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: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 55aed889-ce2c-428c-a8c9-42d229ac0515
📒 Files selected for processing (14)
.agents/pm/history/pm-z0ip.jsonl.agents/pm/tasks/pm-z0ip.toonscripts/bundle-cli.mjsscripts/release/sentry-telemetry-gate.mjssrc/cli/commands/test.tssrc/cli/commands/update-many.tssrc/cli/commands/update.tssrc/cli/commands/validate.tssrc/core/schema/runtime-field-filters.tssrc/core/shared/serialization.tstests/integration/release-automation-contract.spec.tstests/unit/serialization-utils.spec.tstests/unit/update-many-command.spec.tstests/unit/validate-command.spec.ts
✅ Files skipped from review due to trivial changes (1)
- .agents/pm/tasks/pm-z0ip.toon
🚧 Files skipped from review as they are similar to previous changes (5)
- src/cli/commands/update-many.ts
- tests/unit/update-many-command.spec.ts
- src/cli/commands/update.ts
- tests/integration/release-automation-contract.spec.ts
- scripts/release/sentry-telemetry-gate.mjs
| {"ts":"2026-06-01T21:52:05.920Z","author":"codex-audit-20260601","op":"files_add","patch":[{"op":"add","path":"/metadata/files/15","value":{"path":"tests/unit/validate-command.spec.ts","scope":"project","note":"Cover compact versus verbose command-reference diagnostics"}},{"op":"replace","path":"/metadata/updated_at","value":"2026-06-01T21:52:05.920Z"}],"before_hash":"a70966ef26322c8a190da02253c45007dd9cbbc19428a9dc73a5829f579a7029","after_hash":"b81a9119ece675eeb85ecdc2901ac2334b7b77c210f850441a0937da7bea65d9"} | ||
| {"ts":"2026-06-01T21:52:06.002Z","author":"codex-audit-20260601","op":"files_add","patch":[{"op":"replace","path":"/metadata/files/15/note","value":"Runtime filter stable-equality regression lives in existing unit spec to preserve directory-load limits"},{"op":"replace","path":"/metadata/files/15/path","value":"tests/unit/update-many-command.spec.ts"},{"op":"replace","path":"/metadata/files/14/note","value":"Covers order-insensitive linked-test JSON assertions"},{"op":"replace","path":"/metadata/files/14/path","value":"tests/unit/test-command.spec.ts"},{"op":"replace","path":"/metadata/files/13/note","value":"Cover stableValueEquals primitive and object comparison branches"},{"op":"replace","path":"/metadata/files/13/path","value":"tests/unit/serialization-utils.spec.ts"},{"op":"add","path":"/metadata/files/16","value":{"path":"tests/unit/validate-command.spec.ts","scope":"project","note":"Cover compact versus verbose command-reference diagnostics"}},{"op":"replace","path":"/metadata/updated_at","value":"2026-06-01T21:52:06.002Z"}],"before_hash":"b81a9119ece675eeb85ecdc2901ac2334b7b77c210f850441a0937da7bea65d9","after_hash":"81cdb1bd73d6e122c58d30ef42143be62adbb8bbd5e2ac4acf13868d879633f2"} | ||
| {"ts":"2026-06-01T21:52:13.248Z","author":"codex-audit-20260601","op":"files_add","patch":[{"op":"replace","path":"/metadata/files/16/note","value":"Runtime filter stable-equality regression lives in existing unit spec to preserve directory-load limits"},{"op":"replace","path":"/metadata/files/16/path","value":"tests/unit/update-many-command.spec.ts"},{"op":"replace","path":"/metadata/files/15/note","value":"Covers order-insensitive linked-test JSON assertions"},{"op":"replace","path":"/metadata/files/15/path","value":"tests/unit/test-command.spec.ts"},{"op":"replace","path":"/metadata/files/14/note","value":"Cover stableValueEquals primitive and object comparison branches"},{"op":"replace","path":"/metadata/files/14/path","value":"tests/unit/serialization-utils.spec.ts"},{"op":"replace","path":"/metadata/files/13/note","value":"Covers brief get output and linked projection branches"},{"op":"replace","path":"/metadata/files/13/path","value":"tests/unit/get-append-command.spec.ts"},{"op":"replace","path":"/metadata/files/12/note","value":"Covers handled-error Sentry suppression helper behavior"},{"op":"replace","path":"/metadata/files/12/path","value":"tests/unit/cli-main-errors.spec.ts"},{"op":"replace","path":"/metadata/files/11/note","value":"Covers bundle rebuild and Sentry gate contract"},{"op":"replace","path":"/metadata/files/11/path","value":"tests/integration/release-automation-contract.spec.ts"},{"op":"replace","path":"/metadata/files/10/note","value":"Add primitive-safe stable equality helper for runtime comparisons"},{"op":"replace","path":"/metadata/files/10/path","value":"src/core/shared/serialization.ts"},{"op":"add","path":"/metadata/files/17","value":{"path":"tests/unit/validate-command.spec.ts","scope":"project","note":"Cover compact versus verbose command-reference diagnostics"}},{"op":"replace","path":"/metadata/updated_at","value":"2026-06-01T21:52:13.248Z"}],"before_hash":"81cdb1bd73d6e122c58d30ef42143be62adbb8bbd5e2ac4acf13868d879633f2","after_hash":"91c0c053968830f68c1f8095ad8b4fb8c323defe70c0aaba8026aa293b7ba9a4"} | ||
| {"ts":"2026-06-01T21:52:19.884Z","author":"codex-audit-20260601","op":"tests_add","patch":[{"op":"replace","path":"/metadata/tests/1/command","value":"node scripts/run-tests.mjs test -- tests/unit/serialization-utils.spec.ts tests/unit/cli-main-errors.spec.ts tests/integration/release-automation-contract.spec.ts tests/unit/get-append-command.spec.ts tests/unit/validate-command.spec.ts tests/unit/test-command.spec.ts tests/unit/update-many-command.spec.ts tests/integration/help-runtime.spec.ts"},{"op":"add","path":"/metadata/tests/2","value":{"command":"pnpm build && node scripts/dogfood-package-first.mjs","scope":"project","timeout_seconds":300}},{"op":"replace","path":"/metadata/updated_at","value":"2026-06-01T21:52:19.884Z"}],"before_hash":"91c0c053968830f68c1f8095ad8b4fb8c323defe70c0aaba8026aa293b7ba9a4","after_hash":"dd47ed03296d4ea0be99a17c1f55e442618df654004181762206c1e5f0348aed"} | ||
| {"ts":"2026-06-01T21:52:24.946Z","author":"codex-audit-20260601","op":"tests_add","patch":[{"op":"replace","path":"/metadata/tests/2/command","value":"node scripts/run-tests.mjs test -- tests/unit/serialization-utils.spec.ts tests/unit/cli-main-errors.spec.ts tests/integration/release-automation-contract.spec.ts tests/unit/get-append-command.spec.ts tests/unit/validate-command.spec.ts tests/unit/test-command.spec.ts tests/unit/update-many-command.spec.ts tests/integration/help-runtime.spec.ts"},{"op":"replace","path":"/metadata/tests/1/timeout_seconds","value":240},{"op":"replace","path":"/metadata/tests/1/command","value":"node scripts/run-tests.mjs test -- tests/unit/cli-main-errors.spec.ts tests/integration/release-automation-contract.spec.ts tests/unit/get-append-command.spec.ts tests/unit/validate-command.spec.ts tests/unit/test-command.spec.ts tests/unit/update-many-command.spec.ts tests/integration/help-runtime.spec.ts"},{"op":"replace","path":"/metadata/tests/0/timeout_seconds","value":360},{"op":"replace","path":"/metadata/tests/0/command","value":"node scripts/run-tests.mjs coverage"},{"op":"add","path":"/metadata/tests/3","value":{"command":"pnpm build && node scripts/dogfood-package-first.mjs","scope":"project","timeout_seconds":300}},{"op":"replace","path":"/metadata/updated_at","value":"2026-06-01T21:52:24.946Z"}],"before_hash":"dd47ed03296d4ea0be99a17c1f55e442618df654004181762206c1e5f0348aed","after_hash":"c6097adc1c62cbe4638127bb2bc20ad8c09bd35c1e6462c7f12b1beb21324f02"} | ||
| {"ts":"2026-06-01T21:52:31.425Z","author":"codex-audit-20260601","op":"comment_add","patch":[{"op":"add","path":"/metadata/comments/11","value":{"created_at":"2026-06-01T21:52:31.425Z","author":"codex-audit-20260601","text":"Review follow-up evidence: addressed Gemini/Codex findings by preserving stale bundle cleanup after successful esbuild metafile output, applying compact validate diagnostics to lifecycle/history/command-reference surfaces with verbose expansion, replacing stableStringify equality with primitive-safe stableValueEquals, and narrowing expected Sentry issue ignores to handled CLI errors only. Verified focused suite 8 files/171 tests, full coverage 135 files/2094 tests at 100%, package-first dogfood 87 commands, changelog check unchanged, typecheck/static/docs/security gates, Sentry strict and required telemetry gates."}},{"op":"replace","path":"/metadata/updated_at","value":"2026-06-01T21:52:31.425Z"}],"before_hash":"c6097adc1c62cbe4638127bb2bc20ad8c09bd35c1e6462c7f12b1beb21324f02","after_hash":"96388b8c8911f0fabc19f7a4915ccc7134dc50c3a0132d0e54072bebdd28a30d"} | ||
| {"ts":"2026-06-01T21:58:03.850Z","author":"codex-audit-20260601","op":"test_run_track","patch":[{"op":"add","path":"/metadata/test_runs/2","value":{"run_id":"test-local-mpvr2g5d-hq4uhk","kind":"test","status":"passed","started_at":"2026-06-01T21:52:36.990Z","finished_at":"2026-06-01T21:58:03.842Z","recorded_at":"2026-06-01T21:58:03.842Z","passed":4,"failed":0,"skipped":0}},{"op":"replace","path":"/metadata/updated_at","value":"2026-06-01T21:58:03.850Z"}],"before_hash":"96388b8c8911f0fabc19f7a4915ccc7134dc50c3a0132d0e54072bebdd28a30d","after_hash":"245e2a104f38f2a4c633cc4be7937fff6b14c0fba75fdbac3d28c70f6273cd4a","message":"Track test run summary (test-local-mpvr2g5d-hq4uhk)"} | ||
| {"ts":"2026-06-01T22:03:27.300Z","author":"codex-audit-20260601","op":"comment_add","patch":[{"op":"add","path":"/metadata/comments/12","value":{"created_at":"2026-06-01T22:03:27.300Z","author":"codex-audit-20260601","text":"Review follow-up: Gemini noted stale symlinks in dist/cli-bundle would not be collected by the post-build stale-file cleanup. Updated collectFiles to include Dirent symlinks and added release automation contract coverage. Verification: pnpm build and node scripts/run-tests.mjs test -- tests/integration/release-automation-contract.spec.ts."}},{"op":"replace","path":"/metadata/updated_at","value":"2026-06-01T22:03:27.300Z"}],"before_hash":"245e2a104f38f2a4c633cc4be7937fff6b14c0fba75fdbac3d28c70f6273cd4a","after_hash":"4a8edcdc76b98d606ff393feb09dd6a41d382a778461b12e36b8b827e72111e3"} | ||
| {"ts":"2026-06-01T22:16:51.754Z","author":"codex-audit-20260601","op":"comment_add","patch":[{"op":"add","path":"/metadata/comments/13","value":{"created_at":"2026-06-01T22:16:51.754Z","author":"codex-audit-20260601","text":"Review follow-up: Codex found concurrent bundle cleanups could delete another build's fresh hashed chunks. Added a stale-aware .cli-bundle-build.lock around the esbuild output plus stale cleanup phase so builds serialize while the live bundle stays available for CLI users. Verification: pnpm build, pnpm typecheck, and node scripts/run-tests.mjs test -- tests/integration/release-automation-contract.spec.ts."}},{"op":"replace","path":"/metadata/updated_at","value":"2026-06-01T22:16:51.754Z"}],"before_hash":"4a8edcdc76b98d606ff393feb09dd6a41d382a778461b12e36b8b827e72111e3","after_hash":"1e68bbffb2e1ace19fe573f5d1bd9bd4dcd6b8e714582aabef433573f9e87062"} | ||
| {"ts":"2026-06-01T22:22:28.609Z","author":"codex-audit-20260601","op":"comment_add","patch":[{"op":"add","path":"/metadata/comments/14","value":{"created_at":"2026-06-01T22:22:28.609Z","author":"codex-audit-20260601","text":"Review follow-up: Gemini found stale bundle-lock cleanup could delete a newly acquired lock if multiple builders detected the same stale lock. Changed stale cleanup to atomically rename the stale lock directory to a unique candidate before removing it, preserving mutual exclusion. Verification: pnpm build, pnpm typecheck, and node scripts/run-tests.mjs test -- tests/integration/release-automation-contract.spec.ts."}},{"op":"replace","path":"/metadata/updated_at","value":"2026-06-01T22:22:28.609Z"}],"before_hash":"1e68bbffb2e1ace19fe573f5d1bd9bd4dcd6b8e714582aabef433573f9e87062","after_hash":"7529c675a8d28556d526c1ad9ba6036992735034c01ff55e49e90842b2c123de"} | ||
| {"ts":"2026-06-01T22:32:02.982Z","author":"codex-audit-20260601","op":"comment_add","patch":[{"op":"add","path":"/metadata/comments/15","value":{"created_at":"2026-06-01T22:32:02.982Z","author":"codex-audit-20260601","text":"Review follow-up: Gemini/Codex found immediate stale bundle cleanup could affect already-running lazy imports, stale-lock retry could sleep after a released lock, RegExp values could compare equal through stableStringify, and the Sentry expected-error filter needed message-level narrowing. Added bundle stale-file retention, immediate retry on missing lock stats, RegExp-aware stableValueEquals, and a known expected handled CLI issue allowlist. Verification: pnpm build, pnpm typecheck, focused release/serialization tests, full coverage 135 files/2094 tests at 100%, Sentry strict gate, and required telemetry gate."}},{"op":"replace","path":"/metadata/updated_at","value":"2026-06-01T22:32:02.982Z"}],"before_hash":"7529c675a8d28556d526c1ad9ba6036992735034c01ff55e49e90842b2c123de","after_hash":"4d258cb54e2b6b497b880e5bdd28dae39b10bd54054836d3bdc60f296b78f594"} |
There was a problem hiding this comment.
Avoid direct edits under .agents/pm.
These JSONL mutations are being committed directly into the pm system-of-record area. Please remove this file from the PR and regenerate any needed history/task state through the supported pm workflow instead.
As per coding guidelines, .agents/pm/**: pm is the system of record. Do not edit .agents/pm files directly.
🤖 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 @.agents/pm/history/pm-z0ip.jsonl around lines 39 - 49, Summary: Direct edits
were made to the pm system-of-record JSONL (.agents/pm/history/pm-z0ip.jsonl);
remove them and regenerate via the supported PM workflow. Fix: remove the file
.agents/pm/history/pm-z0ip.jsonl from the PR (undo the changes/patches that
add/replace metadata entries), revert any commits that modified .agents/pm/** so
the repository returns to the canonical pm state, and re-create the intended
history/tasks using the official PM workflow/tool (do not hand-edit .agents/pm
files); ensure future changes use the PM system (refer to the .agents/pm/**
pattern and the specific file pm-z0ip.jsonl when locating the edits).
|
@codex review /gemini review @gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request hardens the package ecosystem SDK and improves agent UX. Key changes include introducing a build lock and stale-file retention in the CLI bundler to prevent concurrent build corruption, omitting body, linked containers, and claim state in get --depth brief to reduce token usage, and making default validation diagnostics more compact unless verbose diagnostics are requested. Additionally, it replaces order-sensitive JSON.stringify comparisons with a primitive-safe stableValueEquals helper, suppresses expected handled CLI errors from Sentry reporting, and updates documentation and tests accordingly. There are no review comments to address, so I have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
@codex review /gemini review @gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request hardens the package ecosystem SDK and agent UX. Key changes include implementing a build lock and stale-file retention in the CLI bundler to prevent concurrent build issues, optimizing get --depth brief to omit token-heavy fields, and introducing a primitive-safe stableValueEquals helper to replace order-sensitive JSON.stringify comparisons. Additionally, validation diagnostics are now compact by default, and expected handled CLI errors are filtered out from Sentry reporting. Feedback suggests expanding stableValueEquals to correctly handle Date, Set, and Map objects, and making the stale-file cleanup in the bundler more robust by ignoring all filesystem errors during lstat and unlink operations.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| export function stableValueEquals(left: unknown, right: unknown): boolean { | ||
| if (Object.is(left, right)) { | ||
| return true; | ||
| } | ||
| if (left === null || right === null || typeof left !== "object" || typeof right !== "object") { | ||
| return false; | ||
| } | ||
| if (left instanceof RegExp || right instanceof RegExp) { | ||
| return left instanceof RegExp && right instanceof RegExp && left.toString() === right.toString(); | ||
| } | ||
| return stableStringify(left) === stableStringify(right); | ||
| } |
There was a problem hiding this comment.
The stableValueEquals helper currently does not handle Date, Set, or Map objects correctly. Since stableStringify relies on JSON.stringify (which serializes Set and Map to empty objects "{}"), different sets or maps will incorrectly compare as equal. Similarly, Date objects might not be compared correctly depending on how sortObjectKeys processes them.
We should explicitly handle Date, Set, and Map instances to ensure robust and correct equality checks.
export function stableValueEquals(left: unknown, right: unknown): boolean {
if (Object.is(left, right)) {
return true;
}
if (left === null || right === null || typeof left !== "object" || typeof right !== "object") {
return false;
}
if (left instanceof RegExp || right instanceof RegExp) {
return left instanceof RegExp && right instanceof RegExp && left.toString() === right.toString();
}
if (left instanceof Date || right instanceof Date) {
return left instanceof Date && right instanceof Date && left.getTime() === right.getTime();
}
if (left instanceof Set || right instanceof Set) {
if (!(left instanceof Set && right instanceof Set) || left.size !== right.size) {
return false;
}
const leftArr = [...left].map(stableStringify).sort();
const rightArr = [...right].map(stableStringify).sort();
return leftArr.every((val, i) => val === rightArr[i]);
}
if (left instanceof Map || right instanceof Map) {
if (!(left instanceof Map && right instanceof Map) || left.size !== right.size) {
return false;
}
const leftArr = [...left.entries()].map(([k, v]) => stableStringify([k, v])).sort();
const rightArr = [...right.entries()].map(([k, v]) => stableStringify([k, v])).sort();
return leftArr.every((val, i) => val === rightArr[i]);
}
return stableStringify(left) === stableStringify(right);
}| const fileStats = await lstat(filePath).catch((lstatError) => { | ||
| if (lstatError && typeof lstatError === "object" && "code" in lstatError && lstatError.code === "ENOENT") { | ||
| return null; | ||
| } | ||
| throw lstatError; | ||
| }); | ||
| if (!fileStats || now - fileStats.mtimeMs < bundleStaleRetentionMs) { | ||
| return; | ||
| } | ||
| await unlink(filePath).catch((error) => { | ||
| if (error && typeof error === "object" && "code" in error && error.code === "ENOENT") { | ||
| return; | ||
| } | ||
| throw error; | ||
| }); |
There was a problem hiding this comment.
During the stale file cleanup process, any transient permission issues, file locks, or other filesystem errors (besides ENOENT) when calling lstat or unlink will throw an error and crash the entire build process.
Since this is a non-critical cleanup step, we should make it robust by catching and ignoring all errors, ensuring that the build itself does not fail due to cleanup issues.
const fileStats = await lstat(filePath).catch(() => null);
if (!fileStats || now - fileStats.mtimeMs < bundleStaleRetentionMs) {
return;
}
await unlink(filePath).catch(() => {});
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
@codex review /gemini review @gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request hardens the package ecosystem SDK and agent UX. Key changes include optimizing the get --depth brief command to omit body, linked, and claim state fields; introducing a compact mode for validation diagnostics with a verboseDiagnostics toggle; implementing a primitive-safe stableValueEquals helper to replace order-sensitive JSON.stringify comparisons; adding concurrency locking and stale file retention to the CLI bundler; and suppressing expected handled CLI errors from Sentry reporting. Feedback highlights a critical issue in stableValueEquals where nested RegExp, Set, or Map instances inside objects or arrays will incorrectly compare as equal because the function falls back to stableStringify (which serializes them to {}). A recursive deep-equality implementation is suggested to resolve this.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| export function stableValueEquals(left: unknown, right: unknown): boolean { | ||
| if (Object.is(left, right)) { | ||
| return true; | ||
| } | ||
| if (left === null || right === null || typeof left !== "object" || typeof right !== "object") { | ||
| return false; | ||
| } | ||
| if (left instanceof RegExp || right instanceof RegExp) { | ||
| return left instanceof RegExp && right instanceof RegExp && left.toString() === right.toString(); | ||
| } | ||
| if (left instanceof Date || right instanceof Date) { | ||
| return left instanceof Date && right instanceof Date && left.getTime() === right.getTime(); | ||
| } | ||
| if (left instanceof Set || right instanceof Set) { | ||
| if (!(left instanceof Set && right instanceof Set) || left.size !== right.size) { | ||
| return false; | ||
| } | ||
| const leftValues = [...left].map(stableStringify).sort(); | ||
| const rightValues = [...right].map(stableStringify).sort(); | ||
| return leftValues.every((value, index) => value === rightValues[index]); | ||
| } | ||
| if (left instanceof Map || right instanceof Map) { | ||
| if (!(left instanceof Map && right instanceof Map) || left.size !== right.size) { | ||
| return false; | ||
| } | ||
| const leftEntries = [...left.entries()].map((entry) => stableStringify(entry)).sort(); | ||
| const rightEntries = [...right.entries()].map((entry) => stableStringify(entry)).sort(); | ||
| return leftEntries.every((value, index) => value === rightEntries[index]); | ||
| } | ||
| return stableStringify(left) === stableStringify(right); | ||
| } |
There was a problem hiding this comment.
The current implementation of stableValueEquals relies on stableStringify (which uses JSON.stringify) for nested object/array comparisons and for elements inside Set and Map. Since JSON.stringify does not natively support RegExp, Set, or Map (serialising them to {}), nested instances of these types will incorrectly compare as equal (e.g., { r: /abc/ } and { r: /def/ } would be considered equal).
Additionally, serialising entire objects to strings is less efficient than a recursive deep-equality check, which can short-circuit as soon as a difference is found.
We should replace this with a fully recursive deep-equality implementation that correctly handles nested arrays, plain objects, RegExp, Date, Set, and Map instances.
export function stableValueEquals(left: unknown, right: unknown): boolean {
if (Object.is(left, right)) {
return true;
}
if (left === null || right === null || typeof left !== "object" || typeof right !== "object") {
return false;
}
if (left instanceof RegExp || right instanceof RegExp) {
return left instanceof RegExp && right instanceof RegExp && left.toString() === right.toString();
}
if (left instanceof Date || right instanceof Date) {
return left instanceof Date && right instanceof Date && left.getTime() === right.getTime();
}
if (left instanceof Set || right instanceof Set) {
if (!(left instanceof Set && right instanceof Set) || left.size !== right.size) {
return false;
}
const rightArray = [...right];
const matched = new Set<number>();
for (const leftVal of left) {
let found = false;
for (let i = 0; i < rightArray.length; i++) {
if (matched.has(i)) {
continue;
}
if (stableValueEquals(leftVal, rightArray[i])) {
matched.add(i);
found = true;
break;
}
}
if (!found) {
return false;
}
}
return true;
}
if (left instanceof Map || right instanceof Map) {
if (!(left instanceof Map && right instanceof Map) || left.size !== right.size) {
return false;
}
const rightEntries = [...right.entries()];
const matched = new Set<number>();
for (const [leftKey, leftVal] of left.entries()) {
let found = false;
for (let i = 0; i < rightEntries.length; i++) {
if (matched.has(i)) {
continue;
}
const [rightKey, rightVal] = rightEntries[i];
if (stableValueEquals(leftKey, rightKey) && stableValueEquals(leftVal, rightVal)) {
matched.add(i);
found = true;
break;
}
}
if (!found) {
return false;
}
}
return true;
}
if (Array.isArray(left) || Array.isArray(right)) {
if (!Array.isArray(left) || !Array.isArray(right) || left.length !== right.length) {
return false;
}
return left.every((val, index) => stableValueEquals(val, right[index]));
}
const leftKeys = Object.keys(left).sort();
const rightKeys = Object.keys(right).sort();
if (leftKeys.length !== rightKeys.length) {
return false;
}
for (let i = 0; i < leftKeys.length; i++) {
if (leftKeys[i] !== rightKeys[i]) {
return false;
}
const key = leftKeys[i];
if (!stableValueEquals((left as Record<string, unknown>)[key], (right as Record<string, unknown>)[key])) {
return false;
}
}
return true;
}Replace the remaining stableValueEquals stringification fallback with recursive comparison for nested arrays, objects, RegExp, Date, Set, and Map values. Add regression coverage for nested non-JSON values and duplicate deep-equal collection entries so the shared equality helper keeps 100% coverage. Verification: node scripts/run-tests.mjs test -- tests/unit/serialization-utils.spec.ts; pnpm typecheck; pnpm build; pnpm changelog:pm:check; node scripts/run-tests.mjs coverage.
|
@codex review /gemini review @gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces several hardening and optimization improvements to the CLI and SDK. Key changes include implementing a build-locking mechanism to serialize concurrent bundle rebuilds, optimizing the get --depth brief command to omit heavy fields for lower-token agent reads, replacing order-sensitive JSON.stringify comparisons with a recursive stableValueEquals helper, and keeping default validation diagnostics compact unless verbose mode is enabled. Additionally, expected handled CLI errors are now suppressed from Sentry. Feedback on the changes suggests converting Sentry issue text to lowercase in sentry-telemetry-gate.mjs to prevent casing mismatches against lowercase patterns.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const combinedText = issueTextValue(issue); | ||
| return KNOWN_EXPECTED_HANDLED_CLI_ISSUE_PATTERNS.some((pattern) => combinedText.includes(pattern)); |
There was a problem hiding this comment.
The error patterns in KNOWN_EXPECTED_HANDLED_CLI_ISSUE_PATTERNS are defined in lowercase, but actual CLI error messages captured by Sentry might contain uppercase characters (e.g., 'Slack webhook request failed' or 'Authentication required...'). To ensure robust matching and prevent the release gate from failing due to casing mismatches, convert combinedText to lowercase before performing the .includes() check.
| const combinedText = issueTextValue(issue); | |
| return KNOWN_EXPECTED_HANDLED_CLI_ISSUE_PATTERNS.some((pattern) => combinedText.includes(pattern)); | |
| const combinedText = issueTextValue(issue).toLowerCase(); | |
| return KNOWN_EXPECTED_HANDLED_CLI_ISSUE_PATTERNS.some((pattern) => combinedText.includes(pattern)); |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d347e76051
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| function isExpectedHandledCliIssue(issue) { | ||
| const metadata = issue && typeof issue === "object" ? issue.metadata : null; | ||
| const type = metadata && typeof metadata.type === "string" ? metadata.type : ""; | ||
| if ((type !== "PmCliError" && type !== "CommandError") || issue?.isUnhandled === true) { |
There was a problem hiding this comment.
When the gate uses the REST API token path (the normal path whenever a Sentry token is present), the project issues response schema lists fields such as level, logger, metadata, title, and type but not isUnhandled (Sentry docs: https://docs.sentry.io/api/events/list-a-projects-issues/). That means issue?.isUnhandled === true is false for API results, so an unresolved unhandled PmCliError/CommandError whose title/value matches one of these expected patterns is moved to ignoredExpected and excluded from the critical/high tally. Please only ignore these issues when the payload positively proves they are handled, or fetch/query the unhandled state before partitioning.
Useful? React with 👍 / 👎.
Make release-gate issue text lowercase at the pattern matching sites for ignored console noise and expected handled CLI errors. Add a release automation contract assertion so expected handled CLI issue matching stays case-insensitive. Verification: node scripts/run-tests.mjs test -- tests/integration/release-automation-contract.spec.ts tests/unit/serialization-utils.spec.ts; pnpm typecheck; pnpm build; pnpm changelog:pm:check; SENTRY_FORCE_ENV_TOKEN=1 SENTRY_AUTH_TOKEN=*** node scripts/release/sentry-telemetry-gate.mjs --json --telemetry-mode off --max-critical 0 --max-high 0.
|
@codex review /gemini review @gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request hardens the package ecosystem, SDK, and agent UX. Key changes include introducing a recursive, order-insensitive stableValueEquals utility to replace order-sensitive JSON.stringify comparisons, optimizing get --depth brief to omit empty fields for lower token usage, implementing a file-locking mechanism in bundle-cli.mjs for safe concurrent builds, and compacting default validation diagnostics. Additionally, expected handled CLI errors are now filtered out from Sentry telemetry. I have no feedback to provide as there are no review comments.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
Codex Review: Didn't find any major issues. You're on a roll. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
…cked counters vs edge-aware pm next), PR#577 lane reconciled zero-leak, 96h7 frontier note Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Summary
This PR closes the concrete fixes from the June 1 package ecosystem / SDK / agent UX audit tracked in
pm-z0ip.pm get --depth briefgenuinely low-token by omitting body, linked containers, and claim-state history unless explicitly requested.pm validate --check-resolution --jsondiagnostics while preserving full output under--verbose-diagnostics.bundle-clifrom deleting the live bundle before rebuilding, preventing transient missing chunk/module failures in shared agent checkouts.Root Cause
The audit found several agent-facing friction points: brief single-item reads still emitted empty shells and loaded history; validation JSON could dump too many remediation commands;
JSON.stringifyequality treated semantically identical objects as different; local build gates could delete livedist/cli-bundlewhile another command was importing it; and expected package/user errors were being counted as Sentry high-priority release blockers.Validation
node scripts/run-tests.mjs test -- tests/unit/cli-main-errors.spec.ts tests/integration/release-automation-contract.spec.ts tests/unit/get-append-command.spec.ts tests/unit/validate-command.spec.ts tests/unit/test-command.spec.ts tests/unit/update-many-command.spec.ts tests/integration/help-runtime.spec.tspassed: 7 files, 164 tests.node scripts/run-tests.mjs coveragepassed: 135 files, 2092 tests, 100% statements/branches/functions/lines.pnpm build,pnpm typecheck,pnpm quality:static,pnpm quality:docs-skills,pnpm security:scan, andpnpm changelog:pm:checkpassed.pm test pm-z0ip --run --progresspassed both linked tests.pm validate --check-resolution --check-history-drift --jsonpassed with known pre-existing compact resolution warnings and no history drift.pm health --check-only --brief --jsonpassed.node scripts/dogfood-package-first.mjspassed in an isolated temp project: 87 commands.Tracker
pm-z0iprecords duplicate-check evidence, sub-agent lane findings, linked files/docs/tests, dogfood evidence, and verification comments.