fix(openshell): make the [openshell] extra enforce ACS policy again

[Imran Siddique (imran-siddique)]

Replaces #3728, which carried six weeks of CI history and follow-up comments. Same branch, same head (efa88590), nothing new in the code. This description is written for the review.

The problem

pip install agent-governance-toolkit-integrations[openshell] installs an openshell_agentmesh module that does nothing except emit a deprecation warning. The v5 migration removed the working adapter and never replaced it with an ACS one, so the extra we still advertise is empty.

What this adds

GovernanceSkill and governed_shell are back, now backed by ACS. Every subprocess.run, subprocess.Popen, os.system and os.popen inside a governed_shell scope goes through session.pre_tool_call as shell.execute before the process starts. The policy-check CLI and the consolidated-package entry point come back with it.

skill = GovernanceSkill.from_manifest("acs.yaml")
with governed_shell(skill):
    subprocess.run(["rm", "-rf", "/data"])  # raises ShellPolicyViolation on DENY

Where to look (15 minutes)

353 lines of production code in skill.py and cli.py, 625 lines of tests, the rest docs and CI wiring.

• skill.py:100: if policy evaluation throws, the command does not run. The broad except Exception is on purpose.
• skill.py:103: any verdict that does not permit (DENY, legacy ESCALATE) blocks before the subprocess starts.
• skill.py:105-111: a TRANSFORM with a malformed target raises instead of running the original command.
• skill.py:138-180: nested and concurrent scopes are tracked in a ContextVar, and the four builtins are restored by reference even when the governed body raises.

Validation

144 checks pass on efa88590, 7 skipped, no conflicts. The native OpenShell CI job builds ACS and runs 33 tests. Removing the gate at skill.py:103 makes three subprocess tests fail, which is how I know they test the gate and not the stub.

Out of scope: v4 policy objects still need migrating to ACS, and this does not add a Vertex provider.

[github-actions]

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

[github-actions]

PR Review Summary

Check	Status	Details
🔍 Code Review	⚠️ Missing	No current-run comment
🛡️ Security Scan	⚠️ Missing	No current-run comment
🔄 Breaking Changes	⚠️ Missing	No current-run comment
📝 Docs Sync	⚠️ Missing	No current-run comment
🧪 Test Coverage	⚠️ Missing	No current-run comment

Verdict: ⚠️ AI review incomplete; ready for human review

AI review comments are untrusted advisory output. The summary reports workflow-generated completion status only, not model-authored pass/fail claims.

[github-actions]

📦 Dependency diff (SBOM)

Comparing main → agent/restore-openshell-acs-integration.

✅ No dependency changes detected.

[github-actions]

🔴 Contributor Check: HIGH

Check	Result
Profile	HIGH
Credential	UNKNOWN
Overall	HIGH

Automated check by AGT Contributor Check.

[MohammadHaroonAbuomar]

Two notes on 9cc02a2 while the restore-or-retire decision is pending. The autofix commit 808dc93 has no Signed-off-by trailer, so the DCO Check fails; a signed amend of that one commit clears it. The CI failure is test_relay.py::test_knock_accept_and_reject_from_binding_is_enforced, a known intermittent failure unrelated to this change (4409 other tests passed); a rerun should clear it. The test change itself is fine: pytest.raises(match=...) replaces the try/except and keeps the restore assertion.

[Imran Siddique (imran-siddique)]

Signed-off 808dc93 as 78933a9 and redid the main merge on top, with the tree unchanged. All 150 checks now pass on 3a4f49f, DCO included.

The relay test passed this time with no agent-mesh change, so it is intermittent, as you said. I found no issue tracking it, and it was absent from the last 15 failed runs of that workflow, so it looks rare.

The two open code-quality threads are false positives. pytest.raises swallows the RuntimeError, so line 344 runs and uses original_run.

[Prayag (prayagupa)]

Focused review comments on request binding, concurrent teardown, and package dependencies.

[MohammadHaroonAbuomar]

• prayagupa Major pyproject: openshell extra is empty though module imports ACS; declare compatible ACS package + clean wheel-install test

All three items are fixed at ce8ee4e and verified by probe: the command is frozen to a tuple before the ACS snapshot and the same frozen value executes in all four wrappers and in the TRANSFORM branch (a second thread mutating the caller's list no longer changes what runs); the originals live in an immutable mapping and an in-flight call survives final-scope teardown with a single restore; the openshell extra declares the ACS range and the CI clean-install step exercises it. 35 tests pass, 22 of 22 checks green. Dismissing; one separate finding follows as a comment.

[MohammadHaroonAbuomar]

• prayagupa's three items are fixed at ce8ee4e and I verified each by probe: the command is frozen before the ACS snapshot and the same frozen tuple is what every wrapper and the TRANSFORM branch executes, so a second thread mutating the caller's list no longer changes what runs; the originals are an immutable mapping and an in-flight call survives final-scope teardown with exactly one restore; the openshell extra declares agent-control-specification>=0.4.0b0,<0.5.0, matching core, and the CI clean-install step installs and imports it from the local wheel. 35 tests pass, 22 of 22 checks green. One thing the probes turned up, pre-existing rather than introduced here, but in the function you just added, so worth folding in.

Fixed at ee7f569 (head e2225fe): _freeze_command now treats any iterable other than str, bytes or PathLike as argv and freezes it to a tuple, so ACS evaluates the real command for a deque, a UserList, dict keys or an iterator, and a deque mutated after approval no longer changes what runs; cwd is resolved once with _freeze_cwd and the wrappers execute that string. Six new tests cover each case and the reply states all fail on the previous head. All 17 checks green. Dismissing; nothing of mine is open on this PR.

[Santosh Kumar Puppala (Santoshkumarpuppala)]

With shell=True, the policy target's executable is not the program that runs. _command_args returns the whole string as one element (skill.py:304-305), and skill.py:90 takes the path basename of it, so rm data/victim.txt is evaluated as executable victim.txt.

Run at 6a8fae44 against a stub ACS that denies executable == 'rm' (native engine not exercised): the control, an argv list, was blocked. The same command through shell=True, os.system and os.popen each deleted the file. The adapter README lists executable as a policy field (README.md:19), so a rule on it misses what a shell string runs.

Splitting on the first token would not cover a chained command like true; rm f. Reporting sh as the executable for shell strings, so shell policy keys on the command text the target already carries, would. Happy to add the test case, or to be told the shell path is a deliberate scope call.

[MohammadHaroonAbuomar]

agent-governance-python/agentmesh-integrations/openshell-skill/openshell_agentmesh/skill.py:304 Confirming the shell=True report on this head with the package's stub ACS and a rule that denies executable == 'rm': an argv list is blocked, but subprocess.run("rm f", shell=True), os.system("rm f"), os.popen("rm f") and "true; rm f" all run and delete the file, because the shell string is returned as a single argv element and line 90 reports its basename as the executable (f.txt, or rm f for /bin/rm f). A bytes command with shell=True behaves the same. Please report sh (or the platform shell) as the executable for shell strings and keep the full text in command, so shell policy keys on the command text, and add the four cases above as tests.

[MohammadHaroonAbuomar]

agent-governance-python/agentmesh-integrations/openshell-skill/openshell_agentmesh/skill.py:86 A second bypass that does not need shell=True: subprocess.run(["x", f], executable="/bin/rm") runs rm and deletes the file while the policy sees executable x and argv ['x', f]. The wrappers ignore Popen's executable= keyword, which replaces the program that runs. Please feed executable= into the target (as the executable, with argv[0] kept) and add a test.