Skip to content

fix(pr_runtime): Check the require_linked_issue filter - #185

Open
philip-paul-mueller wants to merge 5 commits into
huggingface:mainfrom
philip-paul-mueller:fix/pr-runtime-require-linked-issue
Open

philip-paul-mueller wants to merge 5 commits into
huggingface:mainfrom
philip-paul-mueller:fix/pr-runtime-require-linked-issue

Conversation

@philip-paul-mueller

Copy link
Copy Markdown

PRRuntimeOptions, see src/repo2rlenv/spec/options.py:47, defines the flag require_linked_issue to filter out PRs that do not have an associated issue. However, the filter never actually checked that flag.
This PR now adds the a check to the filter, thus now only PRs with a linked issue are considered, matching SWE-bench's approach of sourcing problem statements from issue text.

Note that this PR changes the previous behaviour, because now a linked issue is required by default, while before it was not needed.

The option was defined in PRRuntimeOptions (spec/options.py:47) but
never used in the pipeline. PRs without linked issues fall through to
_build_instruction, which uses the PR body (the fix description) as
the problem statement. The PR body routinely leaks the solution
approach, commit SHAs, and test names — see the comment at
pr_runtime.py:276-278. Filtering them out ensures only PRs with
linked issues (bug reports) are emitted, matching SWE-bench's
approach of sourcing problem statements from issue text.
@philip-paul-mueller

Copy link
Copy Markdown
Author

Maybe the default of PRRuntimeOptions.require_linked_issue should be changed to False to preserve previous behaviour?

Add three unit tests exercising the require_linked_issue option wired
up in 21af74e:

- PR without a linked issue is skipped with 'no_linked_issue' when the
  filter is on (default)
- PR with 'Closes huggingface#123' is kept when the filter is on
- PR without a linked issue is kept when the filter is explicitly off

Uses a small _make_pr / _make_pipeline pair to construct the
PRRuntimePipeline with a stub BootstrapResult, mirroring the pattern
in test_pr_runtime_rejects_missing_bootstrap.
@adithya-s-k

Copy link
Copy Markdown
Collaborator

The filter fix passes locally (59 tests). I agree with your suggestion: default require_linked_issue to False to preserve current behavior, enforce it when explicitly enabled, and update the docs that currently say it isn't enforced. Please add a default-behavior regression too; happy to merge after that and CI.

@philip-paul-mueller

Copy link
Copy Markdown
Author

@adithya-s-k Thanks for your feedback, "I" have added them, let me know if something has to change.

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants