feat: add per-step timestamps to StepState

[shahar-caura]

Summary

Closes #60

• Add StartedAt and CompletedAt timestamp fields to StepState with yaml:"...,omitempty" tags
• Set timestamps in runStep() when status transitions to running and completed/failed
• Old state YAML files without these fields load without error (zero value)

Test plan

• TestSaveAndLoad_StepTimestamps — round-trip serialization
• TestBackwardsCompat_OldStateWithoutTimestamps — old files load cleanly
• TestRun_StepTimestampsPopulated — happy path sets all timestamps
• TestRun_FailedStepHasTimestamps — failed steps also get both timestamps
• make test passes (all packages)

🤖 Generated with Claude Code

[github-actions]

Claude finished @shahar-caura's task in 1m 51s —— View job

---

PR Review: feat: add per-step timestamps to StepState

• Read changed files
• Analyze implementation
• Post review feedback

---

Summary

This PR adds StartedAt and CompletedAt time.Time fields to StepState, setting them in runStep() on status transitions. The implementation is clean, minimal, and backwards-compatible with old YAML files. Tests cover round-trip serialization, backwards compat, happy-path population, and failed-step coverage.

---

1. ResetFrom leaves stale timestamps on reset steps

Severity: Medium
File: internal/state/state.go:193-204
Problem: When ResetFrom(idx) resets steps at idx and beyond to StepPending, it clears Error but leaves StartedAt and CompletedAt from any previous run attempt. This means the persisted YAML will show a pending step with non-zero timestamps — an inconsistent intermediate state — until runStep overwrites them on the next execution.

🤖 Claude Code Prompt (click to copy)

In internal/state/state.go, update the ResetFrom method (lines 193-204) to also clear
StartedAt and CompletedAt when resetting steps to pending.

Current code:
    } else {
        s.Steps[i].Status = StepPending
        s.Steps[i].Error = ""
    }

Change to:
    } else {
        s.Steps[i].Status = StepPending
        s.Steps[i].Error = ""
        s.Steps[i].StartedAt = time.Time{}
        s.Steps[i].CompletedAt = time.Time{}
    }

Also add a test in internal/state/state_test.go: TestResetFrom_ClearsTimestamps
- Create a run state, set Steps[5].StartedAt and Steps[5].CompletedAt to non-zero times
- Call rs.ResetFrom(5)
- Assert Steps[5].StartedAt.IsZero() and Steps[5].CompletedAt.IsZero()
- Assert timestamps for steps BEFORE idx are NOT zeroed (they should be preserved)

---

2. TestRun_StepTimestampsPopulated doesn't assert ordering of StartedAt across steps

Severity: Low
File: internal/pipeline/run_test.go:500-520
Problem: The test verifies each step's StartedAt >= before and CompletedAt <= after, but doesn't assert that step N's StartedAt >= step N-1's CompletedAt. For a sequential pipeline, this ordering invariant is a stronger correctness guarantee than just bounding against wall-clock bookends.

🤖 Claude Code Prompt (click to copy)

In internal/pipeline/run_test.go, extend TestRun_StepTimestampsPopulated (lines 500-520)
to also assert sequential ordering: each step's StartedAt should be >= the previous
step's CompletedAt.

After the existing per-step loop, add:
    for i := 1; i < len(rs.Steps); i++ {
        assert.True(t,
            !rs.Steps[i].StartedAt.Before(rs.Steps[i-1].CompletedAt),
            "step %q StartedAt should be >= previous step %q CompletedAt",
            rs.Steps[i].Name, rs.Steps[i-1].Name,
        )
    }

---

3. No test coverage for resume: timestamps on re-run steps

Severity: Low
File: internal/pipeline/run_test.go
Problem: There's no test verifying that when a previously-failed step is re-run (via resume), its StartedAt is refreshed to the new run time rather than retaining the stale timestamp from the failed attempt. Given the ResetFrom gap noted above, a resume test would also catch regressions.

🤖 Claude Code Prompt (click to copy)

In internal/pipeline/run_test.go, add a test TestRun_ResumeRefreshesTimestamps that
verifies a re-run step gets fresh timestamps:

1. Set up a run that fails at step 4 (run agent) — use mockAgent{err: errors.New("fail")}.
2. Capture step 4's StartedAt after the failed run.
3. Fix the mock agent (set err = nil), call ResetFrom(4) on the run state.
4. Re-run the pipeline.
5. Assert that step 4's new StartedAt is AFTER the StartedAt from the failed run.
6. Assert step 4's CompletedAt is non-zero and >= StartedAt.

This validates that runStep always overwrites timestamps on retry and that no stale
timestamps persist after resume.

---

Suggestions

runStep could log step duration at DEBUG level — CompletedAt.Sub(StartedAt) is now trivially available and would be useful in logs without any extra fields on StepState. This is a nice-to-have, not a code issue.

🤖 Claude Code Prompt (click to copy)

In internal/pipeline/run.go, in the runStep function, add a duration log after the step
completes (both success and failure paths):

On success (after step.CompletedAt = time.Now()):
    logger.Info("step completed", "step", step.Name, "duration", step.CompletedAt.Sub(step.StartedAt))

On failure (after step.CompletedAt = time.Now()):
    logger.Info("step failed", "step", step.Name, "duration", step.CompletedAt.Sub(step.StartedAt), "error", err)

This is a pure additive change and requires no struct changes.

---

🚀 Fix All Prompts

🟡 Fix All Medium/Low Issues

Please fix the following issues in the feat/step-timestamps-60 branch:

1. In internal/state/state.go, update the ResetFrom method (lines 193-204) to also clear
StartedAt and CompletedAt when resetting steps to pending.

Current code:
    } else {
        s.Steps[i].Status = StepPending
        s.Steps[i].Error = ""
    }

Change to:
    } else {
        s.Steps[i].Status = StepPending
        s.Steps[i].Error = ""
        s.Steps[i].StartedAt = time.Time{}
        s.Steps[i].CompletedAt = time.Time{}
    }

Also add a test in internal/state/state_test.go: TestResetFrom_ClearsTimestamps
- Create a run state, set Steps[5].StartedAt and Steps[5].CompletedAt to non-zero times
- Call rs.ResetFrom(5)
- Assert Steps[5].StartedAt.IsZero() and Steps[5].CompletedAt.IsZero()
- Assert timestamps for steps BEFORE idx are NOT zeroed (they should be preserved)

2. In internal/pipeline/run_test.go, extend TestRun_StepTimestampsPopulated (lines 500-520)
to also assert sequential ordering: each step's StartedAt should be >= the previous
step's CompletedAt.

After the existing per-step loop, add:
    for i := 1; i < len(rs.Steps); i++ {
        assert.True(t,
            !rs.Steps[i].StartedAt.Before(rs.Steps[i-1].CompletedAt),
            "step %q StartedAt should be >= previous step %q CompletedAt",
            rs.Steps[i].Name, rs.Steps[i-1].Name,
        )
    }

3. In internal/pipeline/run_test.go, add a test TestRun_ResumeRefreshesTimestamps that
verifies a re-run step gets fresh timestamps:

1. Set up a run that fails at step 4 (run agent) — use mockAgent{err: errors.New("fail")}.
2. Capture step 4's StartedAt after the failed run.
3. Fix the mock agent (set err = nil), call ResetFrom(4) on the run state.
4. Re-run the pipeline.
5. Assert that step 4's new StartedAt is AFTER the StartedAt from the failed run.
6. Assert step 4's CompletedAt is non-zero and >= StartedAt.

🟢 Fix All Suggestions

In internal/pipeline/run.go, in the runStep function, add a duration log after the step
completes (both success and failure paths):

On success (after step.CompletedAt = time.Now()):
    logger.Info("step completed", "step", step.Name, "duration", step.CompletedAt.Sub(step.StartedAt))

On failure (after step.CompletedAt = time.Now()):
    logger.Info("step failed", "step", step.Name, "duration", step.CompletedAt.Sub(step.StartedAt), "error", err)

This is a pure additive change and requires no struct changes.

[shahar-caura]

AI Code Review

Summary

This PR adds StartedAt and CompletedAt timestamp fields to StepState, set during runStep() on status transitions. The implementation is clean for the happy path and failure path, with good test coverage and backwards-compatibility handling. However, the new fields are not cleared in two reset code paths, which will cause stale timestamps to persist after resume.

ResetFrom does not clear timestamps

Severity: Medium
File: internal/state/state.go:191-204
Problem: ResetFrom resets Status and Error for steps but does not zero out StartedAt and CompletedAt, so resumed steps will carry stale timestamps in the persisted YAML until they are overwritten by runStep.
Suggested fix: Add s.Steps[i].StartedAt = time.Time{} and s.Steps[i].CompletedAt = time.Time{} in the else branch (for steps at idx and beyond that are reset to pending).

Claude Code fix prompt

In /Users/sha/code/forge/internal/state/state.go, in the ResetFrom method (lines 193-204), add timestamp clearing for steps reset to pending. In the else branch (line 198-201), after setting Status to StepPending and Error to "", add:
  s.Steps[i].StartedAt = time.Time{}
  s.Steps[i].CompletedAt = time.Time{}

---

Inline failed-step reset in cmd_resume does not clear timestamps

Severity: Medium
File: cmd/forge/cmd_resume.go:67-72
Problem: When resuming without --from, failed steps are reset to pending and their Error is cleared, but StartedAt and CompletedAt are not zeroed, leaving stale timestamps on steps that will be re-run.
Suggested fix: Add rs.Steps[i].StartedAt = time.Time{} and rs.Steps[i].CompletedAt = time.Time{} inside the if rs.Steps[i].Status == state.StepFailed block.

Claude Code fix prompt

In /Users/sha/code/forge/cmd/forge/cmd_resume.go, in the loop at lines 67-72 that resets failed steps, after clearing Status and Error, add:
  rs.Steps[i].StartedAt = time.Time{}
  rs.Steps[i].CompletedAt = time.Time{}

---

Fix All Prompts

Fix All Medium Issues

Please fix the following Medium issues:

1. In /Users/sha/code/forge/internal/state/state.go, in the ResetFrom method (lines 193-204), add timestamp clearing for steps reset to pending. In the else branch (line 198-201), after setting Status to StepPending and Error to "", add:
     s.Steps[i].StartedAt = time.Time{}
     s.Steps[i].CompletedAt = time.Time{}

2. In /Users/sha/code/forge/cmd/forge/cmd_resume.go, in the loop at lines 67-72 that resets failed steps, after clearing Status and Error, add:
     rs.Steps[i].StartedAt = time.Time{}
     rs.Steps[i].CompletedAt = time.Time{}

[shahar-caura]

AI Code Review

Summary

This PR adds StartedAt and CompletedAt timestamp fields to StepState, set in runStep() on status transitions. The implementation is clean and includes good test coverage for happy path, failure path, serialization round-trip, and backwards compatibility. There is one medium-severity issue: ResetFrom and the resume path don't clear stale timestamps when resetting steps.

ResetFrom does not clear stale timestamps

Severity: Medium
File: internal/state/state.go:193-204
Problem: ResetFrom resets Status and Error for steps at idx and beyond, but does not zero out StartedAt and CompletedAt, so resumed steps carry stale timestamps from a previous run attempt.
Suggested fix: In the else branch (steps being reset to pending), also set StartedAt and CompletedAt to their zero value.

Claude Code fix prompt

In internal/state/state.go, in the ResetFrom method (line 193-204), when resetting steps to pending (the else branch at line 199-201), also clear the timestamp fields:
  s.Steps[i].StartedAt = time.Time{}
  s.Steps[i].CompletedAt = time.Time{}

---

Resume path does not clear stale timestamps on failed steps

Severity: Medium
File: cmd/forge/cmd_resume.go:67-72
Problem: When resuming without --from, the loop resets failed steps' Status and Error but does not clear StartedAt and CompletedAt, leaving stale timestamps that will be partially overwritten (only StartedAt gets a new value in runStep, so CompletedAt from the failed attempt persists until the step finishes again).
Suggested fix: Clear both timestamp fields alongside Status and Error when resetting failed steps to pending.

Claude Code fix prompt

In cmd/forge/cmd_resume.go, in the loop at lines 67-72 that resets failed steps to pending, also clear the timestamp fields:
  rs.Steps[i].StartedAt = time.Time{}
  rs.Steps[i].CompletedAt = time.Time{}
Add these two lines after line 70 (rs.Steps[i].Error = "").

---

Missing test for ResetFrom clearing timestamps

Severity: Low
File: internal/state/state_test.go
Problem: The existing TestResetFrom_* tests don't verify that timestamps are cleared, so the bug above has no test coverage.
Suggested fix: Add assertions in TestResetFrom_MiddleStep that reset steps have zero StartedAt and CompletedAt.

Claude Code fix prompt

In internal/state/state_test.go, in TestResetFrom_MiddleStep (around line 261), after calling rs.ResetFrom(5), set timestamps on a step before the reset index and one after, then assert that steps at index >= 5 have zero StartedAt and CompletedAt, while steps before index 5 retain their timestamps.

---

Fix All Prompts

Fix All Medium Issues

Please fix the following MEDIUM issues:

1. In internal/state/state.go, in the ResetFrom method (line 193-204), when resetting steps to pending (the else branch at line 199-201), also clear the timestamp fields:
  s.Steps[i].StartedAt = time.Time{}
  s.Steps[i].CompletedAt = time.Time{}

2. In cmd/forge/cmd_resume.go, in the loop at lines 67-72 that resets failed steps to pending, also clear the timestamp fields:
  rs.Steps[i].StartedAt = time.Time{}
  rs.Steps[i].CompletedAt = time.Time{}
Add these two lines after line 70 (rs.Steps[i].Error = "").

Fix All Low Issues

Please fix the following LOW issues:

1. In internal/state/state_test.go, in TestResetFrom_MiddleStep (around line 261), after calling rs.ResetFrom(5), set timestamps on a step before the reset and verify that steps at index >= 5 have zero StartedAt and CompletedAt after the reset.