feat: value-first Connect-GitHub guidance when the GitHub App is missing

[Gandy2025]

What & why

When github follow cannot start live updates because the First Tree GitHub App is unavailable or does not cover the PR/issue's repository, agents currently tend to report a technical failure and a setup chore. This PR makes that moment value-first: explain what connecting GitHub adds, keep the PR/issue usable either way, and give only an actionable CTA to a confirmed team admin.

Change

• Update the universal githubAttentionBlock and the onboarding first-tree-welcome launcher guidance together so they cannot disagree on role handling.
• Apply the same value-first handling both when no usable App installation exists and when a selected-repositories installation does not cover the owning repository.
• Promise only events First Tree currently delivers:
  • PRs: comments, review activity, new commits, and merge/close state.
  • Issues: comments and open/close state.
• Show a single Connect GitHub CTA only to a confirmed org admin. Members and role-unknown users are told that a team admin can enable live updates, without being routed into an admin-only surface.
• Build the CTA destination from the complete Server: URL returned by first-tree agent status: trim a trailing slash, append /settings/github, and preserve the existing http:// or https:// scheme. If the web origin cannot be resolved, fall back to the words Settings → GitHub instead of emitting a broken link.
• Explicitly prevent user-facing mechanics jargon such as “webhook”, “wire”, “re-follow”, “install the App”, and “action needed”.
• Keep skills/first-tree-welcome/SKILL.md and VERSION aligned at 1.2.2.

Review fixes

Addresses all blockers raised by @baixiaohang and @yuezengwu:

1. The universal briefing now has the same confirmed-admin gate as the welcome skill.
2. URL construction appends to the full Server: URL without placeholder substitution or forced HTTPS.
3. The copy no longer promises CI/check results and distinguishes PR capabilities from issue capabilities.
4. Welcome skill version metadata is consistent.
5. Missing repository coverage now triggers the same universal guidance as a missing installation, matching the CLI's shared 422/NO_APP_INSTALLATION contract.

Verification

• pnpm --filter @first-tree/client test — 1507 passed, 3 skipped.
• Focused briefing regression — 26 passed, including the ≤550-line budget.
• pnpm typecheck — 10/10 tasks passed.
• pnpm check — passed (only pre-existing non-blocking diagnostics).
• Welcome floor/grader + shipped-skill inventory — 43 passed.
• pnpm validate:skill — all 5 shipped skills valid.

Out of scope

• A ?connect deep link that opens the connect panel directly.
• A product-rendered Connect card/button in chat.
• Adjacent Settings/onboarding-page copy outside these two agent instruction surfaces.

[baixiaohang]

Review by baixiaohang-code-reviewer - PR #1767

Recommendation: request changes

• Rationale: The value-first direction is sound, but the two instruction surfaces currently disagree on who may receive the Connect CTA, and the URL-construction wording can still produce a broken link.

Risk level: A

• Path baseline: includes packages/client/** -> A
• Semantic lift: none

PR summary

• Author / repo: Gandy2025 / agent-team-foundation/first-tree
• Problem: After an agent creates a PR or issue, missing GitHub integration coverage is currently presented as a technical failure and chore instead of explaining the live-update value.
• Approach: Reframe the follow failure around checks, reviews, comments, and merge status; add a clickable Settings CTA; align the onboarding launcher copy; and ban leaked implementation jargon.
• Impacted modules: generated agent briefing, first-tree-welcome skill, briefing regression test

Review findings

❌ 1. Keep the admin gate consistent across both instruction sources. The universal briefing unconditionally tells every agent to offer the Settings link after this failure, while the welcome skill correctly limits the link to a confirmed admin and tells members/role-unknown users only that a team admin can enable the capability. A spawned value-task follows the universal rule before the launcher can apply its guard, so non-admins can still be sent to a CTA they cannot act on—the exact behavior the PR says it removed. Please give the briefing the same confirmed-admin/member-or-unknown split (or centralize the rule so the surfaces cannot diverge). [R1/R5 / packages/client/src/runtime/agent-briefing.ts:504]

❌ 2. Derive the URL from the Server: value without placeholder substitution. first-tree agent status prints a complete URL (for example, https://first-tree.example), but the new instruction says to take that value as <your-host> inside https://<your-host>/settings/github; a literal substitution can yield https://https://first-tree.example/settings/github, and it also discards an http:// scheme used by self-hosted/local deployments. Please instruct the agent to trim a trailing slash from the full Server URL and append /settings/github (as the sibling seed skill already does), with the existing words-only fallback when the web origin cannot be resolved. [R4 / skills/first-tree-welcome/SKILL.md:407]

Action taken

• Submitted request changes.

[yuezengwu]

Requesting changes.

The goal is sound: this PR rewrites the missing-GitHub-App follow guidance in the universal agent briefing and first-tree-welcome so agents lead with the value of live PR/issue updates, offer a Settings CTA, and avoid leaking implementation jargon. It also adds a briefing regression test and bumps the welcome skill's install version. There are no core data-structure or database changes.

I independently confirm the two blockers already raised by baixiaohang, and found two additional correctness issues:

1. Blocker — preserve the admin gate in the universal briefing. packages/client/src/runtime/agent-briefing.ts:504 tells every agent to offer the Settings link, while the immediately preceding working-posture rule and first-tree-welcome correctly say members/role-unknown users must not be routed into admin-only setup. A spawned task follows the universal block before the launcher can repair the message, so this still sends non-admins to a CTA they cannot act on. Mirror the confirmed-admin vs member/unknown split in both surfaces (or centralize the wording).

2. Blocker — append to the full Server: URL; do not substitute it into a host placeholder. first-tree agent status prints a complete URL, including its scheme (apps/cli/src/commands/agent/status.ts:81). Substituting that into https://<your-host>/settings/github can produce https://https://… and breaks valid http:// self-hosted/local deployments. Tell the agent to trim a trailing slash from the full Server URL and append /settings/github, and use the words-only Settings → GitHub fallback when the web origin cannot be resolved. Apply this to the briefing, welcome skill, and regression assertion.

3. Blocker — do not promise check results that First Tree does not deliver. The new value sentence says “checks” will land in chat, but normalizeGithubEvent has no check_run, check_suite, status, or workflow_run path; packages/server/src/__tests__/github-normalize.test.ts:812 explicitly locks workflow_run to null. The GitHub App event contract also omits check events. The universal sentence is additionally used for issues, which do not have reviews or merge status. Use entity-appropriate capabilities the current pipeline actually delivers (for example comments, review activity/new commits, and lifecycle state), or add check delivery as a separately tested product change.

4. Version metadata is inconsistent. skills/first-tree-welcome/VERSION is bumped to 1.2.2, but skills/first-tree-welcome/SKILL.md:3 still declares version: 1.2.1. Prior welcome-skill bumps update both; please keep them aligned.

Local verification on commit 35378665: the focused agent-briefing Vitest file passes (26/26), the quick skill validator passes, and Biome passes for the changed TypeScript files. Those checks are green, but the new string test currently locks the ambiguous placeholder rather than the safe URL derivation/role behavior above.

[Gandy2025]

Addressed all four review blockers in 60ef005d:

• mirrored the confirmed-admin vs member/unknown gate in the universal briefing;
• derive the CTA by trimming the complete Server: URL and appending /settings/github, preserving its scheme and falling back to words-only Settings → GitHub;
• replaced unsupported CI/check promises with entity-appropriate events that the current normalizer delivers;
• aligned the welcome skill frontmatter and VERSION at 1.2.2.

I also strengthened the deterministic regression to lock the role split, PR/issue capability split, safe URL derivation, fallback, and jargon ban. Verification: client 1507 passed; typecheck 10/10; Biome passed; welcome/inventory 43 passed; all shipped-skill validators passed.

@baixiaohang @yuezengwu please re-review the latest commit when ready.

[baixiaohang]

Review by baixiaohang-code-reviewer - PR #1767 (latest head 60ef005d)

Recommendation: request changes

• Rationale: The four previously reported blockers are fixed, but the universal rule still misses another failure represented by the same follow error contract, so the two steering surfaces can still diverge.

Risk level: A

• Path baseline: includes packages/client/** -> A
• Semantic lift: none

PR summary

• Author / repo: Gandy2025 / agent-team-foundation/first-tree
• Problem: Agents should present unavailable GitHub live updates as an optional, value-first upgrade instead of exposing setup mechanics after creating a PR or issue.
• Approach: Split the delivered capabilities by PR vs issue, gate the CTA to confirmed admins, derive a safe Settings URL from the full Server origin, and keep the universal briefing aligned with the onboarding launcher.
• Impacted modules: generated agent briefing, first-tree-welcome skill, briefing regression test

Review findings

❌ 1. Apply the value-first rule to repository-coverage failures too. The follow service deliberately maps both “no App installation” and “the installation does not cover this repo” to HTTP 422, and the CLI collapses every 422 into NO_APP_INSTALLATION. The welcome skill correctly triggers when the App “is not installed on, or does not cover” the owning account/repo, but the universal briefing still triggers only when the org “has not installed” the App. In an ordinary task using a selected-repositories installation that excludes the new PR’s repo, the agent can therefore fall back to the raw mechanics-heavy CLI error instead of the new value-first/admin-gated guidance. This also contradicts the PR’s claim that the two instruction surfaces can no longer disagree. Please use the same usable-coverage condition in the universal block (for example, App unavailable or not covering the owning repo/account). [R1/R5 / packages/client/src/runtime/agent-briefing.ts:504]

Resolved from the prior round

• ✅ Confirmed-admin vs member/unknown split is aligned.
• ✅ URL derivation appends to the complete Server URL and preserves its scheme.
• ✅ PR/issue capabilities now match current delivery and projection behavior.
• ✅ Welcome skill metadata is aligned at 1.2.2.

Action taken

• Submitted request changes on 60ef005d.

[yuezengwu]

Approved on 60ef005d.

The PR keeps the intended value-first change across both instruction surfaces: the universal briefing and first-tree-welcome now explain the live-update benefit without mechanics jargon, while preserving the existing explicit-follow contract. There are no core data-structure or database changes.

All four requested fixes are present:

• the universal briefing mirrors the confirmed-admin vs member/unknown gate;
• the CTA appends /settings/github to the complete Server: URL, preserves its scheme, and falls back to words-only Settings → GitHub;
• PR and issue copy now names only entity-appropriate activity the current pipeline exposes, with no CI/check promise;
• welcome skill frontmatter and VERSION both read 1.2.2.

The regression now scopes assertions to the GitHub Attention block and locks the role split, PR/issue capability split, safe URL derivation/fallback, and jargon ban. Local re-verification: focused agent-briefing Vitest 26/26, client typecheck, quick skill validation, Biome, and git diff --check all pass. No additional findings.

[yuezengwu]

Superseding my approval after a concurrent review surfaced one remaining blocker that I then verified in the code path.

Blocker — apply the universal value-first rule to repository-coverage failures as well as a missing installation. declareEntityFollow throws the same UnprocessableError for both cases: no usable App installation and repo-not-accessible when a selected-repositories installation cannot see the owning repo (packages/server/src/services/github-entity-follow.ts:374-413). The CLI then maps every HTTP 422 to NO_APP_INSTALLATION (apps/cli/src/commands/github/_shared.ts:38-45).

The welcome skill correctly handles “not installed on, or does not cover” the repo/account, but githubAttentionBlock still triggers only when the org “has not installed” the App. In an ordinary task whose new PR lives outside the installation's selected repository set, the agent can therefore receive the same mechanics-heavy CLI error without the new value-first/admin-gated handling. This leaves the two instruction surfaces divergent.

Please broaden the universal condition to the same usable-coverage condition as the welcome skill (App unavailable or not covering the owning repo/account), and lock that condition in the focused regression. The four previously requested fixes remain resolved; no other findings.

[Gandy2025]

Addressed the remaining repository-coverage blocker in 6bfcd4c2.

The universal condition now matches the welcome skill and the CLI's shared 422/NO_APP_INSTALLATION contract: it applies when the App is not installed or when the installation does not cover the GitHub account/repo that owns the PR/issue. The focused regression explicitly locks both sides of that condition.

Latest verification: focused briefing 26/26, full client 1507 passed, client typecheck passed, Biome passed, and git diff --check passed. All previous review fixes remain intact.

@baixiaohang @yuezengwu please re-review the latest commit when ready.

[yuezengwu]

Approved on latest head 6bfcd4c2.

The remaining coverage-condition blocker is resolved: the universal githubAttentionBlock now applies both when no usable GitHub App installation exists and when the installation does not cover the GitHub account/repository that owns the PR or issue. This matches the welcome skill and the server/CLI's shared HTTP 422 → NO_APP_INSTALLATION contract. The focused regression explicitly locks both halves of the condition.

All prior fixes remain intact: confirmed-admin CTA gating, safe full-Server-URL derivation and words-only fallback, truthful entity-specific activity copy, jargon ban, and aligned welcome skill version 1.2.2. There are no core data-structure or database changes.

Local final verification: focused agent-briefing Vitest 26/26, client typecheck, Biome, quick skill validation, and git diff --check all pass. No remaining findings.

[baixiaohang]

Review by baixiaohang-code-reviewer - PR #1767

Recommendation: approve

• Rationale: The latest head resolves every reported correctness gap while preserving the intended value-first, role-appropriate GitHub guidance across both agent instruction surfaces.

Risk level: A

• Path baseline: includes packages/client/** -> A
• Semantic lift: none

PR summary

• Author / repo: Gandy2025 / agent-team-foundation/first-tree
• Problem: When live GitHub updates are unavailable after an agent creates a PR or issue, users should see the optional collaboration value instead of internal failure mechanics or an unactionable admin setup chore.
• Approach: Keep the PR/issue usable, describe only entity-appropriate activity, gate the Connect CTA to confirmed admins, derive its URL safely from the complete Server origin, and apply the same guidance to missing-installation and missing-repository-coverage failures.
• Impacted modules: generated agent briefing, first-tree-welcome skill, focused briefing regression coverage

Review findings

• ✅ The universal briefing and welcome skill now agree on installation/coverage conditions, role gating, URL derivation and fallback, truthful PR/issue capabilities, and user-facing terminology.
• ✅ Skill version metadata is aligned at 1.2.2, and the focused regression locks the corrected contract.
• ✅ Latest-head CI is fully green; no remaining findings.

Action taken

• Approved latest head 6bfcd4c2 after explicit authorization.