feat(harness)!: steer the running agent by default - #7377
waynesun09 wants to merge 6 commits into
Conversation
PR Summary by Qodofeat(harness)!: steer the running agent by default
AI Description
Diagram
High-Level Assessment
Files changed (12)
|
|
🤖 Review · Commit: |
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Site previewPreview: https://515e3fd3-site.fullsend-ai.workers.dev Commit: |
… agents Four things from the qodo review on #7377. fix.md said a harness that opts out "cancels the in-flight fix run and starts a new one". That has been wrong since #7007 made preserving unconditional: no stage job cancels a run in flight any more, so an opted-out harness lets the run finish and the queued run works from the PR's current state. The sentence read fine, which is why it survived two passes. Grepping the rest for the same mistake turned up four more places where the alternative to steering was still described as cancellation — bugfix-workflow.md ("instead of cancelling it"), this page's own opening, the receipt rationale ("costs more than cancelling does today", "where cancel-and-restart produces one"), and the same comparison in ADR 0101's Decision. None is about the opt-out, all are stale in the same way: they compare steering against a behaviour that no longer exists. They now compare it against preserving alone, which is what an opted-out harness actually gets, and what the queued run does in either case. harness-reference.md's eligibility paragraph still required `enabled: true` and promised a printed reason for every declined watch, both of which this PR changed. It now names the three conditions — not explicitly disabled, a runtime that can take a message, a GitHub Actions job with a work item — and says the reason is printed only when the harness set `enabled: true` itself. The warning gate had no test. TestStartSteerWatcherDeclineMessage is table-driven over an ineligible fixture: no block and a block that only tunes max_steers stay quiet, `enabled: true` prints the reason, `enabled: false` stays quiet. The max_steers row is there because it is the case the pointer exists for — it must read as "not an explicit request", not as an opt-out. The rollout paragraph gains a fourth open precondition. agents#1163 teaches the envelope to code, fix, review and triage only; prioritize, retro and scribe are outside it and no harness anywhere sets `steer:`, so with the default on an eligible run on one of those three delivers an envelope the definition ignores, the runtime acks it, the receipt is posted, and the queued run skips — the update is dropped silently. The fix belongs in the agents repo, so this is recorded as a gate rather than changed here. ADR 0101 stays at 102 lines of content. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
7c73f43 to
9f28f23
Compare
|
🤖 Finished Review · ✅ Success · Started 4:35 PM UTC · Completed 5:00 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $6.83 |
|
Risk Assessment: moderate (2/5) DetailsWeighted composite (~2.27 -> 2, moderate) anchored to the prior round's 2/5 assessment: low Tier-1 metadata risk (no protected/security/CI/dependency paths, non-bot non-first-time author) dominates at 50% weight, offsetting elevated Tier-2 churn/fix-history in internal/harness/* and a Tier-3 mismatch where the PR's default-on flip supersedes the linked issue's own off-by-default acceptance criterion without a recorded issue comment. This score is informational and does not affect the review verdict, which independently rests on the fail-open finding above. Previous runRisk Assessment: moderate (2/5) DetailsWeighted composite (~2.25 -> 2) driven down by low Tier-1 metadata risk (no protected/security/CI/dependency paths, non-first-time non-bot author) despite a heavy-churn, fix-prone harness module in Tier-2 and a breaking on-by-default flip that contradicts the linked issue's own acceptance criterion in Tier-3; the sub-agent's independent recomputation did not anchor to the prior 4/high sticky-comment score, noting the prior score may have weighted the PR's self-applied risk/high label and admitted rollout gaps more heavily than the deterministic Tier1-3 signal buckets support. Orchestrator note: this divergence from the prior two 4/high assessments is presented as-is per the risk-assessment sub-agent's own output; it is informational and does not affect the review verdict, which independently rests on the fail-open finding above. Previous run (2)Risk Assessment: high (4/5) DetailsRe-review anchoring preserves the prior high (4/5): Tier 1 signals fall in the same buckets as the prior review despite growth from 14/507 to 19/602 lines (added tests and doc touch-ups), and Tier 2/3 signals still reaffirm risk - internal/harness/* remains a heavy-churn, fix-prone module, and the breaking on-by-default flip still contradicts the linked issue's own validation criterion with four release preconditions explicitly unmet per the PR body. Previous run (3)Risk Assessment: high (4/5) DetailsRe-review anchoring preserves the prior high (4/5): Tier 1 signals are unchanged (14 files/507 lines, large blast radius, no protected/security/dependency paths, same non-bot author) despite the branch restack, and Tier 2/3 signals reaffirm rather than mitigate risk - high churn and fix/revert density in internal/harness/*, plus a breaking fleet-wide default flip that still contradicts the linked issue own validation criteria while four release preconditions remain explicitly unmet. Previous run (4)Risk Assessment: high (4/5) DetailsLarge-blast-radius breaking change that flips a fleet-wide harness default in a file with heavy recent churn, while directly contradicting the linked issue's own validation criteria and shipping despite the author's explicit acknowledgment that four release preconditions remain unmet. Previous run (5)Risk Assessment: moderate (2/5) DetailsRe-review anchoring preserved at moderate (2/5): Tier 1 signals remain in the same buckets as the prior assessment (13 vs 12 files, 392 vs 388 lines both in the 300-799 bucket, no protected/security/dependency changes, same non-bot/non-first-time author), Tier 2 confirms the same high-churn, multi-author, fix-heavy harness.go pattern already reflected in the prior score, and Tier 3 remains unchanged; the incremental commit since the prior round only made comment/doc-wording fixes with no logic change, so the composite stays at 2 (moderate), with the breaking default-behavior flip still mitigated by the documented per-harness opt-out. Previous run (6)Risk Assessment: moderate (2/5) DetailsRe-review anchoring preserved at moderate (2/5): Tier 1 signals remain in the same buckets as the prior assessment (12 files, 388 vs 304 lines both in the 300-799 bucket, no protected/security/dependency changes, same non-bot/non-first-time author), and Tier 2 confirms the same high-churn, multi-author, fix/revert-heavy core file pattern already reflected in the prior score; the incremental commit only added test coverage and doc-wording fixes to the same file set, so the composite stays at 2 (moderate), unchanged from the prior review, with the breaking-change default flip still mitigated by the documented per-harness opt-out. Previous run (7)Risk Assessment: moderate (2/5) DetailsSmall doc/config-heavy PR (12 files, 304 lines, no protected/security-sensitive paths, no CI or dependency changes) scores low on Tier 1, but touches a high-churn, multi-author core file (internal/harness/harness.go) with fix/revert history, and is a fleet-wide breaking-change default flip mitigated by an available per-harness opt-out, netting a moderate composite score. |
ReviewFindingsHigh
Medium
Next steps:
Previous runReviewFindingsHigh
Medium
Low
Next steps:
Previous run (2)ReviewFindingsHigh
Medium
Low
Next steps:
Previous run (3)ReviewFindingsHigh
Low
Next steps:
Previous run (4)ReviewFindingsHigh
Low
Next steps:
Previous run (5)Looks good to me Previous run (6)ReviewFindingsLow
Next steps:
Previous run (7)ReviewFindingsMedium
Low
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.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
… agents Four things from the qodo review on #7377. fix.md said a harness that opts out "cancels the in-flight fix run and starts a new one". That has been wrong since #7007 made preserving unconditional: no stage job cancels a run in flight any more, so an opted-out harness lets the run finish and the queued run works from the PR's current state. The sentence read fine, which is why it survived two passes. Grepping the rest for the same mistake turned up four more places where the alternative to steering was still described as cancellation — bugfix-workflow.md ("instead of cancelling it"), this page's own opening, the receipt rationale ("costs more than cancelling does today", "where cancel-and-restart produces one"), and the same comparison in ADR 0101's Decision. None is about the opt-out, all are stale in the same way: they compare steering against a behaviour that no longer exists. They now compare it against preserving alone, which is what an opted-out harness actually gets, and what the queued run does in either case. harness-reference.md's eligibility paragraph still required `enabled: true` and promised a printed reason for every declined watch, both of which this PR changed. It now names the three conditions — not explicitly disabled, a runtime that can take a message, a GitHub Actions job with a work item — and says the reason is printed only when the harness set `enabled: true` itself. The warning gate had no test. TestStartSteerWatcherDeclineMessage is table-driven over an ineligible fixture: no block and a block that only tunes max_steers stay quiet, `enabled: true` prints the reason, `enabled: false` stays quiet. The max_steers row is there because it is the case the pointer exists for — it must read as "not an explicit request", not as an opt-out. The rollout paragraph gains a fourth open precondition. agents#1163 teaches the envelope to code, fix, review and triage only; prioritize, retro and scribe are outside it and no harness anywhere sets `steer:`, so with the default on an eligible run on one of those three delivers an envelope the definition ignores, the runtime acks it, the receipt is posted, and the queued run skips — the update is dropped silently. The fix belongs in the agents repo, so this is recorded as a gate rather than changed here. ADR 0101 stays at 102 lines of content. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
…sion Review comment 5701340330 on #7377. steerHarness(false) left Steer nil, which this PR made mean *on*. Two tests were then asserting something other than what their names claim. TestStartSteerWatcher_DisabledHarnessStartsNothing passed because fakeRuntime has no Steerer, not because the harness was off, and TestCheckSteerAlreadyHandled_OffPaths' "steering disabled" case went down the enabled branch and returned false from a failed timeline read instead. steerHarness now always writes the key, and steerHarnessDefault covers the absent-block case that used to be conflated with it. The disabled-watcher test gets a steerable runtime and asserts the fixture is otherwise eligible first, so the opt-out is the only thing left that can stop it. Its companion pins that a default harness is enabled but not explicitly enabled — the two states the fixture used to blur. The off-paths test now also asserts nothing was printed. Returning false is not evidence a guard held: a failed timeline read returns false too, after warning. Without that assertion the mutation is not caught — dropping the SteerEnabled check from checkSteerAlreadyHandled leaves the test passing on the return value while the guarded call reaches the network. With it, the case fails on "Could not check whether this update was already handled: ... 401 Bad credentials", which is both the mutation caught and proof the real code never makes that call. Three descriptions that had not kept up: harness.go's SteerConfig type comment still said disabled by default. The field comment below it was already correct, which is how the type comment survived. harness-reference.md's field merge table had no steer row at all, so the one inheritance rule that now matters — a child with no block inherits the base's opt-out rather than the default — was written down nowhere the table's readers would look. steering.md said the declined-watch warning fires when the harness "named steering itself", which reads as though a cap-only block would qualify. The code requires enabled: true, as the test row added for exactly that case shows. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
9f28f23 to
f92c2ce
Compare
|
Addressed in f92c2ce (rebased onto #7376's e07a57e). The test fixtures now say off explicitly: |
|
🤖 Finished Review · ✅ Success · Started 5:10 PM UTC · Completed 5:41 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $8.70 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Fixed in c0d78a6: the |
…sion Review comment 5701340330 on #7377. steerHarness(false) left Steer nil, which this PR made mean *on*. Two tests were then asserting something other than what their names claim. TestStartSteerWatcher_DisabledHarnessStartsNothing passed because fakeRuntime has no Steerer, not because the harness was off, and TestCheckSteerAlreadyHandled_OffPaths' "steering disabled" case went down the enabled branch and returned false from a failed timeline read instead. steerHarness now always writes the key, and steerHarnessDefault covers the absent-block case that used to be conflated with it. The disabled-watcher test gets a steerable runtime and asserts the fixture is otherwise eligible first, so the opt-out is the only thing left that can stop it. Its companion pins that a default harness is enabled but not explicitly enabled — the two states the fixture used to blur. The off-paths test now also asserts nothing was printed. Returning false is not evidence a guard held: a failed timeline read returns false too, after warning. Without that assertion the mutation is not caught — dropping the SteerEnabled check from checkSteerAlreadyHandled leaves the test passing on the return value while the guarded call reaches the network. With it, the case fails on "Could not check whether this update was already handled: ... 401 Bad credentials", which is both the mutation caught and proof the real code never makes that call. Three descriptions that had not kept up: harness.go's SteerConfig type comment still said disabled by default. The field comment below it was already correct, which is how the type comment survived. harness-reference.md's field merge table had no steer row at all, so the one inheritance rule that now matters — a child with no block inherits the base's opt-out rather than the default — was written down nowhere the table's readers would look. steering.md said the declined-watch warning fires when the harness "named steering itself", which reads as though a cap-only block would qualify. The code requires enabled: true, as the test row added for exactly that case shows. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
3608041 to
01de4ba
Compare
|
🤖 Finished Review · ✅ Success · Started 9:49 PM UTC · Completed 10:06 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $5.88 |
fullsend-ai-review
Bot
added
risk/moderate
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
46aed00 to
015e163
Compare
ADR 0113 made steering opt-in because authenticated receipts did not exist yet. They do now, so the precondition is satisfied and the 2026-09-14 meeting recorded on #6957 confirmed steering is on by default alongside run continuation. SteerConfig.Enabled becomes a *bool. A plain bool cannot carry this default: `steer: {max_steers: 3}` would unmarshal to Enabled=false and silently opt the harness out while it was trying to tune the cap. nil now means on, and `steer: {enabled: false}` is the only way off — which is also the only spelling a reader can tell apart from "I said nothing". The field follows the pattern already used for the other on-by-default blocks in this file (hooks, sanitizers), including BoolDefault, so nothing new is introduced. Composition needs no change and both directions are now covered: a child with no block still inherits the base's whole block, so a base opt-out is inherited rather than overwritten by the default, and a child writing `steer: {enabled: true}` is still whole-block replacement meaning "steer with the defaults". SteerExplicitlyEnabled is new, and startSteerWatcher uses it to decide whether a declined watch is worth a warning. Every decline reason used to imply a harness that asked for steering and did not get it. With steering on by default most declines are ordinary — every local run is not in GitHub Actions, every GitLab run queues instead, a runtime that cannot take a message never could — and warning once per iteration on runs that never asked would be noise. The message now fires only when the harness named steering, which is exactly the set of runs that used to see it. max_steers (2), poll_interval_seconds (30s) and the settle floor keep their defaults, and validation is unchanged: it never read Enabled. BREAKING CHANGE: Steering is on by default from the release that carries this. A harness that omits the `steer:` block, or sets only `max_steers` or `poll_interval_seconds`, now watches for follow-up runs and delivers work-item updates into the running agent. To keep the previous behaviour, add `steer: {enabled: false}` to the harness. Repositories on GitLab, and runs outside GitHub Actions, are unaffected: the watcher declines there as before. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
Every page that described steering described how to switch it on. They now describe how to switch it off, which is the only choice a harness author has left to make. harness-reference.md says the default is true and, in the field note, that `enabled: false` is the only spelling that turns steering off — a block setting just `max_steers` or `poll_interval_seconds` still steers. That is the trap the *bool exists to close, so the reference is where it has to be written down. steering.md's Configuration section flips the same way and says why `enabled` is a pointer. Its Rollout order described enabling one harness at a time; that order existed to hold steering back until its preconditions were met, and they are met — the fleet agent definitions merged, the envelope settled with them, each runtime's steer was observed on OpenShell, and the authenticated receipt is the change below this one. The section now says steering arrives with the release, and names the one observation still outstanding rather than dropping the preconditions silently. review.md, triage.md, fix.md and bugfix-workflow.md each carried a caveat that absorbing a comment mid-run needs steering turned on. The caveat inverts: the absorb is what happens, and the opt-out is the exception. fix.md's ordering flips with it — it led with cancel-and-restart as what a repository sees, which is now the opted-out case. ADR 0113's Decision said opt-in; it now says on by default with the per-harness opt-out, and the receipt paragraph says the satisfied precondition is why the default is on rather than a switch each harness must find. The Consequences bullet "nothing changes for a repository that does not opt in" becomes its honest inverse. The file is not on main, so this is authoring rather than an amendment to an accepted ADR. It stays at 102 lines of content — where it already was, still above the writing-adrs 100-line smell line, which this change neither caused nor fixes. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
… agents Four things from the qodo review on #7377. fix.md said a harness that opts out "cancels the in-flight fix run and starts a new one". That has been wrong since #7007 made preserving unconditional: no stage job cancels a run in flight any more, so an opted-out harness lets the run finish and the queued run works from the PR's current state. The sentence read fine, which is why it survived two passes. That correction is no longer in this commit's diff: restacking onto the rewritten base conflicted on exactly that bullet, and resolving it there was the only way to keep the base's new "one run waits behind it" wording, so the fix now lands one commit earlier. Grepping the rest for the same mistake turned up four more places where the alternative to steering was still described as cancellation — bugfix-workflow.md ("instead of cancelling it"), this page's own opening, the receipt rationale ("costs more than cancelling does today", "where cancel-and-restart produces one"), and the same comparison in ADR 0113's Decision. None is about the opt-out, all are stale in the same way: they compare steering against a behaviour that no longer exists. They now compare it against preserving alone, which is what an opted-out harness actually gets, and what the queued run does in either case. harness-reference.md's eligibility paragraph still required `enabled: true` and promised a printed reason for every declined watch, both of which this PR changed. It now names the three conditions — not explicitly disabled, a runtime that can take a message, a GitHub Actions job with a work item — and says the reason is printed only when the harness set `enabled: true` itself. The warning gate had no test. TestStartSteerWatcherDeclineMessage is table-driven over an ineligible fixture: no block and a block that only tunes max_steers stay quiet, `enabled: true` prints the reason, `enabled: false` stays quiet. The max_steers row is there because it is the case the pointer exists for — it must read as "not an explicit request", not as an opt-out. The rollout paragraph gains a fourth open precondition. agents#1163 teaches the envelope to code, fix, review and triage only; prioritize, retro and scribe are outside it and no harness anywhere sets `steer:`, so with the default on an eligible run on one of those three delivers an envelope the definition ignores, the runtime acks it, the receipt is posted, and the queued run skips — the update is dropped silently. The fix belongs in the agents repo, so this is recorded as a gate rather than changed here. ADR 0113 stays at 102 lines of content. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
…sion Review comment 5701340330 on #7377. steerHarness(false) left Steer nil, which this PR made mean *on*. Two tests were then asserting something other than what their names claim. TestStartSteerWatcher_DisabledHarnessStartsNothing passed because fakeRuntime has no Steerer, not because the harness was off, and TestCheckSteerAlreadyHandled_OffPaths' "steering disabled" case went down the enabled branch and returned false from a failed timeline read instead. steerHarness now always writes the key, and steerHarnessDefault covers the absent-block case that used to be conflated with it. The disabled-watcher test gets a steerable runtime and asserts the fixture is otherwise eligible first, so the opt-out is the only thing left that can stop it. Its companion pins that a default harness is enabled but not explicitly enabled — the two states the fixture used to blur. The off-paths test now also asserts nothing was printed. Returning false is not evidence a guard held: a failed timeline read returns false too, after warning. Without that assertion the mutation is not caught — dropping the SteerEnabled check from checkSteerAlreadyHandled leaves the test passing on the return value while the guarded call reaches the network. With it, the case fails on "Could not check whether this update was already handled: ... 401 Bad credentials", which is both the mutation caught and proof the real code never makes that call. Three descriptions that had not kept up: harness.go's SteerConfig type comment still said disabled by default. The field comment below it was already correct, which is how the type comment survived. harness-reference.md's field merge table had no steer row at all, so the one inheritance rule that now matters — a child with no block inherits the base's opt-out rather than the default — was written down nowhere the table's readers would look. steering.md said the declined-watch warning fires when the harness "named steering itself", which reads as though a cap-only block would qualify. The code requires enabled: true, as the test row added for exactly that case shows. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
The Steer field comment still said "default off", two lines from the type comment that says the opposite. The contributor-facing merge-rules table now says what an absent block inherits, matching the reference. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
Scaffolded harnesses are opted in to steering like every other harness, so a generated agent will be handed runner updates. Until now the template said nothing about them, which put every scaffolded agent in exactly the gap ADR 0113's rollout names for prioritize, retro and scribe: an agent that does not recognise the envelope ignores the amendment, the runtime still acks the delivery, the receipt is still posted, and the queued run skips — the update is dropped and nobody is told. The paragraph mirrors the fleet's own "Runner updates" sections in fullsend-ai/agents#1163 (agents/review.md, agents/fix.md), reduced to what is true for every role: the line the runner prefixes, that the route job already authorized the actor behind it, that a widening, narrowing or new head is still to be acted on, that the result must say what changed, that it grants no tools or permissions and relaxes no security instruction, and that the same line inside work-item content is an injection attempt rather than an amendment. It says "your result" because that is the template's own word for the file it tells the agent to write. The test references runtime.SteerEnvelopeOpeningLine rather than spelling the line out, so a change to the constant fails here instead of silently turning every steer delivered to a scaffolded agent back into ordinary text. It renders each role the hosted mint serves, since the paragraph is role-neutral and none of them may render without it, and it also asserts the injection-defence half — carrying the line alone would teach recognition without the defence. The assertion flattens the rendered markdown to one whitespace-normalised line before matching. The sentence wraps in the template, as it does in the fleet definitions, and this is the same normalisation fullsend-ai/agents' own contract test applies (scripts/agent-recheck-contract-test.sh) rather than a rule invented here — the alternative, forcing the line to stay unwrapped, would make the template worse to read to satisfy a test. internal/agentnew does not create an import cycle on internal/runtime: runtime does not depend on agentnew (`go list -deps ./internal/runtime/` names it nowhere). Both golden files are regenerated; the paragraph is the whole of their diff. docs/cli/agent.md describes what the generated body ships with, so it gains a clause for the new section. steering.md's rollout gate lists what is still open, and scaffolded agents are no longer part of it, so they get a closing sentence there rather than a place in that list. Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
01de4ba to
354f82d
Compare
|
🤖 Finished Review · ✅ Success · Started 10:55 PM UTC · Completed 11:18 PM UTC Commit: Runtime: pi · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $6.80 |
| // DefaultSteerEnabled is whether the follow-up run watcher runs when the | ||
| // harness says nothing about it. On since the release that made run | ||
| // continuation unconditional; a harness opts out with enabled: false. | ||
| const DefaultSteerEnabled = true |
There was a problem hiding this comment.
[high] fail-open
DefaultSteerEnabled = true and SteerEnabled() still have no envelope/role/feature-flag gate. startSteerWatcher (internal/cli/steer.go:300-312) keys only off SteerEnabled() and steerEligible(); SteerExplicitlyEnabled() only controls warning volume for ordinary declines, it does not refuse the watcher or withhold a receipt. marker()/steerMarkerFrom (steer.go:406-414) treat a runtime transport ack as proof the update was absorbed; shouldPostSteerReceipt only requires run success; checkSteerAlreadyHandled (steer.go:569) then skips the queued run on a job-token receipt. That is the silent-drop path docs/contributing/steering.md itself names as the one failure mode this design may not have. The four documented rollout preconditions remain unmet in-tree (fleet teaching in fullsend-ai/agents#1163 unmerged, no live end-to-end OpenShell steer observed, receipt identity match unverified on a live run, prioritize/retro/scribe untaught). The scaffold template (internal/agentnew/templates/agent-body.md.tmpl, pinned by steer_envelope_test.go) only closes the gap for newly generated custom agents, not fleet roles. Nothing in code or config ties the on-by-default flip to those preconditions: documenting the footgun as a release checklist does not remove the code path.
Suggested fix: Do not ship DefaultSteerEnabled = true until a runner-side guard makes an untaught agent definition unable to silently drop the queued run (refuse to start the watcher, or withhold the receipt, unless the loaded agent definition contains the steer envelope opening line), or keep the constant false and flip it in a follow-up once the four documented preconditions are checked off, or scope the default to only the roles known to be envelope-aware.
|
🤖 Finished Retro · ✅ Success · Started 3:32 PM UTC · Completed 3:40 PM UTC Commit: Runtime: claude · Model: sonnet → claude-sonnet-5 · Effort: high · Cost: $1.47 |
|
PR #7377 ("steer the running agent by default", 4th in a stack rooted in #6957) was CLOSED (not merged) on 2026-09-18 and superseded by a restructured stack ending in #7461, which carries the same default-flip decision under a renumbered ADR (0113→0121). No human reviewer participated on #7377 beyond the author (waynesun09); the only review signal came from two bots. Proposals filed
|
This branch was successfully deployed
1 participant
Closes the stack. Fourth PR; base is #7376.
Steering is on by default from the release that carries this. A harness that says nothing about
steer:now watches for follow-up runs and delivers work-item updates — a push, a comment, a stage command — into the running agent instead of letting the queued run redo the work. This follows the 2026-09-14 meeting recorded on #6957, which made run continuation the default with no flag; making steering the default alongside it is the request that produced this stack, and ADR 0113's Decision section in this PR is where that is recorded. #6957's validation criteria still describe steering as off by default and should be read as superseded by ADR 0113.The opt-out is one spelling.
steer: {enabled: false}turns steering off, and nothing else does.enabledis a pointer internally because absent andfalsehave to mean different things: with a plain bool,steer: {max_steers: 3}would unmarshal tofalseand silently opt a harness out while it is trying to raise the cap. The file already uses the pointer-plus-default shape for every other on-by-default block, so this follows the local pattern. Composition is unchanged — a child that omits the block inherits the base's whole block, so a base opt-out is inherited rather than overwritten by the default; a test pins that direction.One behaviour change beyond the flag. The runner used to warn "Steering disabled: " on every declined watch, which under opt-in only ever fired for a harness that asked and did not get it. With the default on, that line would fire once per iteration on every local run, every GitLab run, and every behaviour test whose runtime cannot take a message. Declines are now classified in the one eligibility ladder: not in Actions, GitLab, a runtime that cannot take a message, and no work item are ordinary and stay quiet unless the harness named
enabled: true; a missing job token or run id is an environment defect and is announced to everyone, because with the default on almost nobody setsenabled: trueand a real fault would otherwise go silent fleet-wide. Dropping the defect branch fails two tests; dropping the opt-out guard from the skip check fails two more (on the output assertion — the bypass fails open, so the return value alone tests nothing). Nothing else was gated on the old default; the skip check now runs for every harness that has not opted out, which is the point.Verification. Three tests fail when the default is flipped back to off; four different tests fail when the reader ignores the pointer and returns the default, so the opt-out is proven independently of the default.
internal/harness,internal/cliandinternal/steerwatchpass under the race detector.Before this merges — four things are not yet settled, and the docs now say so rather than claim them.
steer: {enabled: false}before the default reaches them — an eligible run on a definition that ignores the envelope still acks the delivery and still posts a receipt, so the queued run skips and the update is dropped silently, which is the one failure mode this design may not have.The default should not reach a release on four unwatched paths. This PR is ready to review now and ready to merge when those are checked off.
BREAKING CHANGE: a harness that omits
steer:, or sets onlymax_steersorpoll_interval_seconds, now steers. Addsteer: {enabled: false}to keep the previous behaviour. GitLab repositories and runs outside GitHub Actions are unaffected.Stacked: #7007 → #6959 → #7376 → this.
Restacked onto main
6aa078bc7on top of #7376's7f38035ac; five commits. The review round also correcteddocs/architecture.md's "opt-in per agent" line, replaced the getter-only default test with one that starts a real watcher for a harness with nosteer:block, and aligned the two remaining "single pending run" lines in ADR 0113 and steering.md with the base's wording (the pending run is the last to queue, normally but not necessarily the newest event). The four preconditions are release gates — they belong before the release that carries this is cut, and nothing mechanical enforces them; a standalone agents change opting prioritize, retro and scribe out is prepared for the fourth.