feat: list fullsend slash commands in onboarding PR body - #6591
Conversation
E2E tests did not runE2E tests run automatically for org/repo members and collaborators on pull requests. For other contributors, a maintainer must add the See E2E testing guide for details. |
1 similar comment
E2E tests did not runE2E tests run automatically for org/repo members and collaborators on pull requests. For other contributors, a maintainer must add the See E2E testing guide for details. |
PR Summary by QodoDocument Fullsend slash commands in onboarding PRs
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Code Review by Qodo
1.
|
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 2:40 PM UTC · Completed 2:52 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $2.53 |
ReviewFindingsLow
|
… body The onboarding (enroll) PR is a user's first touchpoint with fullsend, but its body never mentioned the available slash commands — leading users to guess wrong prefixes (e.g. /agentic_review instead of /fs-review). Add a "Getting started" section to ENROLL_PR_BODY that, for each primary /fs-* command: - shows the supported entity context (issue and/or PR) per ADR 0076 and the dispatch routing, and - links to the agent's reference doc in fullsend-ai/agents (the fullsend.dev docs site is access-gated and unreachable for external onboarding-PR readers). Closes fullsend-ai#2165 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Shai Revivo <srevivo@redhat.com>
80231c3 to
a26319b
Compare
waynesun09
left a comment
There was a problem hiding this comment.
[HIGH] Fix does not cover the update-path PR where the reported incident actually happened
internal/scaffold/fullsend-repo/scripts/reconcile-repos.sh, UPDATE_PR_BODY (line 56) — not on a diff line this PR touches, so noting it here.
This PR only appends the new "Getting started" section to ENROLL_PR_BODY. UPDATE_PR_BODY (used for the "shim is stale, re-sync" path, title chore: update fullsend shim workflow, on the fullsend/onboard branch) is left untouched. The incident that motivated issue #2165 was exactly this update-path PR (title chore: update fullsend shim workflow, body matching UPDATE_PR_BODY verbatim) — the repo was already enrolled when the wrong slash command was typed. Since this diff doesn't touch UPDATE_PR_BODY, that same scenario will still recur, and #2165's own validation criteria ("the next 3 onboarding PRs ... should include slash command documentation") won't be met for update-path PRs.
Suggestion: factor the "Getting started" block into a shared variable and append it to UPDATE_PR_BODY as well, or explicitly scope this PR to "enrollment-only" and open a follow-up issue for the update path so the original incident is actually prevented from recurring.
The incident behind fullsend-ai#2165 happened on a re-sync (update-path) PR, not an enrollment PR. Extract the "Getting started" catalog into a shared GETTING_STARTED_SECTION and append it to both ENROLL_PR_BODY and UPDATE_PR_BODY so already-enrolled repos get the same discoverability. Add TestReconcileReposSlashCommandCatalog, which cross-references every /fs-* command routed by dispatch.yml against the onboarding PR body so the catalog can't silently drift, plus content assertions in TestReconcileReposContent. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Shai Revivo <srevivo@redhat.com>
waynesun09
left a comment
There was a problem hiding this comment.
Review sweep at head 378363f. One inline finding on the new drift guard, plus two findings below that land on lines this diff doesn't touch, so noting them here.
[MEDIUM] Already-open onboarding PRs never get the new body — the exact repo from #2165 stays uncovered
internal/scaffold/fullsend-repo/scripts/reconcile-repos.sh, line 459 (existing-PR branch) — not on a diff line this PR touches.
Verified at head, lines 443-476: --body is only ever passed to gh pr create. In the stale-shim/update branch, when EXISTING_PR is non-empty (line 458), the script pushes the refreshed shim to the branch, logs "Updated shim on existing PR for $REPO" at line 459, and never calls gh pr edit --body. The enrollment branch behaves identically at lines 466-476 (write_shim_to_branch_from_default only, no body reconciliation).
Reconciliation is therefore convergent for shim content but not for PR body: every repo that already has an open fullsend/onboard PR when this merges keeps its old, command-less body indefinitely. That is precisely the mintmaker-renovate-image#456 situation cited in issue #2165, and it means #2165's validation criteria ("the next 3 onboarding PRs should include slash command documentation") is met only for PRs created after this lands.
This is distinct from the already-resolved thread about UPDATE_PR_BODY lacking the section (fixed in e0e72b5) — that was about the body's content; this is about the body never being re-applied to an existing PR.
Suggestion: In both existing-PR branches, reconcile the body too — e.g. gh pr edit "$EXISTING_PR" --repo "$ORG/$REPO" --body "$UPDATE_PR_BODY" (and the ENROLL_PR_BODY equivalent), guarded so it only rewrites bodies the bot owns and only when the body actually differs, to avoid notification churn. At minimum, state the limitation in the PR description so #2165's validation criteria are assessed against newly created PRs only.
[MEDIUM] Per-repo installation onboarding PR still ships no slash-command catalog
internal/repos/scaffold_metadata.go, line 28 — file not touched by this PR.
The fix covers only the org-mode onboard bot. The per-repo installation path (fullsend repos install) builds its own first-touchpoint PR body from defaultScaffoldPRBody (line 28), still just "This PR adds the fullsend scaffold files for per-repo installation.\n\nMerge this PR to activate fullsend workflows." — freshInstallMetadata() consumes it unchanged. Meanwhile reconcile-repos.sh explicitly skips those repos (check_per_repo_guard "$REPO" "enrollment" at line 411, continue at 413), so per-repo installs get slash-command discovery from neither path.
Issue #2165 scoped the ask to the onboard bot's PR body, so this is a completeness gap rather than a scope violation — but the incident class ("user does not know which commands exist") is identical, and this PR's review history already accepted that finding shape: the resolved UPDATE_PR_BODY thread was exactly "the fix misses a population with the same incident class".
Suggestion: Either add the same Getting started block to the per-repo PR body — ideally sourced from one shared definition so the two catalogs cannot diverge — or state in the PR description that per-repo installs are deliberately out of scope and open a follow-up issue.
|
Links should point to the live docs. |
…drift guard Address review feedback on fullsend-ai#6591: - Links in the onboarding "Getting started" catalog now point to the live docs site (https://fullsend.sh/docs/agents/<name>) instead of raw GitHub blob source, matching the existing convention (internal/config/config.go). - Strengthen TestReconcileReposSlashCommandCatalog so it can actually detect the drift it guards against: * broaden the command pattern to /fs-[a-z0-9-]+ (no hyphen truncation) and match the /fullsend alias explicitly * scope catalog assertions to the extracted GETTING_STARTED_SECTION block instead of the whole 600-line script * add a reverse check: every cataloged command must be routed by dispatch.yml * record /fullsend as a deliberate catalog omission via an allow-list Verified both forward and reverse guards fire via mutation tests; bash -n and the full internal/scaffold suite pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Shai Revivo <srevivo@redhat.com>
Done, fixed. |
waynesun09
left a comment
There was a problem hiding this comment.
Review sweep at head 33faae6 (guard-hardening + docs-links commits). Two findings, both inline.
…guard Address review feedback on fullsend-ai#6591: - Add the "Getting started" slash-command catalog to the per-repo fresh-install PR body (defaultScaffoldPRBody). The per-repo install path is the supported model (ADR 0044), but only the per-org onboard bot carried the catalog. Add TestPerRepoOnboardingCatalog, the per-repo analogue of the scaffold-package guard, pinning both catalogs to dispatch.yml so the two onboarding surfaces cannot diverge. - Fix TestReconcileReposSlashCommandCatalog's reverse check, which extracted a spurious /fullsend from the docs URL hosts (https://fullsend.sh) rather than from a documented command. Scope catalog extraction to the rendered backtick-wrapped bullet form, compare exact tokens via sets instead of substring containment, and apply the /fullsend allow-list symmetrically in both directions. Verified both forward and reverse guards fire via mutation tests for both the per-org and per-repo catalogs; go build, go vet, and the full internal/scaffold and internal/repos suites pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Shai Revivo <srevivo@redhat.com>
Address a second-pass review of the slash-command drift guards. The guards' comments claimed stronger invariants than the code enforced: - Scope dispatch route extraction to `case`-arm labels (dispatchCaseArmRE) instead of scanning the whole file, so a /fs-* token in a comment, URL, or unrelated shell statement can no longer count as a routed command. - Anchor catalog extraction to the rendered "- `cmd`" bullet form so a command name in a docs URL or prose is not counted as documented. - Apply the /fullsend omission allow-list to the forward check only. A command written into a catalog and later dropped from dispatch must fail the reverse check even if it is allow-listed. - Add TestOnboardingCatalogsMatch pinning the per-org (reconcile-repos.sh) and per-repo (gettingStartedCatalog) catalogs to each other, so the two onboarding surfaces cannot diverge in commands, target hints, or descriptions. Verified: both drift guards fire on routed-but-undocumented, documented-but-unrouted, and URL-only-mention mutations; the cross-catalog guard fires on a diverged description; a /fs-* token in a dispatch comment is correctly ignored. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Shai Revivo <srevivo@redhat.com>
What
Adds a Getting started section to the onboarding (enroll) PR body created by the onboard bot on
fullsend/onboardbranches, listing the primary/fs-*slash commands with one-line descriptions.Why
The onboarding PR is a user's first touchpoint with fullsend, but its body never mentioned the available commands. On mintmaker-renovate-image#456 a user typed
/agentic_review(a Qodo command) instead of/fs-review— a discoverability problem. Introducing the commands right in the onboarding PR body addresses this.Change
internal/scaffold/fullsend-repo/scripts/reconcile-repos.sh— extendENROLL_PR_BODYwith:/fs-triage— triage an issue (categorize, label, and assess it)/fs-code— ask the agent to implement a fix for an issue and open a PR/fs-review— request an agent review of a pull request/fs-fix— have the agent address review feedback on a pull request/fs-retro— run a retrospective to capture improvement ideas/fs-prioritize— have the agent prioritize an issueCommand names and their issue-vs-PR semantics were taken from the authoritative dispatcher (
dispatch.ymlissue_commentrouting).Verification
bash -n+ shellcheck (pre-commit) pass; sourced the variable to confirm backticks render as inline code.go test ./internal/scaffold/passes.Closes #2165
🤖 Generated with Claude Code