Add ACP Closure Service (Phase 1: subscription end-date closure) - #1330
Merged
rksk merged 26 commits intoAug 6, 2026
Merged
Conversation
added 19 commits
July 24, 2026 14:23
Covers go.mod, internal/closure/decide.go and its tests, and the StubEndDate placeholder pending entity-service's endDate field (now confirmed live, not yet wired in).
…k chain) Three-tier fallback: business-contact role, then primary contact, then Account Manager nudge. Includes businessContactRole and StubOwnerEmail placeholders pending API-team confirmation.
OAuth2 client-credentials client against csm-integration-service, with RequiredScopes documenting the seven confirmed scope strings and correlation-ID forwarding. Includes SearchProjects, SearchAccountContacts, SearchProjectContacts, UpdateProject, GetAccount. Drops the x-user-id-token pass-through machinery, unused by this headless job. golang.org/x/oauth2 pinned to v0.27.0 to match existing repo convention.
Translates between suspensionProcessState's real JSON blob shape (based_on_subscription_end_date/based_on_due_invoices/based_on_compliance) and closure.NoticeWindow. WithSubscriptionEndDateState preserves based_on_due_invoices and based_on_compliance untouched, verified by a dedicated test using realistic multi-field data. Also corrects internal/entity's stale UpdateProject 401 comment to reflect confirmed M2M-write success, and removes the no-longer-needed internal/auth plan (superseded by entity.NewClient's real OAuth2 implementation).
…ier) Notifier interface with a logging implementation, since real email-sending is not yet built on the entity-service side (deferred pending message- queue design). Future real implementation will satisfy the same interface.
…rite-back orchestration) processProject is the core seam: fetches decision from closure.Decide, resolves recipients via the three-tier fallback, sends notifications through the injected notifier, and writes back suspensionProcessState via the suspensionstate package. Covers: suspend guard (closureState != Suspended before acting), day-0 email-then-suspend ordering with stop-on-failure (both fresh-success and retry-after-partial-failure paths), audience routing (90/60/30 internal- only vs 15/7/0 both), and notify-failure blocking the state write so closure.Decide naturally re-fires on the next run. 9 passing tests. Dry-run is an injection choice (projectUpdater interface), not a boolean threaded through core logic.
Loops through all open projects via SearchProjects, paginating at 100 per page (matching entity-service's cap), calling processProject for each. Two-tier failure handling: a single project's failure is logged and appended to Result.Failures without stopping the sweep; a page-fetch failure itself is fatal and returns a non-nil error, since there's no way to know what projects exist beyond an unreachable page. 4 new tests: single-page evaluation, multi-page offset progression (also validates the exact request-body shape), one-project-failure doesn't block the rest, and page-fetch-failure is fatal. 13 tests total across processProject + Run.
…ding cmd/acp-closure/main.go wires config loading (env-vars-only, CSM_INTEGRATION_* naming), the DRY_RUN safety-first toggle (defaults true unless explicitly set to false), a per-run correlation ID, and the real-vs-dry-run projectUpdater choice, then calls sweep.Run and logs the final Result. internal/sweep/dryrun.go: DryRunProjectUpdater, a logging stand-in for real writes. .env.example documents required config without real secrets. Module-local .gitignore protects .env and build artifacts. Also includes the earlier recipients.go comment update reflecting AM owner-email resolution now being actively worked on by the API team. Component is now runnable end-to-end via go run ./cmd/acp-closure.
the ACP closure service: ProjectView now includes a nested Account
{id, name} reference on /projects/search results (previously only
available on the single-project detail endpoint), and Account person
references (technicalOwner/accountManager/renewalAccountManager) now
carry an email field via a new PersonRef type, replacing the prior
owner-only, email-less EntityRef shape.
The nested Account *projectAccountRef shape was already correctly implemented; only a doc comment incorrectly claimed SearchProjects never carries an account field. Corrected to reflect confirmed reality (both GetProject and SearchProjects include account.id/account.name) — verified via direct Postman testing against real staging data.
Replaces StubOwnerEmail entirely with a real implementation: recipients.PersonRef + AccountManagerEmail (pure function, treats missing AM or missing email as legitimate absence, not an error), and sweep.resolveAccountManagerEmail (I/O half, calls GetAccount and parses the real accountManager.email shape). 6 new tests: 3 pure-package (populated, nil PersonRef, PersonRef-with-nil- email), 3 sweep-integration (real Postman response shape confirming the recipient resolves to a real email, plus the null/missing case sending with an empty recipient, no error). Empirically confirmed via direct Postman testing against a real staging account (technicalOwner/accountManager/renewalAccountManager all populated with real id/name/email). Resolves 4 of the original 5 open dependencies: endDate, M2M auth, suspensionProcessState shape, and AM owner-email. Remaining: the business-contact role literal string, and the real email-sending mechanism (pending WSO2's message-queue design).
Adds an optional TEST_PROJECT_ID env var that, when set, restricts the sweep to a single project (via a new GetProject client method) instead of searching all open projects — used for safely verifying behavior against one dedicated test project before running against the full staging dataset. Documented in .env.example.
Empirically confirmed via Postman that the live staging environment enforces a maximum page size of 50, not 100 as entity-service's own maxLimit constant claims — limit:50 returns 200, limit:51 returns 400. This is the fourth confirmed case this session of live API behavior diverging from entity-service's documented/coded values (alongside endDate, M2M auth, and suspensionProcessState shape) — caught by direct testing rather than trusting code alone. Updated pageSize in run.go and all corresponding test fixtures/assertions that referenced the old value. All 30 tests pass.
Notice gained a ResolvedVia field (recipients.ResolvedVia), logged as resolvedVia on every notice line. Reflects the real fallback tier used for customer/AM-nudge notices; left at zero value for internal notices, which never go through the fallback chain. 2 new notify tests verify the log line carries the correct structured attribute.
…same recipient When both the regular internal notice and the AM-nudge (configure a business contact) would go to the same real recipient, only the am_nudge notice fires now — the Account Manager gets one email, not two, per confirmed product decision. New shouldSuppressInternalNotice(internalRecipient, nudgeRecipient string) bool suppresses only when both are equal AND non-empty — an unresolved/empty AM email is not a real duplicate-email risk, and suppressing there would only hide useful debug signal without any real-world benefit. 4 new tests, including the pre-existing empty-recipient edge case re-verified to still pass unmodified. 34 tests total across the module.
Makefile: vet/test/build/clean targets, mirroring csm-integration- service's shape (no setup target, since this module isn't wired into the shared pre-push hook this round). README.md: quick start, config table, project structure, confirmed live-vs-documented discrepancies, and remaining open dependencies. Notes that 'make test' should be run manually before pushing. CLAUDE.md: deeper architecture/decision record — the pure/IO package split, why csm-integration-service rather than entity-service directly, the dry-run-by-injection design, notice-suppression logic, and suspensionProcessState's real confirmed shape. Verified make test/build/clean all actually work as documented.
Contributor
📝 WalkthroughWalkthroughAdded a Go ACP Closure Service. It authenticates with the CSM Integration Service, evaluates subscription end-date notice windows, resolves recipients, records suspension state, supports dry-run execution, and includes tests, configuration, and documentation. ChangesACP closure workflow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ACPCLI
participant SweepRun
participant EntityClient
participant ClosureRules
participant LoggingNotifier
participant ProjectUpdater
ACPCLI->>SweepRun: Run one closure sweep
SweepRun->>EntityClient: Fetch projects or scoped project
SweepRun->>ClosureRules: Evaluate notice window
SweepRun->>EntityClient: Fetch contacts and account data
SweepRun->>LoggingNotifier: Send closure notice
SweepRun->>ProjectUpdater: Persist notice state or suspension
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (3)
acp-closure-service/internal/entity/client_test.go (1)
75-81: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert bearer authentication in
TestDoSuccess.The handler returns success when the Authorization header is absent. A regression that removes OAuth2 authentication from resource requests would pass this test. Require
Authorization: Bearer test-token. The OAuth2 transport is responsible for adding this header. (pkg.go.dev)Proposed fix
if r.Method != http.MethodPost || r.URL.Path != "/projects/search" { t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) } +if got := r.Header.Get("Authorization"); got != "Bearer test-token" { + t.Errorf("Authorization = %q, want %q", got, "Bearer test-token") +}🤖 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 `@acp-closure-service/internal/entity/client_test.go` around lines 75 - 81, Update the HTTP handler in TestDoSuccess to assert that the request Authorization header exactly equals “Bearer test-token”, alongside the existing method and path checks. Keep the success response unchanged so the test verifies the OAuth2 transport adds the required bearer token.acp-closure-service/internal/closure/decide.go (1)
122-136: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSimplify
daysBetweenusingmath.Ceil.
daysBetweenhand-rolls a ceiling-toward-positive-infinity calculation with a branch on sign. This is correct, but it duplicates whatmath.Ceilalready does for both positive and negative inputs. Usemath.Ceilto remove the branch and the manual float-to-int comparison.♻️ Proposed simplification
+import "math" + func daysBetween(now, endDate time.Time) int { - d := endDate.Sub(now) - days := d.Hours() / 24 - if days != float64(int(days)) { - if days > 0 { - days = float64(int(days)) + 1 - } else { - days = float64(int(days)) - } - } - return int(days) + d := endDate.Sub(now) + return int(math.Ceil(d.Hours() / 24)) }🤖 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 `@acp-closure-service/internal/closure/decide.go` around lines 122 - 136, Update daysBetween to return the integer conversion of math.Ceil applied to the duration’s day value, removing the manual float comparison and sign branch while preserving ceiling-toward-positive-infinity behavior for positive and negative durations.acp-closure-service/internal/suspensionstate/suspensionstate_test.go (1)
27-83: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a round-trip test across all six notice windows.
eventTypeToWindowandwindowToEventTypeinsuspensionstate.goare two hand-maintained maps. A table-driven test that callsWithSubscriptionEndDateStatefor each of the sixclosure.NoticeWindowconstants and feeds the result back throughLastNoticeWindowwould catch any future drift between the two maps before it reaches production.🤖 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 `@acp-closure-service/internal/suspensionstate/suspensionstate_test.go` around lines 27 - 83, Extend TestLastNoticeWindow with a table-driven round-trip case covering all six closure.NoticeWindow constants: build each state using WithSubscriptionEndDateState, pass its serialized result to LastNoticeWindow, and assert the original window is returned without error. Use the existing event-type mapping behavior and ensure every notice-window constant is included.
🤖 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 `@acp-closure-service/cmd/acp-closure/main.go`:
- Around line 84-86: Update the result handling in main so that after logging
entries from result.Failures, it exits with status 1 when the collection is
non-empty. Preserve the existing successful return path when no project failures
occur.
- Around line 142-144: Update the environment-loading logic around os.Setenv in
loadDotEnv to use os.LookupEnv, setting values from .env only when the variable
is unset. Preserve explicitly configured empty environment variables so mustEnv
can handle them rather than replacing them.
In `@acp-closure-service/go.mod`:
- Line 3: Update the go directive in go.mod from Go 1.26.4 to 1.26.5, and align
the project’s build images with Go 1.26.5 so all builds enforce the same minimum
patched version.
In `@acp-closure-service/internal/entity/client.go`:
- Around line 104-107: Update the HTTP client setup around tokenCtx and
httpClient so authenticated API requests reject redirects before the OAuth2
transport follows them. Configure the http.Client used by cc.Client with a
CheckRedirect policy that returns http.ErrUseLastResponse, preserving the
existing timeout settings.
In `@acp-closure-service/internal/suspensionstate/suspensionstate.go`:
- Around line 101-118: Update WithSubscriptionEndDateState to validate the
second return value from windowToEventType before constructing section. If
window has no known mapping, return a descriptive error instead of persisting an
empty event_type; preserve the existing serialization flow for valid windows.
In `@acp-closure-service/internal/sweep/sweep.go`:
- Around line 183-211: Update fetchContacts so that after parsing project
contacts, it checks proj.accountID() and returns the project contacts with no
account contacts when the account ID is empty, avoiding SearchAccountContacts;
retain the existing account-contact search and error handling for projects with
a non-empty account ID.
---
Nitpick comments:
In `@acp-closure-service/internal/closure/decide.go`:
- Around line 122-136: Update daysBetween to return the integer conversion of
math.Ceil applied to the duration’s day value, removing the manual float
comparison and sign branch while preserving ceiling-toward-positive-infinity
behavior for positive and negative durations.
In `@acp-closure-service/internal/entity/client_test.go`:
- Around line 75-81: Update the HTTP handler in TestDoSuccess to assert that the
request Authorization header exactly equals “Bearer test-token”, alongside the
existing method and path checks. Keep the success response unchanged so the test
verifies the OAuth2 transport adds the required bearer token.
In `@acp-closure-service/internal/suspensionstate/suspensionstate_test.go`:
- Around line 27-83: Extend TestLastNoticeWindow with a table-driven round-trip
case covering all six closure.NoticeWindow constants: build each state using
WithSubscriptionEndDateState, pass its serialized result to LastNoticeWindow,
and assert the original window is returned without error. Use the existing
event-type mapping behavior and ensure every notice-window constant is included.
🪄 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: 50502ea4-6bff-4056-be5c-048903d68dcf
⛔ Files ignored due to path filters (1)
acp-closure-service/go.sumis excluded by!**/*.sum
📒 Files selected for processing (27)
acp-closure-service/.env.exampleacp-closure-service/.gitignoreacp-closure-service/CLAUDE.mdacp-closure-service/Makefileacp-closure-service/README.mdacp-closure-service/cmd/acp-closure/main.goacp-closure-service/go.modacp-closure-service/internal/apierror/apierror.goacp-closure-service/internal/closure/decide.goacp-closure-service/internal/closure/decide_test.goacp-closure-service/internal/entity/client.goacp-closure-service/internal/entity/client_test.goacp-closure-service/internal/entity/entity.goacp-closure-service/internal/notify/notify.goacp-closure-service/internal/notify/notify_test.goacp-closure-service/internal/recipients/recipients.goacp-closure-service/internal/recipients/recipients_test.goacp-closure-service/internal/suspensionstate/suspensionstate.goacp-closure-service/internal/suspensionstate/suspensionstate_test.goacp-closure-service/internal/sweep/dryrun.goacp-closure-service/internal/sweep/helpers_test.goacp-closure-service/internal/sweep/run.goacp-closure-service/internal/sweep/run_test.goacp-closure-service/internal/sweep/sweep.goacp-closure-service/internal/sweep/sweep_test.goacp-closure-service/internal/sweep/types.goacp-closure-service/internal/sweep/types_test.go
rksk
reviewed
Aug 3, 2026
rksk
left a comment
rksk
left a comment
Contributor
There was a problem hiding this comment.
Four inline comments below on the sweep/orchestration layer. Two of them (the suspend guard and the empty-account-ID path) I'd treat as merge blockers; the other two are hardening for a job that writes to a shared, live data source on a cron.
Separately, and not inline because it isn't tied to one line: notify.LoggingNotifier is the only Notifier implementation, but recordNoticeSent writes actionSendEmailNotification: "SUCCESSFUL" unconditionally. With DRY_RUN=false that means the job records notices as sent, advances based_on_subscription_end_date.event_type, and suspends past-due projects, while no email was ever sent. DRY_RUN defaulting to true is good, but it is one config value away from that outcome. Consider making the absence of a real sender structurally block writes (fail fast in main, or write IGNORED rather than SUCCESSFUL when the notifier does not actually send) rather than relying on the default.
The pure/I-O split, the DI-based dry-run, and the byte-for-byte preservation of the sibling suspensionProcessState sections are all well done.
rksk
reviewed
Aug 3, 2026
rksk
left a comment
rksk
left a comment
Contributor
There was a problem hiding this comment.
One more, on placement: this shouldn't be a new top-level directory. Detail inline on go.mod, since the module path has to change with the move and that ripples through every import in the PR — much cheaper to do now than after merge.
rksk
requested changes
Aug 3, 2026
Rashmika998
reviewed
Aug 3, 2026
added 3 commits
August 3, 2026 14:30
…hmika, and CodeRabbit - Move acp-closure-service under integrations/ to match repo convention - Guard fetchContacts against projects with no linked account - Exit non-zero on partial sweep failures - Bound the pagination loop by Total, not just HasMore - Record IGNORED instead of SUCCESSFUL when the notifier doesn't deliver - Fix suspend guard to check endDateClosureState, not closureState - Make CSM_INTEGRATION_SCOPES a required env var instead of hardcoded - loadDotEnv: use LookupEnv to preserve explicitly-set empty env vars - Reject 3xx redirects in entity.Client to prevent an Authorization leak - suspensionstate: error on an unmapped NoticeWindow instead of writing event_type: ""
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
integrations/acp-closure-service/internal/closure/decide_test.go (1)
73-115: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a same-window re-run case.
The catch-up table covers advancing to a narrower window. It does not cover the daily-idempotency case:
lastNoticeWindow = &w60with 55 days remaining must reportFires = false. That case protects against a repeated notice on every sweep inside the same window.💚 Proposed test case
w90 := NoticeWindow90 + w60 := NoticeWindow60 @@ { name: "already notified at 90, jumped to 10 days remaining: catches up to 15, not re-firing 90", daysRemaining: 10, lastNoticeWindow: &w90, wantWindow: NoticeWindow15, wantFires: true, }, + { + name: "already notified at 60, still inside the 60 window: nothing re-fires", + daysRemaining: 55, + lastNoticeWindow: &w60, + wantFires: false, + }, }🤖 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 `@integrations/acp-closure-service/internal/closure/decide_test.go` around lines 73 - 115, Add a table-driven case to TestDecide_MissedRunCatchesUpToNearestWindow with lastNoticeWindow set to NoticeWindow60 and 55 days remaining, asserting Fires is false. Keep the existing catch-up cases unchanged and ensure the same-window sweep does not produce a notice.integrations/acp-closure-service/internal/closure/decide.go (1)
112-120: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unused exported
StubEndDateAPI.
StubEndDateexists only as a comment and declaration, ignoresprojectID, and returns the zerotime.Time. Delete the function or keep any future end-date seam unavailable untilentity-serviceexposesendDate.🤖 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 `@integrations/acp-closure-service/internal/closure/decide.go` around lines 112 - 120, Remove the exported StubEndDate function and its associated placeholder comments from the closure package. Do not add a replacement seam or retain the unused projectID parameter and zero time.Time return.integrations/acp-closure-service/internal/recipients/recipients.go (1)
74-94: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider skipping contacts that have no email.
ResolveCustomerContactselects the first contact by role or primary flag only. It does not checkRecipient, and no AM nudge fires.The package already models missing contact data as normal for this data source (see the
PersonRef.Emailnote at Line 98). If an empty contact email is possible upstream, treat it the same as no contact at that tier.♻️ Proposed tier selection that requires an email
for _, c := range projectContacts { - if hasBusinessContactRole(c) { + if hasBusinessContactRole(c) && c.Email != "" { return Resolution{ CustomerContact: &Contact{Name: c.Name, Email: c.Email}, ResolvedVia: ResolvedViaBusinessContact, } } } for _, c := range accountContacts { - if c.IsPrimary { + if c.IsPrimary && c.Email != "" { return Resolution{ CustomerContact: &Contact{Name: c.Name, Email: c.Email}, ResolvedVia: ResolvedViaPrimaryContact, } } }🤖 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 `@integrations/acp-closure-service/internal/recipients/recipients.go` around lines 74 - 94, Update ResolveCustomerContact so both the business-contact loop and primary account-contact loop skip contacts whose Email is empty before selecting them. Preserve the existing tier priority, resolution metadata, and NeedsAMNudge fallback when no eligible contact has an email.integrations/acp-closure-service/internal/sweep/run_test.go (1)
59-94: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the page size as well as the offset.
This test already decodes
searchProjectsRequest, but it checksPagination.Offsetonly. No test assertsPagination.Limit == 50. The comment inrun.go(Lines 27-31) records thatlimit: 51returns a 400 against the live API, so the exact page size is a load-bearing constant. Add the assertion here to protect it.♻️ Proposed assertion
gotOffsets = append(gotOffsets, req.Pagination.Offset) + if req.Pagination.Limit != 50 { + t.Errorf("Pagination.Limit = %d, want 50", req.Pagination.Limit) + }Based on learnings, this follows the coding guideline "
Runmust paginate/projects/searchwith a page size of exactly 50".🤖 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 `@integrations/acp-closure-service/internal/sweep/run_test.go` around lines 59 - 94, Update TestRun_MultiPagePaginatesUntilHasMoreFalse to assert req.Pagination.Limit equals 50 for each decoded searchProjectsRequest, alongside the existing offset checks. Preserve the current pagination and result assertions.Source: Coding guidelines
integrations/acp-closure-service/internal/sweep/sweep_test.go (1)
86-99: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a
processProject-level preservation test for the sibling suspension dimensions.
integrations/acp-closure-service/internal/sweep/sweep_test.goonly decodesbased_on_subscription_end_datein update-body assertions. No test givesprocessProjectaSuspensionProcessStatewith populatedbased_on_due_invoicesandbased_on_compliance, then asserts those keys are unchanged inupdater.calls[0].body. Add one case with all three dimensions, fire the 90-day window, and compare the two untouched subtrees against the input so regression that drops sibling Phase 2 data does not pass.🤖 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 `@integrations/acp-closure-service/internal/sweep/sweep_test.go` around lines 86 - 99, Add a processProject-level test in sweep_test.go that supplies SuspensionProcessState with populated based_on_subscription_end_date, based_on_due_invoices, and based_on_compliance values, triggers the 90-day window, then decodes updater.calls[0].body and asserts the due-invoices and compliance subtrees match the input unchanged while retaining the existing 90_days_notice assertion.Source: Coding guidelines
🤖 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 `@integrations/acp-closure-service/internal/notify/notify.go`:
- Around line 70-79: Update LoggingNotifier.Send to stop logging
notice.Recipient at Info level, while preserving the existing ResolvedVia field
and all other required notice metadata. Redact or omit the recipient value so
customer and Account Manager contact details are never written to logs.
In `@integrations/acp-closure-service/internal/sweep/sweep.go`:
- Around line 136-142: Validate that the contact selected by
ResolveCustomerContact has a non-empty email before sending the KindCustomer
notification in the sweep flow. If the CustomerContact email is empty, reject
that resolution and continue the existing fallback chain so it reaches
NeedsAMNudge instead of calling ntf.Send with an empty Recipient.
In `@integrations/acp-closure-service/README.md`:
- Around line 30-39: Update both Go version references in the README—under the
runtime details and prerequisites—to require Go 1.26.5+ instead of Go 1.26+,
matching the version declared in go.mod.
---
Nitpick comments:
In `@integrations/acp-closure-service/internal/closure/decide_test.go`:
- Around line 73-115: Add a table-driven case to
TestDecide_MissedRunCatchesUpToNearestWindow with lastNoticeWindow set to
NoticeWindow60 and 55 days remaining, asserting Fires is false. Keep the
existing catch-up cases unchanged and ensure the same-window sweep does not
produce a notice.
In `@integrations/acp-closure-service/internal/closure/decide.go`:
- Around line 112-120: Remove the exported StubEndDate function and its
associated placeholder comments from the closure package. Do not add a
replacement seam or retain the unused projectID parameter and zero time.Time
return.
In `@integrations/acp-closure-service/internal/recipients/recipients.go`:
- Around line 74-94: Update ResolveCustomerContact so both the business-contact
loop and primary account-contact loop skip contacts whose Email is empty before
selecting them. Preserve the existing tier priority, resolution metadata, and
NeedsAMNudge fallback when no eligible contact has an email.
In `@integrations/acp-closure-service/internal/sweep/run_test.go`:
- Around line 59-94: Update TestRun_MultiPagePaginatesUntilHasMoreFalse to
assert req.Pagination.Limit equals 50 for each decoded searchProjectsRequest,
alongside the existing offset checks. Preserve the current pagination and result
assertions.
In `@integrations/acp-closure-service/internal/sweep/sweep_test.go`:
- Around line 86-99: Add a processProject-level test in sweep_test.go that
supplies SuspensionProcessState with populated based_on_subscription_end_date,
based_on_due_invoices, and based_on_compliance values, triggers the 90-day
window, then decodes updater.calls[0].body and asserts the due-invoices and
compliance subtrees match the input unchanged while retaining the existing
90_days_notice assertion.
🪄 Autofix
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: 16c3e829-297f-4c9e-9c9e-ff9927afd111
⛔ Files ignored due to path filters (1)
integrations/acp-closure-service/go.sumis excluded by!**/*.sum
📒 Files selected for processing (30)
.gitignoreREADME.mdintegrations/acp-closure-service/.env.exampleintegrations/acp-closure-service/.gitignoreintegrations/acp-closure-service/CLAUDE.mdintegrations/acp-closure-service/Makefileintegrations/acp-closure-service/README.mdintegrations/acp-closure-service/cmd/acp-closure/main.gointegrations/acp-closure-service/cmd/acp-closure/main_test.gointegrations/acp-closure-service/go.modintegrations/acp-closure-service/internal/apierror/apierror.gointegrations/acp-closure-service/internal/closure/decide.gointegrations/acp-closure-service/internal/closure/decide_test.gointegrations/acp-closure-service/internal/entity/client.gointegrations/acp-closure-service/internal/entity/client_test.gointegrations/acp-closure-service/internal/entity/entity.gointegrations/acp-closure-service/internal/notify/notify.gointegrations/acp-closure-service/internal/notify/notify_test.gointegrations/acp-closure-service/internal/recipients/recipients.gointegrations/acp-closure-service/internal/recipients/recipients_test.gointegrations/acp-closure-service/internal/suspensionstate/suspensionstate.gointegrations/acp-closure-service/internal/suspensionstate/suspensionstate_test.gointegrations/acp-closure-service/internal/sweep/dryrun.gointegrations/acp-closure-service/internal/sweep/helpers_test.gointegrations/acp-closure-service/internal/sweep/run.gointegrations/acp-closure-service/internal/sweep/run_test.gointegrations/acp-closure-service/internal/sweep/sweep.gointegrations/acp-closure-service/internal/sweep/sweep_test.gointegrations/acp-closure-service/internal/sweep/types.gointegrations/acp-closure-service/internal/sweep/types_test.go
added 4 commits
August 5, 2026 13:48
- Don't log the raw notice recipient (PII) in LoggingNotifier.Send; ResolvedVia already gives the observability that's actually needed - ResolveCustomerContact now requires a non-empty email before accepting a business/primary contact match, so a contact on file with no email falls through to the AM nudge instead of resolving to an empty Recipient - Fix README's stated Go version (1.26+ -> 1.26.5+) to match go.mod
LoggingNotifier is currently the only 'sender' that exists (real email sending isn't implemented yet), so the recipient in the log is the only way to verify from logs alone who a notice would have gone to during this Phase 1 rollout. Worth revisiting once real sending exists.
Contributor
|
@coderabbitai resume |
Contributor
✅ Action performedReviews resumed. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@integrations/acp-closure-service/internal/notify/notify.go`:
- Line 75: Remove the "recipient", notice.Recipient structured-log field from
the notification logging call, while retaining kind, window, projectID, and
resolvedVia. Add a regression test for the relevant notification logging path
that verifies recipient data is absent from the emitted log attributes.
🪄 Autofix
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: 70b2ac20-1c0e-49ae-9b33-1f9845f9d537
📒 Files selected for processing (2)
integrations/acp-closure-service/internal/notify/notify.gointegrations/acp-closure-service/internal/notify/notify_test.go
💤 Files with no reviewable changes (1)
- integrations/acp-closure-service/internal/notify/notify_test.go
rksk
approved these changes
Aug 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements Phase 1 of the ACP (Account Closure Process) migration from
ServiceNow — a Go-based scheduled component that replaces the legacy
ServiceNow subscription end-date closure automation.
Scope: subscription end-date closure only (90/60/30/15/7/0-day notice
cascade, then suspend). Invoice- and compliance-based closure (Phase 2)
are explicitly out of scope — see
docs/legacy-servicenow-reference/for the original ServiceNow Script Include source this replaces.
Closes wso2-enterprise/digiops-cs#2524
Architecture
New top-level Go module (
acp-closure-service/), deployed as a ChoreoScheduled Task (run-to-completion CLI, not a long-running process).
Calls
csm-integration-service(notentity-servicedirectly) via M2Mclient-credentials auth — see CLAUDE.md for the full architecture
rationale.
Package structure:
internal/closure— pure decision logic (days-remaining, cascadingnotice-window thresholds)
internal/recipients— three-tier customer contact fallback(business-contact role → primary contact → notify Account Manager)
internal/entity— API client for csm-integration-serviceinternal/suspensionstate— translates between the realsuspensionProcessStateJSON blob and internal types, preservinginvoice/compliance sections untouched
internal/notify— notification interface (currently logs only —real email-sending is pending WSO2's message-queue design)
internal/sweep— orchestrates the full daily sweep, with a dry-runtoggle (
DRY_RUNenv var, defaults totrue) via dependency injectionTesting
throughout development
staging (1 known, pre-existing data issue, unrelated to this code)
POC project) against the same real data, confirming identical behavior
to local testing
Notes for reviewers
DRY_RUNdefaults totrue— no real writes or emails happen unlessexplicitly set to
falsewhere live API behavior diverged from documented/coded values
Summary by CodeRabbit
New Features
Documentation
Tests