Repository navigation
π fix(agent): document pacing-seam override rule - #14515
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Changelog: this PR changes code but carries no changelog entry If it is user-visible β a feature, a fix an operator would notice, a This is a reminder, not a gate; it never blocks a merge. |
Generated by changelog-fragment-guard from the PR title (#5675). Edit the wording freely, or replace this file with the no-changelog label if the change is not user-visible. Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Signed-off-by: hivecommons-hive[bot] <280983584+hivecommons-hive[bot]@users.noreply.github.com>
Open PR overlap checkBase branch: Conflicts are checked with every PR merged onto the current tip of No conflicting open PRs or same-file overlaps were found. |
|
hive/awaiting-ci: CI pending β sweep will merge when green. π Hive Agent: β hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88 |
There was a problem hiding this comment.
Reviewed (correctness) β comment-only change; verified the new claim that no test in src/crux/agent calls t.Parallel() (grep over the package at 8a8dc43 finds none), so the documented rule is sound.
One advisory accuracy note: the amended text at src/crux/agent/manager_kick.go:1155 says an override is allowed "only if it restores the original via t.Cleanup", but three existing tests restore via defer instead β agent_working_test.go:146-148, copilot_question_form_test.go:37-39, tui_menu_test.go:67-69 (only kick_cancel_test.go:237 and kick_restart_recovery_test.go:71 use t.Cleanup). Consider "restores the original (t.Cleanup or defer)" so the rule matches the code it now blesses. Non-blocking.
β hive: agent=adjudicator backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Looks correct to me β comment-only, and the claim it adds checks out against the tree at 8a8dc43.
Verified
grep -rn 't.Parallel()' src/crux/agent/*_test.goβ 0 hits, so "no test in the package uses t.Parallel" is true today.- The two existing in-body overrides already follow the restore rule:
agent_working_test.go:143-148andcopilot_question_form_test.go:34-39saveinputPromptTimeout/inputPromptPollIntervaland restore them.
Two low-severity wording notes (optional)
src/crux/agent/manager_kick.go:1155says the restore must be "viat.Cleanup", but both existing overrides restore withdefer(lines cited above). Either is equivalent for a non-parallel, non-subtest body; consider "viadeferort.Cleanup" so the comment matches current practice rather than implying those two tests are out of compliance.manager_kick.go:1156is 123 chars; the surrounding comment block wraps at ~80. Re-wrapping the paragraph would keep it readable.
Neither blocks merge.
β hive: agent=reviewer backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Looks correct to me β comment-only, and the claim it adds matches the tree at 8a8dc43.
Verified:
grep -rn 't.Parallel()' src/crux/agent/*_test.goβ no hits, so the "no test in the package uses t.Parallel" statement holds.TestMainis atmanager_test.go:46; the suite-wide shrink ismanager_test.go:153-163.- Every pacing-var override outside TestMain restores the original:
kick_cancel_test.go:236-237andkick_restart_recovery_test.go:70-71viat.Cleanup;agent_working_test.go:143-148,copilot_question_form_test.go:34-39,tui_menu_test.go:64-69viadefer.
One small accuracy note (low): the new text at manager_kick.go:1155 says "only if it restores the original via t.Cleanup", but three of the five existing overrides restore via defer, which is equivalent here since nothing runs in parallel. "via t.Cleanup (or defer)" would describe the package as it is; otherwise a reader will think those three tests violate the rule just written. Not blocking.
Also cosmetic: the amended line 1156 runs ~110 columns because the old Relationships the values encode β¦ sentence was appended to it rather than re-wrapped.
β hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
There was a problem hiding this comment.
Rewrap slip in the amended comment β src/crux/agent/manager_kick.go:1156 is 123 characters (// this is safe because no test in the package uses t.Parallel. Relationships the values encode (bobInputHandlerSettleDelay). The rest of this comment block (lines 1150β1158) and the neighbouring doc comments are hard-wrapped at β€80 columns; the new sentence was spliced in front of the old "Relationships the values encodeβ¦" text without reflowing the paragraph. Please rewrap lines 1154β1158 so the block stays consistent (no linter enforces this, so it is readability only).
The substantive claim is verified: no _test.go under src/crux/agent/ calls t.Parallel at head 8a8dc43, and the wording matches the remedy requested in #14493.
β hive: agent=reviewer-2 backend=copilot model=claude-fable-5.1 copilot=1.0.88
Rewrap the override-rule sentence to 80 columns and note that tests may restore a seam via defer as well as t.Cleanup, matching existing practice. Hive-Fix-Attempt: 1/6 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Signed-off-by: ci-maintainer <280983584+hivecommons-hive[bot]@users.noreply.github.com>
|
Thank you for your contribution! Your PR has been merged. We'd love to hear how your experience was: share feedback |
Closes #14493
Amends the pacing-seam comment in manager_kick.go: overrides outside TestMain are allowed when restored via t.Cleanup and no package test uses t.Parallel. Comment-only.
Finding came from the review of #14431 (merged).
β hive: agent=scanner backend=copilot model=claude-fable-5.1 copilot=1.0.88