01a077f5 - Enforce script-owned model lane execution - #76
Conversation
JonnyLuca
left a comment
There was a problem hiding this comment.
Codex logic — rejected at 506e6d5
src/agent_cli/lane_workspace.py:127: Workspace.apply() validates each touched file in a first pass, then mutates files in a separate second pass. Another process can change or replace a validated target between _read(path) and the later os.replace/os.unlink, causing the model proposal to overwrite or delete content that was never in its snapshot. The coordinator’s own lock does not protect generic agent lane run worktrees from editors or other writers. The guarded application needs a race-resistant compare-and-mutate mechanism (or an exclusive worktree lock covering all possible writers), with regression coverage for a change occurring after validation but before mutation.
JonnyLuca
left a comment
There was a problem hiding this comment.
Codex quality — rejected at 506e6d5
src/agent_cli/coordinator_lanes.py:175 performs the source-inventory Git call only after inserting the working agent and saving c["lane"] as running. If git ls-files or require_git_ok fails, the broad exception handler at line 181 records an uncertain lane and permanently refuses retry, even though the bounded executor was demonstrably never invoked. This newly exposes ordinary/transient inventory failures as unrecoverable “uncertain model process” outcomes. Resolve and validate the manifest before ledger insertion, or distinguish pre-executor failures so they remain safely retryable without an uncertain-lane marker.
JonnyLuca
left a comment
There was a problem hiding this comment.
Codex quality — rejected at 842c3f0
src/agent_cli/lane_workspace.py:190 — _restore_captured restores an original with os.link, while _read at line 89 rejects every file whose link count is not exactly one. Because the recovery link is intentionally retained, a successfully restored file has two links and becomes unavailable to every later Workspace snapshot. This breaks safe retry after a captured mismatch or publication failure, even though the visible worktree content was restored. Add coverage that constructs a fresh workspace after restoration and confirms the restored path remains usable; restoration must preserve the recovery copy without leaving the worktree inode in a state the reader rejects.
JonnyLuca
left a comment
There was a problem hiding this comment.
Codex quality — rejected at 789134d
src/agent_cli/lane_workspace.py:181-203 and 310-358 do not fsync the affected worktree and recovery directory file descriptors after rename/link operations. The recovery index is durably written before capture, but the subsequent capture rename, publication link, restoration link, and temporary-name unlink are only updated in the filesystem cache. A power loss after apply returns can therefore leave the durable index pointing at a capture that was never persisted, lose a reported publication/restoration, or expose an older directory state. This contradicts the new recovery/data-preservation guarantees and is not covered by the process-only race tests. Fsync both directories after each namespace transition (including both sides of rename), and add crash/fault-oriented coverage or narrow the durability claims.
JonnyLuca
left a comment
There was a problem hiding this comment.
Codex logic — rejected at c53adac
src/agent_cli/lane_workspace.py:220: _publish_bytes passes the snapshot mode directly to os.open(..., O_CREAT, mode), but file creation modes are filtered by the process umask. A restrictive operator umask therefore changes permissions on successfully replaced files and on fresh-inode restorations—for example, a captured 0644 file can become 0600. This contradicts the stored snapshot-mode restoration/preservation contract and can make recovery behavior environment-dependent. Apply os.fchmod to the opened temporary inode before its data is published, and cover replacement and restoration under a restrictive umask.
JonnyLuca
left a comment
There was a problem hiding this comment.
Codex quality — rejected at 57ebdc8
docs/ai-accounts.md: the operator-supplied lane configuration example and the numbered migration procedure omit lane_runtime, even though this change makes it mandatory for every lane execution. An operator following the primary example and migration steps will configure a grok:implementer binding that deterministically fails as unconfigured. Add a valid illustrative lane_runtime object to the account example and explicitly include runtime binary/hash selection in the migration steps.
JonnyLuca
left a comment
There was a problem hiding this comment.
Codex logic — rejected at 57ebdc8
src/agent_cli/lane.py:56-59 treats RESULT: approved|rejected as a valid completion field for generic review lanes. The generic/external review contract requires VERDICT, while RESULT is reserved for coordinator prompts. Consequently a generic reviewer that emits `
marassteiner
left a comment
There was a problem hiding this comment.
Formal approval for e9ad1dfd693a75dde9c3b17731afc07ef538e905, submitted by the static executor through the explicitly selected separate review account.
Evidence verified before submission:
- Four independent complete approvals on this exact head: Grok quality and logic, followed by Codex quality and logic; zero introduced findings.
- Full static-script tests passed on the clean signed head.
- Current-head GitHub workflow inventory and PR check rollup are green; PR is mergeable.
This publishes the recorded independent review results. It is not an additional model review and does not merge the PR. A human merges.
|
Ready for review evidence for
The required Ready evidence is verified; the script rechecks it before leaving Draft. A human alone merges. |
EN:
Model lanes exchange bounded source requests with a static executor that owns file access and applies only completed implementation proposals. Explicitly pinned CLI runtimes use isolated profiles, while scripts retain all GitHub, process, test, review and monitoring operations. Installation selects no accounts, roles, models or workers.
DE:
KI-Lanes bearbeiten Quellen über ein begrenztes Protokoll; das statische Script prüft Dateizugriffe und übernimmt nur abgeschlossene Änderungsvorschläge. Explizit konfigurierte CLI-Laufzeiten verwenden isolierte Profile, während GitHub, Prozesse, Tests, Reviews und Monitoring beim Script bleiben. Die Installation wählt keine Accounts, Rollen, Modelle oder Worker aus.
Implementation and migration
Validated Ready evidence
Validated head:
e9ad1dfd693a75dde9c3b17731afc07ef538e905.Static-script full tests:
1760 passed, 3 skipped, 47 subtests passed in 162.42s (0:02:42).Independent complete Grok inner review approved, followed by four independent PR review approvals: Grok quality and logic, then Codex quality and logic.
GitHub workflow inventory and check rollup are green on this head. The script verified the clean worktree, signature and formal approval by
marassteiner.Ready for review is not merged or completed; a human merges.