Skip to content

Document the ActCommand mutation contract and add integration coverage #70

Description

@bradjin8

Problem

ActCommand.build() (cli/localci/core/executor.py:202-282) is pure — it only reads self to assemble an argv list. The surprising side effect lives in the executor: JobExecutor._execute_process() mutates the caller's ActCommand instance in place, writing back act_cmd.env_file and act_cmd._executor_owned_env_file = True (executor.py:600-601) and act_cmd.secret_file / act_cmd._executor_owned_secret_file = True (executor.py:620-621). run() then depends on that mutation for cleanup in its finally block via _cleanup_temp_files(act_cmd) (executor.py:528-529, 734-755). This mutation contract — "pass an ActCommand to the executor and it will populate temp-file fields and ownership flags on your object" — is undocumented: the ActCommand docstring (executor.py:146-150) says nothing about it, and the _executor_owned_* fields are repr=False (executor.py:178-179), making the behavior invisible to callers. Reusing one ActCommand across executor calls, or building it expecting purity, is a latent footgun.

Acceptance Criteria

  • The ActCommand docstring (executor.py:146-150) documents that env_file, secret_file, and the _executor_owned_* flags are populated as side effects by JobExecutor, and that an instance is single-use per execution
  • JobExecutor.run() / _execute_process() docstrings state that they mutate the passed-in ActCommand and own the resulting temp files (cleanup happens in run()'s finally)
  • An integration test (marked integration, under cli/tests/integration/) exercises the real end-to-end path and asserts the receiver was mutated (temp files created during the run, ownership flags set, and files removed after cleanup)
  • The contract note clarifies expected behavior when an ActCommand is reused or shared across concurrent executions (documented as unsupported, or made safe)
  • Tests pass in CI (if applicable)
  • PR approved by at least 1 reviewer

Implementation Notes

  • Primary file: cli/localci/core/executor.py. The mutation sites are _execute_process lines 600-601 (env file) and 620-621 (secret file); cleanup is _cleanup_temp_files (lines 734-755), invoked from run()'s finally (line 529).
  • Note on the finding's phrasing: the eval referred to this as a "build" side effect, but ActCommand.build() is actually pure — the receiver-state mutation is performed by JobExecutor._execute_process. Ground the docs/tests in the real symbols above.
  • Existing unit coverage already asserts the mutation with mocked subprocesses (cli/tests/test_executor.py:731-793 for env/secret materialization; :833-855 for executor-owned cleanup). This issue adds the missing integration coverage (real act/subprocess) and, importantly, the written contract — reuse the integration marker declared in pyproject.toml:40-42 and the fixtures in cli/tests/integration/.
  • Gotcha: integration tests require act + Docker and are excluded from the default run (norecursedirs = ["integration"], pyproject.toml:43); follow the pattern in cli/tests/integration/test_act_subprocess.py and gate appropriately so CI without Docker still passes.

References

  • Eval finding: Test 7 (interface-contract) — "ActCommand mutation contract is side-effectful"; Test 31 (testing) — "No integration test coverage for mutation behavior"
  • Related files: cli/localci/core/executor.py, cli/tests/test_executor.py, cli/tests/integration/test_act_subprocess.py, cli/pyproject.toml

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Fields

No fields configured for issues without a type.

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions