fix(#5074): add preflight dependency check for validation_loop scripts - #5192
Conversation
E2E tests are runningAuthorization passed for this commit. See the E2E Tests workflow for results. |
Site previewPreview: https://6d579d6a-site.fullsend-ai.workers.dev Commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 7:30 AM UTC · Completed 7:47 AM UTC |
ReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Previous runReviewFindingsMedium
Low
Labels: PR modifies harness schema (ValidationLoop struct) and CLI runner (preflight check execution) Previous runReviewFindingsMedium
Low
Previous run (2)ReviewFindingsMedium
Low
Labels: PR modifies harness schema (ValidationLoop struct) and CLI runner (preflight check execution) |
![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
requires-manual-review
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
Ran an independent multi-pass review focused on the new preflight_check mechanism. Three prior comments below were revised in place with severity changes based on further evidence (the test-coverage gap corresponds to a currently-failing codecov/patch check, not just a style note; the context-cancellation gap deviates from this repo's own established timeout conventions in tokenscope.go/preflight_github.go; the pre/post-script scope gap is explicit in issue #5074's stated "Expected Behavior"). Four new findings: a composition bug that can silently drop preflight_check when a harness overrides validation_loop, a missing ${VAR}/${FULLSEND_DIR} expansion pass, a missing RunnerEnv merge, and a discarded exec error where a reusable helper already exists. The previously flagged documentation gaps and the sh -c/capitalization notes stand as already assessed — no change there.
waynesun09
left a comment
waynesun09
left a comment
There was a problem hiding this comment.
One additional finding below couldn't be anchored as an inline comment because internal/harness/compose.go isn't part of this PR's diff (no file/line in it to attach a review comment to).
[MEDIUM] internal/harness/compose.go:601 — preflight_check skips the allowlist/audit pipeline and character validation every sibling executable harness field goes through
ValidationLoop's other executable-content fields, Script and Schema, are file-path references: when inherited through base composition they're routed through resolveBaseScripts (compose.go:601-620 for base-level fields, :649-670 for forge-level), which enforces validateBaseRelPath (rejects null bytes, traversal, absolute paths, embedded URLs) and fetchBaseFile (checks the org's allowed_remote_resources allowlist, content-addresses/caches the file, and writes an auditBaseFetch log entry). PreflightCheck (harness.go:211, a raw shell-command string, not a path) never goes through compose.go at all — it only travels as part of the whole-struct copy child.ValidationLoop = base.ValidationLoop (compose.go:541-542 and 1243-1245), with no per-field check.
Separately, harness.go's Validate()/validateSecurity() apply regex character-class validation to Role, Slug, Agent (basename), Model, and Providers (harness.go:390-423), but PreflightCheck — the one field in this struct directly interpolated into sh -c "..." (run.go:677) and run unsandboxed on the host with os.Environ() before openshell/sandbox setup — gets none of that scrutiny.
This is a real, verified inconsistency, though downgraded from an initial HIGH assessment: it isn't a new class of risk (pre_script/post_script already execute host-side with full env access today, and a remote base: URL's overall content is still hash-pinned and allowlist-checked as a whole file via fetchBaseURL, so it isn't literally unauthenticated). It is, however, a genuine defense-in-depth gap relative to this struct's own established pattern — and it's exactly the mechanism the companion scaffold/agents-repo fix (see the inline finding on triage.yaml) would exercise once preflight_check entries are added to fullsend-ai/agents' harness YAMLs, which many downstream org configs consume via this same base: composition path.
Suggestion: Bring PreflightCheck into the same trust model as Script/Schema: either require it to reference a script file (subject to validateBaseRelPath + fetchBaseFile + auditBaseFetch), or, if an inline command must stay supported, route it through resolveBaseScripts (or an equivalent path) for allowlist participation, plus add basic content/length validation in Validate()/validateSecurity(), consistent with how Role/Slug/Agent/Model/Providers are already treated.
waynesun09
added
the
ok-to-test
|
/fs-fix Please address the following unresolved review findings from this PR's review threads (verbatim below, most severe first). Two multi-pass reviews (bot + human) converged on these independently. HIGH —
|
|
🤖 Finished Fix · ✅ Success · Started 11:47 PM UTC · Completed 11:59 PM UTC |
- Carry forward PreflightCheck during composition when a child harness overrides validation_loop without setting its own preflight_check, preventing silent loss of inherited preflight checks (compose.go, both mergeBaseIntoChild and mergeForgeConfigInto merge sites) - Use exec.CommandContext with 30s timeout (mirroring preflightGitHubTimeout pattern) so preflight checks respect ctx cancellation and cannot hang indefinitely; surface distinct "timed out" error message (run.go) - Rewrite preflight tests to call runAgent directly via the established useFakeOpenshell fixture, covering StepStart/StepFail/StepDone messaging and error-wrapping logic; add timeout test case (run_test.go) - Add composition tests for PreflightCheck carry-forward and child override semantics (compose_test.go) Addresses review feedback on #5192
🔧 Fix agent — iteration 1 (human-triggered)Addressed all 3 review findings: (1) fixed silent PreflightCheck loss during composition by carrying the field forward at both merge sites, (2) added context-awareness and 30s timeout to preflight command execution, (3) rewrote all preflight tests to exercise actual runAgent control flow and added a timeout test. All changes include corresponding test coverage. Fixed (3):
Tests: passed Decision points
Updated by fullsend fix agent |
|
/fs-review |
|
🤖 Finished Review · ✅ Success · Started 12:30 AM UTC · Completed 12:48 AM UTC |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
These harnesses will have no effect, open a followup issue on fullsend-ai/agents to introduce that pre-check. |
|
Agreed — removed the scaffold edits in 456a40d and filed fullsend-ai/agents#422 for the actual fix (adding |
|
🤖 Finished Retro · ✅ Success · Started 3:49 PM UTC · Completed 4:09 PM UTC |
Retro: PR #5192 — preflight dependency check for validation_loopTimeline: Code agent created PR on 2026-07-16. Review agent ran 2026-07-22 (3 findings). Human reviewer waynesun09 conducted independent multi-pass review escalating 7 findings (2 HIGH, 5 MEDIUM). Fix agent ran 2026-07-23, addressed 3/10 findings (both HIGHs + 1 MEDIUM test-adequacy rewrite). Human committed remaining fixes 2026-07-24: Fix agent effectivenessThe fix agent was given 10 explicit findings in the Review agent vs human reviewer
The meta-pattern across 4 of 6 gaps: the review agent analyzed each code site in isolation rather than asking "what pipeline does the sibling field go through, and does this new field go through the same pipeline?" Evidence for existing issues
Autonomy assessmentThis PR does not support relaxing human review requirements for harness schema + CLI runner changes. The 55% gap rate on MEDIUM+ findings, including correctness and security gaps, demonstrates the review agent cannot yet substitute for human review on cross-cutting feature PRs touching these paths. The agent's self-escalation capability (context/timeout finding) is a genuine strength but insufficient alone. Proposals filed
|
The dispatch "Check for existing PRs" guard used --search "N in:title,body", which matches any PR that mentions the issue number anywhere in its body — including unrelated cross-references — and skipped code dispatch whenever one existed. Its bot-authored-PR exclusion also never worked: GraphQL's Bot.login omits the REST "[bot]" suffix, so the literal string comparison against "fullsend-ai-coder[bot]" never matched. Replace both with a GraphQL query for closedByPullRequestsReferences, which only returns PRs that actually close the issue via Fixes/Closes/ Resolves keywords, and match the bot exclusion on __typename == "Bot" instead of a REST-formatted login string. The query also returns MERGED PRs regardless of includeClosedPrs, so filter on .state == "OPEN" explicitly — otherwise a long-merged closer would permanently block re-dispatch on a reopened issue. Add issues: read to both jobs' permissions, since the query now resolves through the Issue type rather than pull-requests-only fields. Separate stderr from the query's stdout so an incidental warning on an otherwise-successful call can't pollute the result and trigger a false-positive skip. internal/scaffold/fullsend-repo/scripts/pre-code.sh had the identical bug (same substring search, same broken bot-login comparison) and runs as a second gate in the same code-dispatch pipeline, so the original false-positive-skip failure mode could still recur through that path. Apply the same fix there and correct pre-code-test.sh's mock fixtures, which hardcoded the REST-suffixed bot login format and so validated the wrong assumption about what the API actually returns. Verified against production data: #5569 (false positive: PR #5192 only mentions the issue, now correctly proceeds), #1320 (historical: closed by now-merged PR #2373, now correctly proceeds since the closer is no longer open), #5560 (bot-authored closer, correctly excluded), and #5575 itself (open, human-authored PR #5578 with "Fixes #5575" in its body, correctly detected as blocking). Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
The dispatch "Check for existing PRs" guard used --search "N in:title,body", which matches any PR that mentions the issue number anywhere in its body — including unrelated cross-references — and skipped code dispatch whenever one existed. Its bot-authored-PR exclusion also never worked: GraphQL's Bot.login omits the REST "[bot]" suffix, so the literal string comparison against "fullsend-ai-coder[bot]" never matched. Replace both with a GraphQL query for closedByPullRequestsReferences, which only returns PRs that actually close the issue via Fixes/Closes/ Resolves keywords, and match the bot exclusion on __typename == "Bot" instead of a REST-formatted login string. The query also returns MERGED PRs regardless of includeClosedPrs, so filter on .state == "OPEN" explicitly — otherwise a long-merged closer would permanently block re-dispatch on a reopened issue. Add issues: read to both jobs' permissions, since the query now resolves through the Issue type rather than pull-requests-only fields. Separate stderr from the query's stdout so an incidental warning on an otherwise-successful call can't pollute the result and trigger a false-positive skip. internal/scaffold/fullsend-repo/scripts/pre-code.sh had the identical bug (same substring search, same broken bot-login comparison) and runs as a second gate in the same code-dispatch pipeline, so the original false-positive-skip failure mode could still recur through that path. Apply the same fix there and correct pre-code-test.sh's mock fixtures, which hardcoded the REST-suffixed bot login format and so validated the wrong assumption about what the API actually returns. Verified against production data: #5569 (false positive: PR #5192 only mentions the issue, now correctly proceeds), proceeds since the closer is no longer open), #5560 (bot-authored closer, correctly excluded), and #5575 itself (open, human-authored PR Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
The dispatch "Check for existing PRs" guard used --search "N in:title,body", which matches any PR that mentions the issue number anywhere in its body — including unrelated cross-references — and skipped code dispatch whenever one existed. Its bot-authored-PR exclusion also never worked: GraphQL's Bot.login omits the REST "[bot]" suffix, so the literal string comparison against "fullsend-ai-coder[bot]" never matched. Replace both with a GraphQL query for closedByPullRequestsReferences, which only returns PRs that actually close the issue via Fixes/Closes/ Resolves keywords, and match the bot exclusion on __typename == "Bot" instead of a REST-formatted login string. The query also returns MERGED PRs regardless of includeClosedPrs, so filter on .state == "OPEN" explicitly — otherwise a long-merged closer would permanently block re-dispatch on a reopened issue. Add issues: read to both jobs' permissions, since the query now resolves through the Issue type rather than pull-requests-only fields. Separate stderr from the query's stdout so an incidental warning on an otherwise-successful call can't pollute the result and trigger a false-positive skip. internal/scaffold/fullsend-repo/scripts/pre-code.sh had the identical bug (same substring search, same broken bot-login comparison) and runs as a second gate in the same code-dispatch pipeline, so the original false-positive-skip failure mode could still recur through that path. Apply the same fix there and correct pre-code-test.sh's mock fixtures, which hardcoded the REST-suffixed bot login format and so validated the wrong assumption about what the API actually returns. Verified against production data: #5569 (false positive: PR #5192 only mentions the issue, now correctly proceeds), #1320 (historical: closed by now-merged PR #2373, correctly proceeds since the closer is no longer open), #5560 (bot-authored closer, correctly excluded), and #5575 itself (open, human-authored PR #5578 with a "Fixes" keyword, correctly detected as blocking). Also bump closedByPullRequestsReferences's first from 20 to 100 (the connection's API max) at all three call sites, since a long-lived, repeatedly-reopened issue could otherwise silently truncate past the 20th closing-PR reference; add null-safety around the nodes array and author login so a missing field degrades gracefully instead of erroring or printing "null"; sanitize captured stderr before interpolating it into a ::warning:: workflow command; fix a pre-code-test.sh case that mocked a query failure while asserting the "no linked PRs" behavior; and document a third bot-login format returned by gh's own --json output (app/<slug> with a separate is_bot flag). Assisted-by: Claude (fix, review), Grok (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
The dispatch "Check for existing PRs" guard used --search "N in:title,body", which matches any PR that mentions the issue number anywhere in its body — including unrelated cross-references — and skipped code dispatch whenever one existed. Its bot-authored-PR exclusion also never worked: GraphQL's Bot.login omits the REST "[bot]" suffix, so the literal string comparison against "fullsend-ai-coder[bot]" never matched. Replace both with a GraphQL query for closedByPullRequestsReferences, which only returns PRs that actually close the issue via Fixes/Closes/ Resolves keywords, and match the bot exclusion on __typename == "Bot" instead of a REST-formatted login string. The query also returns MERGED PRs regardless of includeClosedPrs, so filter on .state == "OPEN" explicitly — otherwise a long-merged closer would permanently block re-dispatch on a reopened issue. Add issues: read to both jobs' permissions, since the query now resolves through the Issue type rather than pull-requests-only fields. Separate stderr from the query's stdout so an incidental warning on an otherwise-successful call can't pollute the result and trigger a false-positive skip. internal/scaffold/fullsend-repo/scripts/pre-code.sh had the identical bug (same substring search, same broken bot-login comparison) and runs as a second gate in the same code-dispatch pipeline, so the original false-positive-skip failure mode could still recur through that path. Apply the same fix there and correct pre-code-test.sh's mock fixtures, which hardcoded the REST-suffixed bot login format and so validated the wrong assumption about what the API actually returns. Verified against production data: #5569 (false positive: PR #5192 only mentions the issue, now correctly proceeds), #1320 (historical: closed by now-merged PR #2373, correctly proceeds since the closer is no longer open), #5560 (bot-authored closer, correctly excluded), and #5575 itself (open, human-authored PR #5578 with a "Fixes" keyword, correctly detected as blocking). Also bump closedByPullRequestsReferences's first from 20 to 100 (the connection's API max) at all three call sites, since a long-lived, repeatedly-reopened issue could otherwise silently truncate past the 20th closing-PR reference; add null-safety around the nodes array and author login so a missing field degrades gracefully instead of erroring or printing "null"; sanitize captured stderr before interpolating it into a ::warning:: workflow command; fix a pre-code-test.sh case that mocked a query failure while asserting the "no linked PRs" behavior; and document a third bot-login format returned by gh's own --json output (app/<slug> with a separate is_bot flag). Assisted-by: Claude (fix, review), Grok (review) Signed-off-by: Wayne Sun <gsun@redhat.com>
The dispatch "Check for existing PRs" guard used --search "N in:title,body", which matches any PR that mentions the issue number anywhere in its body — including unrelated cross-references — and skipped code dispatch whenever one existed. Its bot-authored-PR exclusion also never worked: GraphQL's Bot.login omits the REST "[bot]" suffix, so the literal string comparison against "fullsend-ai-coder[bot]" never matched. Replace both with a GraphQL query for closedByPullRequestsReferences, which only returns PRs that actually close the issue via Fixes/Closes/ Resolves keywords, and match the bot exclusion on __typename == "Bot" instead of a REST-formatted login string. The query also returns MERGED PRs regardless of includeClosedPrs, so filter on .state == "OPEN" explicitly — otherwise a long-merged closer would permanently block re-dispatch on a reopened issue. Add issues: read to both jobs' permissions, since the query now resolves through the Issue type rather than pull-requests-only fields. Separate stderr from the query's stdout so an incidental warning on an otherwise-successful call can't pollute the result and trigger a false-positive skip. internal/scaffold/fullsend-repo/scripts/pre-code.sh had the identical bug (same substring search, same broken bot-login comparison) and runs as a second gate in the same code-dispatch pipeline, so the original false-positive-skip failure mode could still recur through that path. Apply the same fix there and correct pre-code-test.sh's mock fixtures, which hardcoded the REST-suffixed bot login format and so validated the wrong assumption about what the API actually returns. Verified against production data: #5569 (false positive: PR #5192 only mentions the issue, now correctly proceeds), proceeds since the closer is no longer open), #5560 (bot-authored closer, correctly excluded), and #5575 itself (open, human-authored PR Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
The dispatch "Check for existing PRs" guard used --search "N in:title,body", which matches any PR that mentions the issue number anywhere in its body — including unrelated cross-references — and skipped code dispatch whenever one existed. Its bot-authored-PR exclusion also never worked: GraphQL's Bot.login omits the REST "[bot]" suffix, so the literal string comparison against "fullsend-ai-coder[bot]" never matched. Replace both with a GraphQL query for closedByPullRequestsReferences, which only returns PRs that actually close the issue via Fixes/Closes/ Resolves keywords, and match the bot exclusion on __typename == "Bot" instead of a REST-formatted login string. The query also returns MERGED PRs regardless of includeClosedPrs, so filter on .state == "OPEN" explicitly — otherwise a long-merged closer would permanently block re-dispatch on a reopened issue. Add issues: read to both jobs' permissions, since the query now resolves through the Issue type rather than pull-requests-only fields. Separate stderr from the query's stdout so an incidental warning on an otherwise-successful call can't pollute the result and trigger a false-positive skip. internal/scaffold/fullsend-repo/scripts/pre-code.sh had the identical bug (same substring search, same broken bot-login comparison) and runs as a second gate in the same code-dispatch pipeline, so the original false-positive-skip failure mode could still recur through that path. Apply the same fix there and correct pre-code-test.sh's mock fixtures, which hardcoded the REST-suffixed bot login format and so validated the wrong assumption about what the API actually returns. Verified against production data: #5569 (false positive: PR #5192 only mentions the issue, now correctly proceeds), proceeds since the closer is no longer open), #5560 (bot-authored closer, correctly excluded), and #5575 itself (open, human-authored PR Assisted-by: Claude Signed-off-by: Wayne Sun <gsun@redhat.com>
2 participants
When a validation_loop declares a preflight_check command, the runner now executes it during the preflight phase — before sandbox creation. This catches missing host-side dependencies (e.g. python3-jsonschema) immediately instead of after the agent has already completed (~74s of wasted execution).
Scope: this covers
validation_loop.preflight_checkonly, notpre_script/post_script. Issue #5074's "Expected Behavior" section calls for all three; extending to pre_script/post_script is tracked separately in #5568 rather than done here, sincePreflightCheck's current nesting insideValidationLoopdoesn't naturally extend to the other two without a design decision on where it should live.Changes:
harness schema (preflight_check YAML key)
before openshell/sandbox setup, with the same
${VAR}expansionand
RunnerEnvmerging thatvalidation_loop.schemagetsbase composition inheritance of the new field
Not included: this PR does NOT update fullsend's own scaffold harness YAML files (triage, review, fix, prioritize, retro). An earlier revision did, but that has no effect for fullsend-ai's own agents — per
fullsend-ai/.fullsend'sconfig.yaml, those agents resolve from the pinnedfullsend-ai/agentscommit before ever falling back to this local scaffold. The actual fix for fullsend-ai's own production agents is tracked at fullsend-ai/agents#422 instead. This PR's Go-level schema/execution changes are generic infrastructure that any harness (including ones fromfullsend-ai/agents) can use once that field is set.Note: pre-commit could not run in this environment (network 403 on hook fetch). The post-script runs an authoritative pre-commit on the runner.
Also found during review (out of scope for this diff): the
internal/clitests that callrunAgent("code", fixtureDir)(includingTestRunAgent_PreflightCheck_*) aren't hermetic — with aGH_TOKEN/GITHUB_TOKENresolvable in the environment, agent resolution can silently fetch the live harness fromfullsend-ai/agentsinstead of using the test's local fixture. Pre-existing, not introduced by this PR (an untouched older test has the same exposure); CI only passes today becauselint.ymlhappens to blank those env vars. Tracked at #5569.Relates to #5074 (validation-loop leg only; see scope note above and #5568 for the rest)
Post-script verification
agent/5074-preflight-dep-check)83397e5355462dea996845ed2a1ec019d32f30ac..HEAD)