[hivecommons-hive]

Closes #14529

Summary

The Broker.Run function declared an unnamed return and used a deferred closure to set FinishedAt. With unnamed returns, the defer runs after the value is already copied to the caller, so the assignment had no effect for successful pushes.

Changes

• Changed Run() signature to have named results (res Result, err error)
• Changed initial res assignment from := to = since res is now a named return
• The defer now correctly updates the returned value
• Added test to verify FinishedAt is set on successful push and equals injected clock

Testing

Extended TestRunPrefersAnExplicitBaseRef with Now injection and FinishedAt assertion to verify successful runs record both StartedAt and FinishedAt timestamps.

— hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88

[kubestellar-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign clubanderson for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[github-actions]

Open PR overlap check

Base branch: v5

Conflicts are checked with every PR merged onto the current tip of v5, so a PR that only needs a rebase is not reported as conflicting with its siblings.

No conflicting open PRs or same-file overlaps were found.

[hivecommons-hive]

hive/awaiting-ci: CI pending — sweep will merge when green.

---

🐝 Hive Agent: scanner | Instance: hosted-available-oke-11-placeholder-r05x | SHA: 93db1d4

— hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

Looks correct to me. Read Run at the PR head (src/crux/pushbroker/pushbroker.go:306-431): every exit is an explicit return res, err or return b.fail(res, …), so with the named result the deferred res.FinishedAt = b.now() now lands on the value the caller receives. The inner if err := … shadows are harmless for the same reason — there is no bare return anywhere in the body.

Two things I checked that the body does not spell out:

• fail() (pushbroker.go:810-812) already set FinishedAt, which is why only the success path and the validate() early return at :309-312 were returning the zero time. Both are covered by this change; the defer now also re-stamps FinishedAt after fail(), which is redundant but not wrong.
• The added assertion in TestRunPrefersAnExplicitBaseRef is a real regression test: with the old unnamed return, res.FinishedAt would be the zero time and !res.FinishedAt.Equal(fixed) fails. Broker.Now exists at :287 and now() at :838-842 honours it.

The changelog fragment name fixed-crux-pushbroker-finished-at.md matches the <category>-<slug>.md form the guard requires.

— hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

Read src/crux/pushbroker/pushbroker.go at 93db1d4 end-to-end, not just the hunk. The fix is correct: with the named results at pushbroker.go:306, return res, nil (line 429) assigns the named res before the deferred closure at line 308 runs, so FinishedAt now lands on the value the caller receives. Every other return goes through b.fail (line 810–816), which already set FinishedAt; the defer overwriting it there is harmless. The commit, err := … / if err := … redeclarations inside the body are all either same-scope reuse or inner-scope shadows that never reach a bare return, so no path returns a stale err. Broker.Now exists (line 287) and now() honours it (838–843), so the test's injected clock is a real assertion — it fails on the old code (FinishedAt zero) and passes on the new.

One precision note, not a blocker: the validate() error path at line 309–312 returns directly without b.fail, so before this change it also lacked FinishedAt. The changelog fragment's "previously only failures did" is slightly generous — "only failures routed through fail()" is the accurate statement. Fine to leave as is.

Looks correct to me. Remaining checks are still running; nothing here needs a human decision.

— hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88

[hivecommons-hive]

Evidence bundle `#1453093db1d4` sha256:bc2a3600a0ae7be9b454280c1eb06e89625846d2be2aab5f0dcb0db2883eff5f — download from the dashboard or `hivectl review evidence #14530`.

[github-actions]

Thank you for your contribution! Your PR has been merged.

We'd love to hear how your experience was: share feedback