Repository navigation
feat(cli): gap-report --to files a report where the person chose - #7724
Conversation
meta-cla
Bot
added
the
CLA Signed
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
PR Analysis Report📚 Storybook Preview🧪 Sandbox PreviewNo new or modified components detected. Bundle Size SummaryNo component packages changed. Accessibility AuditStatus: No accessibility violations detected. Visual Regression40 of 482 shot(s) changed. A change here is a question, not a failure: check whether the after is the
and 20 more. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approval needs a maintainer to provide or identify the governing spec for the new route: spec:AST-031 FR13 and DEC-4 are still authority: draft on main, so what must be settled is whether routing a report about an integration to that integration's own issuesUrl after the handlers, with its consent and skip rules, becomes current authority. Separately, three changes are needed before this can be approved:
- The own-handler skip matches by package name, so when
astryx.configre-exports the integration'sgapReport(deduplicated into the project slot), the route still files at itsissuesUrl(packages/cli/api/gap-report/gap-report.mjs:684-692). isInsidecompares against theissuesUrlpath, so anhttps://github.com/<owner>/<repo>/issues/newURL does not recognize an issue a handler already filed in that repository, and a second issue is filed (gap-report.mjs:664-665).- The change replaces documented behaviour, so
spec:AST-017FR5 and FR7 call for a[feat]changeset rather than[fix].
[Automated review]
| if ( | ||
| entries.some( | ||
| entry => | ||
| entry.handlerType === 'integration' && | ||
| entry.handlerName === target.package, | ||
| ) | ||
| ) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
This matches the target's own handler by package name only. When astryx.config re-exports the integration's gapReport, identity dedupe keeps just the project entry (line 315), so this check misses it and the report reaches the integration twice: through its handler and again at its issuesUrl. Comparing each entry's handler.handle with the target integration's own __gapReport.handle would cover that case, and a test with a re-exporting config would pin it.
| const base = outer.pathname.replace(/\/+$/u, ''); | ||
| return inner.pathname === base || inner.pathname.startsWith(`${base}/`); |
There was a problem hiding this comment.
For a GitHub URL in the /issues/new form, which githubRoute accepts and the CLI's default issues URL uses, base becomes /<owner>/<repo>/issues/new, so a receipt such as https://github.com/<owner>/<repo>/issues/43 is not inside it and, with --confirm-public, a second issue is filed in the same repository. Testing containment against the repository's /issues root from githubRoute would fix it.
| '@astryxdesign/cli': patch | ||
| --- | ||
|
|
||
| [fix] `astryx gap-report` now also sends a report about an integration to that integration's own `issuesUrl` when report handlers are loaded, so a package's tracker is no longer skipped whenever any handler exists. A GitHub URL still needs `--confirm-public`. The route is skipped for Core, when the integration's own handler ran, and when a handler already filed the report inside that URL. |
There was a problem hiding this comment.
This replaces documented behaviour (one handler used to keep a report away from issuesUrl), and spec:AST-017 FR5 says such a change is not a fix; under FR7 a compatible new capability is [feat].
… issues/new repositories
There was a problem hiding this comment.
Approval needs a maintainer to provide or identify the governing spec for the new target route: spec:AST-031 FR13 and DEC-4 are still authority: draft on main, so what must be settled is whether a report about an integration also goes to that integration's own issuesUrl after the handlers, with these consent and skip rules, including this branch's readings that a GitHub …/issues/new URL covers issues already filed in that repository and that “inside” stops at path segments. Independently of that decision, one change is needed before this can be approved:
- With
--confirm-publicand no workinggh, a run whose handler filed now endspartialand exits 1 instead offiledand 0, a breaking exit-behaviour change underspec:AST-017FR1 and FR3 that a[feat]patch cannot carry, so keep a failed target route out of the overall status and exit code, or ship it as[breaking]in a declared minor.
Smaller points: the same aggregate change heads a lone failing handler “Gap report partially filed” with nothing filed, the route reuses the “No automatic handler is available” message right under a handler that filed, and the docs say the route is skipped when the integration's own handler “ran” although the code skips whenever that handler is configured.
[Automated review]
| ); | ||
| // The package the report is about also receives it at its own issuesUrl, | ||
| // unless that place already has it (spec:AST-031/FR13). | ||
| if (needsTargetRoute(target, entries, deliveries, project)) { |
There was a problem hiding this comment.
With --confirm-public and no working gh, this route's failed delivery makes aggregateStatus return partial, so a run whose handler filed now exits 1; on the base it exits 0, because gh is never called once a handler exists. Under spec:AST-017 FR1 and FR3 that is a breaking exit-behaviour change, which a [feat] patch cannot carry while main declares 0.7.2. Keeping a failed target route out of status and the exit code when handlers ran, with a test, keeps those runs green; otherwise this needs [breaking] in a declared minor.
| status: 'routed_only', | ||
| url: target.issuesUrl, | ||
| message: | ||
| 'No automatic handler is available; open the routed URL to file this report.', |
There was a problem hiding this comment.
When this helper serves the target route, this message appears directly under a handler that filed, so “No automatic handler is available” is no longer true there. A route-specific message (for example, that this is the integration's own tracker) would fix it.
| 'Sends a gap report to every configured handler: the project config handler first, then each integration handler in config order. ' + | ||
| 'Each handler has 30 s to finish, and its output goes to stderr. Public handlers run only with confirmPublic; internal handlers always run. ' + | ||
| "With no handler, it files a GitHub issue for the owning package only with confirmPublic (without it nothing is sent), or returns the package's issues URL when that is not on GitHub. " + | ||
| "With handlers, a report about an integration that sets its own issues URL also goes there the same way, unless that integration's own handler ran or a handler already filed it there. " + |
There was a problem hiding this comment.
needsTargetRoute skips the route when the integration's own handler is in the effective set, not only when it ran: a public own handler without --confirm-public is consent_required, never runs, and the route is still skipped. A routed_only or skipped receipt URL inside the issuesUrl also counts, not only a filed one. “is configured” and “a handler's receipt URL already points inside it” would match the code; the command doc (line 19) and the guide (line 170) use the same wording.
…omatic target route
josephfarina
changed the title
There was a problem hiding this comment.
Changes requested: --to and the destination listing follow architecture:cli-surface INV36 and DEC-9 and the default route is unchanged, but two small fixes are needed first.
- Treat
to: nullas absent and match any other value against the destinations directly, so an empty, blank or over-long--tofails withERR_UNKNOWN_PACKAGEand the destination list, as INV36 requires, instead of “to is required.” (packages/cli/api/gap-report/gap-report.mjs:803-804). - Render the Destinations text from the JSON fields (for example
recordsoverto,defaultandissuesUrl) so it stays a projection of the JSON under INV4 and INV5, since the default entry's prose atpackages/cli/clients/cli/commands/gap-report.mjs:155is not in the JSON anddefaultnever appears.
[Automated review]
| /** @type {GapReportDestination | undefined} */ | ||
| let chosen; | ||
| if (options.to !== undefined) { | ||
| const to = requiredText(options.to, 'to', 214); |
There was a problem hiding this comment.
to: null (the value GapReportDestination.to documents for the default), '' and blank values reach requiredText here and fail with ERR_MISSING_ARGUMENT “to is required.”, and a value over 214 characters fails with ERR_INVALID_ARGUMENT; none lists the destinations. INV36 asks for ERR_UNKNOWN_PACKAGE with the list for a --to that names no destination, and this function already treats package: null and detail: null as absent. Treating options.to == null as absent and matching the trimmed string against the destinations directly would cover both, with a test for to: null and for to: ''.
| list( | ||
| destinations.map(destination => | ||
| destination.default | ||
| ? 'default (leave out --to): the configured report handlers, or the owning package issues URL when there are none' |
There was a problem hiding this comment.
This prose is not in the JSON entry ({to: null, default: true, issuesUrl: null}), and no entry's default field reaches the text, so the repository's text/JSON parity checks (INV4, INV5) fail once a destination exists; the only gap-report parity case runs without one. Rendering the entries with records(destinations, {fields: ['to', 'default', 'issuesUrl']}), or carrying this description in the JSON, keeps the two views in step.
…tinations text mirrors the JSON
There was a problem hiding this comment.
Approval recommended: --to and the destination listing follow architecture:cli-surface INV36 and DEC-9, the default route is unchanged, and the two fixes requested on the previous revision (the to validation and the Destinations text) are in place with tests. No further changes are needed from you.
[Automated review]
Problem
astryx gap-reportalways sends a report to the configured handlers (or, with none, the owning package'sissuesUrl). When a handler is loaded, an integration that names its own tracker inissuesUrlcan never receive a report, and nothing tells the person that other places exist.Specification
architecture:cli-surfaceINV36 and DEC-9 (the person chooses), withspec:AST-031FR13 pointing to them; merged in #7749.Change
--to, the report goes where it goes today.--to <package>(API:options.to) sends the report only to that loaded integration'sissuesUrl; handlers do not run. Same delivery as the zero-handler fallback: a GitHub issues URL is filed withghonly with--confirm-public(consent_requiredotherwise); any other URL comes back asrouted_only.--tothat names no loaded integration with anissuesUrl, including an empty, blank or over-long value, fails withERR_UNKNOWN_PACKAGEbefore anything is sent; the message andsuggestionslist the destinations.to: nullin the API counts as absent, likepackage: null.issuesUrl:--list-categoriesaddsmeta: {destinations, ask};--toaddsdestinationsandask;to,default,issuesUrl) and the line "Ask the person where to file this report, then pass their choice with --to . Never pick a package destination yourself; leave --to out to use the default."{to, default, issuesUrl}; the default is{to: null, default: true, issuesUrl: null}.--tooption, a new example, and a "Let the person choose where a report goes" section in the debug and gap reports guide. Response-type rows updated (README regenerated).issuesUrland its skip rules in this PR; they are gone.Compatibility
Additive. New option; new optional
metaongap-report.categoriesand new optionaldestinations/askongap-report.file, present only when a destination exists. Existing fields, statuses and exit codes are unchanged.--towith--list-categoriesis refused withERR_INVALID_ARGUMENT, like the other filing options.[feat]patch changeset.Evidence
Tests that fail on current main and pass here (8):
issuesUrl;--towith a GitHubissuesUrl:consent_requiredand noghcall without consent; with consent,gh issue create --repo acme/widgets; the shared handler never runs;--towith a non-GitHubissuesUrl:routed_only;--tonaming an unloaded package, or a loaded integration without anissuesUrl:ERR_UNKNOWN_PACKAGElisting the choices, and the handler never runs;--list-categorieslists the destinations inmeta, adds none with only the default, and refuses--to;--to --jsonroutes.After review: an empty, blank or over-long
--togetsERR_UNKNOWN_PACKAGEwith the list, andto: nullis the default route (4 more tests, failing on the previous head); the text Destinations section now mirrors the JSON fields.Testing
pnpm check:repo,readme:check,typecheck:strict,typecheck:json-api,typecheck:authoring,typecheck:template-docs,cli-api-types-verify, eslint on the changed filesvitest run --project node(full node project)