Nonblocking: Extra lines

This ought to be:

      final var activityObject = Json
          .createObjectBuilder()
          .add("start_offset", act.startOffset().toString())
          .add("anchored_to_start", act.anchoredToStart())
          .add("anchor_id", act.anchorId())
          .add("name", act.name());

Otherwise you can't actually save what the activity got reanchored to.

Blocking: Missing a request.append("}") before postRequest. See L710 for reference.

Suggestion: Alternate GQL Proposal:

mutation deletePlanActivityDirectives($planId: Int! = %d, $directiveIds: [Int!]!=%s) {
  delete_activity_directive(where: {_and: {plan_id: {_eq: $planId}, id: {_in: $directiveIds}}}) {
    affected_rows
  }
}

planId has the format 1, directiveIds has the format [1, 2, 3].
This lets you avoid needing to build up the statement per-directive.

A test that will show that only the relevant activities will be deleted is to create a plan with some activites, branch it, then perform a scheudling run that deletes some of the activities. The branch will be unaffected, as will the remaining activities in the plan.

Idle question seeing this and L444: why is actFromInitialPlan an Optional instead of calling .get during assignment?

That is, why isn't L435 final var actFromInitialPlan = initialPlan.getActivityById(activity.id()).get();

I can't select it in a comment, but this is a question re: L429-434: Why are we changing the activity's arguments on these lines? Asking as I'm concerned about the following case:

An activity type has a controllable duration, and the parameter that controls it has a default value. There's an instance in the plan that is using this default duration (so its arguments would not contain the parameter). This plan is then scheduled. Wouldn't this activity cause an exception to be raised on L455?

updatePlanActivityDirectiveAnchors ought to be called between modify and delete. Not only would this let the method become private (and have storeFinalPlan actually store the final plan), but if it's not done in this order, then a preserved anchor tree will cause the scheduling run to fail during deleteActivityDirectives, since you can't delete an activity that is an active anchor.

Actually, I just checked updatePlanActivityDirectiveAnchors and it only runs for new activities. This means that either

1. that method needs to be changed to run for modified activities too, including supporting reanchoring existing activities to the plan, (in which case, this comment can be disregarded), or
2. the comment about including anchorId in the fields being updated in modifyActivityDirectives needs to have its scope increased to support reanchoring to activities that are newly-created in scheduling.

If 2) is chosen, I still think updatePlanActivityDirectiveAnchors ought to be moved into this function

Blocking: None of these tests appear to excersize deleting and reanchoring activities that were already in the plan, only ones that were created as part of scheduling. Please add tests where activities in the plan prior to scheduling are 1) deleted and 2) reachored so that the new GQL code can be tested. I would especially prefer if these tests involved multiple existing activities being reanchored/have their start times adjusted to unique offsets to fully test the modifyActivityDirecitves behavior, since it's chaining sub-mutations.

Why was this removed instead of changing the else to an else if (!(edit instanceof Edit.Delete))? Additionally, is there any value in having a procedure track updated and deleted activities as well as added ones?

Nonblocking: could you add a couple of inline comments to this function? The outer else branch isn't doing much, but that's hard to see without dissecting the Kotlin-isms.

Also L188 is an extra newline.

Nonblocking: Does Kotlin have the same kind of streamable support Java does? Ie val childDirectives = directives.filter{ d -> d.start is DirectiveStart.Anchor && d.start.parentId == directive.id }. (or even skipping childDirectives and chaining into a forEach)

I don't know if that would be more performant, but it may help collapse L199-L202 from a forloop -> switch -> if statement into what that works out into, which is, "get the activities directly anchored to this one, then loop over them and resolve according to anchor strat"