bugfix: Fix issue where builders could resume completed tasks after being disabled

[Stubbjax]

This change fixes an issue introduced by #1870 that allows builders to resume an already completed task after being disabled.

When assigning a new build task to a builder, if the target building is an already-completed building, then the new task is ignored.

Before

BEFORE.mp4

After

AFTER.mp4

[greptile-apps]

Greptile Summary

This PR fixes a bug introduced in #1870 where a builder (Dozer/Worker) could resume a build task on an already-completed structure after recovering from a temporary disable (EMP, hack, underpowered). The fix moves the disable/resume logic out of Object::onDisabledEdge and into each AI module's own onDisabledEdge override, and adds a OBJECT_STATUS_UNDER_CONSTRUCTION guard before re-issuing a build task on recovery.

• cancelTask gains a rememberTask flag; a new setPreviousTask helper guards against DOZER_TASK_INVALID before indexing m_task[], closing the previously reported UB crash path.
• resumePreviousTask now dispatches on task type: BUILD is only resumed if the target is still under construction; REPAIR and FORTIFY resume unconditionally if the target still exists; clearPreviousTask is always called afterward.
• onDisabledEdge selectively remembers tasks only for recoverable disable types (EMP, hacked, subdued, underpowered), preventing spurious resumptions after a unit exits a transport.

Confidence Score: 5/5

• Safe to merge — the core logic is correct and the previously reported crash and task-drop regressions are both addressed.
• The two blocking issues from the earlier review (out-of-bounds array access in setPreviousTask and silent loss of REPAIR/FORTIFY tasks in resumePreviousTask) are fully resolved. The architectural change — moving the cancel/resume logic from Object::onDisabledEdge into the module's own override — is clean and the two implementations (Generals and GeneralsMD) are consistent. Remaining feedback is cosmetic only.
• GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/AIUpdate/DozerAIUpdate.cpp has a minor indentation inconsistency in clearPreviousTask; all other files are straightforward.

Important Files Changed

Filename	Overview
Generals/Code/GameEngine/Include/Common/Module.h	Adds a no-op virtual onDisabledEdge(Bool nowDisabled) to the base BehaviorModule class so subclasses can opt in to disable/re-enable callbacks.
Generals/Code/GameEngine/Source/GameLogic/Object/Object.cpp	Adds module-iteration loop in onDisabledEdge (Generals previously had none), delegating disable-edge handling to each behavior module. Clean change.
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Object.cpp	Removes the old hard-coded DozerAIInterface cancel/resume block from onDisabledEdge; the module's own onDisabledEdge now handles this via the existing loop, eliminating duplication.
Generals/Code/GameEngine/Source/GameLogic/Object/Update/AIUpdate/DozerAIUpdate.cpp	Core fix: setPreviousTask guards against DOZER_TASK_INVALID; resumePreviousTask now checks OBJECT_STATUS_UNDER_CONSTRUCTION for BUILD tasks and handles REPAIR/FORTIFY; onDisabledEdge selectively remembers tasks only for EMP/HACKED/SUBDUED/UNDERPOWERED disables. Bugfix comment date references 2025.
Generals/Code/GameEngine/Source/GameLogic/Object/Update/AIUpdate/WorkerAIUpdate.cpp	Identical structural changes to DozerAIUpdate.cpp applied to WorkerAIUpdate. Also fixes the currentVersion → version variable name in the xfer function.
GeneralsMD/Code/GameEngine/Source/GameLogic/Object/Update/AIUpdate/DozerAIUpdate.cpp	Mirror of Generals DozerAIUpdate changes. Minor inconsistency: clearPreviousTask() body uses 2-space indentation instead of tabs (unlike the rest of the file and the Generals counterpart).

Sequence Diagram

sequenceDiagram
    participant O as Object
    participant M as BehaviorModule loop
    participant D as DozerAIUpdate / WorkerAIUpdate

    Note over O,D: Builder gets EMP'd / hacked / underpowered
    O->>M: onDisabledEdge(true)
    M->>D: onDisabledEdge(true)
    D->>D: "getCurrentTask() != INVALID?"
    alt has active task
        D->>D: "isDisabledByType(EMP|HACKED|SUBDUED|UNDERPOWERED)?"
        alt recoverable disable type
            D->>D: "cancelTask(currentTask, rememberTask=true)"
            D->>D: setPreviousTask(task) — saves m_task[task] info
        else permanent disable (e.g. HELD)
            D->>D: "cancelTask(currentTask, rememberTask=false)"
            D->>D: clearPreviousTask()
        end
    end

    Note over O,D: Builder recovers (disable lifted)
    O->>M: onDisabledEdge(false)
    M->>D: onDisabledEdge(false)
    D->>D: resumePreviousTask()
    alt "previousTask == BUILD"
        D->>D: findObjectByID(targetID)
        D->>D: testStatus(UNDER_CONSTRUCTION)?
        alt building still in progress
            D->>D: newTask(BUILD, target)
        else building already completed
            Note over D: task silently discarded (the fix!)
        end
    else "previousTask == REPAIR or FORTIFY"
        D->>D: findObjectByID(targetID)
        D->>D: newTask(task, target) if target exists
    end
    D->>D: clearPreviousTask()

Loading

_{Reviews (14): Last reviewed commit: "chore: Clear previous task if we're not ..." | Re-trigger Greptile}

[xezon]

@Stubbjax Is this now fixed up?

[Stubbjax]

@Stubbjax Is this now fixed up?

It should be.

[xezon]

This needs polishing.

[bobtista]

This changes behavior for DOZER_TASK_REPAIR. In non-retail-compatible builds, main attempts to reissue any saved task, but this implementation only reissues DOZER_TASK_BUILD and then clears the saved repair task. Is this what we want?
If not, could have the OBJECT_STATUS_UNDER_CONSTRUCTION check remain specific to BUILD while preserving existing non-build resumption behavior. DOZER_TASK_FORTIFY follows the same code path, but does anything use it? This applies to the mirrored implementations too.

[xezon]

Please continue work on this fix.

[xezon]

Are these 4 complete?

Maybe do the checklist by exclusion instead?

Bool attemptToResumeTask = !isDisabledByType(DISABLED_HELD) && ... ;

[Stubbjax]

Yes, this mirrors the conditions in Object::setDisabledUntil and Object::clearDisabled.

[xezon]

Ok this link is rather unintuitive. Can we perhaps consolidate the conditions across the 3 places so that they are unlikely to go out of sync?

[Stubbjax]

Would it make more sense to do such refactors to unrelated logic in a subsequent change?

[xezon]

Yes if it is not forgotten. It's a side quest spawned from this.

[OmarAglan]

@codex

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3886b38525

ℹ️ 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".

[xezon]

If cancelTask is called twice with rememberTask=true, then it looks like setPreviousTask will set a task that is malformed. Maybe setPreviousTask needs more validation.

[Stubbjax]

Can you give an example?

[xezon]

Maybe

	struct DozerTaskInfo
	{
		DozerTaskInfo()
		{
			m_targetObjectID = INVALID_ID;
			m_taskOrderFrame = 0;
		}

		Bool isValidTask() const
		{
		  return m_taskOrderFrame != 0;
		}

		ObjectID m_targetObjectID;				///< target object ID of task
		UnsignedInt m_taskOrderFrame;			///< logic frame we decided we wanted to do this task
	} m_task[ DOZER_NUM_TASKS ];				///< tasks we want to do indexed by DozerTask


void DozerAIUpdate::setPreviousTask(DozerTask task)
{
	if (task == DOZER_TASK_INVALID)
		return;

	if (!m_task[task].isValidTask())
		return;

	m_previousTask = task;
	m_previousTaskInfo = m_task[task];
}

[xezon]

Better move it up so it is consistent with the call order in DozerAIUpdate::cancelTask