chore: sync public mirror from internal#810
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
PR SummaryMedium Risk Overview It adds a turnkey Maestro code-review CI installer— Review hygiene gains Smaller runtime tweaks: expanded bundled system prompt engineering discipline, bounded MCP/LSP teardown after non-interactive runs, and trimmed whitespace on telemetry correlation IDs from metadata. Reviewed by Cursor Bugbot for commit 02e5b88. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment has been minimized.
This comment has been minimized.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
📝 Info: Two parallel review-workflow implementations are kept in sync by parity test
The PR adds two nearly identical implementations of buildMaestroReviewWorkflow: one in packages/github-agent/src/workflows/maestro-review-workflow.ts (using process.env.MAESTRO_PACKAGE_NAME and writeFileSync) and one in src/installers/maestro-review-workflow.ts (using getPackageNameOverride() and writeTextFileAtomic). The test at test/workflows/maestro-review-workflow.test.ts:89-102 asserts output parity for shared options. However, getPackageNameOverride() at src/package-metadata.ts:57-61 is functionally identical to reading process.env.MAESTRO_PACKAGE_NAME, so the parity holds. The writeTextFileAtomic default createDirs: true ensures directory creation equivalent to the github-agent's explicit mkdirSync call. No divergence risk currently, but if either copy is modified without updating the other, the parity test will catch it.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (severity === "none") { | ||
| return minSeverity === "none" && hasActionableReviewFeedback(thread); | ||
| } |
There was a problem hiding this comment.
📝 Info: Dead code branch in threadBlocksFeedbackAudit can never execute
At scripts/pr-feedback-audit.mjs:364, the condition minSeverity === "none" is unreachable because line 359 already returns early when minSeverity === "none". This makes the hasActionableReviewFeedback function (defined at line 350) dead code — it is only referenced at line 364 and can never execute. The function works correctly regardless, but a future developer modifying the early return at line 359 might incorrectly assume line 364 handles a different subset of the "none" case.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const allowsMocks = MOCKS_ALLOWED_MARKER.test(text); | ||
| MOCKS_ALLOWED_MARKER.lastIndex = 0; | ||
| const explicitlyStrict = MOCKS_STRICT_MARKER.test(text); | ||
| MOCKS_STRICT_MARKER.lastIndex = 0; |
There was a problem hiding this comment.
📝 Info: Global regex flags on module-level constants are safe here but fragile
The MOCKS_ALLOWED_MARKER and MOCKS_STRICT_MARKER constants at src/agent/swarm/plan-parser.ts:25-28 use the g (global) flag, making them stateful via lastIndex. The code correctly resets lastIndex = 0 after each .test() call, and String.prototype.replace with a global regex sets lastIndex = 0 at the start of its execution (per spec), so the final state after each extractTaskMetadata call leaves both regexes at lastIndex = 0. However, this pattern is fragile — any future async usage or forgetting a reset would introduce intermittent matching failures. A safer alternative would be to use non-global regexes for .test() and global only inside .replace(), or create regex instances locally.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (!coverage.ok) { | ||
| const error = coverage.message ?? "Swarm coverage gate failed"; | ||
| this.state.status = "failed"; | ||
| this.state.error = error; | ||
| this.state.completedAt = Date.now(); | ||
| this.emit({ | ||
| type: "swarm_fail", | ||
| swarmId: this.state.id, | ||
| error, | ||
| }); | ||
| this.emit({ | ||
| type: "swarm_complete", | ||
| swarmId: this.state.id, | ||
| state: cloneState(this.state), | ||
| }); | ||
| return cloneState(this.state); |
There was a problem hiding this comment.
🚩 Coverage gate failure skips swarm_start event — may affect event consumers
When the coverage gate fails in src/agent/swarm/executor.ts:624-639, the executor emits swarm_fail and swarm_complete but never emits swarm_start. This is intentional (no work was started), but any event consumer that expects every swarm_complete to be preceded by a swarm_start would break. The test at test/agent/swarm-executor-coverage-gate.test.ts:44-46 validates this event sequence. Worth checking if publishSwarmRuntimeEvent or other downstream listeners make ordering assumptions.
Was this helpful? React with 👍 or 👎 to provide feedback.
| minSeverity: "none", | ||
| prs: [], |
There was a problem hiding this comment.
📝 Info: Default minSeverity 'none' in parseFeedbackAuditArgs subtly changes behavior for direct callers
The parseFeedbackAuditArgs function at scripts/pr-feedback-audit.mjs:24 defaults minSeverity to "none". Previously, the script blocked on ALL unresolved threads. Now with --min-severity none, it blocks on threads with non-informational comments only (skipping trusted bot summaries via informationalReviewFeedback). The package.json script review:unresolved-threads explicitly passes --min-severity none, so it gets the new behavior intentionally. Any external caller of parseFeedbackAuditArgs without --min-severity would also get this new default, which silently skips trusted-bot summary threads that previously would have counted.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if ( | ||
| !isInteractive || | ||
| mode === "rpc" || | ||
| mode === "headless" || | ||
| parsed.headless | ||
| ) { | ||
| await cleanupNonInteractiveRuntimeResources(); | ||
| } |
There was a problem hiding this comment.
🚩 Non-interactive cleanup does not cover agents-md mode
The cleanup condition at src/main.ts:2209-2214 uses !isInteractive || mode === "rpc" || mode === "headless" || parsed.headless. When --agents-init is used without additional messages, isInteractive is true (because parsed.messages.length === 0) and none of the other conditions fire, so MCP/LSP cleanup is skipped. Since agents-md mode runs runSingleShotMode which may have initialized MCP servers, the process could hang briefly on exit waiting for connection timeouts. This is not a regression (no such cleanup existed before this PR), but it's a gap in the new cleanup logic that could cause delayed process exit.
Was this helpful? React with 👍 or 👎 to provide feedback.
This comment has been minimized.
This comment has been minimized.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Bugbot Autofix prepared fixes for both issues found in the latest run.
Preview (4c7c524167)diff --git a/.husky/pre-commit b/.husky/pre-commit
--- a/.husky/pre-commit
+++ b/.husky/pre-commit
@@ -12,5 +12,17 @@
# Run Composer Guardian (secrets + CI hygiene) before heavier tasks.
bash "$ROOT/scripts/guardian.sh" --trigger pre-commit
-bun run bun:lint && bun run build && bun run bun:compile
+STAGED_FORMAT_FILES=()
+while IFS= read -r staged_file; do
+ STAGED_FORMAT_FILES+=("$staged_file")
+done < <(
+ git diff --cached --name-only --diff-filter=ACMR |
+ grep -E "\\.(ts|tsx|js|jsx|mjs|cjs|json|jsonc|css|md|yml|yaml)$" || true
+)
+
+if [ "${#STAGED_FORMAT_FILES[@]}" -gt 0 ]; then
+ bunx biome check "${STAGED_FORMAT_FILES[@]}"
+else
+ echo "No staged Biome-supported files to check."
+fi
'
diff --git a/package.json b/package.json
--- a/package.json
+++ b/package.json
@@ -119,7 +119,7 @@
"ci:plan": "node scripts/plan-ci-checks.mjs",
"pr:ready": "node scripts/pr-ready-to-merge.mjs",
"pr:feedback": "node scripts/pr-feedback-audit.mjs",
- "review:unresolved-threads": "node scripts/pr-feedback-audit.mjs --recent-days 3 --check",
+ "review:unresolved-threads": "node scripts/pr-feedback-audit.mjs --recent-days 3 --check --min-severity none",
"review:feedback-dashboard": "node scripts/pr-feedback-dashboard.mjs --repo evalops/maestro-internal --recent-days 3 --limit 50",
"verify:runtime-deps": "node scripts/check-runtime-deps.js && node scripts/check-docker-runtime-workspaces.mjs && node scripts/check-packed-bundled-workspaces.mjs",
"tui": "npm run cli",
diff --git a/packages/github-agent/src/index.ts b/packages/github-agent/src/index.ts
--- a/packages/github-agent/src/index.ts
+++ b/packages/github-agent/src/index.ts
@@ -34,3 +34,9 @@
export { GitHubApiClient } from "./github/client.js";
export { GitHubReporter, type TaskProgress } from "./github/reporter.js";
export { GitHubWebhookServer } from "./webhooks/server.js";
+export {
+ MAESTRO_REVIEW_WORKFLOW_PATH,
+ buildMaestroReviewWorkflow,
+ writeMaestroReviewWorkflow,
+ type MaestroReviewWorkflowOptions,
+} from "./workflows/maestro-review-workflow.js";
diff --git a/packages/github-agent/src/workflows/maestro-review-workflow.test.ts b/packages/github-agent/src/workflows/maestro-review-workflow.test.ts
new file mode 100644
--- /dev/null
+++ b/packages/github-agent/src/workflows/maestro-review-workflow.test.ts
@@ -1,0 +1,160 @@
+import { mkdtempSync, readFileSync, rmSync } from "node:fs";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
+import { afterEach, describe, expect, it } from "vitest";
+import {
+ MAESTRO_REVIEW_WORKFLOW_PATH,
+ buildMaestroReviewWorkflow,
+ writeMaestroReviewWorkflow,
+} from "../index.js";
+
+const tempDirs: string[] = [];
+
+afterEach(() => {
+ for (const dir of tempDirs.splice(0)) {
+ rmSync(dir, { force: true, recursive: true });
+ }
+});
+
+describe("maestro review workflow generator", () => {
+ it("emits a pull_request workflow that runs maestro exec and comments", () => {
+ const yaml = buildMaestroReviewWorkflow();
+ expect(yaml).toContain("name: Maestro Code Review");
+ expect(yaml).toContain("on:\n pull_request:");
+ expect(yaml).toContain("pull-requests: write");
+ expect(yaml).toContain(
+ 'export MAESTRO_MERGE_BASE_SHA="$(git merge-base "${MAESTRO_BASE_SHA}" "${MAESTRO_HEAD_SHA}")"',
+ );
+ expect(yaml).toContain(
+ "maestro exec --provider 'anthropic' --output-last-message review.md",
+ );
+ expect(yaml).toContain(
+ "from merge base ${MAESTRO_MERGE_BASE_SHA} to ${MAESTRO_HEAD_SHA}",
+ );
+ expect(yaml).toContain("gh pr comment");
+ expect(yaml).toContain('node-version: "20"');
+ expect(yaml).toContain("npm install -g 'maestro@latest'");
+ expect(yaml).toContain("GITHUB_PERSONAL_ACCESS_TOKEN: ${{ github.token }}");
+ expect(yaml).toContain(
+ [
+ 'gh pr comment "${MAESTRO_PR_NUMBER}" --edit-last --body-file review.md || \\',
+ ' gh pr comment "${MAESTRO_PR_NUMBER}" --body-file review.md',
+ ].join("\n"),
+ );
+ });
+
+ it("reads the default package override at build time", () => {
+ const previous = process.env.MAESTRO_PACKAGE_NAME;
+ try {
+ process.env.MAESTRO_PACKAGE_NAME = "@example/from-env";
+ expect(buildMaestroReviewWorkflow()).toContain(
+ "npm install -g '@example/from-env@latest'",
+ );
+ } finally {
+ if (previous === undefined) {
+ delete process.env.MAESTRO_PACKAGE_NAME;
+ } else {
+ process.env.MAESTRO_PACKAGE_NAME = previous;
+ }
+ }
+ });
+
+ it("is deterministic for the same options", () => {
+ expect(buildMaestroReviewWorkflow({ model: "claude-opus-4-8" })).toBe(
+ buildMaestroReviewWorkflow({ model: "claude-opus-4-8" }),
+ );
+ });
+
+ it("threads model, version, node, and api-key-secret options", () => {
+ const yaml = buildMaestroReviewWorkflow({
+ provider: "anthropic",
+ model: "claude-opus-4-8",
+ maestroPackage: "@example/maestro",
+ maestroVersion: "1.2.3",
+ nodeVersion: "22",
+ apiKeySecretName: "MODEL_API_KEY",
+ });
+ expect(yaml).toContain(
+ "maestro exec --provider 'anthropic' --model 'claude-opus-4-8'",
+ );
+ expect(yaml).toContain("npm install -g '@example/maestro@1.2.3'");
+ expect(yaml).toContain('node-version: "22"');
+ expect(yaml).toContain("ANTHROPIC_API_KEY: ${{ secrets.MODEL_API_KEY }}");
+ });
+
+ it("maps provider secret names to Maestro runtime env names", () => {
+ expect(buildMaestroReviewWorkflow({ provider: "openai" })).toContain(
+ "OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }}",
+ );
+ expect(
+ buildMaestroReviewWorkflow({
+ provider: "openai",
+ apiKeySecretName: "maestro_OpenAI_review_key",
+ }),
+ ).toContain("OPENAI_API_KEY: ${{ secrets.maestro_OpenAI_review_key }}");
+ expect(
+ buildMaestroReviewWorkflow({
+ provider: "custom-provider",
+ apiKeyEnvName: "CUSTOM_PROVIDER_API_KEY",
+ apiKeySecretName: "CUSTOM_PROVIDER_REVIEW_SECRET",
+ }),
+ ).toContain(
+ "CUSTOM_PROVIDER_API_KEY: ${{ secrets.CUSTOM_PROVIDER_REVIEW_SECRET }}",
+ );
+ });
+
+ it("omits the model flag when no model is given", () => {
+ expect(buildMaestroReviewWorkflow()).toContain(
+ "maestro exec --provider 'anthropic' --output-last-message",
+ );
+ expect(buildMaestroReviewWorkflow()).not.toContain("--model");
+ });
+
+ it("infers a provider from the configured API key env var", () => {
+ expect(
+ buildMaestroReviewWorkflow({ apiKeySecretName: "OPENAI_API_KEY" }),
+ ).toContain("maestro exec --provider 'openai' --output-last-message");
+ expect(
+ buildMaestroReviewWorkflow({ apiKeyEnvName: "OPENAI_API_KEY" }),
+ ).toContain("maestro exec --provider 'openai' --output-last-message");
+ expect(
+ buildMaestroReviewWorkflow({
+ apiKeyEnvName: "OPENAI_API_KEY",
+ apiKeySecretName: "CUSTOM_REVIEW_SECRET",
+ }),
+ ).toContain("maestro exec --provider 'openai' --output-last-message");
+ });
+
+ it("quotes shell-interpreted option values", () => {
+ const yaml = buildMaestroReviewWorkflow({
+ model: "foo'; echo nope",
+ maestroPackage: "@example/maestro",
+ maestroVersion: "1.2.3-beta.1",
+ });
+
+ expect(yaml).toContain("--model 'foo'\\''; echo nope'");
+ expect(yaml).toContain("npm install -g '@example/maestro@1.2.3-beta.1'");
+ });
+
+ it("rejects unsafe workflow structure values", () => {
+ expect(() =>
+ buildMaestroReviewWorkflow({ apiKeySecretName: "BAD-NAME" }),
+ ).toThrow("apiKeySecretName");
+ expect(() =>
+ buildMaestroReviewWorkflow({ apiKeyEnvName: "BAD-NAME" }),
+ ).toThrow("apiKeyEnvName");
+ expect(() =>
+ buildMaestroReviewWorkflow({ model: "safe\necho unsafe" }),
+ ).toThrow("model");
+ });
+
+ it("writes the workflow to the conventional path", () => {
+ const repoRoot = mkdtempSync(join(tmpdir(), "maestro-review-"));
+ tempDirs.push(repoRoot);
+ const written = writeMaestroReviewWorkflow(repoRoot);
+ expect(written).toBe(join(repoRoot, MAESTRO_REVIEW_WORKFLOW_PATH));
+ expect(readFileSync(written, "utf8")).toContain(
+ "name: Maestro Code Review",
+ );
+ });
+});
diff --git a/packages/github-agent/src/workflows/maestro-review-workflow.ts b/packages/github-agent/src/workflows/maestro-review-workflow.ts
new file mode 100644
--- /dev/null
+++ b/packages/github-agent/src/workflows/maestro-review-workflow.ts
@@ -1,0 +1,259 @@
+/**
+ * Maestro code-review CI installer
+ *
+ * Emits a GitHub Actions workflow that runs `maestro exec` on every pull
+ * request and posts a merge-readiness review as a PR comment. This is the
+ * turnkey "install Maestro into CI" path: an installer skill (or a user) calls
+ * buildMaestroReviewWorkflow() / writeMaestroReviewWorkflow() to drop the
+ * workflow into a target repository.
+ *
+ * The emitter is deterministic so the installer can detect drift and the
+ * generated file reviews cleanly.
+ */
+
+import { mkdirSync, writeFileSync } from "node:fs";
+import { dirname, join } from "node:path";
+
+/** Repo-relative path the workflow is written to. */
+export const MAESTRO_REVIEW_WORKFLOW_PATH =
+ ".github/workflows/maestro-review.yml";
+
+export interface MaestroReviewWorkflowOptions {
+ /** Node major version used to run Maestro in CI. Default "20". */
+ nodeVersion?: string;
+ /** npm package installed in CI. Defaults to MAESTRO_PACKAGE_NAME or "maestro". */
+ maestroPackage?: string;
+ /** Version/tag of the package to install. Default "latest". */
+ maestroVersion?: string;
+ /**
+ * Optional provider passed to `maestro exec --provider` when no model is set.
+ * Defaults to the provider inferred from the API key secret/env var and falls
+ * back to "anthropic".
+ */
+ provider?: string;
+ /** Optional model id passed to `maestro exec --model`. */
+ model?: string;
+ /**
+ * Name of the repository secret holding the model provider API key. Defaults
+ * to the provider's expected runtime env var (e.g. "ANTHROPIC_API_KEY").
+ */
+ apiKeySecretName?: string;
+ /**
+ * Environment variable exposed to Maestro. Defaults to the provider-specific
+ * API key env var so custom secret names still work at runtime.
+ */
+ apiKeyEnvName?: string;
+}
+
+const PROVIDER_API_KEY_ENV_NAMES: Record<string, string> = {
+ anthropic: "ANTHROPIC_API_KEY",
+ openai: "OPENAI_API_KEY",
+ "openai-codex": "OPENAI_CODEX_TOKEN",
+ "azure-openai": "AZURE_OPENAI_API_KEY",
+ google: "GEMINI_API_KEY",
+ evalops: "MAESTRO_EVALOPS_ACCESS_TOKEN",
+ groq: "GROQ_API_KEY",
+ cerebras: "CEREBRAS_API_KEY",
+ openrouter: "OPENROUTER_API_KEY",
+ mistral: "MISTRAL_API_KEY",
+ deepseek: "DEEPSEEK_API_KEY",
+ xai: "XAI_API_KEY",
+ zai: "ZAI_API_KEY",
+ writer: "WRITER_API_KEY",
+ moonshot: "MOONSHOT_API_KEY",
+ dashscope: "DASHSCOPE_API_KEY",
+ minimax: "MINIMAX_API_KEY",
+};
+
+interface ResolvedOptions {
+ nodeVersion: string;
+ maestroPackage: string;
+ maestroVersion: string;
+ provider: string;
+ model?: string;
+ apiKeySecretName: string;
+ apiKeyEnvName: string;
+}
+
+function validateEnvName(name: string, optionName: string): string {
+ if (!/^[A-Z_][A-Z0-9_]*$/.test(name)) {
+ throw new Error(
+ `${optionName} must be a valid GitHub Actions env name: ${name}`,
+ );
+ }
+ return name;
+}
+
+function validateSecretName(name: string): string {
+ if (!/^[A-Za-z_][A-Za-z0-9_]*$/.test(name)) {
+ throw new Error(
+ `apiKeySecretName must be a valid GitHub Actions secret name: ${name}`,
+ );
+ }
+ return name;
+}
+
+function getDefaultApiKeyEnvName(provider: string): string {
+ return (
+ PROVIDER_API_KEY_ENV_NAMES[provider] ??
+ `${provider.toUpperCase().replace(/[^A-Z0-9]/g, "_")}_API_KEY`
+ );
+}
+
+function validateYamlScalar(value: string, optionName: string): string {
+ if (/\r|\n/.test(value)) {
+ throw new Error(`${optionName} must not contain newlines`);
+ }
+ return value;
+}
+
+function shellQuote(value: string): string {
+ return `'${value.replaceAll("'", "'\\''")}'`;
+}
+
+const PROVIDER_BY_API_KEY_ENV_VAR: Record<string, string> = {
+ ANTHROPIC_API_KEY: "anthropic",
+ GEMINI_API_KEY: "google",
+ GOOGLE_GEMINI_CLI_TOKEN: "google-gemini-cli",
+ GOOGLE_ANTIGRAVITY_TOKEN: "google-antigravity",
+ OPENAI_API_KEY: "openai",
+ OPENAI_CODEX_TOKEN: "openai-codex",
+ OPENAI_CODEX_ACCESS_TOKEN: "openai-codex",
+ CODEX_API_KEY: "openai-codex",
+ AZURE_OPENAI_API_KEY: "azure-openai",
+ MAESTRO_EVALOPS_ACCESS_TOKEN: "evalops",
+ WRITER_API_KEY: "writer",
+ XAI_API_KEY: "xai",
+ GROQ_API_KEY: "groq",
+ CEREBRAS_API_KEY: "cerebras",
+ OPENROUTER_API_KEY: "openrouter",
+ ZAI_API_KEY: "zai",
+ MISTRAL_API_KEY: "mistral",
+ DEEPSEEK_API_KEY: "deepseek",
+ MOONSHOT_API_KEY: "moonshot",
+ KIMI_API_KEY: "moonshot",
+ DASHSCOPE_API_KEY: "dashscope",
+ QWEN_API_KEY: "dashscope",
+ MINIMAX_API_KEY: "minimax",
+};
+
+function inferProviderFromApiKeyEnvVar(
+ apiKeyName?: string,
+): string | undefined {
+ return apiKeyName ? PROVIDER_BY_API_KEY_ENV_VAR[apiKeyName] : undefined;
+}
+
+function resolveOptions(
+ options: MaestroReviewWorkflowOptions,
+): ResolvedOptions {
+ const inferredProvider =
+ inferProviderFromApiKeyEnvVar(options.apiKeySecretName) ??
+ inferProviderFromApiKeyEnvVar(options.apiKeyEnvName);
+ const provider = validateYamlScalar(
+ options.provider ?? inferredProvider ?? "anthropic",
+ "provider",
+ );
+ const apiKeyEnvName = validateEnvName(
+ options.apiKeyEnvName ?? getDefaultApiKeyEnvName(provider),
+ "apiKeyEnvName",
+ );
+ return {
+ nodeVersion: validateYamlScalar(options.nodeVersion ?? "20", "nodeVersion"),
+ maestroPackage: validateYamlScalar(
+ options.maestroPackage ?? process.env.MAESTRO_PACKAGE_NAME ?? "maestro",
+ "maestroPackage",
+ ),
+ maestroVersion: validateYamlScalar(
+ options.maestroVersion ?? "latest",
+ "maestroVersion",
+ ),
+ provider,
+ model:
+ options.model === undefined
+ ? undefined
+ : validateYamlScalar(options.model, "model"),
+ apiKeySecretName: validateSecretName(
+ options.apiKeySecretName ?? apiKeyEnvName,
+ ),
+ apiKeyEnvName,
+ };
+}
+
+/**
+ * Build the `.github/workflows/maestro-review.yml` contents as a YAML string.
+ */
+export function buildMaestroReviewWorkflow(
+ options: MaestroReviewWorkflowOptions = {},
+): string {
+ const resolved = resolveOptions(options);
+ const modelFlag = resolved.model
+ ? ` --model ${shellQuote(resolved.model)}`
+ : "";
+ const packageSpec = `${resolved.maestroPackage}@${resolved.maestroVersion}`;
+ // Keep ${...} placeholders as literal shell expansions in the generated YAML.
+ const reviewPrompt =
+ "Use the pr-review skill to review the changes in this pull request " +
+ "(#${MAESTRO_PR_NUMBER}) from merge base ${MAESTRO_MERGE_BASE_SHA} to " +
+ "${MAESTRO_HEAD_SHA} (equivalent to the three-dot PR diff " +
+ "${MAESTRO_BASE_SHA}...${MAESTRO_HEAD_SHA}). Produce a " +
+ "merge-readiness review ordered by severity, with file and line " +
+ "references.";
+
+ return `# Generated by Maestro (install-code-review). Re-running the installer
+# overwrites this file; hand edits may be lost.
+name: Maestro Code Review
+
+on:
+ pull_request:
+ types: [opened, synchronize, reopened]
+
+permissions:
+ contents: read
+ pull-requests: write
+
+concurrency:
+ group: maestro-review-\${{ github.event.pull_request.number }}
+ cancel-in-progress: true
+
+jobs:
+ review:
+ runs-on: ubuntu-latest
+ steps:
+ - uses: actions/checkout@v4
+ with:
+ fetch-depth: 0
+ - uses: actions/setup-node@v4
+ with:
+ node-version: ${JSON.stringify(resolved.nodeVersion)}
+ - name: Install Maestro
+ run: npm install -g ${shellQuote(packageSpec)}
+ - name: Review pull request
+ env:
+ ${resolved.apiKeyEnvName}: \${{ secrets.${resolved.apiKeySecretName} }}
+ GH_TOKEN: \${{ github.token }}
+ GITHUB_PERSONAL_ACCESS_TOKEN: \${{ github.token }}
+ MAESTRO_PR_NUMBER: \${{ github.event.pull_request.number }}
+ MAESTRO_BASE_SHA: \${{ github.event.pull_request.base.sha }}
+ MAESTRO_HEAD_SHA: \${{ github.event.pull_request.head.sha }}
+ run: |
+ export MAESTRO_MERGE_BASE_SHA="$(git merge-base "\${MAESTRO_BASE_SHA}" "\${MAESTRO_HEAD_SHA}")"
+ maestro exec --provider ${shellQuote(resolved.provider)}${modelFlag} --output-last-message review.md \\
+ "${reviewPrompt}"
+ gh pr comment "\${MAESTRO_PR_NUMBER}" --edit-last --body-file review.md || \\
+ gh pr comment "\${MAESTRO_PR_NUMBER}" --body-file review.md
+`;
+}
+
+/**
+ * Write the review workflow into the target repository, creating the
+ * `.github/workflows` directory if needed. Returns the absolute path written.
+ */
+export function writeMaestroReviewWorkflow(
+ repoRoot: string,
+ options: MaestroReviewWorkflowOptions = {},
+): string {
+ const target = join(repoRoot, MAESTRO_REVIEW_WORKFLOW_PATH);
+ mkdirSync(dirname(target), { recursive: true });
+ writeFileSync(target, buildMaestroReviewWorkflow(options), "utf8");
+ return target;
+}
diff --git a/scripts/pr-feedback-audit.mjs b/scripts/pr-feedback-audit.mjs
--- a/scripts/pr-feedback-audit.mjs
+++ b/scripts/pr-feedback-audit.mjs
@@ -5,12 +5,22 @@
export const GH_OUTPUT_MAX_BUFFER_BYTES = 64 * 1024 * 1024;
+export const REVIEW_FEEDBACK_SEVERITY_RANK = Object.freeze({
+ none: 0,
+ low: 1,
+ medium: 2,
+ high: 3,
+ p1: 4,
+ p0: 5,
+});
+
export function parseFeedbackAuditArgs(argv) {
const args = {
alsoPublic: [],
check: false,
includeResolved: false,
limit: 20,
+ minSeverity: "none",
prs: [],
recentDays: 0,
repo: "",
@@ -33,6 +43,9 @@
args.limit = Number(argv[++index] ?? "");
sawLimit = true;
break;
+ case "--min-severity":
+ args.minSeverity = String(argv[++index] ?? "").toLowerCase();
+ break;
case "--recent-days":
args.recentDays = Number(argv[++index] ?? "");
break;
@@ -61,9 +74,12 @@
}
if (args.prs.length === 0 && args.recentDays === 0) {
throw new Error(
- "Usage: node scripts/pr-feedback-audit.mjs [--repo owner/name] [--check] [--include-resolved] [--recent-days days] [--limit count] [--also-public public-pr] <pr-number-or-url> [...]",
+ "Usage: node scripts/pr-feedback-audit.mjs [--repo owner/name] [--check] [--include-resolved] [--min-severity none|low|medium|high|p1|p0] [--recent-days days] [--limit count] [--also-public public-pr] <pr-number-or-url> [...]",
);
}
+ if (!(args.minSeverity in REVIEW_FEEDBACK_SEVERITY_RANK)) {
+ throw new Error(`--min-severity must be one of ${Object.keys(REVIEW_FEEDBACK_SEVERITY_RANK).join(", ")}`);
+ }
return args;
}
@@ -271,22 +287,162 @@
.slice(0, 240);
}
+function firstNonblankLine(body) {
+ return String(body ?? "")
+ .split(/\r?\n/u)
+ .map((line) => line.trim())
+ .find(Boolean) ?? "";
+}
+
+function escapeRegExp(value) {
+ return String(value).replace(/[.*+?^${}()|[\]\\]/gu, "\\$&");
+}
+
+function reviewMetadataLines(body) {
+ return String(body ?? "")
+ .split(/\r?\n/u)
+ .map((line) =>
+ line
+ .trim()
+ .replace(/^(?:[-*+]\s+)?(?:#{1,6}\s*)?/u, "")
+ .replace(/^[^\p{L}\p{N}\[!_*]+/u, ""),
+ )
+ .filter(Boolean);
+}
+
+function hasExplicitReviewPriority(body, priority) {
+ return reviewMetadataLines(body)
+ .some(
+ (line) =>
+ line === priority ||
+ line.startsWith(`${priority}:`) ||
+ line.startsWith(`[${priority}]`),
+ );
+}
+
+function hasExplicitReviewSeverity(body, severity, badgeLabel) {
+ const severityPattern = new RegExp(
+ `^(?:\\*\\*|__)?${escapeRegExp(severity)}(?:\\*\\*|__)?(?::.*)?$`,
+ "iu",
+ );
+ const badgePattern = new RegExp(
+ `^!\\[${escapeRegExp(badgeLabel)}\\](?:\\([^)]*\\))?(?::.*)?$`,
+ "iu",
+ );
+ return reviewMetadataLines(body).some(
+ (line) => severityPattern.test(line) || badgePattern.test(line),
+ );
+}
+
+export function informationalReviewFeedback(body, author) {
+ const firstLine = firstNonblankLine(body);
+ const text = String(body ?? "");
+ const trustedReviewBot =
+ /^(?:cursor|coderabbitai|chatgpt-codex-connector|devin-ai-integration)\b/iu.test(
+ String(author ?? ""),
+ );
+ const summaryComment = /^##\s+(?:PR\s+Summary|Summary|Walkthrough)\b/iu.test(
+ firstLine,
+ );
+ const infoSection = /(?:^|\n)\s*(?:📝\s*)?\*\*Info:/u.test(text);
+ return (
+ trustedReviewBot &&
+ (summaryComment || (infoSection && reviewFeedbackSeverity(text) === "none"))
+ );
+}
+
+export function reviewFeedbackSeverity(body) {
+ const text = String(body ?? "");
+ if (hasExplicitReviewPriority(text, "P0")) return "p0";
+ if (hasExplicitReviewPriority(text, "P1")) return "p1";
+ if (hasExplicitReviewSeverity(text, "High Severity", "High Badge")) {
+ return "high";
+ }
+ if (hasExplicitReviewSeverity(text, "Medium Severity", "Medium Badge")) {
+ return "medium";
+ }
+ if (hasExplicitReviewSeverity(text, "Low Severity", "Low Badge")) {
+ return "low";
+ }
+ return "none";
+}
+
+function firstComment(thread) {
+ return thread.comments?.nodes?.[0];
+}
+
+function nonInformationalThreadComments(thread) {
+ return (thread.comments?.nodes ?? []).filter(
+ (comment) =>
+ !informationalReviewFeedback(comment.body, comment.author?.login),
+ );
+}
+
+export function reviewThreadSeverity(thread) {
+ const candidates = nonInformationalThreadComments(thread)
+ .map((comment) => [reviewFeedbackSeverity(comment.body), comment])
+ .filter(([severity]) => REVIEW_FEEDBACK_SEVERITY_RANK[severity] > 0);
+ const [severity] =
+ candidates.sort(
+ ([left], [right]) =>
+ REVIEW_FEEDBACK_SEVERITY_RANK[right] -
+ REVIEW_FEEDBACK_SEVERITY_RANK[left],
+ )[0] ?? [];
+ return severity ?? "none";
+}
+
+function hasActionableReviewFeedback(thread) {
+ return (thread.comments?.nodes ?? []).some(
+ (comment) =>
+ !informationalReviewFeedback(comment.body, comment.author?.login),
+ );
+}
+
+export function threadBlocksFeedbackAudit(thread, minSeverity = "high") {
+ if (thread.isResolved) return false;
+ if (minSeverity === "none") {
+ return nonInformationalThreadComments(thread).length > 0;
+ }
+ const severity = reviewThreadSeverity(thread);
+ if (severity === "none") {
+ return minSeverity === "none" && hasActionableReviewFeedback(thread);
+ }
+ return (
+ REVIEW_FEEDBACK_SEVERITY_RANK[severity] >=
+ REVIEW_FEEDBACK_SEVERITY_RANK[minSeverity]
+ );
+}
+
+export function visibleFeedbackAuditThreads(
+ threads,
+ { includeResolved = false, minSeverity = "high" } = {},
+) {
+ return threads.filter((thread) => {
+ if (threadBlocksFeedbackAudit(thread, minSeverity)) {
+ return true;
+ }
+ return includeResolved && thread.isResolved;
+ });
+}
+
function printThread(thread) {
const location = [thread.path, thread.line ?? thread.startLine]
.filter(Boolean)
.join(":");
- const firstComment = thread.comments?.nodes?.[0];
+ const first = firstComment(thread);
const status = thread.isResolved
? "resolved"
: thread.isOutdated
? "unresolved, outdated"
: "unresolved";
- console.log(`- ${thread.id} ${status}${location ? ` at ${location}` : ""}`);
- if (firstComment?.url) {
- console.log(` ${firstComment.url}`);
+ console.log(
+ `- ${thread.id} ${status}, severity=${reviewThreadSeverity(thread)}${location ? ` at ${location}` : ""}`,
+ );
+ if (first?.url) {
+ console.log(` ${first.url}`);
}
- if (firstComment?.body) {
- console.log(` ${firstComment.author?.login ?? "reviewer"}: ${summarizeBody(firstComment.body)}`);
+ if (first?.body) {
+ console.log(` ${first.author?.login ?? "reviewer"}: ${summarizeBody(first.body)}`);
}
}
@@ -312,17 +468,20 @@
}
const uniqueTargets = dedupeFeedbackAuditTargets(targets);
- let unresolvedCount = 0;
+ let blockingCount = 0;
for (const input of uniqueTargets) {
const threads = fetchReviewThreads(input.owner, input.repo, input.number);
- const visibleThreads = args.includeResolved
- ? threads
- : threads.filter((thread) => !thread.isResolved);
- const unresolved = threads.filter((thread) => !thread.isResolved);
- unresolvedCount += unresolved.length;
+ const blocking = threads.filter((thread) =>
+ threadBlocksFeedbackAudit(thread, args.minSeverity),
+ );
+ const visibleThreads = visibleFeedbackAuditThreads(threads, {
+ includeResolved: args.includeResolved,
+ minSeverity: args.minSeverity,
+ });
+ blockingCount += blocking.length;
console.log(
- `${input.owner}/${input.repo}#${input.number}: ${unresolved.length} unresolved review thread(s), ${threads.length} total`,
+ `${input.owner}/${input.repo}#${input.number}: ${blocking.length} blocking review thread(s) at or above ${args.minSeverity}, ${threads.length} total`,
);
if (visibleThreads.length === 0) {
console.log(" no matching review threads");
@@ -333,7 +492,7 @@
}
}
- if (args.check && unresolvedCount > 0) {
+ if (args.check && blockingCount > 0) {
process.exit(1);
}
}
diff --git a/skills/incident-triage/SKILL.md b/skills/incident-triage/SKILL.md
--- a/skills/incident-triage/SKILL.md
+++ b/skills/incident-triage/SKILL.md
@@ -25,12 +25,30 @@
## Workflow
1. Confirm the symptom, start time, affected surface, and urgency.
-2. Load `reference/triage.md` for the detailed timeline and mitigation checklist.
+2. Load learned guidelines (see below) and `reference/triage.md` for the detailed timeline and mitigation checklist.
3. Gather only scoped evidence from granted tools and local files. Keep customer data and internal handles out of the normal answer.
4. Build a timeline with known, inferred, and unknown entries separated.
5. Identify the likely owner, immediate mitigation, verification signal, and follow-up issue.
6. End with current state, blast radius, next action, and what evidence was withheld or unavailable.
+7. Record what you learned (see below) so the next incident starts from it.
+## Learned Guidelines
+
+This skill accumulates alert-type knowledge across runs in
+`~/.maestro/skills/incident-triage/guidelines.md`.
+
+- **At the start of a run**, read that file if it exists and treat its entries as
+ priors to verify — which tools, dashboards, owners, and mitigations a given
+ alert type needed last time. Confirm they still hold; do not trust them
+ blindly.
+- **At the end of a run**, append one concise entry mapping the alert type or
+ symptom you handled to the tools/interfaces, likely owner, and mitigation that
+ actually worked. Keep entries short and free of secrets and customer data.
+
+Programmatic callers can use `loadLearnedGuidelines` /
+`appendLearnedGuideline` / `formatLearnedGuidelinesForPrompt` from
+the main Maestro package (`src/skills/learned-guidelines.ts`).
+
## Toolbox
Run `toolbox/incident-timeline` to emit the required incident report skeleton.
diff --git a/skills/install-code-review/SKILL.md b/skills/install-code-review/SKILL.md
new file mode 100644
--- /dev/null
+++ b/skills/install-code-review/SKILL.md
@@ -1,0 +1,69 @@
+---
+name: install-code-review
+description: Install automated Maestro code review in a GitHub repository's CI — add a pull_request workflow that runs `maestro exec` and posts a merge-readiness review as a PR comment. Use when the user wants to set up automated PR review, wire Maestro into CI/CD, or "install code review".
+license: Complete terms in LICENSE.txt
+compatibility: "Maestro skill packages with write/bash access to a checked-out GitHub repository."
+allowed-tools:
+ - read
+ - write
+ - bash
+builtin-tools:
+ - read
+ - write
+mode: build
+metadata:
+ version: "0.1.0"
+ category: evalops-operations
+ artifactSchema: evalops.maestro.skill.install_code_review.v1
+---
+
+# Install Code Review
+
... diff truncated: showing 800 of 3745 linesYou can send follow-ups to the cloud agent here. |
| export async function cleanupNonInteractiveRuntimeResources(): Promise<void> { | ||
| let timeout: ReturnType<typeof setTimeout> | undefined; | ||
| const timeoutPromise = new Promise<"timeout">((resolve) => { | ||
| timeout = setTimeout( | ||
| () => resolve("timeout"), | ||
| NON_INTERACTIVE_CLEANUP_GRACE_MS, | ||
| ); | ||
| }); | ||
| const cleanupPromise = (async (): Promise<"done"> => { | ||
| try { | ||
| const [{ mcpManager }, { lspManager }] = await Promise.all([ | ||
| import("../mcp/manager.js"), | ||
| import("../lsp/manager.js"), | ||
| ]); | ||
| await Promise.allSettled([ | ||
| mcpManager.disconnectAll(), | ||
| lspManager.shutdownAll(), | ||
| ]); | ||
| } catch { | ||
| // Best-effort shutdown must not mask the command's original result. | ||
| } | ||
| return "done"; | ||
| })(); | ||
| try { | ||
| await Promise.race([cleanupPromise, timeoutPromise]); | ||
| } finally { | ||
| if (timeout) { | ||
| clearTimeout(timeout); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
📝 Info: Non-interactive cleanup races against a 5-second timeout but orphaned I/O may delay exit
The cleanupNonInteractiveRuntimeResources function (src/runtime/non-interactive-cleanup.ts:3-33) races MCP/LSP shutdown against a 5-second timer. If the timeout fires first, the function returns, but the cleanupPromise continues running in the background with its pending I/O (socket disconnects). The clearTimeout in the finally block prevents the timer from keeping the event loop alive, but any in-progress socket operations from mcpManager.disconnectAll() or lspManager.shutdownAll() could still delay process exit beyond the timeout. In practice this is acceptable for a best-effort shutdown, but worth noting that the process may not exit immediately after main() returns if MCP servers are slow to disconnect.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Summary
evalops/maestro-internalevalops/maestroas a generated public mirror of the private source of truth38b20ebe7c13a7a5775940572015beb65b14a6ad4037f5829f88f1e8bd786e417020bfbd8dabe96033file(s) to copy/update and0stale file(s) to delete0Source-of-truth status
Public Mirror Drift Audit
@evalops/maestrohttps://github.com/evalops/maestro-internal@main (38b20ebe7c13)https://github.com/evalops/maestro@main (4037f5829f88)330public_projection_has_driftSample Changed Paths
Guidance
Let internal main generate and merge the public sync PR before relying on public main.
Drift sample
Public-only commits since last generated sync
Validation
sync-public-release-mirrorworkflow inpublic-treemodeTest Plan
sync-public-release-mirrorworkflow inpublic-treemoderequire-internal-prcheck confirms internal source PR lineageStaged Rollout
evalops/maestro-internal@38b20ebe7c13a7a5775940572015beb65b14a6ad, including existing hidden/evaluation surfaces, and keeps public package parity behind the established public-source-provenance gate.