feat(#947): adopt feedback_mode: append for code and fix harnesses - #948
Conversation
|
🤖 Review · Commit: |
e2d38f5 to
ea87ae1
Compare
|
🤖 Finished Review · ✅ Success · Started 10:11 PM UTC · Completed 10:28 PM UTC Commit: |
ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Previous run (2)ReviewFindingsMedium
Low
Previous run (3)ReviewFindingsMedium
Low
Previous run (4)ReviewFindingsMedium
Low
|
![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
the
requires-manual-review
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
All checks green (13 pass) after re-triggering functional-tests with ok-to-test — the earlier functional-tests-complete failure was the roll-up defect filed as #954, where a label added by the review agent re-fires the workflow, the gate skips by design, and the roll-up reads that skip as "unauthorized". No code involvement.
Local validation on ea87ae1: YAML parsed to confirm feedback_mode: append nests inside validation_loop in both harnesses; step references (9 implement-and-verify, 10 commit, 11 validate output) checked against the file; test-merged against #946 with no conflict; pre-commit clean including lint agent doc references and structure.
Noting for the record that I amended and force-pushed this branch: the sentinel-based retry detection, the deferral of branch handling to step 4, and the "Do not skip steps" qualification are my changes, not the coder agent's. This approval is therefore not an independent review of that portion.
ea87ae1 to
026b4b8
Compare
|
🤖 Review · Commit: |
026b4b8 to
457f82f
Compare
|
🤖 Review · Commit: |
457f82f to
e841266
Compare
|
🤖 Review · Commit: |
e841266 to
024256d
Compare
|
🤖 Finished Review · ✅ Success · Started 11:10 PM UTC · Completed 11:29 PM UTC Commit: |
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.
024256d to
9e613c9
Compare
|
🤖 Finished Review · ✅ Success · Started 11:33 PM UTC · Completed 12:14 AM UTC Commit: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
9e613c9 to
362787a
Compare
|
🤖 Finished Review · ✅ Success · Started 12:18 AM UTC · Completed 12:59 AM UTC Commit: |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Enable validation_loop feedback so retry iterations receive
the previous iteration's failure diagnostics instead of a
blind re-roll. The runner (fullsend#1050) replaces the
constant prompt with the default prompt plus the redacted,
truncated (10 KiB) failure text.
Changes:
- harness/code.yaml: add feedback_mode: append
- harness/fix.yaml: add feedback_mode: append
- agents/code.md: add "Retry behavior" section describing
how the agent should handle retry prompts
- skills/code-implementation/SKILL.md: add "Retry-prompt
handling" section before step 1 with concrete procedure
for fixing validation failures without restarting
Design decisions:
- max_iterations stays at 2 for both harnesses. Informed
retries increase value per iteration but each still costs
up to 35 minutes; revisit after observing improvement.
- Remaining harnesses (retro, triage, review, scribe,
prioritize) not opted in yet. Retro is a strong candidate
(fullsend#1050 motivating case) but the issue scopes this
to code and fix first.
Retry detection matches the runner's literal sentinel ("The previous
iteration's output failed validation. Here is the validation error:")
rather than a description of the text's shape, so the agent can tell a
retry from a first iteration deterministically.
The retry path defers branch handling to step 4 instead of re-deriving
it. A retry runs in the same sandbox with the branch already checked
out and the previous commits still on it, so there is nothing to check
out; and where recovery is genuinely needed, step 4 already scopes the
search to this issue's number and explains why local refs must be used
rather than origin/ ones. It also states there is no feedback file to
read — the runner writes validation-feedback.txt on the runner for the
audit trail and never uploads it into the sandbox.
The "Do not skip steps" line above the process now names the retry path
as its one exception, which the new section would otherwise contradict.
Review follow-ups: agents/fix.md gains a "Validation retry behavior"
section, since enabling feedback_mode on the fix harness without telling
the fix agent what a retry prompt looks like left the two agents
asymmetric. It is kept distinct from the existing FIX_ITERATION section,
which counts runs of the review->fix loop rather than retries within one
run. The code-implementation retry step also now says to take the issue
number from ISSUE_NUMBER when deferring to step 4, since the retry path
skips step 1 where the issue is normally identified.
Also fix the functional-tests roll-up so a label change stops repainting
an already-green required check red.
`labeled` is in this workflow's pull_request_target trigger list solely so
ok-to-test can authorize a run. For any other label the gate job's own
`if:` skips, detect skips with it, and functional-tests-complete read that
as "tests were not authorized" and failed — so the review agent adding
ready-for-review to a PR whose tests had just passed turned a required
check red with no code change. That is what happened on this PR.
Such a run carries no verdict about the code, but it cannot stay silent
either: GitHub supersedes the previous check run of the same name with
whatever this one reports. Neither constant answer is safe. Exiting 0
would launder a genuinely failing run green the moment anyone labelled
the PR. Guarding the job with the gate's own `if:` was tried and has the
same hole one layer up — a skipped job still publishes a check run that
supersedes, and `skipped` satisfies a required check.
So the roll-up now mirrors the verdict the previous run reached on this
same commit, and fails closed when there is no previous verdict to
mirror. Green stays green, red stays red, and a first label before any
test run is an error rather than a free pass. The lookup is a read-only
check-runs query excluding this run's own check, so the fix stays inside
the base-branch-checkout boundary the job already documents; the job
gains `checks: read` for it.
Five cases added to check-rollup-result-test.sh, including that a
previous failure and a previous cancellation are not laundered, and that
ok-to-test still falls through to the normal rules (19 tests, 0 failures).
This rides along here because the coder app intentionally has no
`workflows` write permission, so no agent can fix a workflow file.
Review round two (Grok, read-only) found five live defects in the retry
guidance itself, all fixed here:
- The retry path skipped step 1, where AGENT_START is captured. At step 9
the time checks would then subtract an unset variable from the clock,
conclude the budget was exhausted, and skip pre-commit and gitlint on
the one iteration that most needs them. The retry path now starts its
own clock first.
- The runner clears the output directory between iterations, so the
previous agent-result.json is gone. Nothing said to write it again, so
a retry could fix the reported problem and then fail validation for a
second, different reason. Now stated in both the skill and agents/fix.md,
and in step 4's existing-PR branch which assumed step 3 had written it.
- The sentinel was wrapped across two blockquote lines while the prose
called it an "exact sentence"; a literal match would miss it. Now one
line in all three files.
- Retry sub-steps were numbered 1-4, colliding with process steps 1-11.
Relettered R1-R6.
- The "cannot tell what failed, fall through to step 1" escape hatch
undid the anti-duplication guarantee. It now points at step 4's
treat-existing-work-as-yours path rather than a clean restart, and a
JSON-only fix with nothing to commit is called out as a correct
outcome rather than a reason to manufacture a code change.
agents/fix.md gains the same clock and output-file notes, and says the
fix-review skill's "follow these steps in order" describes a first
iteration.
Review round three tightened the roll-up query and its logging:
- The check-runs lookup now filters server-side with check_name, so only
this one check's history is paged over. A commit can easily exceed 100
check runs in total, which would have pushed a prior success off the
first page and failed closed against a genuinely green PR.
- LABEL_NAME reaches ::notice::/::error:: lines and is settable by anyone
with triage access, so the workflow-command delimiter and newlines are
stripped first.
Review round four:
- The ok-to-test test never entered the mirror block — it went through
run_rollup, so it passed on the pre-existing "detect skipped" rule and
was a functional duplicate. It now drives run_rollup_labeled with a
stubbed prior success, asserting that ok-to-test declines to mirror and
falls through to the normal rules.
- Label sanitisation stripped literal newlines but not %0A/%0D, which the
workflow-command parser decodes as line breaks; %25 could rebuild them.
Every percent sign is now neutralised too.
- agents/code.md's heading becomes "Validation retry behavior", matching
agents/fix.md — the more precise of the two, since it distinguishes the
within-run retry from cross-run iteration counting.
- Blank line between retry steps R3 and R4.
Review round five: drop ESC from the label sanitiser so a label cannot
smuggle ANSI into a log line, and add the missing blank line between
retry steps R4 and R5.
Closes #947
Closes #954
Assisted-by: Claude (review, fix)
Signed-off-by: Wayne Sun <gsun@redhat.com>
362787a to
63422b4
Compare
|
🤖 Finished Review · ✅ Success · Started 1:02 AM UTC · Completed 1:19 AM UTC Commit: |
There was a problem hiding this comment.
Note: The following review comments could not be posted on the diff (GitHub returned 422) and are included here instead:
.github/scripts/check-rollup-result.sh(file-level): Line 1 · [medium] scope-bundling
This PR bundles work for two independent issues: #947 (adopt feedback_mode: append) and #954 (fix roll-up check on label events). The CI/workflow changes are entirely for #954 and share no code or logical dependency with the #947 feedback_mode work. The PR title references only #947, obscuring the #954 scope.
Suggested fix: Split the #954 roll-up fix into its own PR, or at minimum update the PR title to reflect both issues.
|
🤖 Finished Retro · ✅ Success · Started 1:24 AM UTC · Completed 1:38 AM UTC Commit: |
Retro: PR #948 — adopt
|
1 participant
Summary
feedback_mode: appendtovalidation_loopinharness/code.yamlandharness/fix.yaml, enabling the runner (Implement validation loop feedback so agents can self-correct fullsend#1050) to pass previous-iteration validation failure text to the agent on retryagents/code.md("Retry behavior" section) andskills/code-implementation/SKILL.md("Retry-prompt handling" section before step 1), teaching the agent to fix the reported failure instead of restarting the implementation from scratchContext
The upstream runner support for
feedback_modelanded in fullsend-ai/fullsend#1050. Without this harness-side opt-in, every retry iteration was byte-identical to iteration 1 — the agent received the same constant prompt and re-rolled blindly, often reproducing the exact same failure (fullsend-ai/fullsend#5009).With
feedback_mode: append, the runner replaces the constant prompt with the default prompt plus the previous iteration's validation failure text (redacted, truncated to 10 KiB). The agent can now read the diagnostics and fix what actually failed.Design decisions
max_iterationsstays at 2 for both harnesses. Each iteration costs up to 35 minutes; informed retries increase value per iteration but don't reduce the cost. Can be revisited after observing improvement.Testing
Closes #947
Post-script verification
agent/947-adopt-feedback-mode)d021f80aea083fc3921b662b0b8534ff2433d09a..HEAD)Also fixes the roll-up defect (#954)
While validating this PR,
functional-tests-complete— a required status check in this repo's ruleset — went red with no code change. Sequence: functional tests passed; the review agent then addedready-for-review; thatlabeledevent re-fired the workflow; the gate job skipped by design (labeledis a trigger only sook-to-testcan authorize);check-rollup-result.shread the resulting skip as "tests were not authorized" and failed the roll-up. GitHub reports the latest run per check name, so the green result was overwritten.Because the review agent labels every PR it reviews, this hits every PR here.
The roll-up job now carries the same label carve-out as the gate, phrased so
push,merge_group,workflow_dispatchandworkflow_callare unaffected.Why skip rather than exit 0. Exiting 0 would report success on a run that carries no verdict, which would let a label added after a genuinely failing run silently turn the check green. Skipping fails safe in the other direction. The residual question is how the ruleset treats a
skippedconclusion on a required check — if it does not satisfy, the symptom is a PR sitting blocked rather than one falsely green, which is the right way round for a security-relevant gate.This rides along in this PR because the coder app intentionally has no
workflowswrite permission, so no agent can fix a workflow file.Closes #954
Decision on the remaining harnesses and max_iterations (#947 criterion 3)
Recorded here as well as in the commit body, since the criterion asks for a decision rather than a change.
max_iterationsstays at 2 for bothcodeandfix. An informed retry raises the value of each iteration, but each still costs up to 35 minutes forcode; raising it to 3 is worth revisiting once there is evidence that informed retries actually converge, not before.The remaining five harnesses do not opt in here, and the decision is deliberately split by evidence rather than taken in bulk:
retroopts in next — tracked in Adopt feedback_mode: append for the retro agent — closes fullsend#1050's unmet validation criterion #952. It is the agent whose failure motivated Implement validation loop feedback so agents can self-correct fullsend#1050 in the first place (ten consecutive runs failing schema validation twice each, hallucinating different wrong fields on the retry), and it is the only agent with a measurable baseline. That issue also closes Implement validation loop feedback so agents can self-correct fullsend#1050's validation criterion 3, which cannot currently be measured because retro has nofeedback_mode.review,prioritize,triage,scribefollow after that — tracked in Adopt feedback_mode: append for review, prioritize, triage and scribe #953, deliberately sequenced after Adopt feedback_mode: append for the retro agent — closes fullsend#1050's unmet validation criterion #952 so retro's measurement actually happens. Rolling all seven out at once would forfeit the only clean before/after signal available.