Repository navigation
Conversation
kokhlo
left a comment
There was a problem hiding this comment.
Verified this end to end and it fixes what #128321 describes, on a worktree at 9e6c0a7: with a stubbed restart_safe_gateway_child_argv and a fake Popen that reads the handoff file back, _launch_external_cron_worker(job, extra_prompt="ONE-OFF CONTEXT") writes extra_prompt: 'ONE-OFF CONTEXT' into ~/.hermes/cron/external-workers/<exec>.json, and the worker restores it into run_one_job. Choosing the payload channel over the trigger_job stamp is also the right call — the stamp would survive an upgrade, while an old worker reading a new payload just loses one fire.
One coverage hole, and it is the half of the fix where the bug actually lived. Your new assertion in test_external_worker_adopts_execution_and_runs_payload_once hand-writes the payload JSON, so it pins the consumer. Nothing pins the producer. Dropping one line from json.dump —
{
"job": job,
- "extra_prompt": extra_prompt,
"profile_home": str(_get_hermes_home().resolve()),— reproduces the reported bug exactly (one-off context silently ignored on every managed install) and leaves the file green: 128 passed, 1 failed, and that 1 is the pre-existing home_io_guard failure, which I reproduced identically on the PR base 99721dca, so it is environmental and not yours. The assert_called_once_with(job, extra_prompt=None) updates do catch the call site losing the kwarg, which is why the mutation has to be the payload line specifically to slip through.
Three tests close it; all three pass on your head, and the two producer-side ones go red (KeyError: 'extra_prompt') the moment the payload line is dropped:
def test_run_one_job_forwards_one_off_prompt_into_the_handoff(monkeypatch):
"""#128321: the transient context from ``cronjob(action='run', prompt=...)`` has to reach
the external worker, and the handoff is the only channel it has on a managed install."""
import cron.scheduler as scheduler
launch = Mock(return_value=True)
run = Mock(side_effect=AssertionError("agent ran inside gateway"))
monkeypatch.setattr(scheduler, "_launch_external_cron_worker", launch)
monkeypatch.setattr(scheduler, "run_job", run)
job = {"id": "job-1", "execution_id": "exec-1"}
assert scheduler.run_one_job(job, adapters={"discord": object()}, extra_prompt="ONE-OFF") is True
launch.assert_called_once_with(job, extra_prompt="ONE-OFF")
run.assert_not_called()
def test_launch_external_cron_worker_writes_one_off_prompt_into_handoff_payload(
tmp_path, monkeypatch
):
"""#128321: the payload on disk is where the prompt either survives or dies -- the worker
subprocess can only read what ``json.dump`` wrote."""
import cron.scheduler as scheduler
from tools.process_registry import GatewayChildDispatch
monkeypatch.setenv("HERMES_HOME", str(tmp_path))
monkeypatch.setattr(scheduler, "_get_hermes_home", lambda: tmp_path)
monkeypatch.setattr(
"tools.process_registry.restart_safe_gateway_child_argv",
lambda command, **_: GatewayChildDispatch("scoped", ["scope", "--", *command]),
)
_spawned, payloads, _handoff, _get = _stub_external_worker_launch(scheduler, monkeypatch)
job = {"id": "job-1", "execution_id": "exec-1", "prompt": "work"}
assert scheduler._launch_external_cron_worker(job, extra_prompt="ONE-OFF CONTEXT") is True
assert payloads[0]["extra_prompt"] == "ONE-OFF CONTEXT"
def test_planned_fire_writes_no_prompt_into_handoff_payload(tmp_path, monkeypatch):
"""The other direction: a scheduled fire carries no context and must not grow one."""
import cron.scheduler as scheduler
from tools.process_registry import GatewayChildDispatch
monkeypatch.setenv("HERMES_HOME", str(tmp_path))
monkeypatch.setattr(scheduler, "_get_hermes_home", lambda: tmp_path)
monkeypatch.setattr(
"tools.process_registry.restart_safe_gateway_child_argv",
lambda command, **_: GatewayChildDispatch("scoped", ["scope", "--", *command]),
)
_spawned, payloads, _handoff, _get = _stub_external_worker_launch(scheduler, monkeypatch)
job = {"id": "job-1", "execution_id": "exec-1", "prompt": "work"}
assert scheduler._launch_external_cron_worker(job) is True
assert payloads[0]["extra_prompt"] is NoneHERMES_HOME is set in the two payload tests on purpose: without it build_subprocess_env stats a path under the real ~/.hermes and home_io_guard fails the test for an unrelated reason — that is exactly why test_launch_external_worker_uses_restart_safe_scope_and_acknowledges is red on every revision, including yours and 99721dca. Worth a separate monkeypatch.setenv there too if you want that file green locally.
Happy to push these as a test-only commit on a branch if you would rather not carry them.
|
Independent verification on the PR head (9e6c0a7): restart-safe-worker suite 33/33 green on Linux. Covering external worker prompt forwarding keeps the one-off prompt across the handoff. No findings. |
|
Addressed the coverage gap in follow-up commit
Tests: |
|
Addressed the producer-side coverage gap in
Verification: |
What does this PR do?
Forwards the transient
extra_promptthrough the external-worker handoff, restoringcronjob_manage(action="run", prompt=...)semantics on managed (restart-safe) installs.On managed gateways,
run_one_jobhands the job to an external worker process. The handoff payload carried the job dict but droppedextra_prompt— the one-off prompt argument of a manualrunaction — so the worker executed the job with no prompt, silently ignoring the operator's instruction (the in-process path honored it fine). The launch now serializesextra_promptinto the payload file, and the worker passes it back into its ownrun_one_jobinvocation; absent/None behaves exactly as before.Related Issue
Fixes #128321
Type of Change
Changes Made
cron/scheduler.py::_launch_external_cron_worker: acceptextra_prompt, write it into the handoff payload JSON.cron/scheduler.py::run_one_job: passextra_promptto the external launch.cron/scheduler.py::_run_external_worker_payload: readextra_promptfrom the payload and forward it torun_one_job.tests/cron/test_restart_safe_worker.py: the adoption test's payload includesextra_promptand asserts the worker'srun_one_jobcall receives it as a kwarg.How to Test
./venv/bin/python -m pytest tests/cron/test_restart_safe_worker.py -q -k "adopts or extra"cronjob_manage(action="run", prompt="run only this once")→ the worker's run reflects the prompt (visible in the execution log).Checklist
Code
pytest tests/ -qand all tests pass (targeted suite:tests/cron/test_restart_safe_worker.py)Documentation & Housekeeping