Repository navigation
docs: record release trigger scoping granularity as an ADR - #2043
Conversation
|
Warning Review limit reached
Next review available in: 21 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughDocuments package-level release-trigger scoping, whitelist enforcement, ChangesRelease trigger scoping
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
a3a1b6d to
242703e
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/adr/0003-release-trigger-scoping-granularity.md`:
- Around line 40-45: The ADR’s call-graph discussion should avoid claiming that
govulncheck can classify every changed file exactly. Rephrase the reachability
statement to acknowledge overapproximation and the assumptions around dynamic
behavior, CI entry points, full-package analysis, and platform-specific paths,
or document the entry-point validation needed to support the claim.
In `@docs/shared-release-inputs.md`:
- Around line 30-36: Update the earlier blanket rule for common Go sources to
state that packages in the shared whitelist trigger both components, while
packages in dispatcherOnlyCommonPackageRoots—including
cli/common/version/—trigger only the dispatcher. Ensure the documentation no
longer claims every common package requires both triggers and remains consistent
with the trigger behavior described around sharedCommonPackageRoots and
dispatcherOnlyCommonPackageRoots.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: eeb8b89f-abad-421d-8c7c-ea99a7cac3ee
📒 Files selected for processing (2)
docs/adr/0003-release-trigger-scoping-granularity.mddocs/shared-release-inputs.md
| Trigger rules resolve which components a shared input reaches by package, not by | ||
| symbol. `sharedCommonPackageRoots` and `dispatcherOnlyCommonPackageRoots` in | ||
| `release_trigger_guard.go` are whitelists whose alignment with | ||
| `go list -deps ./...` is test-enforced, so a `cli/common` package only one | ||
| component links triggers only that component. Within a package on the shared list, every | ||
| non-test Go change triggers both components regardless of which one can actually | ||
| execute the changed code. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align the blanket common-module rule with dispatcher-only packages.
The earlier bullets at Lines 15-16 say every common Go source requires both component triggers, but dispatcherOnlyCommonPackageRoots includes cli/common/version/, which should trigger only the dispatcher. Update the earlier rule to describe the shared whitelist and dispatcher-only exceptions; otherwise this page gives contradictory release instructions.
Suggested wording update
-- Common module sources (non-test `cli/common/**/*.go`, `cli/common/go.mod`, `cli/common/go.sum`) must be
- accompanied by changes under both `cli/project-runner/` and `cli/dispatcher/`.
+- Common module sources in the shared whitelist must be accompanied by changes under both
+ `cli/project-runner/` and `cli/dispatcher/`; dispatcher-only packages such as
+ `cli/common/version/` require only a `cli/dispatcher/` change.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/shared-release-inputs.md` around lines 30 - 36, Update the earlier
blanket rule for common Go sources to state that packages in the shared
whitelist trigger both components, while packages in
dispatcherOnlyCommonPackageRoots—including cli/common/version/—trigger only the
dispatcher. Ensure the documentation no longer claims every common package
requires both triggers and remains consistent with the trigger behavior
described around sharedCommonPackageRoots and dispatcherOnlyCommonPackageRoots.
Reviewing #2036 raised the question of why a runner-only change released the dispatcher too, and answering it took several wrong guesses about the mechanism before the facts were checked. The rule was documented but the decision behind its granularity was not, so the settled question had no reversal condition to consult and was relitigated from scratch. Add ADR 0003 with the decision, why the coarse rule stays, and the two finer alternatives (call-graph reachability, reproducible-build comparison) with their reversal conditions. Record the facts that were guessed wrong during the discussion: sharedInputsHash is a change marker no consumer compares across commits, the gap is a cost trade rather than a limit of static analysis, whitelist alignment with go list -deps is already test-enforced rather than manual, and no-op releases already carry a cli-release approval click today. Keep shared-release-inputs.md operational with the observable behaviour and a pointer to the ADR.
242703e to
9deedf9
Compare
RTA/CHA/VTA compute an overapproximation of dynamic calling behaviour, so per-file trigger classification derived from them is conservative rather than exact: spurious edges can only add a trigger, never drop one. Also note platform-specific files need per-platform analysis runs. Raised by CodeRabbit review on #2043.
1 participant
Why
Reviewing #2036 raised the question of why a runner-only change (
cli/common/errors/busy_editor_state.go, reachable only through the Unity RPC path the dispatcher never uses) released the dispatcher too. The trigger rule was documented indocs/shared-release-inputs.md, but the decision behind its granularity was not, so the settled question had no reversal condition to consult and was relitigated from scratch — with several wrong guesses about the mechanism along the way.What
docs/adr/0003-release-trigger-scoping-granularity.md: records the decision that trigger rules resolve component reachability at package granularity (whitelists test-enforced againstgo list -deps ./...byTestReleaseTriggerGuardCommonPackageWhitelistsMatchGoDependencies) and deliberately stop there. Rationale: the two failure directions are asymmetric (a missed release causes invisible version skew, an extra one costs a beta increment plus onecli-releaseapproval), and package-level scoping has a one-line ground truth to enforce against while call-graph scoping has none. Both finer alternatives — symbol-level call-graph reachability and reproducible-build comparison — are recorded as feasible-but-rejected, each with an explicit reversal condition.docs/shared-release-inputs.md: adds a section stating the observable behaviour (a shared-package change reachable from only one component still releases both, by design, with feat: configurable compile wait timeout and working timeout recovery #2036 as the worked example) and pointing to the ADR before anyone proposes finer scoping.The ADR also records the facts that were guessed wrong during the discussion, to close off the same wrong turns next time:
sharedInputsHashis a change marker no consumer compares across commits, the gap is a cost trade rather than a limit of static analysis, and whitelist alignment is machine-enforced rather than manual.Verification
releaseTriggerRulesinput, so no stamp update is required.go test ./internal/automation/ -run ReleaseTrigger -count=1incli/release-automationpasses (no behaviour touched).Related: #2042 (typed decoding of Unity JSON-RPC error data, filed from the same review thread).