Repository navigation
refactor(runtime): add the steer interface, off by default - #6959
waynesun09 wants to merge 7 commits into
Conversation
|
🤖 Finished Review · ✅ Success · Started 12:56 PM UTC · Completed 1:33 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $21.98 |
PR Summary by QodoSteer in-flight agent runs on work-item updates
AI Description
Diagram
High-Level Assessment
Files changed (46)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Site previewPreview: https://e235161b-site.fullsend-ai.workers.dev Commit: |
|
Risk Assessment: moderate (2/5) DetailsScore preserved at 2 (moderate) under re-review anchoring: Tier 1 metadata signals remain in identical scoring brackets (39 files, 7,242 lines, 1 protected path in AGENTS.md, zero dependency changes, established author), Tier 2 churn in runtime files is largely confined to new steer files without regression history, and Tier 3 confirms an off-by-default opt-in interface slice with high rollback safety. Previous runRisk Assessment: moderate (2/5) DetailsScore preserved at 2 (moderate) under re-review anchoring: Tier 1 metadata signals remain in identical scoring brackets (39 files, 7,240 lines, 1 protected path in AGENTS.md, zero dependency changes, established author), Tier 2 churn in runtime files is largely confined to new steer files without regression history, and Tier 3 confirms an off-by-default opt-in interface slice with high rollback safety. Previous run (2)Risk Assessment: moderate (2/5) DetailsScore preserved at 2 (moderate) under re-review anchoring: Tier 1 metadata scoring brackets remain unchanged despite minor file growth (39 files, 7,228 lines, one protected path in AGENTS.md, no CI or dependency changes, established author), Tier 2 churn on pre-existing runtime files is offset by newly added steer files with no regression history, and Tier 3 confirms an off-by-default opt-in interface slice with high rollback safety. Previous run (3)Risk Assessment: moderate (2/5) DetailsScore preserved at 2 (moderate) under re-review anchoring: Tier 1 metadata (38 files, 6,788 lines, one protected-path touch in AGENTS.md, no CI or dependency changes, established author) remains identical to the prior run, Tier 2 churn on pre-existing runtime files is offset by newly added steer files with no regression history, and Tier 3 confirms an off-by-default opt-in interface slice with high rollback safety. Previous run (4)Risk Assessment: moderate (2/5) DetailsScore preserved at 2 (moderate) under re-review anchoring: Tier 1 metadata (38 files, 6,778 lines, one protected-path touch in AGENTS.md, no CI or dependency changes, established author) remains identical to the prior run, Tier 2 churn on pre-existing runtime files is offset by newly added steer files with no regression history, and Tier 3 confirms an off-by-default opt-in interface slice with high rollback safety. Previous run (5)Risk Assessment: moderate (2/5) DetailsScore preserved at 2 (moderate) under re-review anchoring: Tier 1 metadata (38 files, 6,776 lines, one protected-path touch in AGENTS.md, no CI or dependency changes, established author) remains identical to the prior run, Tier 2 churn on pre-existing runtime files is offset by newly added steer files with no regression history, and Tier 3 confirms an off-by-default opt-in interface slice with high rollback safety. Previous run (6)Risk Assessment: moderate (2/5) DetailsScore preserved at 2 (moderate) under re-review anchoring: Tier 1 metadata (38 files, 6,771 lines, one protected-path touch in AGENTS.md, no CI or dependency changes, established author) remains identical to the prior run, Tier 2 churn on pre-existing runtime files is offset by newly added steer files with no regression history, and Tier 3 confirms an off-by-default opt-in interface slice with high rollback safety. Previous run (7)Risk Assessment: moderate (2/5) DetailsScore preserved at 2 (moderate) under re-review anchoring: Tier 1 metadata (38 files, 6,771 lines, one doc-index protected-path touch in AGENTS.md, no CI or dependency changes, established non-bot author) remains unchanged in risk bracket; Tier 2 reflects elevated churn and author contention on existing runtime files while newly introduced steer files carry no prior history; and Tier 3 confirms an off-by-default opt-in interface slice (DefaultSteerEnabled=false) with high rollback safety, yielding a composite score that rounds to 2. Previous run (8)Risk Assessment: moderate (2/5) DetailsScore preserved at 2 (moderate) under re-review anchoring: Tier 1 metadata (38 files, 6,767 lines, one doc-index protected-path touch in AGENTS.md, no CI or dependency changes, established non-bot author) remains within the same risk bracket as the prior assessment; Tier 2 reflects elevated churn and author contention on existing runtime files while new steer files have no prior history; and Tier 3 confirms an off-by-default opt-in interface slice (DefaultSteerEnabled=false) with high rollback safety, yielding a composite score that rounds to 2. Previous run (9)Risk Assessment: moderate (2/5) DetailsScore preserved at 2 (moderate) under re-review anchoring: Tier 1 metadata (39 files, 6,486 lines, one doc-index protected-path touch in AGENTS.md, no CI or dependency changes, established non-bot author) remains consistent with the prior assessment; Tier 2 reflects elevated churn and multi-author contention on existing runner hotspot files while new steer files have no prior history on main; and Tier 3 confirms an off-by-default opt-in interface slice (DefaultSteerEnabled=false) with strong rollback safety, yielding a composite score of 2.13 that rounds to 2. Previous run (10)Risk Assessment: moderate (2/5) DetailsScore preserved at 2 (moderate) under re-review anchoring: Tier 1 metadata (39 files, 6,366 lines, one doc-index protected-path touch in AGENTS.md, no CI or dependency changes, established non-bot author) remains consistent with the prior assessment; Tier 2 reflects elevated churn and coupling on existing runner hotspots while new steer implementations have no history; Tier 3 confirms an off-by-default opt-in interface slice (DefaultSteerEnabled=false) with strong rollback safety, yielding a composite score that rounds to 2. Previous run (11)Risk Assessment: moderate (2/5) DetailsScore preserved at 2 (moderate) under re-review anchoring: Tier 1 metadata (39 files, 6,337 lines, one doc-index protected-path touch in AGENTS.md, no CI or dependency changes, established non-bot author) remains consistent with the prior assessment; Tier 2 reflects elevated churn and coupling on existing runner hotspots while new steer implementations have no history; Tier 3 confirms an off-by-default opt-in interface slice (DefaultSteerEnabled=false) with strong rollback safety, yielding a composite score of 2.31 that rounds to 2. Previous run (12)Risk Assessment: moderate (2/5) DetailsScore preserved at 2 (moderate) under re-review anchoring: Tier 1 metadata (39 files, ~6.2k lines, one authorized protected-path touch, no CI/dependency changes, established non-bot author) remains unchanged from the prior review; Tier 2 shows the same high-churn/multi-author pattern on pre-existing hotspots (harness.go, pi_run.go) while the new steer-* files have no history yet, matching the prior assessment's described profile; Tier 3 still reflects a closed, explicitly scoped, off-by-default interface slice with clear opt-in rollback safety and no warning-level labels. Previous run (13)Risk Assessment: moderate (2/5) DetailsScore preserved at 2 (moderate) under re-review anchoring: Tier 1 metadata (39 files, ~6.2k lines, one authorized protected-path touch, no CI/dependency changes, established non-bot author) remains unchanged from the prior review; Tier 2 shows the same high-churn/multi-author pattern on pre-existing hotspots (harness.go, pi_run.go) while the new steer-* files have no history yet, matching the prior assessments described profile; Tier 3 still reflects a closed, explicitly scoped, off-by-default interface slice with clear opt-in rollback safety and no warning-level labels. Previous run (14)Risk Assessment: moderate (2/5) DetailsScore preserved at 2 (moderate) under re-review anchoring: Tier 1 metadata (39 files, 5921 lines, one authorized protected-path touch, no CI/dependency changes, established non-bot author) is essentially unchanged from the prior review; Tier 2 shows high churn/multi-author activity on the touched harness.go hotspot but no missing change-coupling and very recent last-touch dates; Tier 3 reflects an explicitly scoped, off-by-default interface slice with clear opt-in rollback safety. No signal diverges materially from the prior assessment's described characteristics. Previous run (15)Risk Assessment: moderate (2/5) DetailsScore preserved at 2 (moderate) under re-review anchoring: Tier 1 metadata (39 files, 5929 lines, one authorized protected-path touch, no CI/dependency changes, established non-bot author) is unchanged from the prior review; Tier 2 shows high churn/multi-author/fix-revert activity on the touched harness/runtime hotspot files but all are very recently modified with no coupling, revert, or hack-sentiment signals beyond what the prior review already factored in; Tier 3 reflects an explicitly scoped, off-by-default interface slice with clear opt-in rollback safety. Previous run (16)Risk Assessment: moderate (2/5) DetailsScore preserved at 2 (moderate) under re-review anchoring: Tier 1 metadata (39 files, 5928 lines, one authorized protected-path touch, no dependency/CI changes, established non-bot author) is unchanged from the prior review, and Tier 2 churn/multi-author/fix-revert history on the touched runtime/harness hotspot files is consistent with risk already factored into the prior score rather than a new driver; Tier 3 reflects an explicitly scoped, off-by-default interface slice with clear rollback safety. Weighted composite ~2.25 rounds to 2. Previous run (17)Risk Assessment: moderate (2/5) DetailsLarge diff footprint and one protected-path touch push metadata risk up, and the touched runtime/harness files show active, multi-author churn, but the change adds no CI or dependency risk, ships with strong test coverage, and is an explicitly scoped, off-by-default interface slice with clear rollback safety and a documented, authorized protected-path edit. Previous run (18)Risk Assessment: moderate (2/5) DetailsComposite ~2.36 (0.50x2.125 Tier1 + 0.30x3.0 Tier2 + 0.20x2.0 Tier3) rounds to moderate (2/5): a large but low-risk diff (no CI/dependency changes, one protected-path touch, zero security-sensitive paths per the Tier1 script, established non-bot author) offset by real churn/multi-author/fix-revert and coupling history on the pre-existing runtime/harness files this PR extends, tempered by very recent last-touch dates, zero revert/hack-sentiment signals, and an opt-in feature flag that is off by default and intentionally scoped as a documented narrow slice of the linked issue's larger staged plan. Unchanged from the prior assessment of this same revision (score 2); Tier 1 signals (file count range, protected-path count, dependency status, author type) are identical to the prior run. Note: this review's independent security-triage classifier flagged 27 of 39 files as "security-critical" using broader content/path heuristics for the steer mailbox/interrupt mechanism (distinct from the narrower Tier 1 SECURITY_SENSITIVE_COUNT path-pattern signal, which is 0) — that finding does not change the composite score but is relevant context for reviewers. Previous run (19)Risk Assessment: moderate (2/5) DetailsComposite ≈2.20 rounds to moderate (2/5): Tier 1 reflects a large but otherwise low-risk diff (no CI/dependency changes, one protected-path touch, zero security-sensitive files, established non-bot author); Tier 2 shows real churn/multi-author/fix-revert history on the pre-existing runtime/harness files this PR extends, offset by very recent last-touch dates and zero revert/hack-sentiment signals; Tier 3 stays low because the new steer capability ships off-by-default/opt-in and this is an explicitly intentional narrower slice of a larger staged feature rather than scope creep. Unchanged from the immediately preceding assessment of this same revision. Previous run (20)Risk Assessment: moderate (2/5) DetailsTier 1 metadata (2.13) reflects a large but otherwise low-risk diff: no CI or dependency changes, one protected-path touch, zero security-sensitive files, established non-bot author, moderate test ratio. Tier 2 (2.86) confirms real churn/multi-author/fix-revert history on the pre-existing runtime/harness files this PR extends, offset by very recent last-touch dates and zero revert/hack-sentiment signals. Tier 3 (1.25) stays low because the new steer capability ships off-by-default/opt-in and the PR is an explicitly intentional narrower slice of a larger staged feature rather than scope creep. Weighted composite (0.52.13 + 0.32.86 + 0.2*1.25 ≈ 2.17) rounds to moderate (2/5), unchanged from the immediately preceding re-review of this same revision (Tier 1 signals unchanged; no Tier 2/3 signal provides a specific reason to diverge). Previous run (21)Risk Assessment: moderate (2/5) DetailsAdjusted down from the prior 3/5 (elevated) because this revision carries a linked issue (#6957), enabling full three-tier weighting (50/30/20) instead of the prior no-issue 62/38 split, and SECURITY_SENSITIVE_COUNT is 0 (contradicting the prior rationale's characterization); Tier 1 (2.13) reflects a large but otherwise low-risk diff (no CI/dependency changes, established author, moderate test ratio, one protected-path touch), Tier 2 (2.57) confirms real churn/multi-author/fix-revert history on the pre-existing runtime/harness files this PR extends, and Tier 3 (1.67) is low because the new steer capability is off-by-default/opt-in and the issue's larger scope is an intentionally staged partial implementation, yielding a weighted composite of ~2.17 that rounds to moderate (2/5). Previous run (22)Risk Assessment: elevated (3/5) DetailsElevated from the prior review's moderate score: this revision now touches a protected path (AGENTS.md) and concentrates 26 of 39 files in security-sensitive steer/harness authorization surfaces, pushing Tier 1 up; Tier 2 confirms sustained high churn, multi-author contention, and fix/revert rates on the pre-existing runtime/harness files this PR extends. With no linked issue, weights redistribute to 62% Tier1/38% Tier2, yielding a weighted composite that rounds to elevated (3/5). Previous run (23)Risk Assessment: moderate (2/5) DetailsNo linked issue exists so weights redistribute to Tier1 62% / Tier2 38%; Tier1 stays low (avg ~1.9/5) because protected-path, security-sensitive, CI-workflow, and dependency signals are all zero/false, with only diff size (large, 5293 lines/36 files) and a low test ratio (0.22) pulling individual dimensions up, while Tier2 lands moderate (avg ~2.6/5) driven by genuinely high churn and conventional fix-commit frequency on the pre-existing shared harness/runtime files this PR touches, offset by very recent last-modified dates and near-zero revert/workaround sentiment hits. This independently reproduces the prior assessment score of 2 (moderate); no signal provides a specific reason to move off moderate despite the PR file count being trimmed from ~79 to 36 files in this restructuring. Previous run (24)Risk Assessment: moderate (2/5) DetailsComposite recomputed independently at 2/5 (Tier1=2, Tier2=3, Tier3=2): Tier1 stays low because protected-path, security-sensitive, CI-workflow, and dependency signals are all zero/false, with only diff size (large, 5061 lines/36 files) and low test ratio (0.22) pulling individual dimensions up; Tier2 is moderate, driven by genuinely high churn/fix-grep and multi-author-contention on the pre-existing shared runtime/harness files this PR touches, offset by very recent last-modified dates and zero workaround-sentiment/revert-frequency hits; Tier3 is mild since the PR is an intentional, narrowly-scoped, additive, opt-in slice (steer default-disabled, no caller wired yet) of a larger breaking-change-labeled umbrella issue. This matches the prior assessment score of 2 -- Tier 1 signals are unchanged from the prior run despite the diff growing slightly, and no Tier 2/3 signal provides a specific reason to move off moderate. Previous run (25)Risk Assessment: moderate (2/5) DetailsIndependent recomputation for the restructured 36-file/4807-line diff drops from the prior elevated (3/5): protected_path_count is now 0 and ci_workflow_changed is false (no .github/workflows/ or AGENTS.md content changes in this diff, unlike the pre-split PR), pulling Tier 1 down despite the change-size sub-score remaining high; Tier 2 stays moderate due to continued churn in shared harness/runtime files; Tier 3 is mild since the PR is an intentional, scoped, opt-in slice of a larger issue. The composite genuinely moves to moderate (2/5) because the PR was split and no longer touches protected CI/governance paths. Previous run (26)Risk Assessment: elevated (3/5) DetailsIndependent recomputation confirms the prior elevated score: Tier 1 sits in top-bucket ranges (79 files, 12,615 lines, 2 protected CI workflow paths, no dependency changes, established human author), Tier 2 shows sustained high churn/fix-revert/multi-author activity concentrated in the same hotspot files diluted somewhat by many brand-new files with no history, and Tier 3 stays moderate — design matches the linked issue but full real-world validation is explicitly still pending while the feature ships fully default-off. Weighted composite rounds to 3, matching the prior review; no signal changed enough to move off elevated. Previous run (27)Risk Assessment: elevated (3/5) DetailsRe-review anchoring holds at elevated (3/5): Tier 1 metadata (78 files/12,518 lines, 3 protected CI-adjacent paths, no dependency changes) sits in the same top-bucket ranges as the prior assessment; Tier 2 dilution reflects many brand-new files rather than reduced risk, with the same two hotspots (internal/forge/github/github.go, internal/runtime/codex_run.go) still showing high churn/fix-revert/multi-author patterns; Tier 3 stays moderate with issue #6957 deliberately left open pending real-world validation but the feature shipped fully default-off. No signal justifies moving off the prior elevated score. Previous run (28)Risk Assessment: elevated (3/5) DetailsRe-review anchoring holds at elevated (3/5): Tier 1 metadata (73 files/12,409 lines, 3 protected CI-adjacent paths, no dependency changes) sits in the same top-bucket ranges as the prior assessment; Tier 2 confirms internal/forge/github/github.go and internal/runtime/codex_run.go remain the same high-churn, high fix/revert, multi-author hotspots; Tier 3 stays moderate, with issue #6957 deliberately left open pending real-world validation but the feature shipped fully default-off, so no signal justifies moving off the prior elevated score. Previous run (29)Risk Assessment: elevated (3/5) DetailsRe-review anchoring holds: independently computed Tier 1 signals (73 files/12,498 lines, 3 protected paths, CI workflow touched, 0.27 test ratio) land in the same top-bucket ranges as the prior elevated assessment, Tier 2 confirms the same high-churn, multi-author hotspots with no new concerning file, and Tier 3 stays low with matching issue scope and no risky labels, so the score is preserved at elevated (3/5). Previous run (30)Risk Assessment: elevated (3/5) DetailsHeld at elevated (3/5), unchanged from the prior re-review. Tier 1 metadata moved only within the same scoring buckets since the last assessment (68→70 files, ~11,729→12,350 lines, both already at the top bucket; 2 protected CI workflow paths unchanged; no dependency-file changes; test ratio 0.31→0.29 stays in the same bucket; non-first-time, non-bot author). Tier 2 confirms internal/cli/run.go, internal/forge/github/github.go, and .github/workflows/reusable-dispatch.yml remain the same high-churn, multi-author hotspots already driving the prior score, with no new concerning files introduced. Tier 3 stays low: the linked issue's scope matches the PR, carries no warning-level labels, and the change ships behind a verified default-off flag. This review's own findings corroborate the score: a forge-abstraction-deviation left unresolved for two review rounds and the protected-CI-path change are the concrete drivers of a merge-review-worthy but not high/critical risk profile; no new high-severity code defects were found and several previously flagged issues (steer-budget carryover, workflow-run pagination, receipt collision, console-rendering regression) were independently verified fixed in this revision. Previous run (31)Risk Assessment: elevated (3/5) DetailsDiff has grown slightly to 68 files / ~11,729 lines, still touching 2 protected CI workflow paths (fullsend.yaml, reusable-dispatch.yml) with a large blast radius across runtime, dispatch, CLI, and forge layers. internal/cli/run.go and internal/forge/github/github.go remain high-churn hotspots, reproducing the prior review's top risk driver unchanged. This is offset by a verified default-off gate (steerEligible hard-requires FULLSEND_PRESERVE_RUNS=true), no new dependency-file or supply-chain changes, a solid test ratio (~31% of changed files are tests), an experienced non-first-time author, and a thorough ADR-driven design (ADR 0101) with an explicit staged rollout gated behind a separate agents-repo PR. Score held at elevated (3/5), unchanged from the immediately-prior re-review. Previous run (32)Risk Assessment: elevated (3/5) DetailsVery large diff (65 files, ~11.4k lines, 2 protected CI paths, blast radius large) and a modified hotspot (internal/cli/run.go, internal/forge/github/github.go) with heavy historical fix/revert churn push risk up, offset by a verified default-off opt-in flag, no security-sensitive or dependency-file changes beyond the workflow files already counted as protected paths, solid test ratio, an experienced non-first-time author, and a thorough ADR-driven design with a staged rollout plan, landing the composite at elevated (3) rather than high. Previous run (33)Risk Assessment: high (4/5) DetailsTier 1 metadata is essentially unchanged from the prior assessment (65 files, ~11.4k lines, protected-path count maxed at 2, CI workflow touched, no dependency files, non-bot/non-first-time author), so per re-review anchoring the score is preserved at 4/high; Tier 2 reinforces this with heavy churn/fix-revert rates and change-coupling gaps on the two touched files with prior git history; Tier 3 shows a breaking change to in-flight agent-run control stacked on an unmerged base PR, addressing an issue whose own author states validation criteria are not yet met, though the capability itself is safely gated behind a default-off harness.steer.enabled flag confirmed in the diff. Previous run (34)Risk Assessment: high (4/5) DetailsScore anchored at 4 (high) per re-review anchoring policy. Tier 1 signals essentially unchanged: FILES_CHANGED remains 65, LINES_CHANGED increased marginally from 10936 to 11414, PROTECTED_PATH_COUNT at maximum threshold, CI_WORKFLOW_CHANGED true. Tier 2 churn identical to prior (internal/cli 291, internal/runtime 158 commits in 30d). Breaking behavioral change to in-flight agent run control with the issue still open. Previous run (35)Risk Assessment: high (4/5) DetailsHigh risk anchored from prior assessment: Tier 1 signals essentially unchanged (65 files, 10936 lines, PROTECTED_PATH_COUNT at maximum threshold), Tier 2 churn near-identical (internal/cli 291 and internal/runtime 158 commits in 30d), breaking-change designation, and no mitigating factors justify lowering the score. Previous run (36)Risk Assessment: high (4/5) DetailsHigh risk maintained from prior assessment: PR grew from 46 to 64 files (10920 lines), PROTECTED_PATH_COUNT at maximum threshold, Tier 2 churn signals near-identical (internal/cli 295 and internal/runtime 159 commits in 30d), and no mitigating factors emerged to justify a lower score. Previous run (37)Risk Assessment: high (4/5) DetailsHigh risk due to a breaking change, very large blast radius (46 files, 7637 lines across 9 packages including a new internal/steerwatch/ package), active modification of high-churn areas (internal/cli/ with 298 commits in 30 days, internal/runtime/ with 118), CI workflow changes, and cross-component scope spanning runner and dispatch. The 0.33 test file ratio provides partial mitigation but is modest for a change of this magnitude. |
ReviewFindingsMedium
Low
This is a re-review (prior head
A second file,
Previous runReviewFindingsMedium
Low
This is a re-review (prior head
Both changes are fixes for the two LOW findings raised in the prior review round ( Previous run (2)ReviewFindingsMedium
Low
This is a re-review (prior head Both prior LOW findings ( All three LOW findings above were adversarially challenged. The challenger downgraded the Labels: PR adds substantial harness config surface (steer: block in internal/harness), touches internal/sandbox lifecycle code, and introduces two new ADRs, none of which are currently labeled. Next steps:
Previous run (3)ReviewFindingsMedium
Low
The forge compare API reports this revision as "diverged" from the prior review (9 ahead / 9 behind, 45 files) because the shared
Next steps:
Previous run (4)ReviewFindingsMedium
Only 2 of the 38 changed files differ from the prior review (head SHA
The remaining 36 files, the PR title, and all commit message prefixes ( Correctness (full scope, mandatory) and style-conventions (trivial scope, per re-review policy for a dimension with no unresolved prior findings) independently re-examined this revision and returned no findings. Docs-currency was dispatched because documentation files changed since the prior review; it confirmed the two clarifications are internally consistent with each other and with the rest of the unchanged documentation, and cross-checked the Previous run (5)ReviewFindingsMedium
Note on prior findings not observed in the current diff: the previously-flagged Only 3 files changed since the prior review (head SHA Previous run (6)ReviewFindingsMedium
Low
Note on prior findings not observed in the current diff: the previously-flagged The orchestrator additionally verified, by diffing actual file contents at the prior-reviewed head SHA against the current head SHA, that 36 of the 38 changed files carry no PR-authored change since the prior review — only Previous run (7)ReviewFindingsMedium
Low
Note on prior findings not observed in the current diff: the previously-flagged Next steps:
Previous run (8)ReviewFindingsMedium
Low
Note on prior findings not observed in the current diff: the previously-flagged Previous run (9)ReviewFindingsMedium
Low
Note on prior findings not observed in the current diff: the previously-flagged Next steps:
Previous run (10)ReviewFindingsMedium
Low
Note on prior findings not observed in the current diff / not carried forward: the previously-flagged Next steps:
|
![fullsend-ai-review[bot]](https://avatars.githubusercontent.com/in/3478501?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
fullsend-ai-review
Bot
added
component/runner
|
functional-tests is red on |
Two review findings on #6959, one cosmetic and one that would have lost updates. 1. closeFeedIf was duplicated verbatim on ClaudeRuntime and PiRuntime. Extracted as steerCloseFeedIf in steer_session.go, next to the state machine whose decision it acts on. Both runtimes call it; behaviour is unchanged, and it now has direct tests (the two copies had none, being reachable only from Run's stream handler). 2. Confirming steer-2's Qodo fix surfaced a real defect on the codex path. recordDelivery was called from nextCodexTurn, BEFORE the resumed process launched, so a resume that failed to start — or one codex refused because the thread was gone — still appended a SteerResult. With the marker now rebuilt from RunMetrics.Steers, that would mark the follow-up run consumed, the queued run would skip it, and the update would be lost outright rather than merely delayed. Delivery is now two-phase: nextCodexTurn stakes the message with the resume's start time, and confirmDelivery records it only once that process reported a thread of its own. codex emits thread.started on a resume — verified live, the resumed process repeats the original thread_id — so an empty thread id is proof the resume never took. It is confirmed on the error path too, which is precisely where the resume did not happen. DeliveredAt is still the resume's start, not its end, because that is when the message enters the thread. Verified for the reviewer's question, all three runtimes append a SteerResult only after the delivery signal and never on a failed one, and FollowUpRunID is carried from the SteerMessage in every case: - Claude: appended in noteEcho, driven by the --replay-user-messages echo (isReplay). appendLine increments nothing and queues nothing unless the mailbox write returned exit 0, so a failed write can never acquire a result. Already covered; the failed-write test now also asserts steerResults() stays empty. - pi: same shared machinery, its ack being the rpc `response` with command=prompt and success=true (a success=false response is deliberately not an ack, already tested). Added two pi-level tests — nothing recorded at append time, recorded on the steer's own ack with the right run id, and nothing recorded when the write fails — because the reviewer asked per runtime and pi had none of its own. - Codex: as above, plus tests that an unconfirmed resume records nothing, that a discarded delivery cannot be resurrected by a later confirm, and that confirming with nothing staked (every run's first turn) is a no-op. steerCloseFeedIf, stakeDelivery and confirmDelivery are at 100%. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
|
🤖 Review · Commit: |
|
🤖 Finished Review · ❌ Failure (validation failed after 2 iteration(s)) · Started 4:04 PM UTC · Completed 4:46 PM UTC Commit: Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high |
|
🤖 Finished Review · ✅ Success · Started 4:15 PM UTC · Completed 4:42 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $10.37 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Review · ✅ Success · Started 5:56 PM UTC · Completed 6:16 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $10.41 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Review · ✅ Success · Started 7:42 PM UTC · Completed 8:05 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $10.07 |
|
🤖 Finished Review · ✅ Success · Started 9:13 PM UTC · Completed 9:35 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $9.97 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Review · ✅ Success · Started 3:31 AM UTC · Completed 3:52 AM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $9.53 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Review · ✅ Success · Started 1:06 PM UTC · Completed 1:26 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $5.96 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
maruiz93
left a comment
maruiz93
left a comment
There was a problem hiding this comment.
I think there might be some issues by enabling steering that we should declare in the consequences due to skipping pre-script.
| the item moved; the receipt that lets the queued run skip its work; and whether steering is on by | ||
| default. | ||
|
|
||
| ## Consequences |
There was a problem hiding this comment.
I understand correctly, one of the consequences is that the new input doesn't pass through the pre-script. I believe this is important enough to be written in here.
This could have behaviour and security considerations on agents so they should be explicitly declared so agent creators are aware of them.
There was a problem hiding this comment.
Agreed, and added in c35c90d. ADR 0113 now has a Consequences bullet for this (in 4b8a665):
- The pre-script runs once, on the host, for the event that dispatched the run. An update the run absorbs goes straight into the sandbox, so no per-event gate a pre-script implements applies to it: payload validation, a merged or draft check, an author skip-list, an iteration cap, an instruction size cap.
- The runner applies its own gates instead: the
Routejob's authorization of the event, the split into one amendment and context, the envelope's sanitizer and size bounds, the exclusion of fullsend's own output, andmax_steers. - An agent creator whose gate must hold for every event keeps it in the dispatch arm, enforces it in the agent from the envelope, or leaves steering off for that harness. The post-script is unaffected.
The steer section of harness-reference.md now says the same in one sentence, so agent creators see it where they configure steering.
|
🤖 Finished Review · ✅ Success · Started 7:47 PM UTC · Completed 8:05 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $7.11 |
|
🤖 Finished Review · ✅ Success · Started 9:00 PM UTC · Completed 9:21 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $8.48 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Review · Commit: |
|
🤖 Finished Review · ✅ Success · Started 11:59 PM UTC · Completed 12:25 AM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $14.33 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
🤖 Finished Review · ✅ Success · Started 3:45 PM UTC · Completed 4:14 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $11.74 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
A runtime that can take a message into a session already running implements Steerer. RunParams.Steerable asks for that session to be kept open; Run then returns only after Settle and the agent's current turn, so a runtime that does not implement it is unchanged. Delivery is acknowledged rather than assumed: a steer counts as delivered only when the runtime echoes that specific message back, matched by the message's own identity. The mailbox the live runtimes read sits in a directory the agent can write to, so an echo matching nothing outstanding is ignored rather than counted. The envelope renders the runner's message as the bytes the agent definitions match on, versioned separately as a contract between the two repositories. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
Claude Code reads stream-json input, so the session is started with stdin connected to an in-sandbox feeder tailing a mailbox file and Steer appends one line to it. `tail -n +1 -f` starts at the first line, so the opening prompt is delivered by the same channel as every later steer, and the file stays open for each append. The ack is --replay-user-messages: the runtime re-emits each consumed stdin line with isReplay set. Tool results arrive as "user" lines too, so only the replay flag distinguishes them — counting "user" lines would let the run settle with a steer still unread in the mailbox. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
pi takes a message into a running session over rpc, so the same mailbox feeds its stdin and Steer appends one line. pi's ack carries the id the runner sent, which is a stronger match than content: the steer feed pairs each echo with the message it acknowledges and ignores one that matches nothing outstanding. agent_settled is pi's end-of-prompt marker rather than an end-of-run signal, so on a steerable run it ends the turn and not the session. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
…the thread codex exec has no live input channel, so a steer stops the current process and resumes the same thread with the message as the next prompt. That is why Steerer is two methods rather than one send: the interrupt and the settle are different operations on this runtime. A resume is recorded only once the new process reports a thread of its own. codex emits thread.started on a resume, so an empty thread id means the resume never took and the steer did not reach the agent; recording it at stake time would claim an update had been acted on when it never was. The interrupt stops the sandbox and starts it again (new sandbox.Stop and sandbox.Start primitives with phase waits): an exec's processes are not expected to exit with the caller, and a stop is the released way to end them while keeping the disk state the resume reads. The turn is marked before the stop, so its relay-closed exit counts as the runner's own interrupt, not an agent failure or a missed resume. On the pinned OpenShell 0.0.116 a stop takes about 46 s, exported as CodexSteerInterruptCost for settle budgets. The caller holds the sandbox lock across Steer, so the credential refreshers wait the stop out instead of being cut off mid-write. A process the integrity guards refuse (exit 97 or 98) is the run's verdict: it is not retried as a missed resume, and an earlier turn's success does not stand in for it. A first turn that opens no thread ends the run, since there is nothing to resume onto. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
`steer:` configures whether a run absorbs updates to its work item while it is still working, and the ceilings on doing so: max_steers per run and the poll interval. It composes like security — a child replaces the base's block whole rather than field by field — so a child that sets one key gets the defaults for the rest instead of a silent mixture of two layers. enabled is a pointer so that absent and false mean different things. A block tuning only max_steers says nothing about whether steering is on, so it takes the default rather than reading as an opt-in or an opt-out, and the same config keeps its meaning when the default changes. Off by default while the surfaces steering depends on land. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
…ing it ADR 0106 stops the cancellation, and this ADR addresses what preserving alone leaves standing: output computed from state the subject has moved past, a queued run that redoes the same work, and no way for a person to reach a run already working. It decides only that steering happens. The interface, provenance and actor authority, how a run learns its item moved, the receipt, and the default are five decisions of their own, named here in prose and recorded separately. ADR 0106 is annotated rather than superseded: a steered run revisits its no-polling clause and nothing else — it still invokes no run of its own, every event still creates its pending run, and the timeout window is unchanged. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
The sandbox is the constraint: the runner reaches it only through exec and upload, and an exec is a sibling of the agent process rather than its parent, so there is no stdin to write to from outside. ADR 0117 records the options that fails on — restart, upload, exec stdin — and the one that does not: a mailbox the agent's own stdin is already tailing. The envelope is pinned as a v1 contract because two repositories read it: the runner writes those bytes and the agent definitions match on them, so fixing them in either would make a change there a silent break here. steering.md carries only what this change ships — concurrency, the contract, the transport and the configuration. The sections on how a run learns its item moved, and on what the queued run reads, arrive with those changes. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
|
🤖 Finished Review · ✅ Success · Started 6:55 PM UTC · Completed 7:19 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $10.58 |
What this is now
The interface slice of the steering stack: the runtime capability a steer is delivered through, the harness block that configures it, and the two ADRs that decide it. Steering is off by default here — nothing in this PR turns it on, and no caller invokes it yet.
This PR was previously the whole of steering. It has been cut down: the watcher, provenance checks, the receipt and skip check,
fullsend steer, and the on-by-default flip now live in the PRs stacked above this one, each with its own ADR.Builds on #7007 (ADR 0106 implementation). Tracking issue: #6957 — which is also
the authority for the one protected-path edit here, the
AGENTS.mdtopic-indexrow pointing at
docs/contributing/steering.md.Implements the design recorded in #6957 (closed as completed; this stack ships it).
What lands
runtime.Steerer— an optional capability,Steer+Settle.RunParams.Steerableasks a steerable runtime to keep its session open;Runthen returns only afterSettleand the agent's current turn. A runtime that does not implement it is unchanged.steer:harness block —enabled,max_steers,poll_interval_seconds, with base composition and validation.enabledis a pointer so absent andfalsestay distinguishable.docs/normative/steer-envelope/v1— the envelope pinned as a versioned contract, because the runner writes those bytes and the agent definitions infullsend-ai/agentsmatch on them.Notes for review
fullsend rundoes not poll for later events; a steered run does. The annotation names that clause and nothing else — steering still invokes no run of its own, every event still creates its pending run, and the timeout window is unchanged. 0106's deferral of content provenance and actor authority stands, and is the mandate of the ADR in the PR above this one.DefaultSteerEnabledisfalse. The flip is the last PR in the stack, and waits on the PRs between.max_steersandpoll_interval_secondsare parsed and validated here but have no consumer yet — the code that spends the cap and paces the interval arrives with the change that looks for updates. The field comments and the docs say so rather than describing them as live.steering.mdis a skeleton: concurrency, the contract, the transport, configuration. The provenance, watcher, ceilings and receipt sections arrive with the PRs that implement them.SteerandSettle. Both write into the running sandbox — the mailbox append and the feeder kill, or codex's sandbox stop and start, which end every process in the sandbox and would otherwise cut off a credential refresher's upload mid-write..envcould use to run code inside the agent and forge its delivery echo (NODE_OPTIONS/NODE_PATH/LD_*on Claude Code,LD_*on pi). The ordinary pi launch is tracked on pi launch does not clear LD_* loader variables after sourcing .env #7580.Verification
go build ./...,go vet ./...,go test -race ./internal/runtime/... ./internal/harness/...all clean.pre-commit run --from-ref <base> --to-ref HEADpasses, ADR and markdown-link lints included; every commit was hook-validated in isolation as it was made. No file in this PR referencessteerwatch, the receipt, the skip check, orfullsend steer.