feat(#5994): add status_notifications support to per-repo config - #5997
feat(#5994): add status_notifications support to per-repo config#5997ralphbean wants to merge 1 commit into
Conversation
perRepoConfig gains a StatusNotifications() accessor (backed by a new Notifications field, following the same overlay -> base -> code-defaults fallback chain used by other per-repo settings) so per-repo installs can enable comment start/completion notifications the same way org installs can. StatusNotifications() moves to the shared ConfigReader interface since both config modes now implement it, which lets setupStatusNotifier in run.go read 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. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Ralph Bean <rbean@redhat.com>
ralphbean
added
the
fullsend-fix
PR Summary by QodoAdd status_notifications to per-repo config and migrate carry-over
AI Description
Diagram
High-Level Assessment
Files changed (11)
|
Code Review by Qodo
1. Procedures not numbered
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| For per-org installs, nest it under `defaults`: | ||
|
|
||
| ```yaml |
There was a problem hiding this comment.
2. Procedures not numbered 📜 Skill insight ✧ Quality
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.
Agent Prompt
## 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
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
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.
2 participants
Fixes #5994
Summary
perRepoConfiggets aStatusNotifications()accessor (backed by anew
Notificationsfield) following the same overlay -> base ->code-defaults fallback chain used by other per-repo settings
(
ConfigRoles,ConfigRuntime, etc).StatusNotifications()moves fromOrgConfigReaderto the sharedConfigReaderinterface, since both config modes now implement it.setupStatusNotifierininternal/cli/run.goreads it directlyinstead of type-asserting to
OrgConfigReader.repos migratenow carriesstatus_notificationsover into thegenerated per-repo
config.yamlinstead of warning that it has noper-repo equivalent (per-repos migrate does not carry over org config fields or register repos in mint #5822 context in the issue).
docs/cli/repos.mdconfig carry-over table, anddocs/guides/getting-started/operations.mdshows 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/behaviourteste2e harness (per-repo-only)can exercise the reaction feature.
Test plan
go build ./...go vet ./...gofmt -lcleanmake lintgo test ./internal/config/... ./internal/repos/... ./internal/cli/...— all greenround-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