fix(devtools): pytest slot ownership is the pytest pool, entered through agentctl - #4681
fix(devtools): pytest slot ownership is the pytest pool, entered through agentctl#4681Sinity wants to merge 6 commits into
Conversation
The Sinnix runtime is renamed from sinnixd to agentctl. Every consumer of its job environment (pytest slot, verify receipts, declared operation guards, agent-job bounds, the service-context proofs) reads AGENTCTL_<NAME> first and falls back to SINNIXD_<NAME> through devtools.agent_env.runtime_env, so the fallback can be removed in one place. The cgroup classifiers recognise both slice families (agentctl-<pool>.slice, sinnixd-pueue-<pool>.slice) and the queue runner is found under either name. devtools/sinnixd_service_context.py is renamed to devtools/agentctl_service_context.py. The descriptor cross-contract test now runs against pkgs/agentctl's parser; it no longer asserts the retired parameters/service operation fields, which that parser rejects at parse time. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YHwAE9cH8DjXFSorzfSxD8
…ough agentctl
holds_pytest_slot() is true only inside the pytest pool: a cgroup under
agentctl-pytest.slice (or sinnixd-pueue-pytest.slice), AGENTCTL_POOL=pytest
(or SINNIXD_QUEUE_POOL), or the explicit POLYLOGUE_PYTEST_SLOT=held escape.
A job id, principal or operation name is never ownership, so a lane queues
like everything else and a managed lane running bare pytest is refused with
the devtools test route.
The direct pueue client is gone. A caller outside the pool submits the new
declared operation pytest_focused (pool pytest, result pytest) through
`agentctl job start <checkout> pytest_focused --workspace <checkout> --
<launch file>`, waits on `agentctl job get`, and reads the exit status from
the job view plus the typed timeout receipt beside the log; a killed waiter
cancels with `agentctl job cancel`. The operation also accepts a devtools
test selection and runs it in place.
Descriptor: [workspace] keeps verification_operations = ["verify_quick"] and
declares verify = {focused, candidate = "hosted:verify", corpus} and
publish = "pr"; verify_affected stays declared for manual use and is in no
publication list.
devtools verify: a change set inside .agentctl/**, *.md and .github/** selects
no pytest step and records selection "none" with its reason in the receipt;
the hosted verify job now fails unless the newest run receipt succeeded with
a pytest step or a recorded "none" selection (devtools.verify_receipt_check).
Managed runs dispose of both temporary trees on every exit except a failed
run's, including a signalled waiter.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YHwAE9cH8DjXFSorzfSxD8
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8c0a3800b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| job_id = _document(started, verb="job start").get("job_id") | ||
| if not isinstance(job_id, int) or isinstance(job_id, bool): | ||
| raise PytestSlotUnavailableError(REFUSAL.format(reason=f"`{AGENTCTL} job start` returned no job id")) |
There was a problem hiding this comment.
When agentctl job start --json returns its normal UUID job identifier (the same identifier this commit requires for AGENTCTL_JOB_ID and agentctl-job-<UUID>.service), this integer-only check rejects the successful submission as having no job ID. Consequently, every devtools test or hosted devtools verify invocation outside the pytest pool fails before polling the queued job, instead of waiting for the required pytest_focused run. Accept the runtime's string UUID and pass it unchanged to job get/job cancel.
AGENTS.md reference: AGENTS.md:L151-L155
Useful? React with 👍 / 👎.
| if receipt is None or receipt.get("status") != "timed_out": | ||
| raise | ||
| returncode = 124 | ||
| with _on_exit(lambda: _reap_job(job_id, env=client, launch_path=launch_path), on_exit): |
There was a problem hiding this comment.
If the waiter receives a signal or _wait_for raises while agentctl job cancel is refused or temporarily unavailable, _reap_job deliberately leaves the launch file because the queued job may still start, but this second exit action unconditionally deletes the launch document's TMPDIR and basetemp paths. The surviving job then runs with missing temporary directories, causing tempfile users to fall back outside containment or pytest to fail; cleanup should occur only after cancellation is confirmed, while a job that may still run must retain all resources referenced by its launch file.
Useful? React with 👍 / 👎.
Polylogue's deployed devtools still read SINNIXD_* until Sinity/polylogue#4681 lands; the executor exports both names until then. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YHwAE9cH8DjXFSorzfSxD8
- pytest_slot reads agentctl's terminal phases: `timeout` (not `timed-out`); cancelled, vanished, slot_occupied and the launch-side phases each refuse with their meaning. - The declared-operation guard (Python and the Node proof) checks the exported job id, project and operation and that the process cgroup is under the declared pool's slice; job ids are references, not UUIDs, and units carry no `env -i` to introspect. - The hosted verify receipt check is bound to the run id `devtools verify` wrote to current-run.json (`--run-id`), never to the newest file. - `.agentctl/project.toml` declares `workspace.verify` and `publish`; the keys agentctl ignores and nothing here reads are gone. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YHwAE9cH8DjXFSorzfSxD8
- `.agentctl/project.toml` drops `[conflicts]`, `[owner_adapters]` and the environment preflight; agentctl on the host refuses unknown tables and fields, and nothing in this repository reads them. - `.gitignore` keeps `.agentctl/project.toml` tracked and ignores what agentctl writes beside it. - Tests: the descriptor contract asserts the `verify` and `publish` workspace keys; the receipt test derives its future mtime from the file, not the host clock; the pytest_slot verb helper keeps the job id. Folds #4689. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YHwAE9cH8DjXFSorzfSxD8
… directly The held slot launches pytest as a process group through `Popen`; the rerun tests stand in for that process instead of `subprocess.run`. The agent-tier refusal test places itself in the agent pool, since the test corpus itself runs inside the pytest pool that owns the slot. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YHwAE9cH8DjXFSorzfSxD8
b8c0a38 to
31c723d
Compare
…ough agentctl The slot is owned by the pytest pool cgroup or AGENTCTL_POOL=pytest, never a bare job id. Runs outside a task queue through the declared pytest_focused operation instead of a private pueue client. The descriptor carries the verify table and publish mode agentctl reads; retired keys are gone. Runtime names read AGENTCTL_* with the SINNIXD_* fallback. Merged onto master 31c723d (runner rename, corpus width, slot timeout). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YHwAE9cH8DjXFSorzfSxD8
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 28d91a6187
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const units = [`agentctl-job-${jobId}.service`, `sinnixd-job-${jobId}.service`]; | ||
| if (!/^[0-9a-f]{8}-[0-9a-f]{4}-[1-5][0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/i.test(jobId)) { | ||
| throw new Error("shared-Chrome dev-loop proof requires a Sinnixd job UUID"); | ||
| throw new Error("shared-Chrome dev-loop proof requires a runtime job UUID"); |
There was a problem hiding this comment.
When dev_loop_proof runs in the deployed context modeled by the new service tests—such as AGENTCTL_JOB_ID=polylogue-dev_loop_proof-8e3c63a7 under agentctl-interactive.slice—the Python context guard accepts the job and launches this Node child, but this UUID check rejects it before any Chrome proof runs. The same function then expects a legacy per-job unit rather than the pool-slice contract used by agentctl_service_context.py and live_provider_proof.mjs, so the declared operation always reports shared-Chrome control proof failed on the current runtime; use the same opaque-job-id and pool-slice checks in this child.
AGENTS.md reference: AGENTS.md:L180-L184
Useful? React with 👍 / 👎.
| receipt = _read_timeout_receipt(log_path) | ||
| returncode = _job_exit_status(view, receipt=receipt) | ||
| launch_path.unlink(missing_ok=True) |
There was a problem hiding this comment.
When agentctl job get returns a terminal non-execution phase such as refused, slot_occupied, vanished, or launch-failed, this call raises PytestSlotUnavailableError before the following launch_path.unlink() executes. Because the launch document contains the complete resolved pytest environment—including credentials such as the API key exercised by test_the_client_environment_carries_only_the_allowed_keys—each refused submission leaves a secret-bearing .cache/verify/pytest-slot-<pid>.json behind indefinitely; remove it in a terminal-job cleanup path even when _job_exit_status refuses the run.
Useful? React with 👍 / 👎.
1 participant
Two changes so the Sinnix runtime rename can deploy and the pytest slot cannot be bypassed.
AGENTCTL_*runtime variables first and falls back toSINNIXD_*;devtools/sinnixd_service_context.pybecomesdevtools/agentctl_service_context.py; both unit and slice families are recognised.holds_pytest_slot()is true only inside the pytest pool (cgroup under the pytest slice,AGENTCTL_POOL=pytest, or the explicitPOLYLOGUE_PYTEST_SLOT=heldescape). A job id is never ownership. The direct pueue client is deleted; a caller outside the pool submits the new declared operationpytest_focusedthroughagentctl job start … --waitand cancels throughagentctl job cancel.devtools verifyselects no pytest step for change sets inside.agentctl/**,*.md,.github/**and records the reason; the hostedverifyjob fails unless the newest receipt succeeded with a pytest step or a recorded "none" selection. Managed runs dispose of their scratch trees on every exit.Why correct: ownership is derived from the cgroup the executor places the process in, which no environment edit can forge; the queue entry is the same
agentctl job startroute every other heavy operation uses.Residual risk: the
[workspace].verify/publishdescriptor table is deliberately not in this PR because the deployed agentctl still rejects unknown workspace fields; it follows the deploy in a one-line change.🤖 Generated with Claude Code
https://claude.ai/code/session_01YHwAE9cH8DjXFSorzfSxD8