Migrate TMCH, SMD, and Fee models to java.time

GEMINI.md

@@ -15,7 +15,7 @@ This document outlines foundational mandates, architectural patterns, and projec
 
 ## 2. Time and Precision Handling (java.time Migration)
 
-- **Idiomatic java.time Usage:** Avoid redundant conversions between `Instant` and `DateTime`. If a field or parameter is an `Instant`, use it directly. Do not convert to `DateTime` just to call a deprecated method if an `Instant` alternative exists or can be easily created.
+- **Idiomatic java.time Usage:** Avoid redundant conversions between `Instant` and `DateTime`. If a field or parameter is an `Instant`, use it directly. Do not convert to `DateTime` just to call a deprecated method if an `Instant` alternative exists or can be easily created. Furthermore, you should not call `toInstant()` or `toDateTime()` conversion methods when not strictly necessary; always prefer to use an alternative method that returns the correct type if one exists (e.g. use `tm().getTxTime()` which returns an `Instant` instead of calling `tm().getTransactionTime().toInstant()`).
 - **Millisecond Precision:** Always truncate `Instant.now()` to milliseconds (using `.truncatedTo(ChronoUnit.MILLIS)`) to maintain consistency with Joda `DateTime` and the PostgreSQL schema (which enforces millisecond precision via JPA converters).
 - **Clock Injection:**
     - Avoid direct calls to `Instant.now()`, `DateTime.now()`, `ZonedDateTime.now()`, or `System.currentTimeMillis()`.
@@ -56,6 +56,7 @@ This document outlines foundational mandates, architectural patterns, and projec
 ## Performance and Efficiency
 - **Turn Minimization:** Aim for "perfect" code in the first iteration. Iterative fixes for checkstyle or compilation errors consume significant context and time.
 - **Context Management:** Use sub-agents for batch refactoring or high-volume output tasks to keep the main session history lean and efficient.
+- **Code Formatting:** Do not write custom Python scripts or manual regex replacements to fix code formatting issues (e.g., unused imports, import ordering, line length). Instead, use the project's built-in formatting tools: run `./gradlew spotlessApply` to fix unused/unordered imports and `./gradlew javaIncrementalFormatApply` (or `google-java-format --replace <files>`) to automatically fix Java formatting and indentation errors.
 
 ## General Code Review Lessons & Avoidable Mistakes
 Based on historical PR reviews, avoid the following common mistakes:
@@ -89,11 +90,28 @@ This document captures high-level architectural patterns, lessons learned from l
 - **Committing:** Always create a new commit on the branch if one hasn't been created yet for the branch's specific work. Only perform amending (`git commit --amend --no-edit`) for subsequent changes once the initial commit has been successfully created.
 - **One Commit Per PR:** All changes for a single PR must be squashed into a single commit before merging.
 - **Default to Amend:** Once an initial commit is created for a PR, all subsequent functional changes should be amended into that same commit by default (`git commit --amend --no-edit`). This ensures the PR remains a single, clean unit of work throughout the development lifecycle.
-- **Commit Message Style:** Follow standard Git commit best practices. The subject line (first line) should be concise, capitalized, and **must not end with punctuation** (e.g., a period).
-- **Final Validation:** Always run `git status` as the final step before declaring a task complete to ensure all changes are committed and the working directory is clean.
-- **Commit Verification:** After any commit or amendment, explicitly verify the success of the operation (e.g., using `git status` and reviewing the diff). Never report a Git operation as "done" without having first successfully executed the command and confirmed the repository state.
+- **Commit Message Style:** Follow standard Git commit best practices. The subject line (first line) MUST be a maximum of 50 characters, concise, capitalized, and **must not end with punctuation** (e.g., a period). The body MUST explicitly encapsulate and summarize all changes made across the entire diff, detailing the "what" and "why" comprehensively.
+- **Strict Completion Verification:** You MUST NEVER declare a task, commit, or amendment as complete until you have explicitly verified that the workspace is clean. You MUST follow this exact sequence of actions across multiple conversational turns if necessary:
+  1. Execute the `git commit` or `git commit --amend` command.
+  2. Wait for the tool to return successfully.
+  3. Execute `git status`.
+  4. Wait for the tool to return and explicitly verify the output contains `nothing to commit, working tree clean` (or similar indication that no unstaged changes remain). If changes remain, stage them and amend the commit, then repeat this verification loop.
+  5. **Only after** step 4 has successfully returned a clean working directory may you generate a text response to the user declaring that the task is complete.
 - **Diff Review:** Before finalizing a task, review the full diff (e.g., `git diff HEAD^`) to ensure all changes are functional and relevant. Identify and revert any formatting-only changes in files that do not contain functional updates to keep the commit focused.
 
+## Self-Review Guidelines
+Before finalizing any PR or declaring a task complete, you MUST perform a thorough, rigorous self-review of your entire diff. Run `git diff HEAD^` (or review the staged changes) and actively verify the following against every modified line:
+
+1. **Imports & FQNs:** Did I leave any fully-qualified class names or static variables inline? Did I add the necessary imports for them? *Crucial Exception:* If the file already imports a class with the identical name (e.g., it uses both `java.time.Duration` and `org.joda.time.Duration`), one MUST remain fully qualified to avoid a compilation conflict.
+2. **Redundant Conversions:** Did I use `toDateTime(clock.now())` where `clock.nowUtc()` would suffice? Did I use `toDateTime(END_INSTANT)` instead of `END_OF_TIME`? Did I use `.toInstant()` or `.toDateTime()` on something that could be avoided by using a different method overload (e.g., `tm().getTxTime()`)?
+3. **Verbose Math:** Did I write any verbose time conversions inline? Are there `DateTimeUtils` methods I should be using instead? If not, should I abstract this math into `DateTimeUtils`?
+4. **Assertion Cleanliness:** Am I polluting test assertions with `toDateTime(...)` wraps? If so, I need to add overloaded assertions to the Truth Subjects instead.
+5. **Diff Scope:** Are there any formatting-only changes in files that I did not functionally modify? If so, revert them. Does the total line count of the diff align with the approved scope (e.g., ~1,000 lines for migrations)?
+6. **Commit Message:** Does the commit message title fit within 50 characters? Does the body encapsulate the entirety of the changes across the diff cleanly and professionally?
+7. **Missing Tests & Coverage:** *Perform a structured check for any new methods or modified behavior.* Did I add a new utility method (like `plusMonths(Instant, int)`) or change core logic? If so, I MUST open the corresponding test file and write tests to cover the new functionality (including edge cases, negative values, and leap years) before considering the task complete. A code review is not thorough if it only checks for compilation. I must actively ensure every new branch of logic has a test.
+
+Only after actively confirming these checks against your diff are you permitted to finalize the task.
+
 ## Refactoring & Migration Guardrails
 
 
@@ -115,7 +133,10 @@ This project treats Error Prone warnings as errors.
 
 ## 🚫 Common Pitfalls to Avoid
 
+- **Redundant Parses:** Never write `toDateTime(Instant.parse(...))` or `toInstant(DateTime.parse(...))`. If you need a `DateTime`, use `DateTime.parse(...)` directly. If you need an `Instant`, use `Instant.parse(...)` directly.
+- **cloneProjectedAtTime vs cloneProjectedAtInstant:** When converting tests and logic that use `clock.now()` to project resource state into the future or past, do not wrap the Java `Instant` in `toDateTime()` just to call `cloneProjectedAtTime()`. Instead, switch the method call to use the native `cloneProjectedAtInstant()` method which is available on all `EppResource` models.
 - **Do not go in circles with the build:** If you see an `InlineMeSuggester` error, apply the suppression to **ALL** similar methods in that file and related files in one turn. Do not fix them one by one.
+- **Exception Conversion in Tests:** When migrating time types (e.g., from Joda `DateTime` to Java `Instant`), be extremely careful with tests that verify parsing failures (e.g., `assertThrows(IllegalArgumentException.class, ...)`). Joda's `DateTime.parse()` throws an `IllegalArgumentException` on failure, but `Instant.parse()` throws a `java.time.format.DateTimeParseException`. You must update the expected exception type in these tests to ensure they actually test the correct behavior, and verify the tests are not failing prematurely on the first line if it contains invalid data meant to be ignored.
 - Dagger/AutoValue corruption: If you modify a builder or a component incorrectly, Dagger will fail to generate code, leading to hundreds of "cannot find symbol" errors. If this happens, `git checkout` the last working state of the specific file and re-apply changes more surgically.
 - **`replace` tool context**: When using `replace` on large files (like `Tld.java` or `DomainBase.java`), provide significant surrounding context. These files have many similar method signatures (getters/setters) that can lead to incorrect replacements.
 

common/src/main/java/google/registry/util/DateTimeUtils.java

@@ -176,6 +176,18 @@ public static Instant plusYears(Instant now, long years) {
         : now.atZone(ZoneOffset.UTC).plusYears(1).plusYears(years - 1).toInstant();
   }
 
+  /** Adds months to a date. */
+  public static Instant plusMonths(Instant now, int months) {
+    checkArgument(months >= 0);
+    return now.atZone(ZoneOffset.UTC).plusMonths(months).toInstant();
+  }
+
+  /** Subtracts months from a date. */
+  public static Instant minusMonths(Instant now, int months) {
+    checkArgument(months >= 0);
+    return now.atZone(ZoneOffset.UTC).minusMonths(months).toInstant();
+  }
+
   /**
    * Subtracts years from a date, in the {@code Duration} sense of semantic years. Use this instead
    * of {@link DateTime#minusYears} to ensure that we never end up on February 29.

core/src/main/java/google/registry/beam/billing/ExpandBillingRecurrencesPipeline.java

@@ -26,7 +26,6 @@
 import static google.registry.util.DateTimeUtils.latestOf;
 import static google.registry.util.DateTimeUtils.minusYears;
 import static google.registry.util.DateTimeUtils.plusYears;
-import static google.registry.util.DateTimeUtils.toDateTime;
 import static org.apache.beam.sdk.values.TypeDescriptors.voids;
 
 import com.google.common.collect.ImmutableMap;
@@ -402,7 +401,7 @@ private void expandOneRecurrence(
                       .getRenewPrice(
                           tld,
                           billingRecurrence.getTargetId(),
-                          toDateTime(eventTime),
+                          eventTime,
                           1,
                           billingRecurrence,
                           Optional.empty())

core/src/main/java/google/registry/bsa/persistence/DomainsRefresher.java

@@ -45,6 +45,7 @@
 import google.registry.model.tld.Tld;
 import google.registry.model.tld.Tld.TldType;
 import google.registry.util.BatchedStreams;
+import java.time.Duration;
 import java.time.Instant;
 import java.util.HashSet;
 import java.util.List;
@@ -95,7 +96,7 @@ public final class DomainsRefresher {
   public DomainsRefresher(
       Instant prevRefreshStartTime,
       Instant now,
-      java.time.Duration domainTxnMaxDuration,
+      Duration domainTxnMaxDuration,
       int transactionBatchSize) {
     this.prevRefreshStartTime = prevRefreshStartTime.minus(domainTxnMaxDuration);
     this.now = now;

core/src/main/java/google/registry/flows/CheckApiAction.java

@@ -35,6 +35,7 @@
 import static google.registry.persistence.PersistenceModule.TransactionIsolationLevel.TRANSACTION_REPEATABLE_READ;
 import static google.registry.persistence.transaction.TransactionManagerFactory.replicaTm;
 import static google.registry.pricing.PricingEngineProxy.isDomainPremium;
+import static google.registry.util.DateTimeUtils.toInstant;
 import static google.registry.util.DomainNameUtils.canonicalizeHostname;
 import static org.json.simple.JSONValue.toJSONString;
 
@@ -165,7 +166,7 @@ private Map<String, Object> checkDomainName(InternetDomainName domainName) {
       metricBuilder.status(SUCCESS).availability(availability);
       responseBuilder.put("status", "success").put("available", availability.equals(AVAILABLE));
 
-      boolean isPremium = isDomainPremium(domainString, now);
+      boolean isPremium = isDomainPremium(domainString, toInstant(now));
       metricBuilder.tier(isPremium ? PREMIUM : STANDARD);
       responseBuilder.put("tier", isPremium ? "premium" : "standard");
       if (!AVAILABLE.equals(availability)) {

core/src/main/java/google/registry/flows/domain/DomainCreateFlow.java

@@ -53,6 +53,7 @@
 import static google.registry.persistence.transaction.TransactionManagerFactory.tm;
 import static google.registry.util.DateTimeUtils.END_INSTANT;
 import static google.registry.util.DateTimeUtils.plusYears;
+import static google.registry.util.DateTimeUtils.toInstant;
 
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
@@ -313,7 +314,7 @@ public EppResponse run() throws EppException {
       // at this point so that we can verify it before the "after validation" extension point.
       signedMarkId =
           tmchUtils
-              .verifySignedMarks(launchCreate.get().getSignedMarks(), domainLabel, now)
+              .verifySignedMarks(launchCreate.get().getSignedMarks(), domainLabel, toInstant(now))
               .getId();
     }
     verifyNotBlockedByBsa(domainName, tld, now, allocationToken);
@@ -327,7 +328,7 @@ public EppResponse run() throws EppException {
         eppInput.getSingleExtension(FeeCreateCommandExtension.class);
     FeesAndCredits feesAndCredits =
         pricingLogic.getCreatePrice(
-            tld, targetId, now, years, isAnchorTenant, isSunriseCreate, allocationToken);
+            tld, targetId, toInstant(now), years, isAnchorTenant, isSunriseCreate, allocationToken);
     validateFeeChallenge(feeCreate, feesAndCredits, defaultTokenUsed);
     Optional<SecDnsCreateExtension> secDnsCreate =
         validateSecDnsExtension(eppInput.getSingleExtension(SecDnsCreateExtension.class));
@@ -434,13 +435,21 @@ public EppResponse run() throws EppException {
     FeesAndCredits responseFeesAndCredits =
         shouldShowDefaultPrice
             ? pricingLogic.getCreatePrice(
-                tld, targetId, now, years, isAnchorTenant, isSunriseCreate, Optional.empty())
+                tld,
+                targetId,
+                toInstant(now),
+                years,
+                isAnchorTenant,
+                isSunriseCreate,
+                Optional.empty())
             : feesAndCredits;
 
     BeforeResponseReturnData responseData =
         flowCustomLogic.beforeResponse(
             BeforeResponseParameters.newBuilder()
-                .setResData(DomainCreateData.create(targetId, now, registrationExpirationTime))
+                .setResData(
+                    DomainCreateData.create(
+                        targetId, toInstant(now), toInstant(registrationExpirationTime)))
                 .setResponseExtensions(createResponseExtensions(feeCreate, responseFeesAndCredits))
                 .build());
     return responseBuilder

core/src/main/java/google/registry/flows/domain/DomainDeleteFlow.java

@@ -422,7 +422,7 @@ private Money getGracePeriodCost(
       // If we updated the autorenew billing event, reuse it.
       DateTime autoRenewTime =
           billingRecurrence.getRecurrenceTimeOfYear().getLastInstanceBeforeOrAt(now);
-      return getDomainRenewCost(targetId, autoRenewTime, 1);
+      return getDomainRenewCost(targetId, toInstant(autoRenewTime), 1);
     }
     return tm().loadByKey(checkNotNull(gracePeriod.getBillingEvent())).getCost();
   }

core/src/main/java/google/registry/flows/domain/DomainFlowTmchUtils.java

@@ -38,10 +38,10 @@
 import java.security.cert.CertificateExpiredException;
 import java.security.cert.CertificateNotYetValidException;
 import java.security.cert.CertificateRevokedException;
+import java.time.Instant;
 import javax.xml.crypto.MarshalException;
 import javax.xml.crypto.dsig.XMLSignatureException;
 import javax.xml.parsers.ParserConfigurationException;
-import org.joda.time.DateTime;
 import org.xml.sax.SAXException;
 
 /** TMCH utility functions for domain flows. */
@@ -55,7 +55,7 @@ public DomainFlowTmchUtils(TmchXmlSignature tmchXmlSignature) {
   }
 
   public SignedMark verifySignedMarks(
-      ImmutableList<AbstractSignedMark> signedMarks, String domainLabel, DateTime now)
+      ImmutableList<AbstractSignedMark> signedMarks, String domainLabel, Instant now)
       throws EppException {
     if (signedMarks.size() > 1) {
       throw new TooManySignedMarksException();
@@ -75,7 +75,7 @@ public SignedMark verifySignedMarkValidForDomainLabel(SignedMark signedMark, Str
     return signedMark;
   }
 
-  public SignedMark verifyEncodedSignedMark(EncodedSignedMark encodedSignedMark, DateTime now)
+  public SignedMark verifyEncodedSignedMark(EncodedSignedMark encodedSignedMark, Instant now)
       throws EppException {
     if (!encodedSignedMark.getEncoding().equals("base64")) {
       throw new Base64RequiredForEncodedSignedMarksException();