Repository navigation
A test that runs agac runs the tree it lives in (1270) - #1321
Open
Muizzkolapo wants to merge 6 commits into
Open
Muizzkolapo wants to merge 6 commits into
Muizzkolapo wants to merge 6 commits into
Conversation
The integration tests that drive the CLI launch the venv's `agac` console script as a subprocess. That script imports `agent_actions` from wherever the editable install points, and pytest.ini's `pythonpath = .` reaches only the pytest process. From a second worktree the subprocess therefore runs the main checkout's code, and those tests pass or fail on it whatever the change under test. The new test drops a tool into a project and lets `agac list-udfs` import it through the launcher the batch CLI tests share. The tool records which `agent_actions` the CLI process imported. Run from a worktree with no PYTHONPATH set, it reports the main checkout's package.
The integration tests that drive the CLI as a subprocess launched the venv's `agac` console script themselves, nine times across six files. The script imports `agent_actions` from wherever the editable install points, and pytest.ini's `pythonpath = .` reaches only the pytest process. From a second worktree with no PYTHONPATH set, those tests ran the main checkout's code. With the main checkout 41 commits behind, all four tests in test_a_batch_action_keeps_no_row_its_guard_filters.py failed on a worktree that held the fix they pin, and passed once PYTHONPATH named the worktree. Every launch now goes through run_agac in tests/_support/agac_cli.py. It runs the console script with this tree first on PYTHONPATH, keeping any PYTHONPATH already set after it. The RED test now calls it directly. A second test fails on any test outside tests/manual that launches the console script some other way. Such a launch passes in CI, where the editable install is the tree under test, so nothing else would catch it. The issue's other suggestion, `python -m agent_actions.cli.main`, does not fix it alone. The tests run the CLI from a copy of the project, -m puts that directory on the path, and the editable install still wins. tests/integration/README.md says how to run agac from a test. The changelog entry is filed under the hood: no user of the package sees a difference.
Nothing pinned what run_agac does with a PYTHONPATH already set. The probe test inherited the shell's PYTHONPATH, which under the usual test command and in CI names the tree, so it passed with the fix reverted inside run_agac; putting the inherited entry first, or dropping it, passed everywhere. A PYTHONPATH a caller passed in env= was silently replaced by the tree rather than kept behind it. run_agac now reads PYTHONPATH from the caller's env merged over the process's, and keeps it behind the tree either way. The probe test clears PYTHONPATH so it measures run_agac rather than the shell. A new test, inherited and passed in env=, sets PYTHONPATH to a decoy whose `agent_actions` raises on import and whose own module the probe imports: the run fails unless the tree is first and the entry is kept. A PYTHONPATH entry outranks the editable install, so it fails in CI too. Reverting the fix, putting the entry first, dropping it, or pointing REPO one level too shallow each fails it with PYTHONPATH unset, naming the tree, or naming another checkout; the previous run_agac fails its env= case.
The audit matched one regex per line, for `agac` after `/`, `[` or
`which(` on the same line. The changelog, the integration README and the
previous commit said it failed on any test that launched `agac` some other
way, but `Path(sys.executable).with_name("agac")`, `python -m
agent_actions.cli.main`, a list ruff has wrapped so `"agac",` sits on its
own line, and a shell string all passed it while running the main
checkout. Nothing showed the pattern matched anything: a regex that never
matched passed too. Its tests/manual exemption skipped any directory named
manual anywhere under tests/.
The audit now parses each test file and flags a string that is exactly
`agac` or `agent_actions.cli.main`, wherever it sits, and a call whose
first argument is a command line, plain or f-string, starting with `agac`.
A parametrized test shows it finding each shape: a path segment, a sibling
name, a list element, a wrapped list, a which() lookup, -m, and a shell
string and f-string. The old regex misses five of the eight, and a
detector that finds nothing misses all of them. Only tests/manual itself
is exempt, beside the launcher and the audit, which names what it looks
for. Over today's tree the audit flags nothing.
The README and the changelog entry now say which shapes are caught and
that the launcher keeps an existing PYTHONPATH behind the tree.
The audit flagged a call whose first positional argument was a command line starting with `agac`, so `subprocess.run(args="agac run ...", shell=True)` passed it while running the main checkout, though the README and the changelog entry say a call handed such a command line fails it. The audit now reads the `args` keyword when a call has no positional argument, and the test that shows it finding each launch shape has a case for it, which fails without the change.
…othing The audit fails on any test that names `agac` as a string of its own or hands a call a command line starting with `agac`. The docs check, tests/unit/cli/test_a_documented_command_exists.py, does both: it picks out the code-block lines whose first word is `agac`, gives the command tree it walks `agac` as its name, and writes a page whose first line is prose starting with `agac`. It reads those commands as text and walks the command tree in-process; it launches nothing, so it runs this tree whatever the editable install points at. The audit now passes over that file beside the launcher and itself, and the integration README says so.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Stacked on #1313 (#1275).
Validated, not trusted
Path(sys.executable).parent / "agac": true. Nine launches in six files;_agacintest_retry_selection_under_batch.pyserves three more.agent_actionsfrom wherever the editable install points: true. WithPYTHONVERBOSE, the console script run from a worktree loadsagent_actions/__init__from the main checkout, and the worktree's oncePYTHONPATHnames it. From a neutral cwd,import agent_actionsresolves to the main checkout too.test_a_batch_action_keeps_no_row_its_guard_filters.pyfail on a worktree that holds their fix: true. The main checkout was 41 commits behind. WithPYTHONPATHunset, the files that runagacgave 4 failed, exactly those, 155 passed and 1 xfailed. With it set, the four pass.CliRunnertests are unaffected: true. They use the pytest process's path, which pytest.ini sets.PYTHONPATH": partly. The launches inheritos.environ, so aPYTHONPATHset on the command line reaches them. pytest.ini'spythonpathnever becomes an environment variable.[sys.executable, "-m", "agent_actions.cli.main"]would not fix it. The tests run the CLI from a copy of the project, and-mputs that directory on the path, not the tree. From a directory outside the tree,python -m agent_actions.cli.main --versionloads the main checkout.Root cause
A console script's path is its bin directory, then site-packages, where the editable install's
.pthnames the main checkout. pytest.ini'spythonpath = .changes only the pytest process's path and never reaches a subprocess.The fix
tests/_support/agac_cli.py::run_agacruns the console script with the tree the tests live in first onPYTHONPATH. All nine launches call it, and the shared_agacand_run_workflowhelpers wrap it. The console script stays:-mwould not help, and it is what users run.PYTHONPATHalready set stays, behind the tree, whether inherited or passed inenv=.agac list-udfsimports a tool that recordsagent_actions.__file__from inside the CLI process, and the test asserts it is this tree's. It runs withPYTHONPATHcleared, and with a decoyagent_actionsonPYTHONPATHthat raises on import. APYTHONPATHentry outranks the editable install, so this fails in CI too if the tree is not first or the entry is lost.tests/manualthat namesagacoragent_actions.cli.mainas a string of its own, or hands a call a command line starting withagac, positionally or asargs=. A positive control shows it finds nine launch shapes.Behaviour changes, stated
agacas a subprocess now importsagent_actionsfrom its own tree, with or withoutPYTHONPATH. In CI and the main checkout the tree is the editable install, so nothing changes there.envnow getos.environplusPYTHONPATH. The timeout stays 300 s.tests/manualthat launchesagacwithoutrun_agacfails the audit.Not covered here
sys.executable -csubprocesses (intest_topological_sort_is_deterministic.py,test_inferred_dependency_order_is_deterministic.pyandtest_cli_hardening.py) get the tree only from the cwd; two also dropPYTHONPATH. Run from another directory, they import the editable install.tests/manualsmoke runner runs theagacon PATH. It is exempt from the audit because it is run by hand.Related Issue
Fixes #1270
Type of Change
Checklist
task changelog:new)tests/integration/test_a_cli_test_runs_the_tree_it_lives_in.py:agaca test runs importsagent_actionsfrom this tree;PYTHONPATHalready set, inherited or passed, stays on the path behind this tree;agaclaunched in each of nine shapes;agacbut through the shared launcher.agacthemselves now callrun_agac.task checkpasses (ruff, format, mypy)task testpasses on this stack (12687 passed at the top of A retry resumes a halted action in full instead of narrowing it (1267) #1327 (agac retry completes a halted action on the records it names, never answering the ones past the halt #1267); only the 8 known sandbox failures;agacsubprocess tests run withPYTHONPATHset). At this branch's tip, its own tests and the cli, workflow, processing and llm/batch unit suites pass.Verification
PYTHONPATHunset, the probe fails because the CLI process imported the main checkout'sagent_actions/__init__.py. WithPYTHONPATHset it passes, which is the masking the issue describes. With the fix, every file that runsagacpasses withPYTHONPATHunset, where four tests failed before.PYTHONPATHunset, set to the tree, and set to the main checkout, and each is killed in all three:PYTHONPATH;args=.with_name("agac"),-m agent_actions.cli.main, a wrapped list and a shell string, while the docs said it caught any other launch;PYTHONPATHunset, and a droppedPYTHONPATHnot at all;PYTHONPATHpassed inenv=was dropped;tests/manualexemption matched any folder namedmanual.python -csubprocesses, which are outside this issue's console-script scope.Restacked onto #1313
tests/unit/cli/test_a_documented_command_exists.py(The run-modes page retries a batch action with agac retry (1279) #1310, The run-modes page documents an agac batch retry command that does not exist #1279) namesagacas a string while it walks the command tree in-process, and launches nothing. The audit read that as a launch and failed on it (1 failed, 12 passed). The commit exempts that one file, andtests/integration/README.mdsays why. The audit is not weakened otherwise; if the file is renamed, the audit fails again.