feat(agentctl): validate packet completion from runtime evidence - #13
feat(agentctl): validate packet completion from runtime evidence#13Sinity wants to merge 6 commits into
Conversation
1. docs/sinnixd.md: Clarify that packet scope covers the immutable start→sealed-final range; start/final heads come from the two job records, not from fields in the Beads binding; GitHub branch protection owns review/check authority, not AgentCTL. 2. contracts.py: Reject write_scope: null at the gateway/launch boundary. The prior guard used `if scope is not None`, which silently accepted null and created a scope-less packet. The fix checks `if "write_scope" in binding` so any present value must be a valid non-empty path list. 3. delivery.py: Deletion evidence must exactly equal the deleted path set for the packet range. The prior check allowed overclaims (set(deletions) >= deleted); the fix requires set(deletions) == deleted so unrelated paths are also rejected. 4. runner.py: Preserve the underlying cause when _seal_packet_result cannot read or parse the worker result. The prior code set delivery = None on OSError or JSONDecodeError, losing the reason. The fix re-raises as RunnerError with a typed, non-sensitive message using the existing bounded error mechanism. 5. test_service.py: Add a composed test that runs the real runner seal through exact-head verification into delivery validation, proving one accepted add/delete range and one tampered sealed envelope rejection. Reuses existing fixtures and helpers; no new harness. Inherited failures: test_agent_runner_revalidates_checkout_and_writes_ a_bounded_result_fixture (SINNIX env identity), test_workspace_restack_ detaches_child_after_squash_equivalent_parent_disappears (git merge error), test_real_user_systemd_service_cgroup_cancels_descendants (systemd/cgroup access) — all pre-exist on 2a2f30e. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus 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 |
![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: b53e2c663f
ℹ️ 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".
| principal=principal, | ||
| timeout_seconds=operation.timeout_seconds, | ||
| checkout=checkout.to_dict() if checkout is not None else None, | ||
| contract=dict(contract), |
There was a problem hiding this comment.
When a bound verification runs on a tree whose tree+environment result is already cached—or whose identical verification is active—_start_declared_locked returns that existing record before this new contract is stored. The returned verifier therefore carries an absent or different Beads binding, and _packet rejects it against the packet job, making valid packet delivery fail whenever the verification cache is warm. Include the binding in cache/coalescing identity or bypass reuse for bound verification jobs.
Useful? React with 👍 / 👎.
| review = self._review_after_push(workspace_id) | ||
| return {**review, "published": True, "created": created, "publication_output": publication_output} | ||
| return {**review, "published": True, "created": created, "publication_output": publication_output, "completion": receipt} |
There was a problem hiding this comment.
If the workspace becomes dirty after the post-push _verified_workspace call but before or during PR creation/review reconciliation, review_status still succeeds because it checks only the Git HEAD, and this returns the earlier completion receipt without rerunning the clean/scope/deletion checks. This contradicts the new delivery guarantee that the complete precondition is repeated after review inspection and can hand a closure consumer a completion artifact for a workspace that is no longer clean; rerun _verified_workspace after _review_after_push before returning.
Useful? React with 👍 / 👎.
| or len(scope) > 128 | ||
| or any( | ||
| not isinstance(path, str) | ||
| or not path | ||
| or len(path.encode()) > 1024 |
There was a problem hiding this comment.
A valid binding may contain 128 paths of up to 1024 bytes each, but packet sealing caps the entire JSON envelope at MAX_RESULT_BYTES (64,000 bytes), and deletion evidence must repeat every deleted path. Consequently, a packet deleting enough individually valid scoped paths—such as 128 paths around 500 bytes—can never seal successfully even with a correct report. Validate the aggregate encoded size against the envelope budget or reduce these per-entry/count limits.
Useful? React with 👍 / 👎.
| if isinstance(write_scope, list): | ||
| binding["write_scope"] = write_scope |
There was a problem hiding this comment.
For a Bead with a valid write_scope, this addition makes the runner parse the agent's last message as JSON and later requires an exact five-field delivery schema, but the gateway prompt still merely asks the worker to “report evidence plus residuals” and neither supplies the field names nor requests JSON. Since no managed skill or other prompt surface defines this schema, normal scoped gateway launches will return prose and fail in _seal_packet_result even after successfully completing the task; include the exact output contract in the packet prompt.
Useful? React with 👍 / 👎.
| if isinstance(encoded_scope, str): | ||
| try: | ||
| write_scope = json.loads(encoded_scope) | ||
| except json.JSONDecodeError: | ||
| write_scope = None | ||
| if isinstance(write_scope, list): |
There was a problem hiding this comment.
When the Bead contains a write_scope value that is invalid JSON or decodes to anything other than a list, this code silently omits the scope and launches an ordinary unsealed agent job. That is a fail-open downgrade of an explicitly declared packet policy: the worker runs without the packet result contract or eventual scope evidence, rather than receiving the bounded typed failure promised for malformed scopes. Reject the launch when a present scope cannot be decoded and validated.
AGENTS.md reference: AGENTS.md:L40-L42
Useful? React with 👍 / 👎.
| packet_job_id = arguments.get("packet_job_id") | ||
| return self.delivery.publish( | ||
| *publish_arguments, | ||
| **({"packet_job_id": packet_job_id} if isinstance(packet_job_id, str) else {}), |
There was a problem hiding this comment.
If a direct API caller includes packet_job_id with a non-string value, both publication routes accept the field but omit it from the call, silently executing ordinary delivery without any packet binding, scope, or worker-report validation. A malformed packet-aware request should fail rather than publish or land through the less restrictive fallback; validate a present identifier with _job_argument before dispatching.
Useful? React with 👍 / 👎.
|
Superseded by direct master integration in 68da68f. The packet hardening behavior and coverage are present in the evolved runtime implementation, and master passed the full default check. |
1 participant
Summary
Problem
A successful worker exit and prose summary did not prove that a packet was committed, clean, within its declared scope, verified at the exact final head, or ready for delivery. That left campaign coordination dependent on manual reconstruction and allowed malformed result artifacts to escape the typed runner boundary.
Solution
AgentCTL now derives packet completion from its existing job and workspace state. The seal binds the packet start and final heads, computes the actual changed range, checks exact path and deletion obligations, requires the declared verifier and delivery evidence, and revalidates the state after publication. Gateway plumbing carries the packet contract without taking ownership of Beads or campaign scheduling.
Malformed JSON, invalid UTF-8, null or non-list write scopes, dirty workspaces, omitted or extra paths, missing deletions, stale verification, and delivery drift produce bounded typed failures. The documentation describes this as a packet execution primitive rather than a campaign engine.
Verification
nix build --accept-flake-config .#sinnixd --no-link— completed successfully atb53e2c663f40bc06bf883697c5ecc6594f3d4da1.280 passed, 1 skippedduring independent review of the implementation and hardening series.1 passedat the final correction head.