Repository navigation
Add package ecosystem smoke harness and SDK test helper - #108
Conversation
Implement pm-vnjh by adding a reusable external npm package smoke harness that discovers pm-package npm modules, installs each in isolated PM_PATH/PM_GLOBAL_PATH temp projects, runs package doctor, and probes runtime contracts. Expose activateExtensionForTest through the public SDK testing surface so package authors can exercise the real extension activation and capability guardrails without private loader imports. Document the package-author and ecosystem smoke workflows, refresh CHANGELOG.md through pm-changelog 2026.6.4, and update the Sentry release gate to treat handled one-error validation CommandErrors as expected CLI errors. Verification: pm test pm-vnjh --run --progress passed 11/11 linked commands including coverage at 100%, typecheck, build, docs/static/security gates, pm-changelog external smoke, Sentry/telemetry gate, and telemetry stack health.
|
@codex review Please review the package ecosystem smoke harness, public SDK testing helper, pm-changelog update, and Sentry gate handled-error classification in this PR. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
Warning Review limit reached
More reviews will be available in 17 minutes and 43 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 (6)
📝 WalkthroughWalkthroughThis PR delivers SDK in-memory extension testing utilities, a comprehensive external npm package ecosystem smoke harness for verification after build, a pm-changelog extension release (2026.6.4) with new opt-in CLI flags, release gate refinements for Sentry validation, and corresponding documentation updates. ChangesSDK testing utilities and ecosystem smoke infrastructure
Estimated code review effort🎯 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 a reusable external npm package smoke harness to test package compatibility in isolated environments, along with a public activateExtensionForTest SDK helper to facilitate in-memory extension testing for package authors. It also adds several opt-in features to the pm-changelog extension (such as breaking changes, semver suggestions, body previews, and emoji prefixes) and updates the Sentry release gate configuration. The review feedback highlights several robustness improvements for the new smoke harness script, including resolving the repository root relative to the script location, capturing execution errors from spawnSync, and safely catching potential cleanup failures in rmSync. Additionally, it is recommended to defensively check that capabilities are arrays before spreading them in the SDK testing helper.
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.
| import { spawnSync } from "node:child_process"; | ||
| import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; | ||
| import { tmpdir } from "node:os"; | ||
| import path from "node:path"; | ||
|
|
||
| const repoRoot = process.cwd(); | ||
| const cliPath = path.join(repoRoot, "dist", "cli.js"); | ||
| const defaultQuery = "keywords:pm-package"; | ||
| const ecosystemMarkers = ["pm-package", "pm-cli", "pm-extension", "pm-cli-extension"]; |
There was a problem hiding this comment.
Using process.cwd() to resolve the repository root can cause the script to fail if it is executed from a subdirectory (e.g., from within the scripts/ directory itself). It is more robust to resolve the repository root relative to the script's own location using import.meta.url.
import { spawnSync } from "node:child_process";
import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs";
import { tmpdir } from "node:os";
import path from "node:path";
import { fileURLToPath } from "node:url";
const __dirname = path.dirname(fileURLToPath(import.meta.url));
const repoRoot = path.resolve(__dirname, "..");
const cliPath = path.join(repoRoot, "dist", "cli.js");
const defaultQuery = "keywords:pm-package";
const ecosystemMarkers = ["pm-package", "pm-cli", "pm-extension", "pm-cli-extension"];| function runCommand(command, args, options = {}) { | ||
| const startedAt = Date.now(); | ||
| const result = spawnSync(command, args, { | ||
| cwd: options.cwd ?? repoRoot, | ||
| encoding: "utf8", | ||
| env: options.env ?? process.env, | ||
| maxBuffer: 20 * 1024 * 1024, | ||
| timeout: options.timeoutMs, | ||
| }); | ||
| const tookMs = Date.now() - startedAt; | ||
| return { | ||
| code: result.status ?? 1, | ||
| signal: result.signal ?? null, | ||
| stdout: result.stdout ?? "", | ||
| stderr: result.stderr ?? "", | ||
| took_ms: tookMs, | ||
| command: [command, ...args].join(" "), | ||
| }; | ||
| } |
There was a problem hiding this comment.
If spawnSync fails to execute the command (for example, if npm is not installed or not found in the system's PATH), it returns an object with result.error (e.g., ENOENT), and result.status is null. Currently, runCommand ignores result.error and returns an empty stderr, which makes debugging extremely difficult as the error message will be completely empty. We should capture result.error and populate stderr with its message.
| function runCommand(command, args, options = {}) { | |
| const startedAt = Date.now(); | |
| const result = spawnSync(command, args, { | |
| cwd: options.cwd ?? repoRoot, | |
| encoding: "utf8", | |
| env: options.env ?? process.env, | |
| maxBuffer: 20 * 1024 * 1024, | |
| timeout: options.timeoutMs, | |
| }); | |
| const tookMs = Date.now() - startedAt; | |
| return { | |
| code: result.status ?? 1, | |
| signal: result.signal ?? null, | |
| stdout: result.stdout ?? "", | |
| stderr: result.stderr ?? "", | |
| took_ms: tookMs, | |
| command: [command, ...args].join(" "), | |
| }; | |
| } | |
| function runCommand(command, args, options = {}) { | |
| const startedAt = Date.now(); | |
| const result = spawnSync(command, args, { | |
| cwd: options.cwd ?? repoRoot, | |
| encoding: "utf8", | |
| env: options.env ?? process.env, | |
| maxBuffer: 20 * 1024 * 1024, | |
| timeout: options.timeoutMs, | |
| }); | |
| const tookMs = Date.now() - startedAt; | |
| return { | |
| code: result.status ?? (result.error ? 1 : 0), | |
| signal: result.signal ?? null, | |
| stdout: result.stdout ?? "", | |
| stderr: result.stderr || (result.error ? result.error.message : ""), | |
| took_ms: tookMs, | |
| command: [command, ...args].join(" "), | |
| }; | |
| } |
| } finally { | ||
| if (!options.keepTemp) { | ||
| rmSync(tempRoot, { recursive: true, force: true, maxRetries: 10, retryDelay: 100 }); | ||
| } | ||
| } |
There was a problem hiding this comment.
If rmSync fails (for example, due to temporary file locks on Windows or permission issues), it will throw an unhandled exception in the finally block. This will crash the entire script and prevent subsequent packages from being smoke-tested. Wrapping rmSync in a try...catch block ensures that a cleanup failure for one package does not abort the entire test run.
| } finally { | |
| if (!options.keepTemp) { | |
| rmSync(tempRoot, { recursive: true, force: true, maxRetries: 10, retryDelay: 100 }); | |
| } | |
| } | |
| } finally { | |
| if (!options.keepTemp) { | |
| try { | |
| rmSync(tempRoot, { recursive: true, force: true, maxRetries: 10, retryDelay: 100 }); | |
| } catch (cleanupError) { | |
| console.error(`Failed to clean up temp directory ${tempRoot}:`, cleanupError); | |
| } | |
| } | |
| } |
| options.name ?? | ||
| (typeof manifest.name === "string" && manifest.name.trim().length > 0 ? manifest.name.trim() : "test-extension"); | ||
| const layer = options.layer ?? "project"; | ||
| const capabilities = [...(options.capabilities ?? (manifest.capabilities as ExtensionCapability[] | undefined) ?? [])]; |
There was a problem hiding this comment.
If manifest.capabilities is not an array (e.g., if it is malformed or not provided as an array in a custom test manifest), spreading it directly with ... will throw a runtime TypeError: is not iterable. We should defensively check if options.capabilities or manifest.capabilities are arrays before spreading them to prevent runtime crashes.
const capabilities = [
...(Array.isArray(options.capabilities)
? options.capabilities
: Array.isArray(manifest.capabilities)
? (manifest.capabilities as ExtensionCapability[])
: []),
];
There was a problem hiding this comment.
Code Review
This pull request introduces a reusable external npm package ecosystem smoke harness to discover and test external packages in isolated environments, and adds activateExtensionForTest to the public SDK testing surface for in-memory extension activation testing. It also updates Sentry telemetry gates, documentation, and adds corresponding integration and unit tests. Feedback on these changes highlights a potential resource leak and missing directory creation in the smoke harness script, as well as a potential TypeError in the SDK testing helper if manifest.capabilities is not an array when spread.
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 tempRoot = mkdtempSync(path.join(tmpdir(), "pm-external-package-smoke-")); | ||
| const projectRoot = path.join(tempRoot, "project"); | ||
| const pmPath = path.join(projectRoot, ".agents", "pm"); | ||
| const globalPath = path.join(tempRoot, "global"); | ||
| mkdirSync(projectRoot, { recursive: true }); | ||
| writeFileSync(path.join(projectRoot, "README.md"), "# External package smoke project\n", "utf8"); | ||
|
|
||
| const env = { | ||
| ...process.env, | ||
| PM_PATH: pmPath, | ||
| PM_GLOBAL_PATH: globalPath, | ||
| PM_AUTHOR: "external-package-smoke", | ||
| PM_TELEMETRY_SOURCE_CONTEXT: process.env.PM_TELEMETRY_SOURCE_CONTEXT || "external-package-smoke", | ||
| }; | ||
| const commands = []; | ||
| const startedAt = Date.now(); | ||
|
|
||
| function record(label, args) { | ||
| const result = runPm(label, args, env, { | ||
| cwd: projectRoot, | ||
| timeoutMs: options.timeoutMs, | ||
| }); | ||
| commands.push(result.entry); | ||
| if (result.entry.code !== 0) { | ||
| throw new Error(result.entry.error || `${label} failed`); | ||
| } | ||
| return result.payload; | ||
| } | ||
|
|
||
| try { | ||
| record("init", ["init", "--defaults", "--author", "external-package-smoke"]); | ||
| const install = record("install", ["install", `npm:${packageName}`, "--project"]); | ||
| const doctor = record("package doctor", ["package", "doctor", "--project", "--detail", "deep", "--trace"]); | ||
| const contracts = record("runtime contracts", ["contracts", "--runtime-only", "--availability-only"]); | ||
|
|
||
| const summary = doctor?.details?.summary ?? {}; | ||
| const activationFailures = summary.activation_failure_count ?? 0; | ||
| const blockingFailures = summary.blocking_failure_count ?? 0; | ||
| if (activationFailures > 0 || blockingFailures > 0) { | ||
| throw new Error(`package doctor reported activation=${activationFailures} blocking=${blockingFailures}`); | ||
| } | ||
|
|
||
| const availableActions = Array.isArray(contracts?.action_availability) | ||
| ? contracts.action_availability.filter((entry) => entry?.available === true).map((entry) => entry.action) | ||
| : []; | ||
| return { | ||
| package: packageName, | ||
| ok: true, | ||
| took_ms: Date.now() - startedAt, | ||
| temp_root: options.keepTemp ? tempRoot : undefined, | ||
| installed_count: install?.details?.installed_count ?? null, | ||
| activation_failure_count: activationFailures, | ||
| blocking_failure_count: blockingFailures, | ||
| warning_codes: doctor?.details?.triage?.warning_codes ?? [], | ||
| available_runtime_actions: availableActions, | ||
| commands, | ||
| }; | ||
| } catch (error) { | ||
| return { | ||
| package: packageName, | ||
| ok: false, | ||
| took_ms: Date.now() - startedAt, | ||
| temp_root: options.keepTemp ? tempRoot : undefined, | ||
| error: error instanceof Error ? error.message : String(error), | ||
| commands, | ||
| }; | ||
| } finally { | ||
| if (!options.keepTemp) { | ||
| rmSync(tempRoot, { recursive: true, force: true, maxRetries: 10, retryDelay: 100 }); | ||
| } | ||
| } |
There was a problem hiding this comment.
There is a potential resource leak if any setup steps (such as mkdirSync or writeFileSync) fail before entering the try block. The temporary directory created by mkdtempSync will not be cleaned up because the finally block is only executed if the try block is entered.
Additionally, the globalPath directory is defined but never created on disk, which could cause issues if the pm CLI expects it to exist.
To fix both issues, declare commands and startedAt outside, start the try block immediately after creating tempRoot, and ensure both projectRoot and globalPath are created recursively.
const tempRoot = mkdtempSync(path.join(tmpdir(), "pm-external-package-smoke-"));
const commands = [];
const startedAt = Date.now();
try {
const projectRoot = path.join(tempRoot, "project");
const pmPath = path.join(projectRoot, ".agents", "pm");
const globalPath = path.join(tempRoot, "global");
mkdirSync(projectRoot, { recursive: true });
mkdirSync(globalPath, { recursive: true });
writeFileSync(path.join(projectRoot, "README.md"), "# External package smoke project\n", "utf8");
const env = {
...process.env,
PM_PATH: pmPath,
PM_GLOBAL_PATH: globalPath,
PM_AUTHOR: "external-package-smoke",
PM_TELEMETRY_SOURCE_CONTEXT: process.env.PM_TELEMETRY_SOURCE_CONTEXT || "external-package-smoke",
};
function record(label, args) {
const result = runPm(label, args, env, {
cwd: projectRoot,
timeoutMs: options.timeoutMs,
});
commands.push(result.entry);
if (result.entry.code !== 0) {
throw new Error(result.entry.error || label + " failed");
}
return result.payload;
}
record("init", ["init", "--defaults", "--author", "external-package-smoke"]);
const install = record("install", ["install", "npm:" + packageName, "--project"]);
const doctor = record("package doctor", ["package", "doctor", "--project", "--detail", "deep", "--trace"]);
const contracts = record("runtime contracts", ["contracts", "--runtime-only", "--availability-only"]);
const summary = doctor?.details?.summary ?? {};
const activationFailures = summary.activation_failure_count ?? 0;
const blockingFailures = summary.blocking_failure_count ?? 0;
if (activationFailures > 0 || blockingFailures > 0) {
throw new Error("package doctor reported activation=" + activationFailures + " blocking=" + blockingFailures);
}
const availableActions = Array.isArray(contracts?.action_availability)
? contracts.action_availability.filter((entry) => entry?.available === true).map((entry) => entry.action)
: [];
return {
package: packageName,
ok: true,
took_ms: Date.now() - startedAt,
temp_root: options.keepTemp ? tempRoot : undefined,
installed_count: install?.details?.installed_count ?? null,
activation_failure_count: activationFailures,
blocking_failure_count: blockingFailures,
warning_codes: doctor?.details?.triage?.warning_codes ?? [],
available_runtime_actions: availableActions,
commands,
};
} catch (error) {
return {
package: packageName,
ok: false,
took_ms: Date.now() - startedAt,
temp_root: options.keepTemp ? tempRoot : undefined,
error: error instanceof Error ? error.message : String(error),
commands,
};
} finally {
if (!options.keepTemp) {
rmSync(tempRoot, { recursive: true, force: true, maxRetries: 10, retryDelay: 100 });
}
}| options.name ?? | ||
| (typeof manifest.name === "string" && manifest.name.trim().length > 0 ? manifest.name.trim() : "test-extension"); | ||
| const layer = options.layer ?? "project"; | ||
| const capabilities = [...(options.capabilities ?? (manifest.capabilities as ExtensionCapability[] | undefined) ?? [])]; |
There was a problem hiding this comment.
If manifest.capabilities is not an array (for example, if a user accidentally passes a string like "commands"), spreading it directly using the spread operator will either throw a TypeError or spread the string into individual characters. To ensure robust defensive programming, check if manifest.capabilities is an array before spreading.
| const capabilities = [...(options.capabilities ?? (manifest.capabilities as ExtensionCapability[] | undefined) ?? [])]; | |
| const manifestCapabilities = Array.isArray(manifest.capabilities) ? manifest.capabilities : []; | |
| const capabilities = [...(options.capabilities ?? manifestCapabilities)]; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/smoke-external-packages.mjs`:
- Around line 220-222: The code currently builds availableActions from
contracts.action_availability by checking entry?.available; update that to use
the runtime contract’s invocable flag instead: filter entries by
entry?.invocable === true when computing availableActions (the
Array.isArray(contracts?.action_availability) branch that maps to entry.action
should remain the same), so availableActions reflects
action_availability[].invocable as described in the SDK/runtime docs.
In `@src/sdk/testing.ts`:
- Line 140: The spread of manifest.capabilities can throw if
manifest.capabilities is not an array; update the logic around the const
capabilities = ... line to validate manifest.capabilities with Array.isArray
before spreading (use options.capabilities if provided, else if
Array.isArray(manifest.capabilities) use that, otherwise fall back to an empty
array) so malformed manifests like { capabilities: "commands" } or numbers won't
crash; reference the existing symbols options.capabilities and
manifest.capabilities and ensure the resulting capabilities is always an array
of ExtensionCapability.
🪄 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: 583df0c8-f3cb-427a-bf19-abf2d5b47ef5
⛔ Files ignored due to path filters (16)
.agents/pm/extensions/pm-changelog/dist/cli.jsis excluded by!**/dist/**.agents/pm/extensions/pm-changelog/dist/cli.js.mapis excluded by!**/dist/**,!**/*.map.agents/pm/extensions/pm-changelog/dist/extension.d.ts.mapis excluded by!**/dist/**,!**/*.map.agents/pm/extensions/pm-changelog/dist/extension.jsis excluded by!**/dist/**.agents/pm/extensions/pm-changelog/dist/extension.js.mapis excluded by!**/dist/**,!**/*.map.agents/pm/extensions/pm-changelog/dist/generator.d.tsis excluded by!**/dist/**.agents/pm/extensions/pm-changelog/dist/generator.d.ts.mapis excluded by!**/dist/**,!**/*.map.agents/pm/extensions/pm-changelog/dist/generator.jsis excluded by!**/dist/**.agents/pm/extensions/pm-changelog/dist/generator.js.mapis excluded by!**/dist/**,!**/*.map.agents/pm/extensions/pm-changelog/dist/index.d.tsis excluded by!**/dist/**.agents/pm/extensions/pm-changelog/dist/index.d.ts.mapis excluded by!**/dist/**,!**/*.map.agents/pm/extensions/pm-changelog/dist/index.jsis excluded by!**/dist/**.agents/pm/extensions/pm-changelog/dist/index.js.mapis excluded by!**/dist/**,!**/*.map.agents/pm/extensions/pm-changelog/dist/types.d.tsis excluded by!**/dist/**.agents/pm/extensions/pm-changelog/dist/types.d.ts.mapis excluded by!**/dist/**,!**/*.map.agents/pm/extensions/pm-changelog/dist/types.js.mapis excluded by!**/dist/**,!**/*.map
📒 Files selected for processing (23)
.agents/pm/epics/pm-u9d0.toon.agents/pm/epics/pm-ugqx.toon.agents/pm/extensions/.managed-extensions.json.agents/pm/extensions/pm-changelog/CHANGELOG.md.agents/pm/extensions/pm-changelog/README.md.agents/pm/extensions/pm-changelog/docs/usage.md.agents/pm/extensions/pm-changelog/manifest.json.agents/pm/extensions/pm-changelog/package.json.agents/pm/features/pm-vnjh.toon.agents/pm/history/pm-u9d0.jsonl.agents/pm/history/pm-ugqx.jsonl.agents/pm/history/pm-vnjh.jsonlCHANGELOG.mddocs/EXTENSIONS.mddocs/SDK.mddocs/TESTING.mdpackage.jsonscripts/release/sentry-telemetry-gate.mjsscripts/smoke-external-packages.mjssrc/sdk/index.tssrc/sdk/testing.tstests/integration/release-automation-contract.spec.tstests/unit/sdk-index.spec.ts
![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: 991a114ab9
ℹ️ 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".
| @@ -177,6 +193,7 @@ export function buildChangelogDocument(options) { | |||
| section_by: sectionBy, | |||
There was a problem hiding this comment.
When --changelog-json is combined with release-window output and visibility flags such as --all-release-tags --limit 1 --suggest-semver or --since-version, the JSON releases array is narrowed via visibleSections, but suggestSemver(options) recomputes over filterItems(options) and ignores those section limits. This can report a major/minor/patch bump because of older hidden releases that are not present in the emitted changelog document; compute the suggestion from the same visibleSections items instead.
Useful? React with 👍 / 👎.
| const manifest = (module as TestExtensionModule).manifest; | ||
| if (manifest && typeof manifest === "object") { | ||
| return manifest; |
There was a problem hiding this comment.
When package tests pass the ESM namespace returned by await import(...) for a default-exported extension, activation later finds default.activate, but this helper only reads module.manifest. A default export shaped like export default defineExtension({ manifest: { capabilities: ["commands"] }, activate(...) { ... } }) therefore falls back to test-extension with an empty capabilities array, causing real command/schema registrations to fail unless the test duplicates metadata in options; check module.default.manifest when the top-level manifest is absent.
Useful? React with 👍 / 👎.
| const limit = options.bodyPreview && options.bodyPreview > 0 ? options.bodyPreview : 0; | ||
| if (limit === 0) | ||
| return ""; | ||
| const body = typeof item.body === "string" ? toSingleLine(item.body) : ""; |
There was a problem hiding this comment.
With the normal npx pm-changelog --body-preview path and the pm changelog generate --body-preview extension command, items are loaded via pm list-all --json / listAllFrontMatter, which do not include the document body unless the body-specific list path is requested. As a result item.body is usually absent here and the new flag silently emits no previews for real pm items; load items with bodies when bodyPreview is set, or document that this only works for custom stdin JSON containing body.
Useful? React with 👍 / 👎.
| ] | ||
| .join(" ") | ||
| .toLowerCase(); | ||
| return haystack.includes("breaking"); |
There was a problem hiding this comment.
For items whose type, tag, or title says something like non-breaking fix, this substring check still marks the item as breaking and makes --suggest-semver recommend a major bump. That is a common release-note phrase for explicitly safe changes, so the breaking detector should require a real breaking token/signal rather than any occurrence inside a negated word.
Useful? React with 👍 / 👎.
Harden the external package smoke harness with script-relative repo root resolution, spawn error reporting, global temp root creation, cleanup guards, and invocable action reporting. Make activateExtensionForTest tolerate malformed manifest capabilities and default-exported manifests, with regression coverage. Record review-response evidence on pm-vnjh and route package-owned pm-changelog follow-ups upstream as issues #26, #27, and #28.
|
@codex review Review-response commit pushed (1bfacca). Please re-review the hardened smoke harness and SDK testing helper changes. |
There was a problem hiding this comment.
Code Review
This pull request introduces a reusable external npm package ecosystem smoke harness and adds activateExtensionForTest to the public SDK testing surface to enable in-memory extension testing for package authors. It also updates the Sentry telemetry gate to handle one-error validation issues and upgrades the pm-changelog extension with several opt-in features. Feedback on the changes suggests making the readTestExtensionManifest helper more robust by allowing it to fall back to reading metadata properties directly from standard extension modules when the nested manifest property is absent.
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.
| function readTestExtensionManifest(module: unknown): Partial<ExtensionManifest> { | ||
| if (module && typeof module === "object") { | ||
| const manifest = (module as TestExtensionModule).manifest; | ||
| if (manifest && typeof manifest === "object") { | ||
| return manifest; | ||
| } | ||
| const defaultManifest = (module as TestExtensionModule).default?.manifest; | ||
| if (defaultManifest && typeof defaultManifest === "object") { | ||
| return defaultManifest; | ||
| } | ||
| } | ||
| return {}; | ||
| } |
There was a problem hiding this comment.
The readTestExtensionManifest helper currently only extracts metadata if it is nested under a manifest property (either directly on the module or on its default export). However, standard extensions created via defineExtension typically export the extension object directly (or as a default export) with metadata properties like name, version, and capabilities directly on it.
To make this helper more robust and intuitive for package authors, we should fall back to reading these metadata properties directly from the module or its default export when the manifest property is absent.
function readTestExtensionManifest(module: unknown): Partial<ExtensionManifest> {
if (module && typeof module === "object") {
const mod = module as Record<string, unknown>;
if (mod.manifest && typeof mod.manifest === "object") {
return mod.manifest as Partial<ExtensionManifest>;
}
const defaultExport = mod.default as Record<string, unknown> | undefined;
if (defaultExport && typeof defaultExport === "object") {
if (defaultExport.manifest && typeof defaultExport.manifest === "object") {
return defaultExport.manifest as Partial<ExtensionManifest>;
}
if ("name" in defaultExport || "capabilities" in defaultExport) {
return defaultExport as Partial<ExtensionManifest>;
}
}
if ("name" in mod || "capabilities" in mod) {
return mod as Partial<ExtensionManifest>;
}
}
return {};
}
|
@codex review Second review-response commit pushed (a75510d). Please re-review the SDK testing helper metadata fallback and added regression coverage. |
There was a problem hiding this comment.
Code Review
This pull request introduces a reusable external npm package ecosystem smoke harness script and exposes a new public SDK testing helper, activateExtensionForTest, for in-memory extension activation testing, along with updated documentation. Additionally, the pm-changelog extension is updated to version 2026.6.4 with several new opt-in features. Review feedback suggests improving the robustness of the smoke harness script by handling JSON parsing errors gracefully, parsing comma-separated keywords in npm search results, and filtering out invalid action entries.
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 entry = { label, code: result.code, took_ms: result.took_ms }; | ||
| if (result.code !== 0) { | ||
| entry.error = summarizeFailure(result); | ||
| return { entry, payload: null }; | ||
| } | ||
| return { entry, payload: parseJsonOutput(result, label) }; |
There was a problem hiding this comment.
If parseJsonOutput throws an error (e.g., due to invalid JSON or empty stdout), runPm will throw an unhandled exception. This bypasses the return statement in runPm, meaning that record never receives the result and cannot push the failed command's entry into the commands array. As a result, the final failure report will be missing the command that caused the JSON parsing failure.
We should wrap the JSON parsing in a try-catch block inside runPm to gracefully capture the failure as a command entry with a non-zero exit code.
| const entry = { label, code: result.code, took_ms: result.took_ms }; | |
| if (result.code !== 0) { | |
| entry.error = summarizeFailure(result); | |
| return { entry, payload: null }; | |
| } | |
| return { entry, payload: parseJsonOutput(result, label) }; | |
| const entry = { label, code: result.code, took_ms: result.took_ms }; | |
| if (result.code !== 0) { | |
| entry.error = summarizeFailure(result); | |
| return { entry, payload: null }; | |
| } | |
| try { | |
| return { entry, payload: JSON.parse(result.stdout) }; | |
| } catch (error) { | |
| entry.code = 1; | |
| entry.error = `Failed to parse JSON output: ${error instanceof Error ? error.message : String(error)}`; | |
| return { entry, payload: null }; | |
| } |
| if (!entry || typeof entry.name !== "string" || entry.name.length === 0) { | ||
| return false; | ||
| } | ||
| const keywords = Array.isArray(entry.keywords) ? entry.keywords : []; |
There was a problem hiding this comment.
In npm search --json, the keywords field can sometimes be returned as a comma-separated string or undefined rather than a pre-parsed array of strings. Checking only Array.isArray(entry.keywords) might cause the search to miss relevant keywords when they are formatted as a string.
We can improve robustness by parsing comma-separated strings into an array of keywords.
| const keywords = Array.isArray(entry.keywords) ? entry.keywords : []; | |
| const keywords = Array.isArray(entry.keywords) | |
| ? entry.keywords | |
| : typeof entry.keywords === "string" | |
| ? entry.keywords.split(/[\s,]+/) | |
| : []; |
| const availableActions = Array.isArray(contracts?.action_availability) | ||
| ? contracts.action_availability.filter((entry) => entry?.invocable === true).map((entry) => entry.action) | ||
| : []; |
There was a problem hiding this comment.
When mapping contracts.action_availability, if entry.action is undefined or not a string, it will map to undefined in the resulting array. It is safer to filter out any entries that do not have a valid string action property to avoid pushing undefined values into available_runtime_actions.
const availableActions = Array.isArray(contracts?.action_availability)
? contracts.action_availability
.filter((entry) => entry?.invocable === true && typeof entry?.action === "string")
.map((entry) => entry.action)
: [];
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a75510d2ee
ℹ️ 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".
| if (Array.isArray(manifest.capabilities)) { | ||
| return manifest.capabilities.filter((capability): capability is ExtensionCapability => typeof capability === "string"); | ||
| } |
There was a problem hiding this comment.
When an in-memory test manifest uses supported legacy capability aliases such as validation or migration, this helper passes the raw strings through here, while real filesystem-loaded manifests are normalized via normalizeManifestCapabilities before activation. Because assertExtensionCapability only recognizes canonical capability names, a package-author test using a runtime-valid alias for schema will fail with a missing-capability activation error even though the same manifest passes the real loader/doctor path.
Useful? React with 👍 / 👎.
| section_by: sectionBy, | ||
| item_count: visibleSections.reduce((sum, section) => sum + section.items.length, 0), | ||
| releases, | ||
| suggested_semver: options.suggestSemver ? suggestSemver(options) : undefined, |
There was a problem hiding this comment.
When --changelog-json --suggest-semver is combined with release-window filters such as --all-release-tags --limit 1 or --since-version, the JSON document's releases and item_count are computed from visibleSections, but this new field recomputes from the unbounded options. That makes the suggested bump count hidden older releases (for example, a breaking item outside the requested limit still reports major for a document containing only non-breaking changes), so downstream release automation can choose the wrong version bump.
Useful? React with 👍 / 👎.
| ] | ||
| .join(" ") | ||
| .toLowerCase(); | ||
| return haystack.includes("breaking"); |
There was a problem hiding this comment.
For the new --breaking-changes / --suggest-semver paths, checking for the substring breaking marks common titles or tags such as non-breaking API cleanup or not-breaking as a breaking change. In those cases the generated changelog gets an erroneous Breaking Changes entry and the semver suggestion is promoted to major even though the item explicitly says it is non-breaking.
Useful? React with 👍 / 👎.
|
@codex review Third review-response commit pushed (ddb4a3a). Please re-review the smoke harness JSON parse failure recording, npm keyword normalization, and runtime action filtering changes. |
There was a problem hiding this comment.
Code Review
This pull request introduces a reusable external npm package ecosystem smoke harness to discover and test external packages in isolated environments, and adds a public SDK testing helper activateExtensionForTest to allow package authors to test extension activation in-memory. Additionally, the pm-changelog extension is updated to version 2026.6.4, introducing new opt-in features such as breaking changes sections, semver suggestions, body previews, and emoji prefixes. There are no review comments, 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.
Summary
pm tracking
Verification
Notes