refactor(execution): reduce complexity across execution handlers - #5034
Merged
Merged
Conversation
_assert_agent_safe_skill_frontmatter caught bare Exception around load_yaml, wider than the documented YAML loader failure mode. Every other load_yaml call site in the codebase catches YAMLError specifically (core/io/yaml_io.py explicitly re-exports it for this purpose); narrow this one to match.
…al_direct_leader The bare `pass` lacked the same disappearance-race justification other predicates in this module already document explicitly. Kept as a trailing comment (not a new line) since the file sits at the REQ-CNST-010 750-line cap.
…m to Literal stage was a bare str used to build log-event names via f-string, so a typo in a caller's stage argument would silently produce an unknown log key. Both call sites already only ever pass "term"/"kill"; no behavior change.
_tether_record_from_dict already raises TypeError for data is None, and the surrounding except (ValueError, KeyError, TypeError) already catches it — the explicit pre-check was dead weight duplicating detection sweep_orphaned_tethers already consolidated into the helper.
…robes The old name only described the run path, but the function also writes the probe cache on both the success and failure paths — matching the naming precision of its sibling helpers in this module.
The old _validate_ prefix was inconsistent with this module's other _validate_* helpers (e.g. _validate_credential_parent), which are pure validation and return None. This one validates AND derives/returns the bare tool tuple in one pass — kept as-is (the derivation is itself part of validating definition.reader_tools), just renamed to match its real scope.
…unters _consecutive_process_state_anomalies returned three independent counters positionally coupled to the records list, unlike its sibling anomaly helpers (_oom_anomalies, _signal_transition_anomalies) which return records plus a single value. Bundle the counters into a frozen dataclass so the shape mirrors those siblings.
_stale_suppression_reason returned prose strings that _continue_stale_suppression then string-matched on to select behavior, so the same string served as both a branch key and a log-template fragment. Introduce a StaleSuppressionReason Literal for the control-flow tokens; the human-readable phrasing stays local to each logger.warning() call site, unchanged. No test asserts on the raw return value of _stale_suppression_reason (only on log event names and structured fields), so this is a pure internal refactor.
…lResultContext _build_stall_result duplicated ~16 of _build_skill_result's own keyword-only params, forwarded 1:1 by name. Bundle the shared fields into a frozen, slotted _SkillResultContext constructed once in _build_skill_result; keep stall_spec as its own kwarg (test_infra_exit_retry_policy_exhaustive.py inspects call sites for stall_spec=_STALE_SPEC/_IDLE_STALL_SPEC by name). _build_skill_result's external signature is unchanged, so its other callers are unaffected.
…ssLaunchPreparation The bare 4-tuple (backend, model identity, launch preparation, add_dirs) is one cohesive launch context, mirroring the existing PostSessionMetrics precedent in _headless_helpers.py for the same pattern. Return-value change only — the 16-parameter input surface is left as-is (no single grouping was found that reduces real complexity for this function's sole caller; see DISCUSS writeup).
_termination.py::_drain_before_escalation and _process_monitor.py's _stale_suppression_reason both combined the same 3 liveness predicates (api_connection, child_processes, dispatch_marker) into a defer/kill decision, duplicated inline on one side. Extract the predicate evaluation into _active_liveness_signals (_process_monitor.py, alongside the predicates it wraps); each site keeps its own ceiling/priority/ logging/pending-tasks semantics unchanged on top — no behavior change, only the literal predicate-combination logic is deduplicated. Update the two AST-based arch guards in test_watcher_signal_consistency.py to hop through the new indirection, mirroring the pattern already used for _session_log_monitor's existing indirection through _stale_suppression_reason. Per DISCUSS investigation: deliberately does NOT unify the two sites' differing ceiling values, priority order, or pending-tasks awareness — that remains an open product decision (whether a completed-but-still- active session's kill should also wait on pending async obligations), tracked separately, not decided here.
CI caught this: commit 9dc8a86 replaced _termination.py's inline predicate-OR logic with a call to the shared _active_liveness_signals helper, but three tests in this file still monkeypatched the old individual _has_active_child_processes/_has_active_api_connection bindings on the _termination module. _has_active_api_connection is no longer imported there at all (AttributeError on patch), and patching _has_active_child_processes there is now a no-op for the deferral path since it only reaches the real predicate via _process_monitor's own _active_liveness_signals. Repoint all three at the new seam; verified by hand that the expected call counts and clock values are unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Execution parsers, process monitors, and result/publication handlers repeat branch logic and mix separate responsibilities. This refactor separates validation, process observation and race coordination, headless launch/result preparation, and GitHub/runtime/quota handling while preserving existing outcomes and ordering.
All 60 candidates in #4995 are accounted for: 55 have lower complexity scores, four are simplified with unchanged scores, and C05 retains its original implementation. The bookkeeping sum for the original functions is 1,163 → 795; the 58 new helpers and 21 moved/renamed identities are measured separately. The issue body contains the complete measurements and rationale.
execution/evidence/reader/to meet existing size limits, preserving facade aliases and filesystem/process ownership.Validation at
e97dbcec94e813742d9e61d686ed3b9ceda87b9b:test_check/task test-all, full configured suite (full_run_reason=large_changeset): 43,634 passed, 615 skipped, 29 xfailed.pre-commit run --all-filesand the complete diff whitespace check passed.C05 was implemented and tested, then restored: its extraction left CC 39 unchanged, exceeded the executor's existing line budget, and moved logger calls out of handlers where ARCH-003 requires them. Retaining the original follows the plan's explicit allowance for a clearer implementation with a documented deviation.
Closes #4995.