feat(#5994): add status_notifications support to per-repo config

[ralphbean]

Fixes #5994

Summary

• perRepoConfig gets a StatusNotifications() accessor (backed by a
  new Notifications field) following the same overlay -> base ->
  code-defaults fallback chain used by other per-repo settings
  (ConfigRoles, ConfigRuntime, etc).
• StatusNotifications() moves from OrgConfigReader to the shared
  ConfigReader interface, since both config modes now implement it.
  setupStatusNotifier in internal/cli/run.go reads it directly
  instead of type-asserting to OrgConfigReader.
• repos migrate now carries status_notifications over into the
  generated per-repo config.yaml instead of warning that it has no
  per-repo equivalent (per-repos migrate does not carry over org config fields or register repos in mint #5822 context in the issue).
• Docs updated: docs/cli/repos.md config carry-over table, and
  docs/guides/getting-started/operations.md shows the per-repo
  (top-level) vs per-org (nested under defaults) shapes.

This unblocks #5957, which needs per-repo installs to be able to
enable reactions so the pkg/behaviourtest e2e harness (per-repo-only)
can exercise the reaction feature.

Test plan

• go build ./...
• go vet ./...
• gofmt -l clean
• make lint
• go test ./internal/config/... ./internal/repos/... ./internal/cli/... — all green
• New unit tests: per-repo config parsing/validation/marshal
  round-trip, fallback-to-parent semantics, org-to-per-repo
  carry-over (with deep-copy aliasing check), migrate carry-over
  (asserted against generated config.yaml content), setupStatusNotifier
  against a per-repo-shaped config.yaml

Assisted-by: Claude Opus 4.6 noreply@anthropic.com

[qodo-code-review]

PR Summary by Qodo

Add status_notifications to per-repo config and migrate carry-over

✨ Enhancement 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Add status_notifications support to per-repo .fullsend/config.yaml with standard fallback
 semantics.
• Promote StatusNotifications() to ConfigReader and simplify CLI notifier setup.
• Carry defaults.status_notifications into generated per-repo config during repos migrate.

Diagram

graph TD
  ORG["Org config.yaml"] --> MIG["repos migrate"] --> OUT["Per-repo .fullsend/config.yaml"]
  REPO["Per-repo config.yaml"] --> PARSE["config parsing"] --> CR["ConfigReader (StatusNotifications)"] --> CLI["setupStatusNotifier"]
  CR --> MIG

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Keep StatusNotifications() org-only and type-assert where needed

• ➕ No interface surface-area increase on ConfigReader
• ➕ Avoids per-repo config schema changes
• ➖ Continues special-casing in CLI/runtime code
• ➖ Blocks per-repo installs from enabling notifications
• ➖ Migration cannot faithfully preserve org behavior

2. Add a standalone optional interface (without embedding into ConfigReader)

• ➕ Keeps ConfigReader smaller for callers that don’t care
• ➕ Allows capability checks via interface assertion where needed
• ➖ Still encourages scattered interface assertions
• ➖ Harder to reason about which readers support which fields
• ➖ Less consistent with other shared, portable settings

Recommendation: The current approach (making status notifications portable, adding a dedicated reader interface, and embedding it into ConfigReader) is the cleanest: it removes org-mode special cases, enables per-repo parity, and makes migration carry-over straightforward while maintaining the established overlay→base→defaults fallback pattern.

Files changed (11) +291 / -37

Enhancement (4) +41 / -28

run.goRead StatusNotifications via ConfigReader in setupStatusNotifier +5/-8

Read StatusNotifications via ConfigReader in setupStatusNotifier

• Removes the org-only 'OrgConfigReader' type assertion when loading config.yaml. Uses the shared 'StatusNotifications()' accessor so both org and per-repo configs can drive notifier behavior.

internal/cli/run.go

config.goAdd per-repo Notifications field with marshal/validate support +33/-16

Add per-repo Notifications field with marshal/validate support

• Introduces 'Notifications *StatusNotificationConfig' on perRepoConfig (YAML: 'status_notifications') and wires it into MarshalYAML and Validate. Updates org→per-repo mapping to deep-copy status_notifications to avoid pointer aliasing.

internal/config/config.go

defaults.goProvide per-repo default StatusNotifications() implementation +3/-0

Provide per-repo default StatusNotifications() implementation

• Implements 'StatusNotifications()' on perRepoDefaults returning nil, matching default behavior when omitted.

internal/config/defaults.go

migrate.goStop warning about status_notifications as non-portable +0/-4

Stop warning about status_notifications as non-portable

• Removes the migrate warning that 'defaults.status_notifications' has no per-repo equivalent, aligning with new portability.

internal/repos/migrate.go

Refactor (1) +19 / -1

interfaces.goPromote StatusNotifications() into shared ConfigReader interface +19/-1

Promote StatusNotifications() into shared ConfigReader interface

• Adds 'StatusNotificationsReader' and embeds it into 'ConfigReader', removing the method from 'OrgConfigReader'. Implements perRepoConfig.StatusNotifications() with local override and parent fallback.

internal/config/interfaces.go

Tests (4) +218 / -6

run_test.goAdd setupStatusNotifier test for per-repo config.yaml shape +28/-0

Add setupStatusNotifier test for per-repo config.yaml shape

• Adds a unit test that writes a per-repo-shaped config.yaml (no org-only keys) and asserts notifier setup succeeds. Validates per-repo status_notifications are honored.

internal/cli/run_test.go

config_test.goTest per-repo status_notifications parsing, fallback, validation, and carry-over +116/-0

Test per-repo status_notifications parsing, fallback, validation, and carry-over

• Adds tests for parsing/marshaling per-repo status_notifications, parent fallback behavior, and validation errors. Adds org→per-repo carry-over tests including a deep-copy mutation check.

internal/config/config_test.go

interfaces_test.goAdd unit coverage for perRepoConfig.StatusNotifications() semantics +20/-0

Add unit coverage for perRepoConfig.StatusNotifications() semantics

• Tests local value precedence, parent fallback, and nil when unset for perRepoConfig.StatusNotifications().

internal/config/interfaces_test.go

migrate_test.goUpdate warnings expectations and assert status_notifications carry-over +54/-6

Update warnings expectations and assert status_notifications carry-over

• Updates existing migrate test to expect only the remaining non-portable field warnings. Adds a new test that validates generated per-repo config.yaml includes carried-over status_notifications and no warning is emitted.

internal/repos/migrate_test.go

Documentation (2) +13 / -2

repos.mdDocument status_notifications as a migrated portable field +1/-1

Document status_notifications as a migrated portable field

• Updates the migrate field mapping table to include 'defaults.status_notifications' → 'status_notifications'. Removes it from the non-portable warnings list.

docs/cli/repos.md

operations.mdClarify per-org vs per-repo status_notifications YAML shapes +12/-1

Clarify per-org vs per-repo status_notifications YAML shapes

• Splits the status notifications documentation into per-org (nested under 'defaults') and per-repo (top-level) examples. Keeps the default behavior note intact.

docs/guides/getting-started/operations.md

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (1)

1. Procedures not numbered ✗ Dismissed 📜 Skill insight ✧ Quality

Description

The status notifications instructions are written as prose with code blocks instead of numbered
steps. This violates the requirement that procedural guide content use numbered (ordered) steps.

Code

docs/guides/getting-started/operations.md[R180-182]

+For per-org installs, nest it under `defaults`:

```yaml

Relevance

●●● Strong

Team often accepts converting procedural prose into numbered steps in guides.

PR-#5778
PR-#2663

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1062079 requires procedures in guides to use numbered steps. The modified section
instructs configuration actions but does not present them as a numbered list.

docs/guides/getting-started/operations.md[178-198]
Skill: writing-user-docs

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Procedural instructions in the guide are presented as prose paragraphs (e.g., "For per-org installs..." / "For per-repo installs...") rather than as an ordered list of numbered steps.

## Issue Context
The guide is instructing users how to configure `status_notifications` for different install modes; these are actionable steps and should be formatted as a numbered list.

## Fix Focus Areas
- docs/guides/getting-started/operations.md[178-198]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Guide not in admin/user 📜 Skill insight ⌂ Architecture

Description

The modified guide file lives under docs/guides/getting-started/, but guides must be placed under
either docs/guides/admin/ or docs/guides/user/. This breaks the required documentation directory
structure and makes the guide harder to discover and maintain.

Code

docs/guides/getting-started/operations.md[R178-180]

+Agent workflows post status comments on issues and PRs when they start and complete. This behavior is controlled by the `status_notifications` section in `config.yaml`.
+
+For per-org installs, nest it under `defaults`:

Relevance

● Weak

Similar guide-relocation requests were explicitly rejected; repo seems to tolerate non admin/user
guide paths.

PR-#5454
PR-#5502

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1062077 requires every guide under docs/guides/ to be placed in either admin/
or user/. The changed file path is docs/guides/getting-started/operations.md, which is neither
of those locations.

docs/guides/getting-started/operations.md[176-190]
Skill: writing-user-docs

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`docs/guides/getting-started/operations.md` is a guide file under `docs/guides/` but it is not located in `docs/guides/admin/` or `docs/guides/user/`, which violates the guide placement requirement.

## Issue Context
This PR modifies the guide and adds/updates content, so the file should be brought into compliance by placing it in the correct subdirectory and updating any links that reference it.

## Fix Focus Areas
- docs/guides/getting-started/operations.md[176-190]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context used

✅ Compliance rules (platform): 54 rules

✅ Skills: writing-user-docs, writing-adrs

Tip of the day

💡 Did you know, you can reply 'qodo' on any finding to push back, ask questions, or dig deeper

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[waynesun09]

Review finding

MEDIUM — Per-field merge-rules reference doc not updated for new status_notifications field

docs/guides/infrastructure/layered-config-reference.md is the authoritative reference for how each per-repo config field resolves through the overlay -> base -> code-defaults chain (ADR 0069 Decision 2). It has an explicit "Per-field merge rules" table (around line 67-81, listing version, runtime, kill_switch, roles, agents, allowed_remote_resources, forge, create_issues) and a matching "Code defaults reference" table (around line 221-235), plus a dedicated subsection for create_issues (around line 211).

Verified on PR head dc3a6da9 that neither table nor any subsection mentions status_notifications, even though this PR adds a Notifications *StatusNotificationConfig field to perRepoConfig (internal/config/config.go) with exactly the replace-whole-object-if-set / fallback-to-parent semantics already documented for create_issues. Readers relying on this doc (cross-linked from docs/architecture.md and docs/problems/governance.md) won't know the field exists or how it merges. Note: this file isn't touched by this PR's diff, so it can't be commented on inline.

Suggestion: add a status_notifications row to the "Per-field merge rules" table (merge rule: "Replace whole object if set", same pattern as create_issues) and to the "Code defaults reference" table (default nil), and optionally a short subsection describing the scalar-override-per-object semantics, consistent with how create_issues is documented.

[ralphbean]

Re: #5997 (comment)

On the getting-started/ placement flag — I think this one's a false positive. The ADR-0023 revision from 2026-05 split the old admin/ directory into getting-started/ and infrastructure/, and getting-started/ is where org-onboarding guides like this one live now. So operations.md isn't misplaced.

The numbered-steps point is worth thinking through separately — following up on that in the review thread on line 182 instead.

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 3:12 PM UTC · Completed 3:43 PM UTC

Commit: e12f7ae · View workflow run →

[ralphbean]

Re: #5997 (review)

Good catch. Pushed e12f7ae — added status_notifications to both tables in layered-config-reference.md, plus a subsection matching create_issues (same replace-whole-object-if-set semantics). Does that cover it?

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[fullsend-ai-review]

Review

Findings

Low

• [naming-convention] internal/config/config.go:520 — The Notifications field name deviates from the codebase pattern where struct fields match the PascalCase of their YAML tag (e.g., CreateIssues → create_issues, KillSwitch → kill_switch). The natural name would be StatusNotifications, but StatusNotifications() is the pre-existing accessor method on the ConfigReader interface, and Go forbids a field and method sharing a name on the same type. The code comment documents this rationale.

• [interface-placement] internal/config/interfaces.go:36 — StatusNotificationsReader is embedded into the shared ConfigReader interface, promoting StatusNotifications() from OrgConfigReader to be required by all ConfigReader implementations. This is consistent with how CreateIssuesReader was promoted when per-repo gained issue creation support, and compile-time assertions ensure all implementations satisfy the interface.

Previous run

Review

Findings

Low

• [naming-convention] internal/config/config.go:519 — The Notifications field name deviates from the codebase pattern where struct fields match the PascalCase of their YAML tag (e.g., CreateIssues → create_issues, KillSwitch → kill_switch). The natural name would be StatusNotifications, but StatusNotifications() is the pre-existing accessor method on the ConfigReader interface, and Go forbids a field and method sharing a name on the same type. The CreateIssues/IssueCreationConfig() precedent solved this by giving the method a different name, but that was established when the method was first created — here the method name pre-exists and renaming it would require updating the shared interface and all consumers. The code comment documents this rationale.

• [interface-placement] internal/config/interfaces.go:33 — StatusNotificationsReader is embedded into the shared ConfigReader interface, promoting StatusNotifications() from OrgConfigReader to be required by all ConfigReader implementations. This is consistent with how CreateIssuesReader was promoted when per-repo gained issue creation support, and compile-time assertions ensure all implementations satisfy the interface.

---

Labels: PR adds status_notifications support to per-repo config (installation/config domain), matching issue #5994 labels

[fullsend-ai-review]

🤖 Review · ⚠️ Cancelled · Started 4:07 PM UTC · Ended 4:20 PM UTC

Commit: 6172aef · View workflow run →

[github-actions]

Site preview

Preview: https://d7d374c1-site.fullsend-ai.workers.dev

Commit: 19db87c1fc87e0726ecb73befc1286a3cadd5111

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 4:21 PM UTC · Completed 4:37 PM UTC

Commit: 19db87c · View workflow run →

[waynesun09]

Approving — no findings from this round's review squad, all prior threads resolved, CI green.

[fullsend-ai-retro]

🤖 Finished Retro · ✅ Success · Started 6:51 PM UTC · Completed 7:10 PM UTC

Commit: 19db87c · View workflow run →

[fullsend-ai-retro]

Retro: PR #5997 — status_notifications per-repo config

Workflow shape: Human-authored PR (ralphbean), 12 files (+303/−37). Three review agent runs (2 successful, 1 cancelled mid-flight when superseded by a new push). No code/fix/triage agent involvement. Agents repo: fullsend-ai/agents@4a12f03.

Timeline

1. Aug 6 — Issue #5994 filed; PR #5997 opened. Qodo bot reviews immediately — 0 bugs, 1 dismissed skill insight.
2. Aug 7 — Human reviewer (waynesun09) posts MEDIUM finding: layered-config-reference.md reference tables not updated for the new status_notifications config field. Author accepts.
3. Aug 10 15:11–15:43 — First review agent run 31402252355 on commit e12f7ae. Dispatches 4 sub-agents (correctness, style-conventions, intent-coherence, docs-currency). Approves with 2 LOW findings (naming deviation, interface placement). Logs note: "PR already has 2 human review(s)."
4. Aug 10 15:26 — waynesun09 posts second MEDIUM finding: StatusNotificationsReader doc comment references "reaction" but no Reaction field exists in StatusNotificationConfig.
5. Aug 10 16:06–16:20 — Second review run 31407218873 cancelled (superseded by new push at 16:19).
6. Aug 10 16:20–16:37 — Final review run 31408420012 on commit 19db87c. Same 2 LOW findings. Approves and applies ready-for-merge.
7. Aug 10 17:36 — waynesun09 approves. Aug 10 18:49 — PR merged.

Review quality

The human reviewer found two actionable MEDIUM-severity issues that the review agent missed across all runs:

1. Missing reference table entries — The PR added status_notifications to perRepoConfig but did not update the merge-rules or code-defaults tables in layered-config-reference.md. The docs-currency sub-agent checked documentation but paradoxically generated false positives (claiming the PR was missing updates it actually contained) instead of catching the real gap. The challenger correctly filtered the false positives.
2. Doc comment referencing non-existent field — The StatusNotificationsReader interface doc comment mentioned "comment/reaction" but StatusNotificationConfig has no Reaction field. The correctness sub-agent found zero findings on the final review commit.

The agent's LOW findings (naming convention rationale, interface placement precedent) were well-reasoned and provided useful context, but were informational rather than actionable.

Evidence for existing issues

• #3893 ("Review agent should flag incomplete API reference tables in documentation PRs"): This retro provides fresh evidence. The docs-currency sub-agent failed to detect that layered-config-reference.md was missing status_notifications rows, while the human reviewer caught it immediately by recognizing the table structure convention.

Other observations

• Late dispatch: The first review agent run arrived 4 days after PR creation (Aug 6 → Aug 10), after the human reviewer had already identified both MEDIUM findings. The agent's review therefore added no incremental value beyond confirming no additional issues.
• Cancelled run cost: Run 31407218873 consumed ~14 minutes of compute before cancellation. This is expected behavior when a push supersedes an in-progress review — no change needed.
• Docs-currency false positives recurred in both successful runs (the sub-agent claimed the PR didn't update docs it actually did). The challenger caught these each time, so the system self-corrected, but the pattern consumed unnecessary challenger budget.

Proposals filed

• Correctness sub-agent should validate doc comments against referenced struct fields (in fullsend-ai/agents)