Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion web/admin/src/lib/layers/analyzeOrg.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { analyzeWorkflowsLayer } from "./workflows";
export type AnalyzeOrgLayersInput = {
org: string;
gh: LayerGithub;
/** Agent roles from org config (drives secret/variable names). */
/** Roles from org config (drives secret/variable names). */
agents: { role: string }[];
/** Repos with `enabled: true` in config (drives enrollment checks). */
enabledRepos: string[];
Expand Down
62 changes: 53 additions & 9 deletions web/admin/src/lib/layers/orgConfigParse.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { describe, expect, it } from "vitest";
import {
agentsFromConfig,
rolesFromConfig,
enabledReposFromConfig,
MAX_ORG_CONFIG_YAML_DEPTH,
MAX_ORG_CONFIG_YAML_UTF8_BYTES,
Expand Down Expand Up @@ -65,28 +65,73 @@ repos: {}
expect(validateOrgConfig(cfg)).toMatch(/non-negative integer/);
});

it("rejects invalid agent role", () => {
it.each(["fullsend", "triage", "coder", "review", "fix", "retro", "prioritize", "e2e"])(
"accepts defaults role %s",
(role) => {
const cfg = parseOrgConfigYaml(`version: "1"
dispatch:
platform: github-actions
defaults:
roles: [${role}]
repos: {}
`);
expect(validateOrgConfig(cfg)).toBeNull();
},
);

it("rejects invalid defaults role", () => {
const cfg = parseOrgConfigYaml(`version: "1"
dispatch:
platform: github-actions
defaults:
roles: [bogus]
repos: {}
`);
expect(validateOrgConfig(cfg)).toMatch(/invalid role/);
});

it("parses agents with source-based entries without error", () => {
const cfg = parseOrgConfigYaml(`version: "1"
dispatch:
platform: github-actions
defaults:
roles: [fullsend]
agents:
- role: not-a-valid-role
- source: harness/triage.yaml
- source: harness/coder.yaml
name: my-coder
- harness/review.yaml
repos: {}
`);
expect(validateOrgConfig(cfg)).toMatch(/invalid agent role/);
expect(validateOrgConfig(cfg)).toBeNull();
});

it("lists agents and enabled repos from config", () => {
it("rolesFromConfig returns defaults.roles as role objects", () => {
const cfg = parseOrgConfigYaml(`version: "1"
dispatch:
platform: github-actions
defaults:
roles: [triage, coder]
repos: {}
`);
expect(rolesFromConfig(cfg)).toEqual([{ role: "triage" }, { role: "coder" }]);
});

it("rolesFromConfig returns empty array when no defaults.roles", () => {
const cfg = parseOrgConfigYaml(`version: "1"
dispatch:
platform: github-actions
repos: {}
`);
expect(rolesFromConfig(cfg)).toEqual([]);
});

it("lists enabled repos from config", () => {
const cfg = parseOrgConfigYaml(`version: "1"
dispatch:
platform: github-actions
defaults:
roles: [fullsend]
agents:
- role: triage
slug: t
repos:
zed:
enabled: false
Expand All @@ -96,7 +141,6 @@ repos:
enabled: true
`);
expect(validateOrgConfig(cfg)).toBeNull();
expect(agentsFromConfig(cfg)).toEqual([{ role: "triage" }]);
expect(enabledReposFromConfig(cfg)).toEqual(["alpha", "beta"]);
});

Expand Down
24 changes: 8 additions & 16 deletions web/admin/src/lib/layers/orgConfigParse.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,11 @@ export type OrgConfigYaml = {
max_implementation_retries?: number;
auto_merge?: boolean;
};
agents?: { role: string; name?: string; slug?: string }[];
agents?: (string | { source?: string; name?: string; enabled?: boolean })[];
repos?: Record<string, { enabled?: boolean; roles?: string[] }>;
};

const VALID_ROLES = new Set(["fullsend", "triage", "coder", "review"]);
const VALID_ROLES = new Set(["fullsend", "triage", "coder", "review", "fix", "retro", "prioritize", "e2e"]);
Comment thread
qodo-code-review[bot] marked this conversation as resolved.

/** 512 KiB — more than sufficient for any realistic org `config.yaml`. */
export const MAX_ORG_CONFIG_YAML_UTF8_BYTES = 512 * 1024;
Expand Down Expand Up @@ -111,12 +111,9 @@ function assertOrgConfigShape(doc: Record<string, unknown>): void {
}
for (let i = 0; i < doc.agents.length; i++) {
const el = doc.agents[i];
if (typeof el === "string") continue;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MEDIUM — Removed typeof guard lets a non-string agents[].name escape as a raw TypeError on the org row

This PR deletes the only type assertion on agent fields (agents[${i}].role must be a string) and replaces it with if (typeof el === "string") continue;, validating nothing inside mappings (orgConfigParse.ts:112-118).

Reproduced by running the shipped helpers in Node: a YAML name: 42 parses to a number under the core schema, derivedAgentName returns it unchanged (if (entry.name) return entry.name; — line 163 does not check the type), agentsFromConfig returns [{name: 42}] without throwing, and the throw only happens later in secretNameForRole (secrets.ts:6): TypeError: r.toUpperCase is not a function. That throw originates inside analyzeSecretsLayer, which runs at orgListRow.ts:104outside the inner try/catch whose comment deliberately degrades gracefully ("invalid YAML — still analyze other layers with empty agents/repos", orgListRow.ts:100) — so it lands in the generic outer catch at orgListRow.ts:133-137 and the whole org row renders a raw JS internal message instead of degrading.

For accuracy: the neighbouring case of a non-string source does not have this problem — source: 123 throws inside sourceBaseName during agentsFromConfig at orgListRow.ts:89, which is inside the inner try, so it degrades correctly. Only the name path escapes.

Related: a quoted enabled: "false" is !== false, so isAgentEnabled keeps the agent enabled with no type check.

Suggestion: In assertOrgConfigShape, assert name and source are strings when present and enabled is a boolean when present, using the same agents[${i}].<field> must be a … message style as the check that was removed.

if (el === null || typeof el !== "object" || Array.isArray(el)) {
throw new Error(`parsing org config: agents[${i}] must be a mapping with a string role`);
}
const role = (el as Record<string, unknown>).role;
if (typeof role !== "string") {
throw new Error(`parsing org config: agents[${i}].role must be a string`);
throw new Error(`parsing org config: agents[${i}] must be a string or mapping`);
}
}
}
Expand Down Expand Up @@ -153,20 +150,15 @@ export function validateOrgConfig(cfg: OrgConfigYaml): string | null {
}
for (const role of cfg.defaults?.roles ?? []) {
if (!VALID_ROLES.has(role)) {
return `invalid role ${JSON.stringify(role)}: must be one of fullsend, triage, coder, review`;
}
}
for (const agent of cfg.agents ?? []) {
if (!VALID_ROLES.has(agent.role)) {
return `invalid agent role ${JSON.stringify(agent.role)}: must be one of fullsend, triage, coder, review`;
return `invalid role ${JSON.stringify(role)}: must be one of fullsend, triage, coder, review, fix, retro, prioritize, e2e`;
}
}
return null;
}

/** Agent rows for secrets-layer analyze (mirrors `config.OrgConfig.Agents`). */
export function agentsFromConfig(cfg: OrgConfigYaml): { role: string }[] {
return (cfg.agents ?? []).map((a) => ({ role: a.role }));
/** Roles for secrets-layer analyze — Go keys secrets by role, not agent name. */
export function rolesFromConfig(cfg: OrgConfigYaml): { role: string }[] {
return (cfg.defaults?.roles ?? []).map((r) => ({ role: r }));
}

/** Enabled repo names for enrollment-layer analyze (sorted). */
Expand Down
4 changes: 2 additions & 2 deletions web/admin/src/lib/orgs/orgListRow.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {
import { CONFIG_FILE_PATH, CONFIG_REPO_NAME } from "../layers/constants";
import { createLayerGithub } from "../layers/githubClient";
import {
agentsFromConfig,
rolesFromConfig,
enabledReposFromConfig,
OrgConfigYamlLimitError,
parseOrgConfigYaml,
Expand Down Expand Up @@ -86,7 +86,7 @@ export async function analyzeOrgForOrgList(
try {
const cfg = parseOrgConfigYaml(raw);
if (validateOrgConfig(cfg) === null) {
agents = agentsFromConfig(cfg);
agents = rolesFromConfig(cfg);
enabledRepos = enabledReposFromConfig(cfg);
}
} catch (e) {
Expand Down
Loading