Skip to content

demo(04-breaking-api-change): 04 — Breaking change to public PaymentClient signature#153

Open
EricCogen wants to merge 1 commit intomainfrom
demo/04-breaking-api-change
Open

demo(04-breaking-api-change): 04 — Breaking change to public PaymentClient signature#153
EricCogen wants to merge 1 commit intomainfrom
demo/04-breaking-api-change

Conversation

@EricCogen
Copy link
Copy Markdown
Owner

04 — Breaking change to public PaymentClient signature

Expected verdict: ❌ Fails — GauntletCI should fire GCI0004 (breaking public API change).

What changed

IPaymentClient.ChargeAsync lost its CancellationToken ct = default
parameter — purportedly because nobody was passing one through. Five files
were updated to compile against the new signature:

  • src/OrderService/Payments/IPaymentClient.cs — interface definition
  • src/OrderService/Payments/PaymentClient.cs — implementation
  • src/OrderService/Processing/OrderProcessor.cs — call site
  • src/OrderService/Controllers/PaymentsController.cs — call site
  • tests/OrderService.Tests/Fakes/FakePaymentClient.cs — test fake

Why this is risky

  • IPaymentClient is a public interface — any downstream package
    implementing it (a custom adapter, a mock) will fail to compile after
    upgrading.
  • Removing a parameter is a binary-incompatible change even if no internal
    caller uses it.
  • The diff looks like a routine cleanup, but it's a SemVer-major change.

What GauntletCI catches

GCI0004 Breaking public API change — a public/protected member's
signature changed in a non-additive way (parameter removed).

…lient signature

See scenarios/04-breaking-api-change/README.md for the expected verdict.
Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GauntletCI found issues in this PR. See inline comments for details.

}

public Task<PaymentResult> ChargeAsync(PaymentRequest request, CancellationToken ct = default)
public Task<PaymentResult> ChargeAsync(PaymentRequest request)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GCI0003 — Behavioral Change Detection

Method signature changed: 'ChargeAsync' in src/OrderService/Payments/PaymentClient.cs

Evidence:

- public Task<PaymentResult> ChargeAsync(PaymentRequest request, CancellationToken ct = default)
+ public Task<PaymentResult> ChargeAsync(PaymentRequest request)

⚠️ Why it matters: Signature changes can break callers that haven't been updated.

💡 Suggested action: Verify all callers are updated and consider adding an overload for backward compatibility.

Confidence: Medium | Severity: Block

}

public Task<PaymentResult> ChargeAsync(PaymentRequest request, CancellationToken ct = default)
public Task<PaymentResult> ChargeAsync(PaymentRequest request)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GCI0004 — Breaking Change Risk

Public API signature changed: 'ChargeAsync' in src/OrderService/Payments/PaymentClient.cs

Evidence:

- public Task<PaymentResult> ChargeAsync(PaymentRequest request, CancellationToken ct = default)
+ public Task<PaymentResult> ChargeAsync(PaymentRequest request)

⚠️ Why it matters: Changing a public method signature is a breaking change for callers not in this diff.

💡 Suggested action: Provide a backward-compatible overload or bump the major version.

Confidence: Medium | Severity: Block

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 88077c36c2

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

public interface IPaymentClient
{
Task<PaymentResult> ChargeAsync(PaymentRequest request, CancellationToken ct = default);
Task<PaymentResult> ChargeAsync(PaymentRequest request);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Preserve CancellationToken in public payment interface

IPaymentClient is a public contract, and removing the CancellationToken parameter from ChargeAsync is a source/binary breaking API change for downstream consumers. Any external implementation or call site compiled against the previous signature will fail to compile after upgrade, even though this repo’s internal call sites were updated, so this should remain backward-compatible (or be released only as an explicit major-version break).

Useful? React with 👍 / 👎.

Comment on lines 43 to +45
var result = await _payments.ChargeAsync(
new PaymentRequest(order.Id, priced.Total, order.Customer.Email),
ct).ConfigureAwait(false);
new PaymentRequest(order.Id, priced.Total, order.Customer.Email))
.ConfigureAwait(false);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Propagate operation cancellation into payment charging

ProcessAsync still accepts a CancellationToken, but this call no longer passes it into payment execution, so request/job cancellation can stop repository calls while the charge path keeps running to completion. In cancellation scenarios (HTTP disconnects, worker shutdown), this can still create authorizations after the caller has abandoned the operation.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant