Skip to content

[ARCHIVED] Benchmark issue 359 #359

Description

@guyoron1

What happened

PR #2947 added tryAgentsRepoFallback in internal/cli/run.go (1495+ lines, 12 files changed), fundamentally altering agent resolution behavior. The functional-tests workflow (.github/workflows/functional-tests.yml) exercises this exact code path (triage eval cases go through runAgentresolveAgentSourcetryAgentsRepoFallback). However, the workflow's paths filter only included eval/**, internal/scaffold/**, .github/workflows/functional-tests.yml, and .github/scripts/** — not internal/cli/. So functional tests never ran on PR fullsend-ai#2947. The allowlist propagation bug shipped undetected and broke main within 18 hours. PR #3425 added internal/cli/run.go to the path filter as part of the fix. The human reviewer waynesun09 further recommended adding internal/harness/** as well, noting internal/cli/** would be too broad (triggering on ~18% of commits).

What could go better

When a PR modifies code that is exercised by path-filtered CI workflows, the review agent should check whether the modified files are in the workflow's path filter. If they aren't, the review agent should flag that relevant integration tests won't run. This is particularly important when a PR adds a new code path (like tryAgentsRepoFallback) that changes the behavior of code already covered by integration tests. The review agent reviewed PR fullsend-ai#2947 across 5+ completed runs and never flagged the path filter gap. Confidence: medium — detecting this requires the review agent to understand which CI workflows exercise which code paths, which may be complex. However, for cases where a PR adds a new function call to a file that is already part of a test pipeline's execution path, a simpler heuristic could work: 'If the PR modifies files under paths not in any CI workflow path filter, and those files contain functions called by code that IS in a filtered path, flag the gap.'

Proposed change

Add a review heuristic: when reviewing a PR that modifies Go files under internal/cli/ or internal/harness/, check whether .github/workflows/functional-tests.yml includes those paths in its trigger filter. If not, flag it as a potential CI coverage gap. More broadly, the review agent should cross-reference modified files against path-filtered CI workflow triggers and flag files that are exercised by tests but not in the filter. For this repo specifically, waynesun09 recommended adding internal/cli/run.go (done in PR fullsend-ai#3425) and internal/harness/** to the functional-tests paths filter — the internal/harness/** addition should still be made.

Validation criteria

On a future PR that modifies internal/cli/run.go or internal/harness/ files: (1) functional-tests workflow triggers on the PR (verifying the path filter is correct), (2) the review agent flags if a modified file is not in any relevant CI path filter. For the immediate repo fix: verify that internal/harness/** is added to the functional-tests path filter (per waynesun09's recommendation that hasn't been implemented yet).


Generated by retro agent from fullsend-ai#3425

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    duplicateThis issue or pull request already existspr-openAn open PR already addresses this issue

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions