Skip to content

feat(web): give Settings → GitHub a scannable row layout - #2248

Open
Gandy2025 wants to merge 5 commits into
mainfrom
feat/github-settings-row-layout
Open

feat(web): give Settings → GitHub a scannable row layout#2248
Gandy2025 wants to merge 5 commits into
mainfrom
feat/github-settings-row-layout

Conversation

@Gandy2025

@Gandy2025 Gandy2025 commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Rework Settings → Integrations → GitHub around a single row shape — leading glyph, what the setting is, what it currently does, and the control that changes it right-aligned — instead of three differently-shaped stacks of label / description / full-width control. Adds a shared SettingRow primitive, moves the connection summary and the GitHub Task Agent picker onto it, and adds a Repositories hand-off.
  • The page is scanned, not read. Keeping "what it is" on the left and "change it" in one right-hand column lets the eye run straight down the controls, and stops a narrow Select from stretching the full page width. Connecting GitHub is also the moment people go looking for "where do I add my repos" — the catalog is provider-neutral and lives on Settings → Repositories, which this page previously offered no route to, so it dead-ended.
  • Connection details then grew past presentation: it now answers whether the installation can actually do the thing the reader is waiting on, deriving its permission readout from the same set the server's task-reply gate enforces (GITHUB_TASK_REPLY_REQUIRED_PERMISSIONS in @first-tree/shared), so the Settings readout and the assignment gate cannot disagree.

Reference for the visual direction: the Multica GitHub settings page (glyph + name + effect on the left, control on the right, one labelled section per concern).

SettingRow deliberately draws no border, background, or radius of its own. The enclosing Section keeps owning the single rule above the block, so this does not reintroduce the double framing #2197 removed — card chrome stays reserved for genuinely selectable surfaces per DESIGN.md Pillar 5. The one borrowed piece of chrome is the small sunken tile behind each row's glyph.

Visual review: /preview/settings-github (DEV-only) mirrors all six states.

Diagnostics: each claim scoped to what it proves

Review found three readouts asserting more than their data supports; all three are fixed here, and each has a regression test:

  • "Required for automatic replies", not "Required by First Tree". issues: write + pull_requests: write gate the GitHub Task Agent's automatic replies only — Context Reviewer, repository coverage, and the capability probe each gate on their own permissions and events. The constant is named GITHUB_TASK_REPLY_REQUIRED_PERMISSIONS to match.
  • Lifecycle events are not "unused". installation / installation_repositories are consumed by handleInstallationLifecycle before buildRule is reached, and are what keeps the installation row and its repository coverage current. They now have their own class ("Kept in sync from"), so a normally configured installation is no longer reported as carrying dead subscriptions.
  • "First seen", not "Connected". github_app_installations.created_at is written by the installation.created webhook while the row is still unbound, and survives a disconnect/rebind — it is not the moment this team connected. This block exists to reconstruct a timeline, which is where a wrong date does the most damage.

Validation

  • pnpm check
  • pnpm typecheck
  • pnpm test@first-tree/shared (74 files / 864 tests), @first-tree/web (248 files / 2284 tests), and @first-tree/server (266 files / 3124 tests) are all green locally, which covers every package this PR touches. Repo-wide pnpm test also has failures in apps/cli and packages/skill-evals; I reproduced both unchanged on the base commit (54bcb109c) in a clean worktree, so they are pre-existing and unrelated.
  • /preview/settings-github re-checked after the diagnostics fixes — the gallery fixture now subscribes to the lifecycle events a real App carries, so all three event classes (activity / lifecycle / dropped) are reviewable.

Change surface: packages/web/src, packages/shared/src (the task-reply permission set + githubPermissionSatisfies), packages/server/src (team-agent-settings.ts reads the shared set), and packages/qa/cases/. No database or migration change.

Change Surface

  • apps/cli public CLI or help output
  • tree onboarding / binding / inspection behavior
  • shipped or planned skill topology
  • docs or contributor-facing repository metadata
  • CI / packaging / release plumbing

Notes

  • package or install behavior changes: none.
  • docs or tests updated to match: new SettingRow DOM test; the GitHub page test covers the Repositories hand-off and its position; the installation-panel test asserts the connection row's name; github-connection-details-dom.test.tsx pins the scoped requirement heading, the lifecycle-vs-unused split, and the First seen label; github-task-reply-required-permissions.test.ts pins the shared set and the exact-match comparison. The /preview/settings-github gallery is updated so it keeps mirroring the shipped layout.
  • QA risk: this stopped being presentation-only once the connection readout started deriving from the same requirement set as the server's task-agent gate. packages/qa/cases/cross-surface/github-settings-connection-panel.md is updated in this PR: its Web-UI expectations now cover the new section headings, the satisfied/blocked permission states, the lifecycle-event classification, the First seen label, and the requirement that the readout and the server gate never disagree.
  • follow-up work: SettingRow is reusable — Settings → GitLab is the obvious next adopter, and a SettingRowList (hairline between consecutive rows) is worth adding when a section first needs more than one row. Also happy to drop the glyph tile if it reads as too much chrome for this system; it is one prop.

Known risks, deliberately not addressed here

  • ACTIVITY_EVENT_LABELS mirrors buildRule in github-normalize.ts with nothing binding them. Add an event there and this page will label it "Subscribed but unused" — wrong in a way that reads authoritative. The same drift is possible for LIFECYCLE_EVENT_LABELS against the webhook route's lifecycle branch. Fixing it properly means moving both lists to @first-tree/shared plus a server test asserting the handlers cover exactly those sets; that is its own change.
  • -ml-3 on the disclosure encodes Button size="sm"'s px-3. If that padding changes, the chevron silently drifts off the row's text column. There is no shared token for "cancel a button's own inset".
  • Settings → GitLab still uses the old stacked layout, so the two provider tabs read differently until it adopts SettingRow.
  • githubPermissionSatisfies is an exact match on purpose. GitHub's write does imply read, so a ranked comparison is arguably more correct — but it has to change all ten call sites at once (github-audience, both publishers, setup-capabilities, org-settings, context-reviewer-common) or the UI and the routing/publish paths disagree again.

🤖 Generated with Claude Code

Rework the GitHub integration page around a single row shape — leading
glyph, what the setting is, what it currently does, and the control that
changes it right-aligned — instead of three differently-shaped stacks of
label / description / full-width control.

- add a shared `SettingRow` primitive for Settings sections. It draws no
  border, background, or radius of its own: the enclosing `Section` keeps
  owning the one rule above the block, so this does not reintroduce the
  double framing #2197 removed.
- connection summary reads as one row ("GitHub App" → "Connected to
  github.com/<login>") with Manage connection / Manage on GitHub on the
  right; the unconnected state uses the same shape, so connecting no
  longer reflows the section.
- GitHub Task Agent moves its picker to the right at a readable minimum
  width instead of stretching the full page; blockers, the Context
  Reviewer note, and errors stay full-width under the row.
- add a Repositories hand-off. Connecting GitHub is when people look for
  "where do I add my repos", but the catalog is provider-neutral and
  lives on Settings → Repositories, which this page previously offered no
  route to.
- mirror all of it in the DEV `/preview/settings-github` gallery.

Co-authored-by: multica-agent <github@multica.ai>

@yuezengwu yuezengwu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

审查结论:通过,未发现 blocker。

这个 PR 的目标是把 Settings → Integrations → GitHub 收敛为可扫描的统一行布局。核心改动包括新增无卡片边框的 SettingRow 基础组件,将 GitHub App 连接摘要和 GitHub Task Agent 选择器迁移到同一左右分栏形态,并在页面末尾增加到 provider-neutral Settings → Repositories 仓库目录的明确入口。

逐文件检查后确认:成员/管理员的读写权限边界未改变;连接详情、挂起提示、连接面板和 Agent 分配/错误状态仍保持原有语义;Repositories 链接指向现有可聚焦的 #code-repositories 目标;新增 DOM 测试覆盖了行组件结构和页面交接位置。没有核心数据结构或数据库变更。

按本次 review 范围仅检查 diff,未额外运行测试或 QA。

@baixiaohang baixiaohang left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Recommendation: approve

  • Rationale: The row abstraction is backed by three concrete settings uses, preserves the existing authorization and state behavior, and adds the repository hand-off at the correct provider-neutral destination.

Risk level: C

  • Path baseline: all changes are under packages/web/** -> C.
  • Semantic lift: none; this does not change subscriptions, WebSocket parsing, or the data layer.

PR summary

  • Author / repo: Gandy2025 / agent-team-foundation/first-tree
  • Problem: Team members and admins currently have to scan three differently shaped GitHub settings blocks, and the page gives no direct route to the provider-neutral repository catalog.
  • Approach: Introduce one borderless SettingRow shape, use it for connection status, GitHub Task Agent assignment, and the Repositories hand-off, while keeping detailed and error content below the primary row.
  • Impacted modules: Web UI primitives, GitHub installation summary, GitHub Task Agent controls, GitHub settings page, and the DEV preview.

Review findings

  • ✅ The production paths preserve member/admin action boundaries, existing query and mutation behavior, the suspended banner, connection disclosure, blockers, and inline errors.
  • ✅ The Repositories link targets the existing focusable #code-repositories destination and stays after Automatic handling, so it does not move provider-neutral configuration into the GitHub domain.
  • ✅ The shared row owns layout only and leaves the enclosing Section as the single visual frame, avoiding duplicate card chrome.
  • ⚠️ Non-blocking follow-up: the initial connection state still renders as a plain Loading… line before switching to the full row, so it does not yet have the settled row footprint.
  • ⚠️ Non-blocking follow-up: the DEV preview uses Install on GitHub for its unconnected row while the shipped summary uses Connect GitHub; keeping the visual artifact exact will make future layout review more reliable.

Action taken

  • Approved.

The disclosure transcribed GitHub's `permissions` / `events` blobs verbatim,
which left the reader to diff them against a requirement they had to already
know — and dropped the two timestamps the API was already returning.

- add `GITHUB_APP_REQUIRED_PERMISSIONS` + `githubPermissionSatisfies` to
  shared, and point the server's `taskReplyInstallationBlocker` at them, so
  "what First Tree needs" has one definition. An admin can no longer be shown
  a healthy connection while the task-reply gate refuses the assignment.
- rewrite the disclosure as `github-connection-details.tsx`: required scopes
  named in prose and marked ready/blocked, other grants demoted to a secondary
  line, consumed events named with ignored subscriptions called out, and an
  Installation block with a copyable id plus `createdAt` / `updatedAt`.
- a shortfall says what it costs ("Agents can't post replies on pull
  requests") and offers Grant on GitHub — admins only, since a member has no
  standing to act on it. It also marks the collapsed toggle, so the one state
  worth acting on isn't hidden behind a click.
- blocked, not needs-you: the fix lives on GitHub, and a First Tree admin role
  alone doesn't establish they can grant it there (DESIGN.md §3).
- the DEV gallery now renders the real component instead of a copy, and gains
  the two shortfall states.

Side effect of routing through `githubPermissionSatisfies`: a stronger grant
now satisfies a weaker requirement, so an `admin`-level scope no longer trips
the gate. GitHub only issues read/write for these two scopes today, so this is
correctness rather than an observable change.

@yuezengwu yuezengwu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

审查结论:需要修改 1 个 blocker。

新增提交的目标是让 Connection details 直接说明安装是否满足 First Tree 的任务回复权限,并把权限需求抽到 shared,供 Web 展示与 server 的 GitHub Task Agent gate 共用。核心改动涉及 packages/shared 的权限判定、packages/server 的候选/分配 blocker,以及新的详情组件;没有数据库变更,但已不再是原 PR 描述所称的纯 Web 展示改动。

Blocker — packages/web/src/pages/github-connection-details.tsx:220

不要把 data.createdAt 标为 Connected。该字段来自 github_app_installations.created_atinstallation.created webhook 会先创建一条未绑定记录,团队可能晚些时候才从 connect panel 绑定;断开后重新绑定也会保留原 createdAt。因此这里会把“First Tree 记录到该 installation 的时间”错误展示为“连接到当前团队的时间”,而本组件正被设计为用于重建排障时间线。请改成不暗示绑定时刻的标签(例如 Recorded / Installation recorded);如果确实需要连接时间,则必须引入并返回真实的 bind timestamp,而不能复用 createdAt

另外请同步更新 PR 描述与验证范围:最新 head 已修改 packages/shared/**packages/server/**,并改变 GitHub Task Agent 的权限 gate,当前 “This change touches only packages/web/src” 已不准确。

按 review 约定仅检查增量 diff,未额外运行测试或 QA。

@baixiaohang baixiaohang left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Recommendation: request changes

  • Rationale: The synchronized commit makes the connection diagnostics more useful, but it introduces contradictory permission semantics and two user-visible facts that do not match the server lifecycle.

Risk level: B-low

  • Path baseline: the PR now includes packages/server/**; there is no DB-structure change and the changed service is outside the B-high path list.
  • Semantic lift: none beyond the server-service baseline.

PR summary

  • Author / repo: Gandy2025 / agent-team-foundation/first-tree
  • Problem: In addition to the scannable GitHub settings layout, admins need Connection details to explain whether the installation can support automatic Issue and pull-request replies.
  • Approach: Add a shared permission requirement/helper, reuse it in the assignment gate and Web readout, humanize permissions and events, and render the real details component in the DEV gallery.
  • Impacted modules: shared GitHub schemas, server GitHub Task Agent gating, GitHub settings diagnostics, and preview/tests.

Review findings

  • ❌ 1. The new permission ordering is not applied to every task-reply reader. githubPermissionSatisfies("admin", "write") now makes the Settings readout and taskReplyInstallationBlocker report ready, but isGithubTaskReplySupported, the stored-installation publisher guard, and the live-token publisher guard still require === "write". An admin grant therefore passes assignment and UI diagnostics, then suppresses the task or fails publication. Route all task-reply permission readers through the same helper/requirement definition, or retain exact-match semantics everywhere. [R1/R5 · packages/shared/src/schemas/github-app.ts:71, packages/server/src/services/github-audience.ts:106, packages/server/src/services/github-task-reply-publisher.ts:543]
  • ❌ 2. GITHUB_APP_REQUIRED_PERMISSIONS and the visible heading Required by First Tree overstate a task-reply-specific requirement as the universal installation contract. Other First Tree capabilities have different gates: Context Reviewer also depends on repository coverage/events, while the compatibility initializer requires administration, contents, and workflows. Scope the constant and copy to GitHub Task Agent / automatic replies instead of declaring these two permissions sufficient for First Tree as a whole. [R3/R5 · packages/shared/src/schemas/github-app.ts:54, packages/web/src/pages/github-connection-details.tsx:171]
  • ❌ 3. The event diagnostic treats every event outside github-normalize.ts as dropped and renders it under Subscribed but unused, but the installation event is handled separately to create, refresh, suspend, unsuspend, and delete the installation row. A normally configured installation will therefore be shown with a false warning. Separate lifecycle events from activity-producing events, or rename the categories so consumed lifecycle traffic is not called unused. [R5 · packages/web/src/pages/github-connection-details.tsx:32, packages/web/src/pages/github-connection-details.tsx:197, packages/server/src/api/webhooks/github-app.ts:217]
  • ❌ 4. The Installation block labels data.createdAt as Connected, but that timestamp is set when the webhook first records the installation while it is still unbound; binding may happen much later, and the row can survive disconnect/reconnect. Without a real binding timestamp, label this as Recorded / First seen rather than presenting an incorrect connection time. [R5 · packages/web/src/pages/github-connection-details.tsx:220, packages/server/src/db/schema/github-app-installations.ts:127, packages/server/src/services/github-app-installations.ts:190]

The previously reviewed row-layout changes remain sound; these findings are limited to the synchronized commit.

Action taken

  • Submitted request changes.

Anything a row rendered below its first line reset to the section's left
edge while the row's own title sat indented past the glyph tile, so an
expanded Connection details block read as a detached slab against a
second, further-left margin.

- SettingRow indents its children by the new `--setting-row-indent` (glyph
  tile + the gap after it) when the row has a glyph, so extras hang off the
  title rather than the container.
- the details disclosure cancels its button's own `px-3`, putting the
  chevron on that same column, and indents the open blocks by chevron +
  gap so they line up under the toggle's label.

Measured on /preview/settings-github: row title and chevron both land at
x=83, every detail block label at x=103, and the block's right-hand action
still ends flush with the row's own buttons at x=883.

@yuezengwu yuezengwu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

审查结论:继续 Request changes;本次增量未解决上一轮 blocker。

最新提交的目标是让 SettingRow 的 children 对齐到 glyph 后的文字列,并让 Connection details 的 disclosure 与展开内容沿同一列排布。核心改动仅涉及 SettingRow、其 DOM 测试、一个全局缩进 token,以及详情页的局部对齐;这部分 diff 本身未发现新的 blocker,也没有核心数据结构或数据库变更。

但上一轮 4 个必须修复的问题在当前 head 中仍保持原样:

  1. githubPermissionSatisfies 的层级语义仍未覆盖 audience 与 publisher 的精确 === "write" 判断。
  2. Task reply 专用权限仍被命名为通用的 GITHUB_APP_REQUIRED_PERMISSIONS / Required by First Tree
  3. installation 等由独立生命周期处理器消费的事件仍会显示为 Subscribed but unused
  4. github_app_installations.created_at 仍被错误标为团队 Connected 时间。

PR 描述中的 “This change touches only packages/web/src” 也仍与当前 shared/server 改动不符。请完成上述修正后再请求复审。

按 review 约定仅检查增量 diff,未额外运行测试或 QA。

@baixiaohang baixiaohang left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Recommendation: request changes

  • Rationale: The new alignment change is sound, but it does not address any of the four correctness blockers from the previous review.

Risk level: B-low

  • Path baseline: the PR includes packages/server/**; there is no database-structure change and the changed service is outside the B-high service set.
  • Semantic lift: none beyond the server-service baseline.

PR summary

  • Author / repo: Gandy2025 / agent-team-foundation/first-tree
  • Problem: The PR now combines a scannable GitHub settings layout with diagnostics and gating intended to tell admins whether an installation can support automatic Issue and pull-request replies.
  • Approach: The latest commit aligns SettingRow extras and expanded connection details under the row's text column; the earlier shared permission helper, server assignment gate, and Web diagnostics remain unchanged.
  • Impacted modules: shared GitHub schemas, server GitHub Task Agent gating, Web settings primitives, GitHub connection diagnostics, and previews/tests.

Review findings

  • ❌ 1. Permission semantics are still inconsistent: the Settings readout and assignment gate accept a stronger admin grant through githubPermissionSatisfies, while the audience and both publisher guards still require exact write, so a configuration reported as ready can suppress the task or fail publication. [R1/R5 · packages/server/src/services/github-audience.ts:106, packages/server/src/services/github-task-reply-publisher.ts:543]
  • ❌ 2. The task-reply-specific permission set is still named and presented as the universal First Tree requirement, although other GitHub App capabilities have different permission, event, and repository-coverage gates. [R3/R5 · packages/shared/src/schemas/github-app.ts:54, packages/web/src/pages/github-connection-details.tsx:183]
  • ❌ 3. The diagnostic still classifies lifecycle events such as installation as Subscribed but unused, even though the server consumes them to create, refresh, suspend, unsuspend, and delete installation records. [R5 · packages/web/src/pages/github-connection-details.tsx:219, packages/server/src/api/webhooks/github-app.ts:217]
  • ❌ 4. data.createdAt is still labeled Connected, although it records when the installation row was first observed and can predate team binding or survive a disconnect/rebind. [R5 · packages/web/src/pages/github-connection-details.tsx:232, packages/server/src/services/github-app-installations.ts:190]

The latest indentation delta introduces no additional blocker. The PR description and validation scope also still need to reflect the existing packages/shared/** and packages/server/** changes.

Static diff review only; I did not run tests or QA.

Action taken

  • Submitted request changes.

…rver

Review follow-ups on this branch.

`githubPermissionSatisfies` ranked levels (admin >= write), but the nine
other installation-permission checks in the server compare exactly —
`github-audience` (whether an event reaches an agent),
`github-task-reply-publisher`, `context-reviewer-publisher`,
`context-reviewer-common`, `setup-capabilities`, `org-settings`. So the
Settings readout and the task-agent gate could accept a grant that routing
and publishing then reject: the contradiction the shared definition exists
to prevent. Narrow it to an exact match; a ranked comparison is a
change-every-call-site decision, not a per-call-site one.

Also:
- the copy control dropped `useCopyFeedback`'s documented `failed` status
  and expressed state by swapping its `aria-label` under a fixed `title`.
  It now carries state in the visible label, like `inline-command.tsx` and
  `invite-link-panel.tsx`, so a non-secure-context failure isn't silent.
- the disclosure's region id was a module constant, so the DEV gallery
  rendered duplicate ids with an ambiguous `aria-controls`. Per instance
  via `useId()`.
- drop `SettingRow`'s unused `titleId` prop and a `gap-1` class that its
  own inline `gap` overrode.
- register `SettingRow` in the DESIGN.md inventory and `/preview/styleguide`,
  which is where a new `components/ui` primitive is supposed to land.
- refresh the cross-surface QA case: the page's section headings changed,
  and the permission readout now has to agree with the server gate, which
  is exactly the kind of assembled-deployment claim that case owns.

@baixiaohang baixiaohang left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Recommendation: request changes

  • Rationale: The latest commit resolves the cross-layer permission comparison, but three user-visible diagnostics still assert facts that do not match the capability and installation lifecycle they describe.

Risk level: B-low

  • Path baseline: the PR includes packages/server/**; there is no database-structure change and the changed service is outside the B-high service set.
  • Semantic lift: none beyond the server-service baseline.

PR summary

  • Author / repo: Gandy2025 / agent-team-foundation/first-tree
  • Problem: The GitHub settings page should be easier to scan while also telling team members whether the installation can support automatic Issue and pull-request replies.
  • Approach: Use a shared row layout, derive the Web permission readout and Task Agent assignment gate from one permission set, expose connection diagnostics, and hand repository configuration off to the provider-neutral catalog.
  • Impacted modules: shared GitHub installation schemas, server GitHub Task Agent gating, Web settings primitives and diagnostics, styleguide/preview surfaces, and cross-surface QA guidance.

Review findings

  • ❌ 1. The permission set and heading still present an automatic-reply requirement as the universal GitHub App contract. Context Reviewer, repository coverage, and the compatibility initializer have different gates, so GITHUB_APP_REQUIRED_PERMISSIONS / Required by First Tree can tell an admin that the installation is generally usable when only task replies have been checked. Scope the constant and copy to GitHub Task Agent / automatic replies. [R3/R5 · packages/shared/src/schemas/github-app.ts:68, packages/web/src/pages/github-connection-details.tsx:186]
  • ❌ 2. installation is still rendered under Subscribed but unused, although the webhook route consumes it through the separate lifecycle handler. This is not only a future drift risk: a normally configured installation is already shown with a false warning today. Either distinguish lifecycle events from activity-producing events or use copy that does not call handled lifecycle traffic unused. [R5 · packages/web/src/pages/github-connection-details.tsx:38, packages/web/src/pages/github-connection-details.tsx:222, packages/server/src/api/webhooks/github-app.ts:217]
  • ❌ 3. data.createdAt is still labeled Connected, although it records when First Tree first stored the installation; the row starts unbound and survives disconnect/rebind. Label it Recorded / First seen, or return a real binding timestamp if the UI needs connection time. [R5 · packages/web/src/pages/github-connection-details.tsx:235, packages/server/src/services/github-app-installations.ts:176]

The prior permission-ordering blocker is resolved: exact-match semantics now agree with the existing audience and publisher guards. The new copy feedback, per-instance disclosure id, styleguide registration, and QA-case updates introduce no additional blocker.

The PR description's validation section still says the change touches only packages/web/src; it should be updated to reflect the shared, server, QA, and design-document changes already present.

Static diff review only; I did not run tests or QA.

Action taken

  • Submitted request changes.

@yuezengwu yuezengwu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

审查结论:继续 Request changes;本次增量修复了上一轮 4 个 blocker 中的 1 个,但其余 3 个仍存在。

这次提交的目标是让 Settings 的权限 readout 与 server 的任务回复 gate 使用相同语义,并补强 Connection details 的可访问性、复制反馈、预览与 QA 说明。确认 githubPermissionSatisfies 已改为精确匹配,因此 UI / assignment gate 不再把 admin 误判为满足 publisher 只接受的 write;新增的唯一 disclosure id 与 Copy 成功/失败反馈也合理。

仍需修复:

  1. 任务回复专用权限仍被表述为整个 First Tree 的安装契约。 GITHUB_APP_REQUIRED_PERMISSIONS 的注释仍称其为 “First Tree can act as the App” 的唯一充分条件,UI 标题仍是 Required by First Tree。但这组 issues: write / pull_requests: write 实际只覆盖 GitHub Task Agent 的自动回复;其他能力还有不同权限、事件和仓库覆盖 gate。请将常量、注释和可见文案限定到 task replies / Automatic handling,例如 GITHUB_TASK_REPLY_REQUIRED_PERMISSIONSRequired for automatic replies。[packages/shared/src/schemas/github-app.ts:54, packages/web/src/pages/github-connection-details.tsx:186]

  2. 生命周期事件仍被误标为 unused。 当前把不在 CONSUMED_EVENT_LABELS 中的所有订阅事件都放进 Subscribed but unused,但 installationinstallation_repositories 会由独立 lifecycle handler 消费,用于创建、刷新、挂起/恢复、删除 installation 记录及刷新仓库覆盖。PR 描述中新列出的“未来列表漂移风险”没有解决当前已经存在的错误诊断。请把生命周期事件单独分类,或改写分类/文案,避免把已消费的生命周期流量称为 unused。[packages/web/src/pages/github-connection-details.tsx:139, packages/web/src/pages/github-connection-details.tsx:212, packages/server/src/api/webhooks/github-app.ts:217]

  3. createdAt 仍被错误标为 Connected installation row 在 installation.created webhook 到达时先以未绑定状态记录,团队可能之后才绑定;断开/重绑还会保留该行。因此这个值不是连接到当前团队的时间。没有真实 bind timestamp 时,请改为 Recorded / First seen。[packages/web/src/pages/github-connection-details.tsx:235, packages/server/src/services/github-app-installations.ts:386]

另外,PR 描述的 Validation 仍写着 “This change touches only packages/web/src”,与当前 packages/shared/**packages/server/** 和 QA case 改动不符,请同步修正。

本次增量没有核心数据结构或数据库变更;它会改变 shared 权限判断语义,并新增跨层 QA 约束。按 review 范围仅检查增量 diff,未额外运行测试或 QA。

Three of this PR's diagnostics asserted more than the data behind them
supports:

- `GITHUB_APP_REQUIRED_PERMISSIONS` / "Required by First Tree" presented
  `issues: write` + `pull_requests: write` as the whole install contract,
  but that set only gates the GitHub Task Agent's automatic replies —
  Context Reviewer, repository coverage, and the capability probe each
  gate on their own permissions and events. An admin reading a green
  checklist could conclude the installation was generally usable.
  Renamed to `GITHUB_TASK_REPLY_REQUIRED_PERMISSIONS`, with the heading
  and comments scoped to that capability.

- `installation` / `installation_repositories` were filed under
  "Subscribed but unused". The webhook route consumes both through
  `handleInstallationLifecycle` before `buildRule` is ever reached, and
  they are what keeps the installation row and its repository coverage
  current — so a normally configured installation was being reported as
  carrying dead subscriptions. They now have their own class ("Kept in
  sync from"), separate from both activity events and dropped ones.

- `createdAt` was labelled "Connected". The row is written by the
  `installation.created` webhook while still unbound and survives a
  disconnect/rebind, so it never records when this team connected.
  Labelled "First seen" — this block exists to reconstruct a timeline,
  which is exactly where a wrong date does the most damage.

The preview fixture now subscribes to the lifecycle events a real App
carries, so the gallery mirrors all three event classes.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@Gandy2025

Copy link
Copy Markdown
Contributor Author

Pushed d700e8273 — the three remaining blockers are fixed, each with a regression test.

1. Task-reply permissions no longer presented as the whole install contract. GITHUB_APP_REQUIRED_PERMISSIONSGITHUB_TASK_REPLY_REQUIRED_PERMISSIONS; the doc comment now says what it does and does not cover, and the heading is Required for automatic replies. Test: github-connection-details-dom.test.tsx → "scopes the requirement list to the capability it actually gates".

2. Lifecycle events are no longer called unused. installation / installation_repositories get their own class, rendered as Kept in sync from: … and excluded from the dropped list — they are consumed by handleInstallationLifecycle before buildRule is reached. The /preview/settings-github fixture now subscribes to both, so the gallery shows all three event classes. Test: "does not call handled lifecycle traffic unused".

3. createdAt is labelled First seen, not Connected, with the reason inline: the row is written unbound by installation.created and survives disconnect/rebind. Test asserts First seen is present and Connected is absent from the details block.

The PR description is rewritten: the change surface now names packages/shared, packages/server, and packages/qa alongside packages/web, the validation section reports the three suites actually run, and the known-risks entry covers LIFECYCLE_EVENT_LABELS drift alongside the activity list.

Verification: pnpm check and pnpm typecheck clean; @first-tree/shared 864 tests, @first-tree/web 2284 tests, @first-tree/server 3124 tests all green locally.

@baixiaohang baixiaohang left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Recommendation: approve

  • Rationale: The latest commit resolves all remaining diagnostic mismatches while preserving the previously reviewed row layout, authorization boundaries, and task-reply gating behavior.

Risk level: B-low

  • Path baseline: the PR includes packages/server/**; there is no database-structure change and the changed service is outside the B-high service set.
  • Semantic lift: none beyond the server-service baseline.

PR summary

  • Author / repo: Gandy2025 / agent-team-foundation/first-tree
  • Problem: Team members need a scannable GitHub settings page whose connection details accurately explain whether automatic Issue and pull-request replies can work, without overstating what that check proves.
  • Approach: Use a shared borderless row layout, derive the Web readout and assignment gate from the task-reply permission set, separate activity/lifecycle/unused subscriptions, label the installation's first-recorded time accurately, and hand repository management off to the provider-neutral catalog.
  • Impacted modules: shared GitHub installation schemas, server GitHub Task Agent gating, Web settings primitives and diagnostics, preview/styleguide surfaces, and cross-surface QA guidance.

Review findings

  • ✅ The permission constant, comments, UI heading, tests, and server reader are now explicitly scoped to automatic task replies; they no longer imply a universal installation readiness verdict.
  • installation and installation_repositories are classified as handled lifecycle traffic rather than unused subscriptions, while genuinely dropped events remain visible separately.
  • createdAt is now presented as First seen, matching the record-before-bind and disconnect/rebind lifecycle.
  • ✅ Exact permission comparison remains consistent across the Settings readout, assignment gate, audience, and publisher paths.
  • ⚠️ The Web event-label maps can still drift from the server handlers, but the current classification is correct and the PR records that coupling as explicit follow-up work.

Static diff review only; I did not run tests or QA.

Action taken

  • Approved.

@yuezengwu yuezengwu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

审查结论:继续 Request changes;本次增量已正确修复权限范围命名与 createdAt 文案,但生命周期事件分类仍有 1 个 blocker。

这次提交的目标是把 Connection details 的每项诊断限定到数据实际能证明的范围:将共享权限集改名为 GitHub Task Agent 自动回复专用,区分 activity / lifecycle / unused 事件,并把 installation 记录时间展示为 First seen。确认 GITHUB_TASK_REPLY_REQUIRED_PERMISSIONS 已在 shared、server gate、Web 与测试中一致落地;Required for automatic repliesFirst seen 也分别解决了上一轮对应问题,PR 描述的变更与验证范围已同步修正。

Blocker — packages/web/src/pages/github-connection-details.tsx:52 / :61 / :243

不要把 installation_repositories 展示为 Kept in sync from: Repository access changes,也不要在注释、测试和 preview 中声称它会保持 installation row 或 repository coverage 最新。当前 server 的 handleInstallationLifecycle 在看到这个 event 时直接 return "noop",既不更新 installation row,也不存储仓库覆盖;仓库目录是在读取时通过 installation token 调 GitHub 的 /installation/repositories 实时获取。因此现在只是把先前的“unused”错误改成了另一个错误事实。

请让分类与当前行为一致:例如只有 installation 使用 Kept in sync from,而 installation_repositories 归入 unused;或者使用不暗示状态更新的中性类别并明确它当前被接受但 no-op。相应调整 DOM 回归测试、preview 注释、QA case 与 PR 描述。[packages/server/src/api/webhooks/github-app.ts:97, packages/server/src/api/webhooks/github-app.ts:101, packages/server/src/api/orgs/github-app.ts:195]

本次增量没有核心数据结构或数据库变更。按 review 范围仅检查增量 diff,未额外运行测试或 QA。

@baixiaohang baixiaohang left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Recommendation: request changes

  • Rationale: The permission scope and timestamp fixes are correct, but the new lifecycle category claims that installation_repositories keeps repository access in sync even though the current server explicitly accepts that event as a no-op.

Risk level: B-low

  • Path baseline: the PR includes packages/server/**; there is no database-structure change and the changed service is outside the B-high service set.
  • Semantic lift: none beyond the server-service baseline.

PR summary

  • Author / repo: Gandy2025 / agent-team-foundation/first-tree
  • Problem: Team members need a scannable GitHub settings page whose diagnostics explain automatic-reply readiness and installation activity without asserting more than the underlying data proves.
  • Approach: Scope the shared permission set to task replies, classify webhook subscriptions for display, label the first-recorded timestamp accurately, and use the shared row layout across the settings page.
  • Impacted modules: shared GitHub installation schemas, server GitHub Task Agent gating, Web settings diagnostics and previews, and cross-surface QA guidance.

Review findings

  • ❌ 1. installation_repositories must not be displayed as Kept in sync from: Repository access changes. handleInstallationLifecycle returns "noop" for that event before reading its action or payload, and the route test pins that behavior. Repository access is instead fetched live from GitHub's /installation/repositories endpoint when the catalog is read. The current label, comments, DOM test, preview fixture explanation, QA case, and PR description therefore replace the previous “unused” error with a different false claim. Classify only installation as state-maintaining; put installation_repositories under unused, or use neutral wording that explicitly says it is accepted but currently no-op. [R5 · packages/web/src/pages/github-connection-details.tsx:49, packages/web/src/pages/github-connection-details.tsx:243, packages/server/src/api/webhooks/github-app.ts:97, packages/server/src/api/orgs/github-app.ts:195]

The renamed task-reply permission contract and First seen timestamp are correct. This review supersedes my approval on the same head after the no-op repository-event path was identified.

Static diff review only; I did not run tests or QA.

Action taken

  • Submitted request changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants