From a26319b5c51c4808bfc938b07773869b2067f5cb Mon Sep 17 00:00:00 2001 From: Shai Revivo Date: Tue, 25 Aug 2026 19:03:12 +0300 Subject: [PATCH 1/5] feat(#2165): list fullsend slash commands in onboarding PR body MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #2165 Co-Authored-By: Claude Opus 4.8 Signed-off-by: Shai Revivo --- .../fullsend-repo/scripts/reconcile-repos.sh | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/internal/scaffold/fullsend-repo/scripts/reconcile-repos.sh b/internal/scaffold/fullsend-repo/scripts/reconcile-repos.sh index 280c9ef2e6..03e8d6c0ab 100755 --- a/internal/scaffold/fullsend-repo/scripts/reconcile-repos.sh +++ b/internal/scaffold/fullsend-repo/scripts/reconcile-repos.sh @@ -38,7 +38,18 @@ UPDATE_PR_TITLE="chore: update fullsend shim workflow" ENROLL_PR_BODY="This PR adds a shim workflow that routes repository events to the fullsend agent dispatch workflow in the \`.fullsend\` config repo. -Once merged, issues, PRs, and comments in this repo will be handled by the fullsend agent pipeline." +Once merged, issues, PRs, and comments in this repo will be handled by the fullsend agent pipeline. + +## Getting started + +Once this PR is merged, interact with fullsend by commenting one of these slash commands. The supported target (issue and/or pull request) is shown for each: + +- \`/fs-triage\` (issue or PR) — Invoke the [triage](https://github.com/fullsend-ai/agents/blob/main/docs/triage.md) agent to categorize, label, and assess an issue. +- \`/fs-code\` (issue only) — Invoke the [code](https://github.com/fullsend-ai/agents/blob/main/docs/code.md) agent to implement a fix for an issue and open a PR. +- \`/fs-review\` (PR only) — Invoke the [review](https://github.com/fullsend-ai/agents/blob/main/docs/review.md) agent to review a pull request. +- \`/fs-fix\` (PR only) — Invoke the [fix](https://github.com/fullsend-ai/agents/blob/main/docs/fix.md) agent to address review feedback on a pull request. +- \`/fs-retro\` (issue or PR) — Invoke the [retro](https://github.com/fullsend-ai/agents/blob/main/docs/retro.md) agent to analyze completed work and propose improvements. +- \`/fs-prioritize\` (issue or PR) — Invoke the [prioritize](https://github.com/fullsend-ai/agents/blob/main/docs/prioritize.md) agent to score an issue for project board ranking." UNENROLL_PR_BODY="This PR removes the fullsend shim workflow. The repo has been set to \`enabled: false\` in the fullsend config. Once merged, this repo will no longer dispatch events to the fullsend agent pipeline." From e0e72b5bc91324f9849eef606fb4112b3fc1c166 Mon Sep 17 00:00:00 2001 From: Shai Revivo Date: Wed, 26 Aug 2026 09:13:22 +0300 Subject: [PATCH 2/5] fix(#2165): document slash commands in update-path PR too The incident behind #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 Signed-off-by: Shai Revivo --- .../fullsend-repo/scripts/reconcile-repos.sh | 19 ++++++--- internal/scaffold/scaffold_test.go | 40 +++++++++++++++++++ 2 files changed, 53 insertions(+), 6 deletions(-) diff --git a/internal/scaffold/fullsend-repo/scripts/reconcile-repos.sh b/internal/scaffold/fullsend-repo/scripts/reconcile-repos.sh index 03e8d6c0ab..81a4e3f6d6 100755 --- a/internal/scaffold/fullsend-repo/scripts/reconcile-repos.sh +++ b/internal/scaffold/fullsend-repo/scripts/reconcile-repos.sh @@ -36,11 +36,10 @@ ENROLL_PR_TITLE="chore: connect to fullsend agent pipeline" UNENROLL_PR_TITLE="chore: disconnect from fullsend agent pipeline" UPDATE_PR_TITLE="chore: update fullsend shim workflow" -ENROLL_PR_BODY="This PR adds a shim workflow that routes repository events to the fullsend agent dispatch workflow in the \`.fullsend\` config repo. - -Once merged, issues, PRs, and comments in this repo will be handled by the fullsend agent pipeline. - -## Getting started +# Shared "Getting started" block appended to both enrollment and update PRs. +# The update path is the first touchpoint for already-enrolled repos (see #2165), +# so it must document the slash commands too. +GETTING_STARTED_SECTION="## Getting started Once this PR is merged, interact with fullsend by commenting one of these slash commands. The supported target (issue and/or pull request) is shown for each: @@ -50,12 +49,20 @@ Once this PR is merged, interact with fullsend by commenting one of these slash - \`/fs-fix\` (PR only) — Invoke the [fix](https://github.com/fullsend-ai/agents/blob/main/docs/fix.md) agent to address review feedback on a pull request. - \`/fs-retro\` (issue or PR) — Invoke the [retro](https://github.com/fullsend-ai/agents/blob/main/docs/retro.md) agent to analyze completed work and propose improvements. - \`/fs-prioritize\` (issue or PR) — Invoke the [prioritize](https://github.com/fullsend-ai/agents/blob/main/docs/prioritize.md) agent to score an issue for project board ranking." + +ENROLL_PR_BODY="This PR adds a shim workflow that routes repository events to the fullsend agent dispatch workflow in the \`.fullsend\` config repo. + +Once merged, issues, PRs, and comments in this repo will be handled by the fullsend agent pipeline. + +${GETTING_STARTED_SECTION}" UNENROLL_PR_BODY="This PR removes the fullsend shim workflow. The repo has been set to \`enabled: false\` in the fullsend config. Once merged, this repo will no longer dispatch events to the fullsend agent pipeline." UPDATE_PR_BODY="This PR updates the fullsend shim workflow to match the current template in the \`.fullsend\` config repo. -The shim content has drifted from the template — this brings it back in sync." +The shim content has drifted from the template — this brings it back in sync. + +${GETTING_STARTED_SECTION}" UPDATE_COMMIT_MSG="chore: update fullsend shim workflow diff --git a/internal/scaffold/scaffold_test.go b/internal/scaffold/scaffold_test.go index 4dbfec4b98..2d24b83fc1 100644 --- a/internal/scaffold/scaffold_test.go +++ b/internal/scaffold/scaffold_test.go @@ -5,6 +5,7 @@ import ( "os" "os/exec" "path/filepath" + "regexp" "strings" "testing" @@ -802,6 +803,45 @@ func TestReconcileReposContent(t *testing.T) { "reconcile-repos.sh should not parse dispatch mode") assert.Contains(t, s, "private repos cannot be enrolled", "reconcile-repos.sh should skip private repos to prevent log exposure") + + // The "Getting started" slash-command catalog (#2165) must appear in both the + // enrollment and update PR bodies. The update path is the first touchpoint for + // already-enrolled repos, which is the scenario the original incident hit. + assert.Contains(t, s, "## Getting started", + "reconcile-repos.sh PR bodies should include the Getting started section") + assert.Contains(t, s, `GETTING_STARTED_SECTION`, + "Getting started block should be shared so it appears in both enroll and update PRs") + assert.Contains(t, s, `ENROLL_PR_BODY=`) + assert.Contains(t, s, `UPDATE_PR_BODY=`) + // Both PR bodies interpolate the shared block. + assert.Equal(t, 2, strings.Count(s, `${GETTING_STARTED_SECTION}`), + "shared Getting started block should be appended to both the enroll and update PR bodies") +} + +// TestReconcileReposSlashCommandCatalog guards against the onboarding PR body's +// slash-command catalog drifting from dispatch.yml's routing. Every /fs-* command +// that dispatch.yml routes on must be documented in reconcile-repos.sh's PR body, +// so a command added/renamed in dispatch.yml without updating the catalog fails CI. +func TestReconcileReposSlashCommandCatalog(t *testing.T) { + dispatch, err := FullsendRepoFile(".github/workflows/dispatch.yml") + require.NoError(t, err) + script, err := FullsendRepoFile("scripts/reconcile-repos.sh") + require.NoError(t, err) + scriptStr := string(script) + + cmdRE := regexp.MustCompile(`/fs-[a-z]+`) + matches := cmdRE.FindAllString(string(dispatch), -1) + require.NotEmpty(t, matches, "expected dispatch.yml to route on /fs-* commands") + + seen := map[string]bool{} + for _, cmd := range matches { + if seen[cmd] { + continue + } + seen[cmd] = true + assert.Contains(t, scriptStr, cmd, + "dispatch.yml routes on %s but reconcile-repos.sh onboarding PR body does not document it", cmd) + } } func TestPrioritizeWorkflowContent(t *testing.T) { From c479ebbcc0629b78190aeddcbdab58f21c9f3b23 Mon Sep 17 00:00:00 2001 From: Shai Revivo Date: Thu, 27 Aug 2026 14:43:33 +0300 Subject: [PATCH 3/5] fix(#2165): point onboarding docs at live site and harden drift guard Address review feedback on #6591: - Links in the onboarding "Getting started" catalog now point to the live docs site (https://fullsend.sh/docs/agents/) 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 Signed-off-by: Shai Revivo --- .../fullsend-repo/scripts/reconcile-repos.sh | 12 +-- internal/scaffold/scaffold_test.go | 76 ++++++++++++++++--- 2 files changed, 71 insertions(+), 17 deletions(-) diff --git a/internal/scaffold/fullsend-repo/scripts/reconcile-repos.sh b/internal/scaffold/fullsend-repo/scripts/reconcile-repos.sh index 81a4e3f6d6..7be3821f8b 100755 --- a/internal/scaffold/fullsend-repo/scripts/reconcile-repos.sh +++ b/internal/scaffold/fullsend-repo/scripts/reconcile-repos.sh @@ -43,12 +43,12 @@ GETTING_STARTED_SECTION="## Getting started Once this PR is merged, interact with fullsend by commenting one of these slash commands. The supported target (issue and/or pull request) is shown for each: -- \`/fs-triage\` (issue or PR) — Invoke the [triage](https://github.com/fullsend-ai/agents/blob/main/docs/triage.md) agent to categorize, label, and assess an issue. -- \`/fs-code\` (issue only) — Invoke the [code](https://github.com/fullsend-ai/agents/blob/main/docs/code.md) agent to implement a fix for an issue and open a PR. -- \`/fs-review\` (PR only) — Invoke the [review](https://github.com/fullsend-ai/agents/blob/main/docs/review.md) agent to review a pull request. -- \`/fs-fix\` (PR only) — Invoke the [fix](https://github.com/fullsend-ai/agents/blob/main/docs/fix.md) agent to address review feedback on a pull request. -- \`/fs-retro\` (issue or PR) — Invoke the [retro](https://github.com/fullsend-ai/agents/blob/main/docs/retro.md) agent to analyze completed work and propose improvements. -- \`/fs-prioritize\` (issue or PR) — Invoke the [prioritize](https://github.com/fullsend-ai/agents/blob/main/docs/prioritize.md) agent to score an issue for project board ranking." +- \`/fs-triage\` (issue or PR) — Invoke the [triage](https://fullsend.sh/docs/agents/triage) agent to categorize, label, and assess an issue. +- \`/fs-code\` (issue only) — Invoke the [code](https://fullsend.sh/docs/agents/code) agent to implement a fix for an issue and open a PR. +- \`/fs-review\` (PR only) — Invoke the [review](https://fullsend.sh/docs/agents/review) agent to review a pull request. +- \`/fs-fix\` (PR only) — Invoke the [fix](https://fullsend.sh/docs/agents/fix) agent to address review feedback on a pull request. +- \`/fs-retro\` (issue or PR) — Invoke the [retro](https://fullsend.sh/docs/agents/retro) agent to analyze completed work and propose improvements. +- \`/fs-prioritize\` (issue or PR) — Invoke the [prioritize](https://fullsend.sh/docs/agents/prioritize) agent to score an issue for project board ranking." ENROLL_PR_BODY="This PR adds a shim workflow that routes repository events to the fullsend agent dispatch workflow in the \`.fullsend\` config repo. diff --git a/internal/scaffold/scaffold_test.go b/internal/scaffold/scaffold_test.go index 2d24b83fc1..7de56dffc2 100644 --- a/internal/scaffold/scaffold_test.go +++ b/internal/scaffold/scaffold_test.go @@ -818,29 +818,83 @@ func TestReconcileReposContent(t *testing.T) { "shared Getting started block should be appended to both the enroll and update PR bodies") } +// commandsNotInOnboardingCatalog lists slash commands that dispatch.yml routes +// on but that are deliberately omitted from the user-facing onboarding catalog, +// so the omission is a recorded decision rather than a regex accident. Anything +// routed by dispatch.yml and not listed here must appear in the catalog. +// - /fullsend: backward-compat alias for the /fs-retro form; /fs-retro is the +// primary command documented in the catalog. +var commandsNotInOnboardingCatalog = map[string]bool{ + "/fullsend": true, +} + +// extractGettingStartedSection returns the body of the GETTING_STARTED_SECTION +// shell assignment in reconcile-repos.sh — the exact block rendered into the +// onboarding PR bodies. Assertions scope to this block rather than the whole +// script so a command name appearing in an unrelated comment or code path cannot +// satisfy the catalog guard. +func extractGettingStartedSection(t *testing.T, scriptStr string) string { + t.Helper() + const marker = `GETTING_STARTED_SECTION="` + start := strings.Index(scriptStr, marker) + require.GreaterOrEqual(t, start, 0, + "expected GETTING_STARTED_SECTION assignment in reconcile-repos.sh") + rest := scriptStr[start+len(marker):] + // The block contains no embedded double quotes, so the next quote closes it. + end := strings.Index(rest, `"`) + require.GreaterOrEqual(t, end, 0, + "GETTING_STARTED_SECTION assignment should be closed with a double quote") + return rest[:end] +} + // TestReconcileReposSlashCommandCatalog guards against the onboarding PR body's -// slash-command catalog drifting from dispatch.yml's routing. Every /fs-* command -// that dispatch.yml routes on must be documented in reconcile-repos.sh's PR body, -// so a command added/renamed in dispatch.yml without updating the catalog fails CI. +// slash-command catalog drifting from dispatch.yml's routing, in both directions: +// - forward: every command dispatch.yml routes on (except deliberately-omitted +// aliases in commandsNotInOnboardingCatalog) must appear in the catalog, so a +// command added/renamed in dispatch.yml without updating the catalog fails CI. +// - reverse: every command documented in the catalog must actually be routed by +// dispatch.yml, so a command removed from dispatch.yml but left in the +// user-facing catalog also fails CI. +// +// The command pattern permits digits and hyphens so a hyphenated command (e.g. +// /fs-fix-stop) is matched in full rather than truncated to a prefix, and the +// /fullsend alias form is matched explicitly. func TestReconcileReposSlashCommandCatalog(t *testing.T) { dispatch, err := FullsendRepoFile(".github/workflows/dispatch.yml") require.NoError(t, err) + dispatchStr := string(dispatch) script, err := FullsendRepoFile("scripts/reconcile-repos.sh") require.NoError(t, err) - scriptStr := string(script) - cmdRE := regexp.MustCompile(`/fs-[a-z]+`) - matches := cmdRE.FindAllString(string(dispatch), -1) - require.NotEmpty(t, matches, "expected dispatch.yml to route on /fs-* commands") + catalog := extractGettingStartedSection(t, string(script)) + + cmdRE := regexp.MustCompile(`/fs-[a-z0-9-]+|/fullsend\b`) + // Forward: dispatch.yml commands must be documented (unless allow-listed). + dispatchCmds := cmdRE.FindAllString(dispatchStr, -1) + require.NotEmpty(t, dispatchCmds, "expected dispatch.yml to route on /fs-* commands") seen := map[string]bool{} - for _, cmd := range matches { - if seen[cmd] { + for _, cmd := range dispatchCmds { + if seen[cmd] || commandsNotInOnboardingCatalog[cmd] { continue } seen[cmd] = true - assert.Contains(t, scriptStr, cmd, - "dispatch.yml routes on %s but reconcile-repos.sh onboarding PR body does not document it", cmd) + assert.Contains(t, catalog, cmd, + "dispatch.yml routes on %s but the onboarding catalog does not document it "+ + "(add it to GETTING_STARTED_SECTION, or to commandsNotInOnboardingCatalog if intentional)", cmd) + } + + // Reverse: cataloged commands must actually be routed by dispatch.yml. + catalogCmds := cmdRE.FindAllString(catalog, -1) + require.NotEmpty(t, catalogCmds, "expected the onboarding catalog to document /fs-* commands") + seenCatalog := map[string]bool{} + for _, cmd := range catalogCmds { + if seenCatalog[cmd] { + continue + } + seenCatalog[cmd] = true + assert.Contains(t, dispatchStr, cmd, + "onboarding catalog documents %s but dispatch.yml does not route on it", cmd) } } From 40b8de2da137b8e2a38f0d86a67cbe675532abf0 Mon Sep 17 00:00:00 2001 From: Shai Revivo Date: Thu, 27 Aug 2026 20:02:52 +0300 Subject: [PATCH 4/5] fix(#2165): cover per-repo onboarding + fix reverse drift guard Address review feedback on #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 Signed-off-by: Shai Revivo --- internal/repos/scaffold_metadata.go | 18 +++++- internal/repos/scaffold_metadata_test.go | 73 ++++++++++++++++++++++++ internal/scaffold/scaffold_test.go | 53 +++++++++++------ 3 files changed, 124 insertions(+), 20 deletions(-) diff --git a/internal/repos/scaffold_metadata.go b/internal/repos/scaffold_metadata.go index 1920659b20..797023f954 100644 --- a/internal/repos/scaffold_metadata.go +++ b/internal/repos/scaffold_metadata.go @@ -18,10 +18,26 @@ type ScaffoldPRMetadata struct { } const ( + // gettingStartedCatalog documents the primary /fs-* slash commands so users + // discover them at their first touchpoint — the fresh-install PR. It mirrors + // the per-org onboarding catalog (GETTING_STARTED_SECTION in + // scripts/reconcile-repos.sh); both surfaces are independently pinned to + // dispatch.yml's routing (per-repo by TestPerRepoOnboardingCatalog, per-org by + // TestReconcileReposSlashCommandCatalog) so they cannot drift apart. See #2165. + gettingStartedCatalog = "\n\n## Getting started\n\n" + + "Once this PR is merged, interact with fullsend by commenting one of these " + + "slash commands. The supported target (issue and/or pull request) is shown for each:\n\n" + + "- `/fs-triage` (issue or PR) — Invoke the [triage](https://fullsend.sh/docs/agents/triage) agent to categorize, label, and assess an issue.\n" + + "- `/fs-code` (issue only) — Invoke the [code](https://fullsend.sh/docs/agents/code) agent to implement a fix for an issue and open a PR.\n" + + "- `/fs-review` (PR only) — Invoke the [review](https://fullsend.sh/docs/agents/review) agent to review a pull request.\n" + + "- `/fs-fix` (PR only) — Invoke the [fix](https://fullsend.sh/docs/agents/fix) agent to address review feedback on a pull request.\n" + + "- `/fs-retro` (issue or PR) — Invoke the [retro](https://fullsend.sh/docs/agents/retro) agent to analyze completed work and propose improvements.\n" + + "- `/fs-prioritize` (issue or PR) — Invoke the [prioritize](https://fullsend.sh/docs/agents/prioritize) agent to score an issue for project board ranking." + // defaultScaffoldPRBody is the PR body for fresh installations. // Only used within this package. defaultScaffoldPRBody = "This PR adds the fullsend scaffold files for per-repo installation.\n\n" + - "Merge this PR to activate fullsend workflows." + "Merge this PR to activate fullsend workflows." + gettingStartedCatalog // DefaultScaffoldBranch is the branch name for fresh installations. DefaultScaffoldBranch = "fullsend/scaffold-install" diff --git a/internal/repos/scaffold_metadata_test.go b/internal/repos/scaffold_metadata_test.go index 2b1ceb920a..8ff37fda0f 100644 --- a/internal/repos/scaffold_metadata_test.go +++ b/internal/repos/scaffold_metadata_test.go @@ -2,12 +2,15 @@ package repos import ( "context" + "regexp" "strings" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "github.com/fullsend-ai/fullsend/internal/forge" + "github.com/fullsend-ai/fullsend/internal/scaffold" ) func TestBuildScaffoldPRMetadata_FreshInstall(t *testing.T) { @@ -170,6 +173,76 @@ func TestDetectExistingVersion(t *testing.T) { }) } +// commandsNotInPerRepoCatalog mirrors commandsNotInOnboardingCatalog in the +// scaffold package: dispatch.yml routes these but they are deliberately omitted +// from the user-facing per-repo onboarding catalog. +// - /fullsend: backward-compat alias for the /fs-retro form; /fs-retro is the +// primary command documented in the catalog. +var commandsNotInPerRepoCatalog = map[string]bool{ + "/fullsend": true, +} + +// TestPerRepoOnboardingCatalog guards the per-repo install PR body's +// slash-command catalog against drift from dispatch.yml's routing, in both +// directions — the per-repo analogue of TestReconcileReposSlashCommandCatalog in +// the scaffold package (which guards the per-org onboarding catalog). Pinning +// both catalogs to the same source (dispatch.yml) keeps the two onboarding +// surfaces from diverging. +func TestPerRepoOnboardingCatalog(t *testing.T) { + dispatch, err := scaffold.FullsendRepoFile(".github/workflows/dispatch.yml") + require.NoError(t, err) + dispatchStr := string(dispatch) + + // dispatch.yml routes on bare command tokens; the catalog renders them as + // backtick-wrapped bullets (e.g. `/fs-triage`). Scope each pattern to its form + // so a command name embedded in a docs URL is not mistaken for a command. + dispatchCmdRE := regexp.MustCompile(`/fs-[a-z0-9-]+|/fullsend\b`) + catalogCmdRE := regexp.MustCompile("`(/fs-[a-z0-9-]+|/fullsend)`") + + dispatchCmds := map[string]bool{} + for _, cmd := range dispatchCmdRE.FindAllString(dispatchStr, -1) { + dispatchCmds[cmd] = true + } + require.NotEmpty(t, dispatchCmds, "expected dispatch.yml to route on /fs-* commands") + + catalogCmds := map[string]bool{} + for _, m := range catalogCmdRE.FindAllStringSubmatch(gettingStartedCatalog, -1) { + catalogCmds[m[1]] = true + } + require.NotEmpty(t, catalogCmds, "expected the per-repo catalog to document /fs-* commands") + + // Forward: dispatch.yml commands must be documented (unless allow-listed). + for cmd := range dispatchCmds { + if commandsNotInPerRepoCatalog[cmd] { + continue + } + assert.True(t, catalogCmds[cmd], + "dispatch.yml routes on %s but the per-repo onboarding catalog does not document it "+ + "(add it to gettingStartedCatalog, or to commandsNotInPerRepoCatalog if intentional)", cmd) + } + + // Reverse: cataloged commands must actually be routed by dispatch.yml (the + // allow-list is applied here too so it stays symmetric with the forward check). + for cmd := range catalogCmds { + if commandsNotInPerRepoCatalog[cmd] { + continue + } + assert.True(t, dispatchCmds[cmd], + "per-repo onboarding catalog documents %s but dispatch.yml does not route on it", cmd) + } +} + +// TestFreshInstallBodyIncludesCatalog verifies the fresh-install PR body carries +// the Getting started catalog, so dropping the append is caught by CI. +func TestFreshInstallBodyIncludesCatalog(t *testing.T) { + fc := forge.NewFakeClient() + notInstalled := false + meta := BuildScaffoldPRMetadata(context.Background(), fc, "acme", "widget", "v0.28.0", + ScaffoldMetadataOpts{GuardInstalled: ¬Installed}) + assert.Contains(t, meta.PRBody, "## Getting started") + assert.Contains(t, meta.PRBody, "`/fs-triage`") +} + func TestRuntimeSection(t *testing.T) { t.Parallel() def := RuntimeSection("") diff --git a/internal/scaffold/scaffold_test.go b/internal/scaffold/scaffold_test.go index 7de56dffc2..f9ad6171e5 100644 --- a/internal/scaffold/scaffold_test.go +++ b/internal/scaffold/scaffold_test.go @@ -856,9 +856,14 @@ func extractGettingStartedSection(t *testing.T, scriptStr string) string { // dispatch.yml, so a command removed from dispatch.yml but left in the // user-facing catalog also fails CI. // -// The command pattern permits digits and hyphens so a hyphenated command (e.g. -// /fs-fix-stop) is matched in full rather than truncated to a prefix, and the -// /fullsend alias form is matched explicitly. +// The two surfaces render commands differently, so each is scoped to its own +// form: dispatch.yml routes on bare tokens (e.g. /fs-triage), while the catalog +// renders them as backtick-wrapped bullets (e.g. `/fs-triage`). Scoping catalog +// extraction to the backtick form keeps URL hosts like https://fullsend.sh from +// being mistaken for a /fullsend command. Membership is compared as exact tokens +// (via sets) rather than substring containment so a hyphenated command (e.g. +// /fs-fix-stop) cannot satisfy the guard against an unrelated prefix (/fs-fix), +// and the allow-list is applied symmetrically in both directions. func TestReconcileReposSlashCommandCatalog(t *testing.T) { dispatch, err := FullsendRepoFile(".github/workflows/dispatch.yml") require.NoError(t, err) @@ -868,32 +873,42 @@ func TestReconcileReposSlashCommandCatalog(t *testing.T) { catalog := extractGettingStartedSection(t, string(script)) - cmdRE := regexp.MustCompile(`/fs-[a-z0-9-]+|/fullsend\b`) + // dispatchCmdRE matches bare command tokens as they appear in dispatch.yml's + // case labels. catalogCmdRE matches only the rendered, backtick-wrapped bullet + // form (the backticks are backslash-escaped in the shell assignment), so a + // command name embedded in a docs URL is not extracted as a catalog command. + dispatchCmdRE := regexp.MustCompile(`/fs-[a-z0-9-]+|/fullsend\b`) + catalogCmdRE := regexp.MustCompile("\\\\?`(/fs-[a-z0-9-]+|/fullsend)\\\\?`") - // Forward: dispatch.yml commands must be documented (unless allow-listed). - dispatchCmds := cmdRE.FindAllString(dispatchStr, -1) + dispatchCmds := map[string]bool{} + for _, cmd := range dispatchCmdRE.FindAllString(dispatchStr, -1) { + dispatchCmds[cmd] = true + } require.NotEmpty(t, dispatchCmds, "expected dispatch.yml to route on /fs-* commands") - seen := map[string]bool{} - for _, cmd := range dispatchCmds { - if seen[cmd] || commandsNotInOnboardingCatalog[cmd] { + + catalogCmds := map[string]bool{} + for _, m := range catalogCmdRE.FindAllStringSubmatch(catalog, -1) { + catalogCmds[m[1]] = true + } + require.NotEmpty(t, catalogCmds, "expected the onboarding catalog to document /fs-* commands") + + // Forward: dispatch.yml commands must be documented (unless allow-listed). + for cmd := range dispatchCmds { + if commandsNotInOnboardingCatalog[cmd] { continue } - seen[cmd] = true - assert.Contains(t, catalog, cmd, + assert.True(t, catalogCmds[cmd], "dispatch.yml routes on %s but the onboarding catalog does not document it "+ "(add it to GETTING_STARTED_SECTION, or to commandsNotInOnboardingCatalog if intentional)", cmd) } - // Reverse: cataloged commands must actually be routed by dispatch.yml. - catalogCmds := cmdRE.FindAllString(catalog, -1) - require.NotEmpty(t, catalogCmds, "expected the onboarding catalog to document /fs-* commands") - seenCatalog := map[string]bool{} - for _, cmd := range catalogCmds { - if seenCatalog[cmd] { + // Reverse: cataloged commands must actually be routed by dispatch.yml (the + // allow-list is applied here too so it stays symmetric with the forward check). + for cmd := range catalogCmds { + if commandsNotInOnboardingCatalog[cmd] { continue } - seenCatalog[cmd] = true - assert.Contains(t, dispatchStr, cmd, + assert.True(t, dispatchCmds[cmd], "onboarding catalog documents %s but dispatch.yml does not route on it", cmd) } } From 5f18e74274c9d776c45fc0ae97228449cce43bfe Mon Sep 17 00:00:00 2001 From: Shai Revivo Date: Thu, 27 Aug 2026 20:19:13 +0300 Subject: [PATCH 5/5] fix(#2165): harden onboarding catalog drift guards 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 Signed-off-by: Shai Revivo --- internal/repos/scaffold_metadata_test.go | 110 ++++++++++++++++++----- internal/scaffold/scaffold_test.go | 81 ++++++++++------- 2 files changed, 140 insertions(+), 51 deletions(-) diff --git a/internal/repos/scaffold_metadata_test.go b/internal/repos/scaffold_metadata_test.go index 8ff37fda0f..4e33fbb66e 100644 --- a/internal/repos/scaffold_metadata_test.go +++ b/internal/repos/scaffold_metadata_test.go @@ -182,6 +182,72 @@ var commandsNotInPerRepoCatalog = map[string]bool{ "/fullsend": true, } +// These drift-guard helpers mirror the ones in the scaffold package's test. +// They live in a different package, so they are duplicated here rather than +// shared. Route extraction is scoped to dispatch.yml's case-arm labels and +// catalog extraction to rendered "- `cmd`" bullets, so a command mentioned in a +// comment, URL, or prose on either side cannot spoof a match. +var ( + // dispatchCaseArmRE matches a case-arm label in dispatch.yml's + // `case "${COMMAND}"` switch, e.g. "/fs-triage)" or "/fs-retro|/fullsend)". + dispatchCaseArmRE = regexp.MustCompile(`(?m)^[ \t]*(/(?:fs-[a-z0-9-]+|fullsend)(?:\|/(?:fs-[a-z0-9-]+|fullsend))*)\)`) + // slashCommandRE matches a single /fs-* or /fullsend command token. + slashCommandRE = regexp.MustCompile(`/(?:fs-[a-z0-9-]+|fullsend)`) + // catalogBulletRE matches a rendered onboarding-catalog bullet. Both catalogs + // use bare backticks at this point (the per-org shell escapes are normalized + // before comparison, and the per-repo Go catalog uses bare backticks). + catalogBulletRE = regexp.MustCompile("(?m)^- `(/(?:fs-[a-z0-9-]+|fullsend))`") + // catalogEntryRE additionally captures the full bullet text (target + + // description) after the command, for cross-catalog comparison. + catalogEntryRE = regexp.MustCompile("(?m)^- `(/(?:fs-[a-z0-9-]+|fullsend))` (.*)$") +) + +// routedDispatchCommands returns the set of slash commands dispatch.yml routes +// on, scoped to case-arm labels. +func routedDispatchCommands(dispatchStr string) map[string]bool { + cmds := map[string]bool{} + for _, arm := range dispatchCaseArmRE.FindAllStringSubmatch(dispatchStr, -1) { + for _, cmd := range slashCommandRE.FindAllString(arm[1], -1) { + cmds[cmd] = true + } + } + return cmds +} + +// catalogCommands returns the set of slash commands documented as bullets in an +// onboarding catalog block. +func catalogCommands(catalog string) map[string]bool { + cmds := map[string]bool{} + for _, m := range catalogBulletRE.FindAllStringSubmatch(catalog, -1) { + cmds[m[1]] = true + } + return cmds +} + +// catalogEntries maps each documented command to its full rendered bullet text +// (target hint + description), for comparing two catalogs entry-for-entry. +func catalogEntries(catalog string) map[string]string { + entries := map[string]string{} + for _, m := range catalogEntryRE.FindAllStringSubmatch(catalog, -1) { + entries[m[1]] = strings.TrimSpace(m[2]) + } + return entries +} + +// extractPerOrgCatalog pulls the GETTING_STARTED_SECTION assignment out of +// reconcile-repos.sh and normalizes the shell backtick-escapes (\`) to the +// rendered backtick form, so it compares directly against the per-repo catalog. +func extractPerOrgCatalog(t *testing.T, script string) string { + t.Helper() + const marker = `GETTING_STARTED_SECTION="` + start := strings.Index(script, marker) + require.NotEqual(t, -1, start, "GETTING_STARTED_SECTION marker not found in reconcile-repos.sh") + rest := script[start+len(marker):] + end := strings.IndexByte(rest, '"') + require.NotEqual(t, -1, end, "unterminated GETTING_STARTED_SECTION assignment") + return strings.ReplaceAll(rest[:end], "\\`", "`") +} + // TestPerRepoOnboardingCatalog guards the per-repo install PR body's // slash-command catalog against drift from dispatch.yml's routing, in both // directions — the per-repo analogue of TestReconcileReposSlashCommandCatalog in @@ -191,27 +257,14 @@ var commandsNotInPerRepoCatalog = map[string]bool{ func TestPerRepoOnboardingCatalog(t *testing.T) { dispatch, err := scaffold.FullsendRepoFile(".github/workflows/dispatch.yml") require.NoError(t, err) - dispatchStr := string(dispatch) - - // dispatch.yml routes on bare command tokens; the catalog renders them as - // backtick-wrapped bullets (e.g. `/fs-triage`). Scope each pattern to its form - // so a command name embedded in a docs URL is not mistaken for a command. - dispatchCmdRE := regexp.MustCompile(`/fs-[a-z0-9-]+|/fullsend\b`) - catalogCmdRE := regexp.MustCompile("`(/fs-[a-z0-9-]+|/fullsend)`") - dispatchCmds := map[string]bool{} - for _, cmd := range dispatchCmdRE.FindAllString(dispatchStr, -1) { - dispatchCmds[cmd] = true - } + dispatchCmds := routedDispatchCommands(string(dispatch)) require.NotEmpty(t, dispatchCmds, "expected dispatch.yml to route on /fs-* commands") - catalogCmds := map[string]bool{} - for _, m := range catalogCmdRE.FindAllStringSubmatch(gettingStartedCatalog, -1) { - catalogCmds[m[1]] = true - } + catalogCmds := catalogCommands(gettingStartedCatalog) require.NotEmpty(t, catalogCmds, "expected the per-repo catalog to document /fs-* commands") - // Forward: dispatch.yml commands must be documented (unless allow-listed). + // Forward: dispatch.yml commands must be documented (unless deliberately omitted). for cmd := range dispatchCmds { if commandsNotInPerRepoCatalog[cmd] { continue @@ -221,17 +274,32 @@ func TestPerRepoOnboardingCatalog(t *testing.T) { "(add it to gettingStartedCatalog, or to commandsNotInPerRepoCatalog if intentional)", cmd) } - // Reverse: cataloged commands must actually be routed by dispatch.yml (the - // allow-list is applied here too so it stays symmetric with the forward check). + // Reverse: every documented command must be routed by dispatch.yml. for cmd := range catalogCmds { - if commandsNotInPerRepoCatalog[cmd] { - continue - } assert.True(t, dispatchCmds[cmd], "per-repo onboarding catalog documents %s but dispatch.yml does not route on it", cmd) } } +// TestOnboardingCatalogsMatch pins the per-org (reconcile-repos.sh) and per-repo +// (gettingStartedCatalog) onboarding catalogs to each other, so the two surfaces +// cannot drift apart in the commands they list or in each command's target hint +// and description. The drift guards ensure each catalog matches dispatch.yml's +// routing; this ensures they also match each other verbatim. +func TestOnboardingCatalogsMatch(t *testing.T) { + script, err := scaffold.FullsendRepoFile("scripts/reconcile-repos.sh") + require.NoError(t, err) + + perOrg := catalogEntries(extractPerOrgCatalog(t, string(script))) + perRepo := catalogEntries(gettingStartedCatalog) + require.NotEmpty(t, perOrg, "expected the per-org catalog to document /fs-* commands") + require.NotEmpty(t, perRepo, "expected the per-repo catalog to document /fs-* commands") + + assert.Equal(t, perOrg, perRepo, + "per-org (reconcile-repos.sh) and per-repo (gettingStartedCatalog) onboarding catalogs "+ + "must document the same commands with the same target hint and description") +} + // TestFreshInstallBodyIncludesCatalog verifies the fresh-install PR body carries // the Getting started catalog, so dropping the append is caught by CI. func TestFreshInstallBodyIncludesCatalog(t *testing.T) { diff --git a/internal/scaffold/scaffold_test.go b/internal/scaffold/scaffold_test.go index f9ad6171e5..5a82f67ac4 100644 --- a/internal/scaffold/scaffold_test.go +++ b/internal/scaffold/scaffold_test.go @@ -847,6 +847,45 @@ func extractGettingStartedSection(t *testing.T, scriptStr string) string { return rest[:end] } +// dispatchCaseArmRE matches a case-arm label line in dispatch.yml's +// `case "${COMMAND}"` switch, e.g. " /fs-triage)" or +// " /fs-retro|/fullsend)". Scoping route extraction to these lines +// keeps a command mentioned in a comment, URL, or unrelated shell statement from +// being counted as routed. +var dispatchCaseArmRE = regexp.MustCompile(`(?m)^[ \t]*(/(?:fs-[a-z0-9-]+|fullsend)(?:\|/(?:fs-[a-z0-9-]+|fullsend))*)\)`) + +// slashCommandRE matches a single /fs-* or /fullsend command token. +var slashCommandRE = regexp.MustCompile(`/(?:fs-[a-z0-9-]+|fullsend)`) + +// catalogBulletRE matches a rendered onboarding-catalog bullet, e.g. +// "- `/fs-triage`". The optional leading backslash accommodates the shell +// assignment (backticks are escaped as \` there); the per-repo Go catalog uses +// bare backticks. Anchoring to the "- " bullet keeps a command mentioned in a +// docs URL or prose from counting as documented. +var catalogBulletRE = regexp.MustCompile("(?m)^- \\\\?`(/(?:fs-[a-z0-9-]+|fullsend))\\\\?`") + +// routedDispatchCommands returns the set of slash commands dispatch.yml routes +// on, scoped to case-arm labels (see dispatchCaseArmRE). +func routedDispatchCommands(dispatchStr string) map[string]bool { + cmds := map[string]bool{} + for _, arm := range dispatchCaseArmRE.FindAllStringSubmatch(dispatchStr, -1) { + for _, cmd := range slashCommandRE.FindAllString(arm[1], -1) { + cmds[cmd] = true + } + } + return cmds +} + +// catalogCommands returns the set of slash commands documented as bullets in an +// onboarding catalog block (see catalogBulletRE). +func catalogCommands(catalog string) map[string]bool { + cmds := map[string]bool{} + for _, m := range catalogBulletRE.FindAllStringSubmatch(catalog, -1) { + cmds[m[1]] = true + } + return cmds +} + // TestReconcileReposSlashCommandCatalog guards against the onboarding PR body's // slash-command catalog drifting from dispatch.yml's routing, in both directions: // - forward: every command dispatch.yml routes on (except deliberately-omitted @@ -856,43 +895,29 @@ func extractGettingStartedSection(t *testing.T, scriptStr string) string { // dispatch.yml, so a command removed from dispatch.yml but left in the // user-facing catalog also fails CI. // -// The two surfaces render commands differently, so each is scoped to its own -// form: dispatch.yml routes on bare tokens (e.g. /fs-triage), while the catalog -// renders them as backtick-wrapped bullets (e.g. `/fs-triage`). Scoping catalog -// extraction to the backtick form keeps URL hosts like https://fullsend.sh from -// being mistaken for a /fullsend command. Membership is compared as exact tokens -// (via sets) rather than substring containment so a hyphenated command (e.g. -// /fs-fix-stop) cannot satisfy the guard against an unrelated prefix (/fs-fix), -// and the allow-list is applied symmetrically in both directions. +// Routed commands are extracted only from dispatch.yml's case-arm labels, and +// documented commands only from rendered catalog bullets, so comments, URLs, or +// prose on either side cannot spoof a match. Membership is compared as exact +// tokens (via sets) rather than substring containment so a hyphenated command +// (e.g. /fs-fix-stop) cannot satisfy the guard against an unrelated prefix +// (/fs-fix). The omission allow-list is applied to the forward check only: a +// command written into the catalog and later dropped from dispatch must fail even +// if it is allow-listed. func TestReconcileReposSlashCommandCatalog(t *testing.T) { dispatch, err := FullsendRepoFile(".github/workflows/dispatch.yml") require.NoError(t, err) - dispatchStr := string(dispatch) script, err := FullsendRepoFile("scripts/reconcile-repos.sh") require.NoError(t, err) catalog := extractGettingStartedSection(t, string(script)) - // dispatchCmdRE matches bare command tokens as they appear in dispatch.yml's - // case labels. catalogCmdRE matches only the rendered, backtick-wrapped bullet - // form (the backticks are backslash-escaped in the shell assignment), so a - // command name embedded in a docs URL is not extracted as a catalog command. - dispatchCmdRE := regexp.MustCompile(`/fs-[a-z0-9-]+|/fullsend\b`) - catalogCmdRE := regexp.MustCompile("\\\\?`(/fs-[a-z0-9-]+|/fullsend)\\\\?`") - - dispatchCmds := map[string]bool{} - for _, cmd := range dispatchCmdRE.FindAllString(dispatchStr, -1) { - dispatchCmds[cmd] = true - } + dispatchCmds := routedDispatchCommands(string(dispatch)) require.NotEmpty(t, dispatchCmds, "expected dispatch.yml to route on /fs-* commands") - catalogCmds := map[string]bool{} - for _, m := range catalogCmdRE.FindAllStringSubmatch(catalog, -1) { - catalogCmds[m[1]] = true - } + catalogCmds := catalogCommands(catalog) require.NotEmpty(t, catalogCmds, "expected the onboarding catalog to document /fs-* commands") - // Forward: dispatch.yml commands must be documented (unless allow-listed). + // Forward: dispatch.yml commands must be documented (unless deliberately omitted). for cmd := range dispatchCmds { if commandsNotInOnboardingCatalog[cmd] { continue @@ -902,12 +927,8 @@ func TestReconcileReposSlashCommandCatalog(t *testing.T) { "(add it to GETTING_STARTED_SECTION, or to commandsNotInOnboardingCatalog if intentional)", cmd) } - // Reverse: cataloged commands must actually be routed by dispatch.yml (the - // allow-list is applied here too so it stays symmetric with the forward check). + // Reverse: every documented command must be routed by dispatch.yml. for cmd := range catalogCmds { - if commandsNotInOnboardingCatalog[cmd] { - continue - } assert.True(t, dispatchCmds[cmd], "onboarding catalog documents %s but dispatch.yml does not route on it", cmd) }