Document ActCommand mutation contract and add integration coverage (#70)

[bradjin8]

Summary

• Document the ActCommand / JobExecutor mutation contract: build() stays pure; _execute_process() mutates the caller's instance (env_file, secret_file, _executor_owned_*) and run() cleans up executor-owned temp files in finally.
• Clarify that ActCommand instances are single-use per execution; reuse and concurrent sharing are unsupported.
• Add an integration test (test_act_command_mutation_and_cleanup) that exercises the real act subprocess path and asserts mutation during the run plus cleanup afterward.

Closes #70.

Test plan

• pre-commit run -a (ruff, ruff-format, mypy)
• cd cli && pytest tests/ — 595 unit tests pass
• cd cli && pytest tests/integration/test_act_command_mutation.py — passes when act + Docker are available; skips otherwise (excluded from default CI via norecursedirs)

Summary by CodeRabbit

• Documentation
  • Expanded docstrings to clearly describe the executor “mutation contract” for environment and secret temp files, including ownership/flag semantics, cleanup timing, and the single-use warning for command instances.
• Tests
  • Added an integration test that verifies executor-owned env_file/secret_file are created during execution, mutation occurs only after expected output, the job completes successfully, and the executor temp files are removed afterward.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8f42f90e-d0c6-4caf-af0a-638158aebfa0

📥 Commits

Reviewing files that changed from the base of the PR and between f3b980f and b102628.

📒 Files selected for processing (1)

• cli/localci/core/executor.py

✅ Files skipped from review due to trivial changes (1)

• cli/localci/core/executor.py

---

📝 Walkthrough

Walkthrough

cli/localci/core/executor.py now documents how JobExecutor mutates ActCommand instances and cleans up executor-owned temp files. A new integration test exercises the real execution path and checks mutation, ownership flags, job success, and cleanup.

Changes

ActCommand mutation contract

Layer / File(s)	Summary
Document mutation contract in executor docstrings cli/localci/core/executor.py	ActCommand, JobExecutor.run, and _execute_process docstrings now describe how env_file/secret_file and _executor_owned_* flags are set, when temp-file cleanup occurs via _cleanup_temp_files, and that instances are single-use with unsupported reuse/concurrency.
Integration test for mutation and cleanup cli/tests/integration/test_act_command_mutation.py	A new integration test builds an ActCommand via ActCommandBuilder, asserts initial unset file/ownership state, uses an on_output callback to detect executor-time mutation, and verifies job success plus removal of executor-owned temp files after the run.

Estimated code review effort: 2 (Simple) | ~10 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Test as test_act_command_mutation
  participant ActCommandBuilder
  participant JobExecutor
  participant ActCommand

  Test->>ActCommandBuilder: build ActCommand from workflow job
  ActCommandBuilder-->>Test: ActCommand with env and secrets
  Test->>JobExecutor: run(ActCommand, on_output callback)
  JobExecutor->>ActCommand: set env_file, secret_file, ownership flags
  JobExecutor->>Test: invoke on_output after temp files exist
  JobExecutor->>JobExecutor: finally _cleanup_temp_files
  JobExecutor-->>Test: job status PASSED
  Test->>ActCommand: assert executor-owned temp files are removed

Loading

Possibly related PRs

• cppalliance/local-ci-test-system#61: The integration test here exercises the same executor-owned secret-file mutation and cleanup contract touched by that PR.

Suggested reviewers: henry0816191, wpak-ai

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: documenting the mutation contract and adding integration coverage.
Linked Issues check	✅ Passed	The docs now describe executor-owned temp files and single-use behavior, and the new integration test verifies mutation and cleanup.
Out of Scope Changes check	✅ Passed	The changes stay within the requested documentation and integration-test scope with no unrelated logic added.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/actcommand-mutation-contract

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (1)

cli/localci/core/executor.py (1)

149-171: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Docstring accurately reflects the mutation contract, but omits the env-file merge behavior.

The contract description is accurate for the "create fresh temp file" path (lines 611-658 below), but it doesn't mention that when a caller-supplied, unowned env_file already exists, _execute_process reads and merges its content into the new temp file (lines 618-626). Since this PR's stated goal is to fully document the mutation contract, worth calling out this merge step so callers relying on a pre-populated env_file understand their data isn't simply discarded.

📝 Suggested docstring addition

     * :attr:`env_file` and :attr:`_executor_owned_env_file` are set when
       :attr:`env` is non-empty (a ``0600`` temp file is created).
+      If a caller-supplied, unowned ``env_file`` already exists, its contents
+      are merged into the new temp file before :attr:`env` entries are appended.
     * :attr:`secret_file` and :attr:`_executor_owned_secret_file` are set when
       :attr:`secrets` is non-empty (a ``0600`` temp file is created).

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cli/localci/core/executor.py` around lines 149 - 171, The docstring for
ActCommand’s mutation contract is missing the env_file merge behavior in
JobExecutor._execute_process. Update the “Executor mutation contract” section to
mention that when a caller-supplied, unowned env_file is already present,
JobExecutor._execute_process reads and merges its contents into the new temp env
file rather than discarding it. Keep the note aligned with the existing
env_file/secret_file ownership language so callers understand how env state is
preserved across ActCommand and JobExecutor.run.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@cli/localci/core/executor.py`:
- Around line 149-171: The docstring for ActCommand’s mutation contract is
missing the env_file merge behavior in JobExecutor._execute_process. Update the
“Executor mutation contract” section to mention that when a caller-supplied,
unowned env_file is already present, JobExecutor._execute_process reads and
merges its contents into the new temp env file rather than discarding it. Keep
the note aligned with the existing env_file/secret_file ownership language so
callers understand how env state is preserved across ActCommand and
JobExecutor.run.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: aa8a7757-2d8c-41d0-a895-19e4fdc1723d

📥 Commits

Reviewing files that changed from the base of the PR and between b95310f and 7f72d78.

📒 Files selected for processing (2)

• cli/localci/core/executor.py
• cli/tests/integration/test_act_command_mutation.py