Detect missing gh aw in dashboard and surface install guidance - #42270
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
gh aw in dashboard and surface install guidance
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
|
✅ Test Quality Sentinel completed test quality analysis. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (0 additions detected in src/, lib/, pkg/, internal/, app/, core/, domain/, services/, api/). |
|
✅ PR Code Quality Reviewer completed the code quality review. |
There was a problem hiding this comment.
Pull request overview
This PR improves the agentic-workflows dashboard canvas by proactively detecting whether gh aw is available (dev binary vs installed GitHub CLI extension vs missing) and surfacing clear install/version status in the UI, preventing downstream dashboard calls when the CLI is unavailable.
Changes:
- Add server-side
gh awavailability probing (version) and expose it via a new/api/cli-statusendpoint. - Update the dashboard UI/state to fetch CLI status up front, show install guidance when missing, and show the detected version/source when available.
- Add Vitest coverage for CLI status detection and missing-extension handling.
Show a summary per file
| File | Description |
|---|---|
| .github/extensions/agentic-workflows-dashboard/web/styles.css | Adds styling to keep install-command text from wrapping in the new banner. |
| .github/extensions/agentic-workflows-dashboard/web/index.html | Surfaces CLI status in the header and adds an install guidance banner + quick “gh aw version” command. |
| .github/extensions/agentic-workflows-dashboard/web/app.js | Updates compiled frontend runtime to fetch CLI status before loading data and seed the command panel. |
| .github/extensions/agentic-workflows-dashboard/test/dashboard-cli.test.ts | Adds tests for version detection, env seeding (CI=1), and missing-extension behavior. |
| .github/extensions/agentic-workflows-dashboard/src/models.ts | Aligns frontend models to CLI JSON payloads and introduces CLIStatus/usage item interfaces. |
| .github/extensions/agentic-workflows-dashboard/src/app.ts | Updates TypeScript source for frontend state machine to include CLI probing and gated fetches. |
| .github/extensions/agentic-workflows-dashboard/extension.mjs | Adds /api/cli-status route and wires the new runner with status probing. |
| .github/extensions/agentic-workflows-dashboard/dashboard-cli.mjs | Extends the CLI runner to detect dev binary vs extension, normalize version parsing, and provide install guidance. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Low
| this.commandInput = this.cliStatus?.command ?? "gh aw version"; | ||
| this.commandOutput = this.cliStatus?.message ?? "gh aw is not installed."; | ||
| }, |
| this.definitions = []; | ||
| this.runs = []; | ||
| this.usage = []; | ||
| this.experiments = []; | ||
| this.loadDefinitionPage(1); | ||
| this.loadRunPage(1); | ||
| this.loadUsagePage(1); | ||
| this.loadExperimentPage(1); | ||
| if (!this.commandOutput) { | ||
| this.runCommand(); | ||
| } | ||
| }, | ||
|
|
||
| setActiveTab(tab) { | ||
| if (this.tabs.some(item => item.id === tab)) { | ||
| this.activeTab = tab; | ||
| } | ||
| }, | ||
|
|
||
| isActiveTab(tab) { | ||
| return this.activeTab === tab; | ||
| }, | ||
|
|
||
| tabCount(tab) { | ||
| if (tab.counter === "definitions") { | ||
| return this.definitions.length; | ||
| } | ||
| if (tab.counter === "runs") { | ||
| return this.runs.length; | ||
| } | ||
| if (tab.counter === "experiments") { | ||
| return this.experiments.length; | ||
| } | ||
| return 0; | ||
| }, | ||
|
|
||
| loadDefinitionPage(page) { | ||
| this.definitionPage = page; | ||
| this.definitionsPaged = paginate(this.definitions, page, this.pageSize); | ||
| }, | ||
|
|
||
| loadRunPage(page) { | ||
| this.runPage = page; | ||
| this.runsPaged = paginate(this.runs, page, this.pageSize); | ||
| if (!this.selectedRun && this.runsPaged.items.length > 0) { | ||
| this.selectedRun = this.runsPaged.items[0] ?? null; | ||
| } | ||
| }, | ||
|
|
||
| loadExperimentPage(page) { | ||
| this.experimentPage = page; | ||
| this.experimentsPaged = paginate(this.experiments, page, this.pageSize); | ||
| }, | ||
|
|
||
| selectRun(id) { | ||
| this.selectedRun = this.runs.find(run => run.id === id) ?? null; | ||
| }, | ||
|
|
||
| viewRunDetails(id) { | ||
| this.selectRun(id); | ||
| this.setActiveTab("details"); | ||
| }, | ||
|
|
||
| dispatchSelectedWorkflow() { | ||
| const definition = this.definitions.find(item => item.id === this.selectedDefinitionId); | ||
| if (!definition) { | ||
| this.flashKind = "error"; | ||
| this.flashMessage = "Select a workflow definition before dispatching."; | ||
| return; | ||
| } | ||
|
|
||
| const sequence = this.runs.length + 1; | ||
| const now = new Date().toISOString(); | ||
| const newRun: WorkflowRun = { | ||
| id: `run-${String(sequence).padStart(5, "0")}`, | ||
| definitionId: definition.id, | ||
| status: "queued", | ||
| createdAt: now, | ||
| updatedAt: now, | ||
| steps: [1, 2, 3, 4].map(step => buildStep(sequence, step, "pending")), | ||
| }; | ||
|
|
||
| this.runs = [newRun, ...this.runs]; | ||
| this.loadRunPage(1); | ||
| this.viewRunDetails(newRun.id); | ||
|
|
||
| this.flashKind = "success"; | ||
| this.flashMessage = `Dispatched ${definition.name} as ${newRun.id}.`; | ||
| }, | ||
|
|
||
| runCommand() { | ||
| const result = runGhCommand(this.commandInput, this.runs); | ||
| this.commandOutput = `$ ${result.command}\n${result.output}`; | ||
| }, | ||
|
|
||
| commandQuickFill(value) { | ||
| this.commandInput = value; | ||
| this.runCommand(); | ||
| }, | ||
|
|
||
| auditDiffQuickFill() { | ||
| const selectedId = this.selectedRun?.id; | ||
| if (!selectedId) { | ||
| return "gh aw audit-diff run-00001 run-00002"; | ||
| } | ||
|
|
||
| const firstRunId = this.runs[0]?.id ?? "run-00001"; | ||
| const secondRunId = this.runs[1]?.id ?? "run-00002"; | ||
| const compareId = selectedId === firstRunId ? secondRunId : firstRunId; | ||
| return `gh aw audit-diff ${selectedId} ${compareId}`; | ||
| }, | ||
|
|
||
| renderMarkdown(markdown) { | ||
| return renderSafeMarkdown(markdown); | ||
| }, | ||
|
|
||
| formatDate, | ||
| runStatusClass: statusClass, | ||
| stepStatusClass, | ||
| })); | ||
| }); | ||
| this.commandInput = this.cliStatus?.command ?? "gh aw version"; | ||
| this.commandOutput = this.cliStatus?.message ?? "gh aw is not installed."; | ||
| } |
🧪 Test Quality Sentinel Report✅ Test Quality Score: 85/100 — Excellent
📊 Metrics & Test Classification (2 tests analyzed)
Go: 0 ( Test inflation check: Test file adds 51 lines; Verdict
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Detect missing gh aw in dashboard
Good overall approach — the PR correctly gates all data loading behind a CLI availability check, and createGhAwRunnerWithStatus is a clean extension of the existing runner pattern. The install-guidance UX is clear and the tests confirm the two key detection paths.
4 inline suggestions (3 minor / 1 non-blocking):
| # | File | Severity | Topic |
|---|---|---|---|
| 1 | dashboard-cli.mjs:40 |
isMissingGhAwExtension may miss backtick error format |
|
| 2 | test/dashboard-cli.test.ts:11 |
🔵 Minor | Version detection test uses stderr, not stdout |
| 3 | web/index.html:22-23 |
🔵 Minor | Stale version label + loading indicator show simultaneously on refresh |
| 4 | dashboard-cli.mjs:73 |
🔵 Minor | findDevBinary called N+1 times per dashboard load |
What looks good
- Fails closed correctly —
init()skips all data fetches whencliStatus?.availableis false; noisy downstream errors are suppressed. - Install banner —
x-show="cliStatus && !cliStatus.available"guards correctly against thenullloading state. - CI=1 injection — Suppressing interactive prompts is the right call for a headless canvas context.
- Dependency injection (
accessFn,execFileFn) — makes the unit tests clean and avoids filesystem/process coupling. - Alpine.start() migration — moving
Alpine.data+Alpine.start()to the module level matches the recommended Alpine 3.x ESM pattern.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 74.6 AIC · ⌖ 7.22 AIC · ⊞ 4.9K
|
|
||
| function isMissingGhAwExtension(error) { | ||
| const output = String(error?.output ?? error?.stderr ?? error?.message ?? ""); | ||
| return /extension not found:\s*aw/i.test(output) || /unknown command ["']aw["'] for ["']gh["']/i.test(output); |
There was a problem hiding this comment.
Backtick-quoted errors may slip past isMissingGhAwExtension.
The gh CLI can use backtick notation in some versions:
unknown command `aw` for `gh`
The current regex only matches " and ', so this variant falls through to the generic source: "error" path. The dashboard still shows available: false and the install banner, but the message shows raw error text instead of the friendly guidance.
Consider adding backtick alternatives:
return /extension not found:\s*aw/i.test(output)
|| /unknown command ["'\`]aw["'\`] for ["'\`]gh["'\`]/i.test(output);@copilot please address this.
| expect(bin).toBe("gh"); | ||
| expect(args).toEqual(["aw", "version"]); | ||
| expect(options.env.CI).toBe("1"); | ||
| callback(null, "", "gh aw version v1.2.3\n"); |
There was a problem hiding this comment.
Test uses stderr for version output, but gh aw version writes to stdout.
The mock passes the version string as the third argument (stderr):
callback(null, "", "gh aw version v1.2.3\n");
// ^^ ^^^^^^^^^^^^^^^^^^^^^
// stdout stderrSince combineIO: true merges both streams, this works today. But it means the primary code path (version on stdout) is untested. If a future refactor changes how stdout is handled, this test would still pass while the production path breaks.
Consider adding a second assertion or a separate test that puts the version string in stdout (second argument) instead, matching the real gh aw version behavior.
@copilot please address this.
| <div class="d-flex gap-2 flex-items-center"> | ||
| <span class="Label Label--accent">Copilot Canvas</span> | ||
| <span class="Label Label--success" x-show="cliStatus && cliStatus.available">gh aw <span x-text="cliStatus?.version"></span> · <span x-text="cliSourceLabel(cliStatus)"></span></span> | ||
| <span class="Label Label--secondary" x-show="loadingCliStatus">Checking gh aw…</span> |
There was a problem hiding this comment.
Version label and loading indicator both visible during refresh().
On refresh, fetchCliStatus() sets loadingCliStatus = true but doesn't clear cliStatus. This means both:
- the green version label (
x-show="cliStatus && cliStatus.available") — still shows the previous value - the grey loading indicator (
x-show="loadingCliStatus") — now true
...render simultaneously in the header until the new status resolves.
Simple fix — clear cliStatus at the start of fetchCliStatus():
async fetchCliStatus() {
this.loadingCliStatus = true;
this.cliStatus = null; // hide stale version label while re-probing
...
}@copilot please address this.
| const runGhAw = createGhAwRunner(options); | ||
| const getStatus = async () => { | ||
| const cwd = options.getWorkspacePath(); | ||
| const devBin = await findDevBinary(cwd, options.accessFn ?? access, options.platform ?? process.platform); |
There was a problem hiding this comment.
findDevBinary is called independently in getStatus() and inside every runGhAw() call.
createGhAwRunnerWithStatus calls createGhAwRunner(options) (which will call findDevBinary on each runGhAw invocation) and calls findDevBinary again inside getStatus(). So the filesystem stat is repeated N+1 times per dashboard load.
Since the workspace path doesn't change between getStatus() and command execution, the devBin result can be cached:
export function createGhAwRunnerWithStatus(options) {
let cachedDevBin; // resolved once, reused
const resolveDevBin = async () => {
if (cachedDevBin === undefined) {
cachedDevBin = await findDevBinary(...) ?? null;
}
return cachedDevBin;
};
// pass resolveDevBin into both getStatus and runGhAw
}Not blocking, but reduces redundant I/O on every command dispatch.
Skills-Based Review Summary 🧠Applied 📋 7 inline comments — overview
Blocking: items 1 (missing dev-binary test) and 3 ( @copilot please address the review comments above.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /tdd, /zoom-out, and /grill-with-docs — requesting changes primarily on test coverage gaps and two architectural points.
📋 Key Themes & Highlights
Key Themes
- Test coverage gaps: The
dev-binarypath (primary path for local development) and the genericerrorpath each have zero tests. The two parsing/detection helpers (parseVersionFromOutput,isMissingGhAwExtension) also lack isolated unit tests. findDevBinarycalled twice:getStatus()and the inner runner both callfindDevBinaryindependently on each operation, causing redundant filesystem access.- Weak typing on
CLIStatus.source:stringloses the discriminated union the rest of the code already assumes. refresh()drops the HTTP response: A non-OK response from/api/refreshis silently ignored.getStatusattached on a function value: Mixes callable and object semantics; a plain return object would be cleaner.
Positive Highlights
- ✅ Clean dependency-injection pattern for
accessFn,execFileFn,platform,env— makes the existing two tests very easy to write and read. - ✅
CI=1addition is the right call for suppressing interactive prompts in all CLI invocations. - ✅ Fail-closed design in
init()— skipping data loads when CLI is unavailable prevents cascading noisy errors. - ✅ Install banner in the HTML is concise and actionable — shows both the message and the exact install command.
- ✅
combineOutputhelper cleanly unifies stdout/stderr for display purposes.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 91.6 AIC · ⌖ 8.26 AIC · ⊞ 6.6K
Comment /matt to run again
| installCommand: "gh extension install github/gh-aw", | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
[/tdd] The test suite covers gh-extension and missing paths but omits two other branches of getStatus: the dev-binary path and the generic error path (source: "error"). Both ship in this PR and should have explicit tests.
💡 Suggested additions
it("detects gh aw version from a local dev binary", async () => {
const execFileFn = vi.fn((bin, args, options, callback) => {
expect(bin).toMatch(/gh-aw$/);
expect(args).toEqual(["version"]);
expect(options.env.CI).toBe("1");
callback(null, "", "gh-aw version v0.9.0\n");
});
const runGhAw = createGhAwRunnerWithStatus({
getWorkspacePath: () => "/workspace",
accessFn: vi.fn(async () => undefined), // binary exists
execFileFn,
platform: "linux",
});
await expect(runGhAw.getStatus()).resolves.toMatchObject({
available: true,
source: "dev-binary",
});
});
it("returns source:error for unexpected gh failures", async () => {
const execFileFn = vi.fn((bin, args, options, callback) => {
callback(Object.assign(new Error("connection refused"), { code: 1 }), "", "connection refused");
});
const runGhAw = createGhAwRunnerWithStatus({
getWorkspacePath: () => "/workspace",
accessFn: vi.fn(async () => { throw new Error("missing"); }),
execFileFn,
});
await expect(runGhAw.getStatus()).resolves.toMatchObject({
available: false,
source: "error",
});
});The dev-binary path is the primary path for local development. Without a test, a regression in findDevBinary detection would go unnoticed.
@copilot please address this.
| const runGhAw = createGhAwRunner(options); | ||
| const getStatus = async () => { | ||
| const cwd = options.getWorkspacePath(); | ||
| const devBin = await findDevBinary(cwd, options.accessFn ?? access, options.platform ?? process.platform); |
There was a problem hiding this comment.
[/zoom-out] findDevBinary is called here inside getStatus, and also called again on every runGhAw(args) invocation (line 60). Each init() or refresh() therefore makes two filesystem access calls — one for status and one for the actual command. Cache the resolved binary path (or share it) to avoid redundant I/O.
💡 Suggested refactor
Consider caching the result of findDevBinary at runner creation time, or computing it once in getStatus and reusing it:
export function createGhAwRunnerWithStatus(options) {
let resolvedBin = undefined; // undefined = not yet resolved
async function getDevBin() {
if (resolvedBin === undefined) {
const cwd = options.getWorkspacePath();
resolvedBin = await findDevBinary(cwd, options.accessFn ?? access, options.platform ?? process.platform);
}
return resolvedBin;
}
const getStatus = async () => { /* use getDevBin() */ };
const runGhAw = async (args) => { /* use getDevBin() */ };
// ...
}This also removes the duplicated options.accessFn ?? access / options.platform ?? process.platform fallbacks that already exist in createGhAwRunner's parameter defaults.
@copilot please address this.
|
|
||
| export interface CLIStatus { | ||
| available: boolean; | ||
| source: string; |
There was a problem hiding this comment.
[/grill-with-docs] source is typed as plain string, losing the discriminated union that the rest of the code relies on. Every call-site that switches on source (e.g. cliSourceLabel) would benefit from compiler-enforced exhaustiveness.
💡 Suggested fix
export interface CLIStatus {
available: boolean;
source: "dev-binary" | "gh-extension" | "missing" | "error";
version: string;
command: string;
installCommand: string;
message?: string;
}This lets TypeScript flag any switch/if chain over source that misses a case, and makes the intent self-documenting.
@copilot please address this.
| }, | ||
|
|
||
| async refresh() { | ||
| await fetch("/api/refresh"); |
There was a problem hiding this comment.
[/zoom-out] refresh() fires fetch("/api/refresh") and discards the response entirely. If the server returns 4xx/5xx, the UI silently proceeds to re-fetch all data, giving the user no signal that the refresh trigger itself failed.
💡 Suggested fix
async refresh() {
const res = await fetch("/api/refresh");
if (!res.ok) {
this.flashMessage = `Refresh failed: ${res.status}`;
this.flashKind = "error";
return;
}
// ... rest of refresh
}At minimum, set flashKind = "error" so the user knows the operation did not succeed.
@copilot please address this.
| } | ||
| }; | ||
|
|
||
| runGhAw.getStatus = getStatus; |
There was a problem hiding this comment.
[/zoom-out] Attaching getStatus as a property of the returned function (runGhAw.getStatus = getStatus) mixes callable and object semantics on one value. As the API grows, consumers need to know that runGhAw is both a runner function and an object with methods.
💡 Suggested alternative
Return a plain object from createGhAwRunnerWithStatus to make the contract explicit and easier to type:
export function createGhAwRunnerWithStatus(options) {
// ...
return {
run: runGhAw, // replaces runGhAw(args) calls
getStatus,
};
}In extension.mjs:
const { run: runGhAw, getStatus } = createGhAwRunnerWithStatus({ ... });
// pass runGhAw to dataAccess, use getStatus for /api/cli-statusThis makes the shape explicit for both JS and TypeScript consumers.
@copilot please address this.
| return match?.[1]?.trim() ?? ""; | ||
| } | ||
|
|
||
| function isMissingGhAwExtension(error) { |
There was a problem hiding this comment.
[/tdd] isMissingGhAwExtension is the key discriminator for the feature's main user-facing branch (source: "missing" vs source: "error"), but it has no isolated unit tests. The two regex patterns rely on gh CLI error message text that could differ across platforms or gh versions.
💡 Suggested tests
import { isMissingGhAwExtension } from "../dashboard-cli.mjs";
// Note: export the function or test via the runner's observable behaviour
// Via getStatus behaviour (already covered by existing tests), or export for direct testing:
describe("isMissingGhAwExtension", () => {
it("matches 'extension not found: aw'", () => {
expect(isMissingGhAwExtension({ output: "extension not found: aw" })).toBe(true);
});
it("matches \"unknown command 'aw' for 'gh'\"", () => {
expect(isMissingGhAwExtension({ output: "unknown command 'aw' for 'gh'" })).toBe(true);
});
it("does not match unrelated errors", () => {
expect(isMissingGhAwExtension({ output: "connection refused" })).toBe(false);
});
});Considering exporting this function so it can be tested directly, even if it remains an implementation detail.
@copilot please address this.
| } | ||
|
|
||
| export function createGhAwRunner({ getWorkspacePath }) { | ||
| function parseVersionFromOutput(output) { |
There was a problem hiding this comment.
[/tdd] parseVersionFromOutput has no isolated unit tests. The regex (/gh(?:-aw| aw) version ([^\r\n]+)/i) needs to handle several edge cases — output from stdout only, output from stderr only, combined output, and empty strings — all of which can silently return "" or "unknown" today.
💡 Suggested tests
import { parseVersionFromOutput } from "../dashboard-cli.mjs";
// export for testing, or test indirectly via getStatus
describe("parseVersionFromOutput", () => {
it("extracts version from 'gh aw version vX.Y.Z'", () => {
expect(parseVersionFromOutput("gh aw version v1.2.3")).toBe("v1.2.3");
});
it("extracts version from 'gh-aw version vX.Y.Z'", () => {
expect(parseVersionFromOutput("gh-aw version v0.9.0")).toBe("v0.9.0");
});
it("returns empty string for empty input", () => {
expect(parseVersionFromOutput("")).toBe("");
});
it("returns empty string for null input", () => {
expect(parseVersionFromOutput(null)).toBe("");
});
it("returns empty string when pattern does not match", () => {
expect(parseVersionFromOutput("something else")).toBe("");
});
});@copilot please address this.
|
@copilot run pr-finisher skill |
There was a problem hiding this comment.
REQUEST_CHANGES — one crash-inducing bug in the new status endpoint, one misleading error message, and two testing gaps that hide those problems.
### Blocking issues
getStatus() dev-binary path has no error handling (comment on dashboard-cli.mjs:75): when findDevBinary returns a path but the binary fails to execute, the rejection propagates uncaught all the way through the /api/cli-status handler. The gh-extension path wraps its exec in try/catch; the dev-binary path — which is the less tested, higher-risk one — does not. This can crash the endpoint in any local dev checkout where the binary is mid-build or compiled for the wrong arch.
**Misleading not
🔎 Code quality review by PR Code Quality Reviewer · 136.7 AIC · ⌖ 9.86 AIC · ⊞ 5.2K
Comment /review to run again
| const cwd = options.getWorkspacePath(); | ||
| const devBin = await findDevBinary(cwd, options.accessFn ?? access, options.platform ?? process.platform); | ||
|
|
||
| if (devBin) { |
There was a problem hiding this comment.
Unhandled rejection if dev binary exists but fails to run: getStatus() wraps the gh extension path in try/catch but leaves the dev-binary execp call completely unprotected — any failure propagates uncaught through the /api/cli-status endpoint.
💡 Suggested fix
Wrap the dev-binary branch the same way as the gh-extension branch:
if (devBin) {
try {
const output = await execp(devBin, ['version'], cwd, { ... });
return { available: true, source: 'dev-binary', ... };
} catch (error) {
return {
available: false,
source: 'error',
version: '',
command: `${devBin} version`,
installCommand: INSTALL_COMMAND,
message: String(error?.output ?? error?.stderr ?? error?.message ?? 'Failed to run dev binary.'),
};
}
}Concrete failure scenarios that crash the endpoint today: binary compiled for wrong arch, X_OK check passes but dynamic linker rejects it at run time, binary is mid-compilation when checked. The gh-extension path handles its own errors gracefully; the dev-binary path — the riskier one — does not.
| output: "Need two valid runs for diff. Example: gh aw audit-diff run-00002 run-00003", | ||
| }; | ||
| this.commandInput = this.cliStatus?.command ?? "gh aw version"; | ||
| this.commandOutput = this.cliStatus?.message ?? "gh aw is not installed."; |
There was a problem hiding this comment.
"gh aw is not installed." displayed even when the status endpoint itself failed: when fetchCliStatus catches a network or server error it sets cliStatus = null. Both init() and refresh() then fall into the same branch as a genuine missing-extension case, printing the hardcoded fallback — actively lying to the user about the cause.
💡 Suggested fix
Distinguish null (fetch error) from { available: false } (CLI confirmed missing):
// init():
if (this.cliStatus === null) {
// errorCliStatus is already set and shown in the banner — don't clobber commandOutput
this.commandInput = 'gh aw version';
this.commandOutput = 'Could not reach the status endpoint. Check the server.';
} else {
// cliStatus is present but available === false
this.commandInput = this.cliStatus.command;
this.commandOutput = this.cliStatus.message ?? 'gh aw is not installed.';
}The same split is needed in refresh() (line 357). The errorCliStatus flag is already wired to an error banner in the HTML; the commandOutput message should not contradict it.
| installCommand: "gh extension install github/gh-aw", | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The only untested code path is the one without error handling: both existing tests force accessFn to throw so findDevBinary always returns null — the dev-binary execp branch (which currently lacks a try/catch) is never exercised.
💡 Suggested tests to add
it('detects version from local dev binary', async () => {
const execFileFn = vi.fn((bin, args, options, cb) => {
expect(bin).toMatch(/gh-aw$/);
expect(args).toEqual(['version']);
cb(null, 'gh-aw version v0.9.0\n', '');
});
const runGhAw = createGhAwRunnerWithStatus({
getWorkspacePath: () => '/workspace',
accessFn: vi.fn(async () => {}), // resolves — binary found
execFileFn,
platform: 'linux',
});
await expect(runGhAw.getStatus()).resolves.toMatchObject({
available: true,
source: 'dev-binary',
version: 'v0.9.0',
});
});
it('returns source:error when dev binary execution fails', async () => {
const execFileFn = vi.fn((bin, args, options, cb) => {
cb(Object.assign(new Error('exec failed'), { code: 1 }), '', 'Segmentation fault');
});
const runGhAw = createGhAwRunnerWithStatus({
getWorkspacePath: () => '/workspace',
accessFn: vi.fn(async () => {}), // resolves — binary found
execFileFn,
platform: 'linux',
});
// After the missing try/catch is fixed, this should resolve rather than reject
await expect(runGhAw.getStatus()).resolves.toMatchObject({
available: false,
source: 'error',
});
});The second test will currently fail (the promise rejects rather than resolves), which confirms the critical bug at line 75 of dashboard-cli.mjs. Add the try/catch fix first, then these tests will pass.
Also worth adding: a test for the generic-error path via the gh extension (non-missing error like permission denied) to pin the message extraction logic.
|
|
||
| function isMissingGhAwExtension(error) { | ||
| const output = String(error?.output ?? error?.stderr ?? error?.message ?? ""); | ||
| return /extension not found:\s*aw/i.test(output) || /unknown command ["']aw["'] for ["']gh["']/i.test(output); |
There was a problem hiding this comment.
Extension-missing detection is tied to undocumented gh CLI error strings that have changed between releases: both regexes match specific prose that can change in any gh update, silently downgrading a 'not installed' result to 'source:error' and showing a raw error string instead of the install banner.
💡 Suggested fix
Gate on exit code first — it is more stable than message text:
function isMissingGhAwExtension(error) {
// exit code 1 means 'unknown command' / missing extension;
// exit code 127 means gh itself not found — don't confuse the two
if (error?.code !== 1) return false;
const output = String(error?.output ?? error?.stderr ?? error?.message ?? '');
return /extension not found:\s*aw/i.test(output) || /unknown command .aw. for .gh./i.test(output);
}Also add a comment to the test pinning the exact error string to the gh version it was sourced from, so future maintainers know when to update it.
| if (err) reject(Object.assign(err, { stderr: stderr ?? "" })); | ||
| else resolve(stdout); | ||
| const output = combineOutput(stdout ?? "", stderr ?? ""); | ||
| if (err) reject(Object.assign(err, { stderr: stderr ?? "", stdout: stdout ?? "", output })); |
There was a problem hiding this comment.
Object.assign mutates the error object that was handed to the callback: if the caller's execFileFn reuses or logs the error before calling the callback (e.g., a spy that stores it, or Node internals that annotate it), the added fields stdout, stderr, and output silently corrupt the original.
💡 Suggested fix
Avoid mutating arguments you did not create:
// Instead of Object.assign(err, {...})
const wrapped = Object.assign(new Error(err.message), { cause: err }, { stderr: stderr ?? '', stdout: stdout ?? '', output });
reject(wrapped);Or simply spread into a plain object:
reject(Object.assign(Object.create(Object.getPrototypeOf(err)), err, { stderr: stderr ?? '', stdout: stdout ?? '', output }));The current tests happen to be safe because each test constructs a fresh Error per callback invocation — the mutation goes unnoticed.
|
@copilot run pr-finisher skill |
…e run data on refresh Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…te logic Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. Both review issues have been addressed in the latest commits:
|
PR is in a merge-ready state. All review threads were addressed in the previous pass (commits
CI must be re-triggered by a maintainer (close/reopen or |
|
🎉 This pull request is included in a new release. Release: |
The dashboard currently assumes
gh awis available, which leaves the canvas in a broken state when the extension is not installed. This change adds explicit CLI availability detection, shows install guidance when missing, and displays the detected version when available.CLI detection
gh awGitHub CLI extension.versionbefore loading workflow data.gh awextensionDashboard UX
gh awis not installed.gh awversion and source in the header when available.Command execution
CI=1to suppress interactive prompts.Code shape
/api/cli-statusendpoint and corresponding frontend state/model updates.Example of the new detection flow: