Skip to content
Open
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
Original file line number Diff line number Diff line change
Expand Up @@ -101,16 +101,31 @@ only touches the webhook where an `installation.created` delivery records the ro

### Web UI (Settings → GitHub)

- Admin, connected: the connected account + type, "Connection" and "Source repos" section headings, and the
"Manage connection" / "Manage on GitHub" / "Connection details" controls; the connect panel exposes Reinstall (step 1,
when installed) and Disconnect (step 2).
- Admin, connected: the connected account + type, the "Connection" / "Automatic handling" / "Repositories" section
headings, and the "Manage connection" / "Manage on GitHub" / "Connection details" controls; the connect panel exposes
Reinstall (step 1, when installed) and Disconnect (step 2). The Repositories section hands off to Settings →
Repositories rather than editing repository URLs here.
- Admin, not connected: a "Connect GitHub" call-to-action.
- Connection details, permissions satisfied: every entry under "Required for automatic replies" reads as ready, and no
GitHub-side call to action appears. The heading stays scoped to that capability — it must not claim the installation
satisfies First Tree generally, since Context Reviewer and repository coverage gate on their own requirements.
- Connection details, a required permission missing (downgrade `issues` or `pull_requests` to `read` on the installation
row): the entry reads as blocked and names what it costs, a "Grant on GitHub" action appears for an admin, and the
shortfall is also visible on the *collapsed* disclosure. This must agree with the server: the same installation makes
`team-agent` assignment fail with `github_app_task_reply_permission_required`, so the readout and the gate never
disagree.
- Connection details, facts: subscribed events First Tree does not consume are listed as unused rather than blended into
the consumed list; `installation` / `installation_repositories` are *not* among them, since the webhook route consumes
both through `handleInstallationLifecycle` — a normally configured installation must never be described as carrying
unused subscriptions. The installation timestamp is labelled "First seen", not "Connected": the row is written unbound
by `installation.created` and survives disconnect/rebind, so it does not record when this team connected. The
installation id copies (a non-secure-context copy failure surfaces "Copy failed" instead of looking inert).
- Google/OIDC admin without GitHub identity: after opening Connect, only the inline link-account state appears. After the
identity is linked and the same Team is restored, the panel shows the linked login and Install remains a separate click.
- Install preflight mismatch: the current First Tree session and selected Team remain unchanged; the recovery message
names the expected GitHub login and offers a retry rather than opening the picker.
- Member: the connection state stays readable, but every admin control (Manage / Disconnect / Connect / Reinstall /
Install / Add source repo) is absent.
- Member: the connection state and the permission diagnosis stay readable, but every admin control (Manage / Disconnect /
Connect / Reinstall / Install / Grant on GitHub) is absent.
- No browser console errors on any state.

## Expected Result
Expand Down
9 changes: 8 additions & 1 deletion packages/server/src/services/team-agent-settings.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import {
GITHUB_TASK_REPLY_REQUIRED_PERMISSIONS,
githubPermissionSatisfies,
ORG_SETTINGS_NAMESPACES,
type OrgGithubFeaturesOutput,
type OrgGithubFeaturesStorage,
Expand Down Expand Up @@ -74,7 +76,12 @@ async function readContextReviewerAgentUuid(db: Database, organizationId: string
function taskReplyInstallationBlocker(installation: InstallationRow | null): SetupBlocker | null {
if (!installation) return null;
if (installation.suspendedAt) return blocker("github_app_suspended", "manage_github_installation");
if (installation.permissions.issues !== "write" || installation.permissions.pull_requests !== "write") {
// Same requirement set the Settings → GitHub readout renders, so an admin is
// never shown a healthy connection while this gate refuses the assignment.
const unsatisfied = Object.entries(GITHUB_TASK_REPLY_REQUIRED_PERMISSIONS).some(
([permission, level]) => !githubPermissionSatisfies(installation.permissions[permission], level),
);
if (unsatisfied) {
return blocker("github_app_task_reply_permission_required", "manage_github_installation");
}
return null;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import { describe, expect, it } from "vitest";
import { GITHUB_TASK_REPLY_REQUIRED_PERMISSIONS, githubPermissionSatisfies } from "../schemas/github-app.js";

describe("GITHUB_TASK_REPLY_REQUIRED_PERMISSIONS", () => {
it("is the set the task-reply gate enforces", () => {
// Both the server's `taskReplyInstallationBlocker` and the Settings →
// GitHub readout derive from this. Changing it changes what an admin is
// told AND what the server will publish, so it is pinned here.
expect(GITHUB_TASK_REPLY_REQUIRED_PERMISSIONS).toEqual({ issues: "write", pull_requests: "write" });
});
});

describe("githubPermissionSatisfies", () => {
it("accepts an exact match", () => {
expect(githubPermissionSatisfies("write", "write")).toBe(true);
expect(githubPermissionSatisfies("read", "read")).toBe(true);
});

it("rejects a weaker grant", () => {
expect(githubPermissionSatisfies("read", "write")).toBe(false);
expect(githubPermissionSatisfies("write", "admin")).toBe(false);
});

it("does NOT rank levels — a stronger grant is not a substitute", () => {
// Every other installation-permission check in the server compares levels
// exactly. Ranking here would let this readout and the task-agent gate
// accept a grant that `github-audience` / the publishers then reject.
expect(githubPermissionSatisfies("admin", "write")).toBe(false);
expect(githubPermissionSatisfies("write", "read")).toBe(false);
});

it("treats an absent permission as unsatisfied", () => {
expect(githubPermissionSatisfies(undefined, "read")).toBe(false);
});
});
2 changes: 2 additions & 0 deletions packages/shared/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -781,6 +781,7 @@ export {
GITHUB_ACCOUNT_TYPES,
GITHUB_APP_CONNECT_STATUSES,
GITHUB_PERMISSION_LEVELS,
GITHUB_TASK_REPLY_REQUIRED_PERMISSIONS,
type GithubAccountType,
type GithubAppConnectBody,
type GithubAppConnectPanelInstallation,
Expand All @@ -801,6 +802,7 @@ export {
githubAppInstallationPermissionsSchema,
githubAppUserTokenMetadataSchema,
githubPermissionLevelSchema,
githubPermissionSatisfies,
} from "./schemas/github-app.js";
export {
GITHUB_TASK_REPLY_BODY_MAX_BYTES,
Expand Down
48 changes: 48 additions & 0 deletions packages/shared/src/schemas/github-app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,54 @@ export type GithubPermissionLevel = z.infer<typeof githubPermissionLevelSchema>;
export const githubAppInstallationPermissionsSchema = z.record(z.string(), githubPermissionLevelSchema);
export type GithubAppInstallationPermissions = z.infer<typeof githubAppInstallationPermissionsSchema>;

/**
* The permission levels an installation must grant before the GitHub Task
* Agent can reply automatically on Issues and pull requests —
* `permission name -> required level`.
*
* Scoped to that one capability, not to the installation as a whole. Other
* First Tree capabilities gate on their own permissions, events, and
* repository coverage (Context Reviewer, the setup-capability probe, …), so
* this set answers "can automatic replies work here", never "is this
* installation generally usable".
*
* Within that scope it is the one definition: the server gates the GitHub Task
* Agent on it (`taskReplyInstallationBlocker`) and the Settings → GitHub
* readout renders it, so an admin is never told replies are fine while the
* server refuses to publish (or the reverse).
*
* `metadata: read` is deliberately absent: GitHub grants it to every App
* install and it cannot be withheld, so listing it would only ever render a
* tautological ✓. The reply publisher still requests it when minting a scoped
* token — that is a token scope, not a gate.
*/
export const GITHUB_TASK_REPLY_REQUIRED_PERMISSIONS = {
issues: "write",
pull_requests: "write",
} as const satisfies Record<string, GithubPermissionLevel>;

/**
* True when `granted` meets a required level.
*
* Deliberately an exact match, not a `read < write < admin` ranking. Every
* other installation-permission check in the server compares levels exactly
* (`github-audience.ts`, `github-task-reply-publisher.ts`,
* `context-reviewer-publisher.ts`, `setup-capabilities.ts`, …). A ranking here
* would let the Settings readout and the task-agent gate accept an `admin`
* grant that routing and publishing then reject — the exact contradiction this
* shared definition exists to prevent. GitHub only ever issues `read` / `write`
* for `issues` and `pull_requests`, so a ranking would buy nothing anyway.
*
* If the ranked semantics are ever wanted, change every call site together,
* not just this one.
*/
export function githubPermissionSatisfies(
granted: GithubPermissionLevel | undefined,
required: GithubPermissionLevel,
): boolean {
return granted === required;
}

/**
* Subscribed event-name list, e.g. `["issues", "pull_request", "push"]`.
* Free-form for the same forward-compat reason as `permissions`.
Expand Down
4 changes: 2 additions & 2 deletions packages/web/DESIGN.md
Original file line number Diff line number Diff line change
Expand Up @@ -308,8 +308,8 @@ const buttonVariants = cva("…base classes…", {
**Inventory** (representative):
- **Actions / inputs:** Button, Input, Label, SegmentedControl, FilterPill,
OptionCard, Command (cmdk), RowActionsMenu
- **Containers / layout:** Card, Panel, Section, Tile, SettingsField, PageHeader,
SectionHeader, FlatSectionHeader, TabBar
- **Containers / layout:** Card, Panel, Section, SettingRow, Tile, SettingsField,
PageHeader, SectionHeader, FlatSectionHeader, TabBar
- **Data:** Table, DenseTable, Breadcrumb, Markdown
- **Status / presence:** Badge, DenseBadge, StateChip, StateDot, StatusGlyph,
PresenceChip, AgentStatusChip
Expand Down
112 changes: 112 additions & 0 deletions packages/web/src/components/ui/__tests__/setting-row.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,112 @@
// @vitest-environment happy-dom

import { act, type ReactElement } from "react";
import { createRoot, type Root } from "react-dom/client";
import { afterEach, beforeEach, describe, expect, it } from "vitest";
import { SettingRow } from "../setting-row.js";

globalThis.IS_REACT_ACT_ENVIRONMENT = true;

async function renderDom(element: ReactElement): Promise<{ container: HTMLElement; root: Root }> {
const container = document.createElement("div");
document.body.appendChild(container);
const root = createRoot(container);
await act(async () => {
root.render(element);
});
return { container, root };
}

beforeEach(() => {
document.body.innerHTML = "";
});

afterEach(() => {
document.body.innerHTML = "";
});

describe("SettingRow", () => {
it("renders name, effect, right-aligned control, and full-width extras", async () => {
const { container, root } = await renderDom(
<SettingRow
title="GitHub Task Agent"
description="Dev Agent One handles Issue activity."
control={<button type="button">Change</button>}
>
<span data-testid="extra">Blocker copy</span>
</SettingRow>,
);

expect(container.textContent).toContain("GitHub Task Agent");
expect(container.textContent).toContain("Dev Agent One handles Issue activity.");
const control = container.querySelector("button");
expect(control?.textContent).toBe("Change");
// The control belongs to the right-hand cluster, not the text column.
expect(control?.closest("[data-setting-row]")).not.toBeNull();
expect(control?.parentElement?.className).toContain("sm:justify-end");
// Extras render below the row line, outside the control cluster.
const extra = container.querySelector('[data-testid="extra"]');
expect(extra?.closest("[data-setting-row]")).not.toBeNull();
expect(extra?.parentElement?.contains(control)).toBe(false);

await act(async () => root.unmount());
});

it("hangs extras off the row's text column when it has a glyph", async () => {
const { container, root } = await renderDom(
<SettingRow icon={<svg aria-hidden />} title="GitHub App">
<span data-testid="extra">Connection details</span>
</SettingRow>,
);

// Without this the block resets to the container edge while the title sits
// indented past the glyph, leaving two competing left edges.
const extraWrapper = container.querySelector('[data-testid="extra"]')?.parentElement;
expect(extraWrapper?.style.paddingLeft).toBe("var(--setting-row-indent)");

await act(async () => root.unmount());
});

it("leaves extras flush when there is no glyph to align to", async () => {
const { container, root } = await renderDom(
<SettingRow title="Connection">
<span data-testid="extra">Detail</span>
</SettingRow>,
);

const extraWrapper = container.querySelector('[data-testid="extra"]')?.parentElement;
expect(extraWrapper?.style.paddingLeft).toBe("");

await act(async () => root.unmount());
});

it("draws no chrome of its own — the enclosing Section owns the rule", async () => {
const { container, root } = await renderDom(<SettingRow title="Connection" />);

const row = container.querySelector<HTMLElement>("[data-setting-row]");
expect(row).not.toBeNull();
expect(row?.style.border).toBe("");
expect(row?.style.borderRadius).toBe("");
expect(row?.style.background).toBe("");

await act(async () => root.unmount());
});

it("spreads call-site hooks onto the row element", async () => {
const { container, root } = await renderDom(<SettingRow data-github-task-agent-controls="admin" title="Agent" />);

const row = container.querySelector<HTMLElement>('[data-github-task-agent-controls="admin"]');
expect(row?.getAttribute("data-setting-row")).toBe("true");

await act(async () => root.unmount());
});

it("omits the description and control slots when not supplied", async () => {
const { container, root } = await renderDom(<SettingRow title="Connection" />);

expect(container.querySelector("p")).toBeNull();
expect(container.querySelector(".sm\\:justify-end")).toBeNull();

await act(async () => root.unmount());
});
});
103 changes: 103 additions & 0 deletions packages/web/src/components/ui/setting-row.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
import type { HTMLAttributes, ReactNode } from "react";
import { cn } from "../../lib/utils.js";

/**
* One configurable thing inside a Settings `Section`: an optional leading
* glyph, the name of the setting, a one-line explanation of what it does, and
* the control that changes it — the control right-aligned on the same line.
*
* Why a row instead of the previous stack (label → description → full-width
* control): a settings section is scanned, not read. Keeping "what it is" on
* the left and "change it" on the right lets the eye run down a single control
* column, and stops a narrow `Select` from stretching across the whole page.
*
* The row draws no border, background, or radius of its own. The enclosing
* `Section` owns the single rule above the block, and card chrome stays
* reserved for genuinely selectable surfaces (DESIGN.md Pillar 5) — the leading
* glyph gets a small sunken tile so the row reads as an object, but the row
* content itself is never boxed.
*
* Extra props (`id`, `data-*`, `aria-*`) spread onto the row element so a call
* site can keep its own test/scroll hooks on the outermost node.
*/
export function SettingRow({
icon,
title,
description,
control,
children,
className,
style,
...rest
}: {
/** Leading line glyph (lucide, `h-4 w-4`), rendered in a sunken tile. */
icon?: ReactNode;
/** What this row configures. */
title: ReactNode;
/** One line on what the setting does / its current effect. */
description?: ReactNode;
/** Right-aligned control — Switch, Select, Button cluster, or a status line. */
control?: ReactNode;
/** Full-width content under the row: disclosures, blockers, inline errors. */
children?: ReactNode;
} & Omit<HTMLAttributes<HTMLDivElement>, "title" | "children">): ReactNode {
return (
<div
data-setting-row
className={cn("flex flex-col", className)}
style={{ gap: "var(--sp-2_5)", padding: "var(--sp-3) 0", ...style }}
{...rest}
>
{/* Stacked on phones so a wide control never overflows; one line with the
control right-aligned from sm up. */}
<div className="flex flex-col gap-2 sm:flex-row sm:items-center sm:gap-4">
<div className="flex min-w-0 flex-1 items-start" style={{ gap: "var(--sp-2_5)" }}>
{icon ? (
<span
aria-hidden
className="inline-flex shrink-0 items-center justify-center"
style={{
width: "var(--sp-7)",
height: "var(--sp-7)",
borderRadius: "var(--radius-input)",
background: "var(--bg-sunken)",
color: "var(--fg-3)",
}}
>
{icon}
</span>
) : null}
<div className="min-w-0">
<div className="text-body font-medium" style={{ color: "var(--fg)" }}>
{title}
</div>
{description ? (
<p className="text-label m-0" style={{ marginTop: "var(--sp-0_5)", color: "var(--fg-3)" }}>
{description}
</p>
) : null}
</div>
</div>
{control ? (
<div className="flex shrink-0 flex-wrap items-center sm:justify-end" style={{ gap: "var(--sp-2)" }}>
{control}
</div>
) : null}
</div>

{/* Extras hang off the row's text column, not the section edge. Without
this they reset to the container's left margin while the row's own
title sits indented past the glyph, so an expanded block reads as a
detached slab with a second, further-left edge. */}
{children ? <div style={{ paddingLeft: textColumnOffset(icon) }}>{children}</div> : null}
</div>
);
}

/**
* Distance from the row's left edge to where its title starts — the glyph tile
* plus the gap after it, or nothing when the row has no glyph.
*/
function textColumnOffset(icon: ReactNode): string | undefined {
return icon ? "var(--setting-row-indent)" : undefined;
}
Loading
Loading