Guided-tour reset: the production call site is untested, and a new test pins the missing handler guard as correct

[tomglenn]

Raised from the review of #1841, which is approved and merging — this is follow-up work, not a blocker.

The call site that carries the fix is unverified

#1841 fixed guided-tour re-entry by calling guidedHandler.resetProgress() at interactive-guided.tsx:388, ahead of the loop in executeStep. That single line is the whole production fix, and nothing tests it.

Mutation experiments in the review deleted only that line:

• 103/103 tests green across the four directly related suites
• 52 suites, 941 tests green across src/components/interactive-tutorial + src/interactive-engine
• 535 suites, 8567 tests green across the whole repository, with eslint and tsc --noEmit also clean

As a control, emptying resetProgress's body does redden the new test — so the delta's tests guard the method, which was already correct. The missing call was the broken half, and that half is still unguarded. A future refactor can delete it and every gate stays green.

The observation point already exists: interactive-guided.test.tsx:137 mocks resetProgress.

Suggested fix: hoist that mock to a module-level fn and assert it fires on a run start, ideally ordered before the first executeGuidedStep, using the callOrder harness the file already applies to waitForReactUpdates vs executeGuidedStep.

A test that makes the natural hardening a breaking change

resetProgress() is the only writer that clears completedSteps, and executeGuidedStep never notices that a stepIndex === 0 arrived with credit already banked. So both in-app callers must each remember to clear independently — and the bug class this originally fixed is exactly "a caller forgot".

The new test carries stale credit into a second sequence when it is not called (guided-handler.test.ts:125-132) asserts completedSteps: [0, 1] on run B's first paint. That pins the absence of a handler-side guard as expected behaviour, so making the handler self-healing later becomes a test-breaking change rather than a green one.

Pick one, not both: reframe that test as characterization a future self-healing handler is free to break, or move the invariant into executeGuidedStep (clear on stepIndex === 0) and let the two caller-side calls become belt-and-braces.

Review: #1841 (review)

[moxious]

🧭 Implementation readiness

Stakes: Nothing changes for the user. This is internal work: it's a test-coverage gap that leaves the actual guided-tour re-entry bugfix from PR #1841 unguarded, so a future refactor could silently delete the fix and every CI gate would stay green.

Sanity check: Confirmed interactive-guided.tsx:388's guidedHandler.resetProgress() call is the sole production call ahead of the guided-step loop and is genuinely untested — deleting only that line was the mutation the reviewer ran, and it passed all tests. Confirmed guided-handler.test.ts:125-132 really does assert completedSteps: [0, 1] on a second run's first paint without a reset between runs, pinning stale-credit-without-reset as expected.

Severity: 🟠 Medium — Not a live bug today, but a silent regression trap on a previously-shipped-broken behavior, plus a characterization test that hardens an arguably-wrong invariant into a breaking-change tripwire for future improvement.

Code basis:

• src/components/interactive-tutorial/interactive-guided.tsx:388 — The sole, currently-untested production call site carrying the fix(interactive-engine): credit guided-tour progress on performance, not display #1841 re-entry fix.
• src/components/interactive-tutorial/interactive-guided.test.tsx:137 — Existing observation point (resetProgress mock) to hoist to module level and assert against, per the issue's suggested fix.
• src/interactive-engine/action-handlers/guided-handler.test.ts:125-132 — Test that pins stale-credit-without-reset as GuidedHandler's expected behavior, making a future handler-side self-heal a breaking change.
• src/interactive-engine/action-handlers/guided-handler.ts:75-76 — resetProgress()'s implementation — confirms completedSteps is only ever cleared here, never on stepIndex === 0.

Test coverage: interactive-guided.test.tsx:137 mocks resetProgress but never asserts it was called, so deleting interactive-guided.tsx:388 passes every test today.

Labels: Proposed additions: severity:MEDIUM, agent/needs-decision

Open judgment calls:

1. Should GuidedHandler.executeGuidedStep self-heal (auto-clear completedSteps when stepIndex === 0 arrives with prior credit banked), or should the two in-app callers (interactive-guided.tsx, live-tab-executor.ts) remain solely responsible for calling resetProgress() before each run? Self-healing removes the "a caller forgot" bug class permanently but changes GuidedHandler's implicit contract and requires updating the characterization test at guided-handler.test.ts:125-132. Staying caller-responsible keeps today's narrower contract but leaves the bug class open to recurrence at any future third call site. Neither the code nor the two PRs' history picks one design over the other.

Traps for the implementer:

• PR fix(interactive-engine): credit guided-tour progress on performance, not display #1841's own body describes resetProgress-on-restart as a "known follow-up, out of scope" — that text is stale relative to what was actually merged. The merge-commit diff confirms the fix line is really in the merged PR. Don't use the PR body alone to conclude the call site doesn't exist.

Verdict: 🟡 Needs a decision on (1) first. (tracking: agent/needs-decision)

Preview generated by the prep-issues tool. Nothing posted.