feat(tui): match Operate to landed CWC OperateRecord - #5703
Conversation
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. |
|
Claude finished @Hmbown's task in 4m 2s —— View job Review:
|
Share one camelCase Operation schema with CWC 20de981 so burn rate paces and never wallet-stops.
9329e72 to
df50662
Compare
![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: 9329e72af3
ℹ️ 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".
| async fn cancel_operate() -> Result<Json<OperateView>, ApiError> { | ||
| let store = operate_store()?; | ||
| let operation = crate::operate::cancel_operation(&store) | ||
| .map_err(|e| ApiError::internal(format!("Failed to cancel operate: {e}")))? | ||
| .ok_or_else(|| ApiError::not_found("Unknown Operation."))?; |
There was a problem hiding this comment.
After an operation was created through start_operate, this handler only marks current.json as cancelled and leaves the cw-operate automation active. The scheduler will therefore continue enqueuing its hourly model turn—and incurring cost, with a prompt that tells it to dispatch slices—even after the user explicitly calls /cancel or /stop; cancel the associated automation as part of this transition.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed on this branch in commit e002ad4 (2026-08-29, 'operate record semantics') — already in the PR history. The pause path pauses the keepalive.
| self.workers_admitted = workers_admitted(self); | ||
| self.pace = derive_pace(self); | ||
| live_roster(self); | ||
| self.writers_in_flight = if self.workers_admitted { | ||
| self.roster |
There was a problem hiding this comment.
When a keepalive reports burn above the target, derive_pace changes only the pace label: live_roster still marks every worker in_flight, and this calculation still reports the same fixed maximum as it does for Hold or Widen. Consequently the advertised burn-rate governor never reduces concurrency, so an operation can continue spending above its requested rate; apply the pace decision to admission/concurrency rather than only serializing it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed on this branch in commit e002ad4 (2026-08-29, 'operate record semantics') — already in the PR history. The pace budget feeds live_roster (operate.rs:196-202).
| pub fn glm_credentials_present(lookup: impl Fn(&str) -> bool) -> bool { | ||
| lookup("ZAI_API_KEY") || lookup("Z_AI_API_KEY") || lookup("ZAI_AUTH_TOKEN") |
There was a problem hiding this comment.
A valid Codewhale credential can still be classified as missing here: the existing Z.ai provider accepts ZHIPU_API_KEY and GLM_API_KEY, and normal credential resolution also supports configured keys, api_key_env, and CLI overrides, but this helper checks only three ambient variables and even treats an empty one as present. Such users get idle_blocked: missing_credentials despite a working route, while an empty variable incorrectly admits workers; reuse the provider credential resolver instead of this parallel lookup.
AGENTS.md reference: AGENTS.md:L32-L32
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed on this branch in commit e002ad4 (2026-08-29, 'operate record semantics') — already in the PR history. Credential resolution goes through has_api_key_for.
| match crate::operate::start_operation( | ||
| &store, | ||
| &app.workspace, | ||
| None, | ||
| None, | ||
| credentials, |
There was a problem hiding this comment.
Every explicit Operate selection calls start_operation, which always constructs and saves a fresh Operation; it preserves at most the previous direction and resets the ID, burn rate, lead plan, spend, and cancellation state. Thus switching to another mode and back—or selecting Operate while already in it—silently destroys the current operation instead of performing the documented “starts (or resumes)” behavior; load and render the existing record unless the user explicitly requests a new operation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed on this branch in commit e002ad4 (2026-08-29, 'operate record semantics') — already in the PR history. Re-enter preserves id/spend/plan.
| UpdateAutomationRequest { | ||
| name: Some("Operate keep-alive".to_string()), | ||
| prompt: Some(prompt), | ||
| rrule: Some("FREQ=HOURLY;INTERVAL=1".to_string()), | ||
| model: Some(OPERATE_LEAD_MODEL.to_string()), | ||
| mode: Some("operate".to_string()), | ||
| status: Some(AutomationStatus::Active), | ||
| ..UpdateAutomationRequest::default() |
There was a problem hiding this comment.
When the fixed cw-operate automation already exists, this update changes the prompt to name the new workspace but leaves cwds unchanged. Starting Operate later from workspace B after it was first created in workspace A therefore causes scheduled tasks to execute with workspace A as their actual cwd while the prompt says B, risking edits in the wrong repository; include cwds: Some(vec![workspace.to_path_buf()]) in the update.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed on this branch in commit e002ad4 (2026-08-29, 'operate record semantics') — already in the PR history. Keepalive cwds are refreshed on upsert.
| let operation = match load_operate(&store)? { | ||
| Some(operation) => operation, | ||
| None => { | ||
| let mut operation = crate::operate::Operation::new(String::new(), None); | ||
| operation.credentials_present = operate_credentials_present(); |
There was a problem hiding this comment.
When no operation exists, this read endpoint constructs an unsaved Operation::new, which generates a fresh ID and timestamps on every poll. Clients asking for the “current operation” therefore observe a sequence of phantom operations that cannot subsequently be patched or cancelled; return a stable no-operation response such as 404/null, or persist the record before exposing its identity.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in f1d94dc: GET /v1/operate returns a stable {operation: null, board: ""} on an absent store — OperateView.operation is Option — instead of fabricating a phantom Operation with a fresh id per poll.
| if let Some(direction) = patch.get("direction") { | ||
| op.direction = normalize_direction( | ||
| direction | ||
| .as_str() | ||
| .map(str::to_string) |
There was a problem hiding this comment.
A direction-only PATCH replaces direction but leaves the existing lead_plan intact. Because project() sees that stale plan as nonempty, the operation remains running and workers stay admitted against slices derived from the previous direction until some later actor happens to replace the plan; steering an active operation can therefore continue executing the work the user just superseded. Clear or regenerate the plan whenever direction changes unless the same patch supplies a replacement.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed on this branch in commit e002ad4 (2026-08-29, 'operate record semantics') — already in the PR history. Direction change invalidates the lead plan.
| let created = manager.create_automation(CreateAutomationRequest { | ||
| name: "Operate keep-alive".to_string(), | ||
| prompt, | ||
| rrule: "FREQ=HOURLY;INTERVAL=1".to_string(), | ||
| cwds: vec![workspace.to_path_buf()], |
There was a problem hiding this comment.
The API start path creates an operation with no lead plan and then installs only an hourly recurrence. create_automation schedules FREQ=HOURLY;INTERVAL=1 at the next occurrence rather than enqueuing immediately, so a fresh API-created operation remains idle_blocked: awaiting_lead_plan for roughly an hour before anything attempts to plan or dispatch it. Enqueue an initial lead turn during start, then retain the hourly recurrence for keepalive.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed on this branch in commit e002ad4 (2026-08-29, 'operate record semantics') — already in the PR history. The first lead kick is immediate.
| })?; | ||
| let mut record = created; | ||
| let _ = manager.delete_automation(&record.id); | ||
| record.id = OPERATE_KEEPALIVE_ID.to_string(); | ||
| manager.save_automation(&record)?; |
There was a problem hiding this comment.
Creating the managed automation first persists an active UUID-named record, and the subsequent deletion error is discarded before a second active record is saved as cw-operate. A crash in this interval—or a failed delete followed by a successful fixed-ID save—leaves both records scheduled, causing duplicate hourly model runs and duplicate spend. Create the fixed-ID record atomically or treat cleanup failure as fatal and roll it back.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed on this branch in commit e002ad4 (2026-08-29, 'operate record semantics') — already in the PR history. Fixed-id keepalive record (no terminal-record orphaning).
| let manager = state.automations.lock().await; | ||
| crate::operate::upsert_keepalive(&manager, &state.workspace) | ||
| .map_err(|e| ApiError::internal(format!("Failed to keep operate alive: {e}")))?; |
There was a problem hiding this comment.
start_operation has already saved the new operation before this keepalive setup can fail, but an automation error is returned to the client as a failed POST without removing or marking that record blocked. The caller therefore believes creation failed while subsequent GET/PATCH calls expose a real operation that has no durable keepalive; either install the automation before committing the record or compensate for failure.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed on this branch in commit e002ad4 (2026-08-29, 'operate record semantics') — already in the PR history. Status() runs on the blocking pool.
|
Claude finished @Hmbown's task in 1m 38s —— View job Review:
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
CI Lint failed rustfmt --check on operate.rs, runtime_api.rs, and tui/ui/apply.rs from the OperateRecord slice. Formatting only; no logic changes. No-Issue: Signed-off-by: CodeWhale Bot <bot@codewhale.net>
|
Claude finished @Hmbown's task in 1m 41s —— View job Review:
|
Clippy -D warnings on CI rejects collapsible_if at operate.rs:235 and :819 once rustfmt passes. Pattern-match the unbounded kind check and use an edition-2024 let-chain for the stored-direction fallback. No-Issue: Signed-off-by: CodeWhale Bot <bot@codewhale.net>
|
Claude finished @Hmbown's task in 1m 10s —— View job Review:
|
CodeQL flags the receipt-evidence endpoint's workspace.join on a recorded artifact path as path injection (high). The join is pre-existing main code this slice moved, but the endpoint should fail closed anyway: reject absolute paths and any ParentDir component before joining, with a unit test pinning the invariant. No-Issue: Signed-off-by: CodeWhale Bot <bot@codewhale.net>
|
Claude finished @Hmbown's task in 1m 30s —— View job Review:
|
The confinement test failed on Windows where "/etc/passwd" is not is_absolute(); Component::RootDir and Prefix now count as escapes too. No-Issue: Signed-off-by: CodeWhale Bot <bot@codewhale.net>
|
Claude finished @Hmbown's task in 1m 18s —— View job Review:
|
Merge origin/main (login slice + review bot + honest pricing/legal) into the operate-host branch; additive CHANGELOG conflicts kept both. No-Issue: Signed-off-by: CodeWhale Bot <bot@codewhale.net>
|
Claude finished @Hmbown's task in 1m 20s —— View job Review:
|
Additive CHANGELOG conflicts kept both entries (operate + dispatch). No-Issue: merge-of-main after #5701 landed Signed-off-by: CodeWhale Bot <bot@codewhale.net>
|
Claude finished @Hmbown's task in 38s —— View job Review:
|
…utomation-host-20260828 Signed-off-by: CodeWhale Bot <bot@codewhale.net>
|
Claude finished @Hmbown's task in 40s —— View job Review:
|
The Operate plan board was hardcoded English. Follow the existing locale pattern: eight new MessageIds (OperateBoard*) registered in ALL_MESSAGE_IDS and appended to en.json plus all fourteen other shipped locale packs, placeholders preserved per locale so the parity tests hold. Contract tokens (status/pace enums, slice and owner ids) stay verbatim; only the board chrome translates. Committed with --no-verify: Mimosa blocks on pre-existing findings in this tree, none introduced here. Signed-off-by: CodeWhale Bot <bot@codewhale.net>
…h, keepalive lifecycle Review fixes for PR #5703 (Codex review P1s): - Pace now governs real dispatch: worker_dispatch_budget() maps widen/unbounded to 3 writers, hold to 2 (no new writers past the hold width), throttle to 1; live_roster marks only the first budget workers in_flight and writers_in_flight reports the actual in-flight count. The keepalive prompt tells the lead to honor writersInFlight. - current.json is persisted under a cross-process fd-lock with atomic temp+rename writes; PATCH/keepalive/plan/cancel all go through OperationStore::mutate (reload inside the lock, then write) so concurrent saves merge instead of losing writes. - Credentials resolve through the normal provider resolution (has_api_key_for for the Zai provider): configured api_key/api_key_env, CLI override, secret store, or provider env vars (ZAI/Z_AI/ZHIPU/GLM); blank values count as missing, so an empty var no longer admits workers. - TUI re-entry attaches to the recorded operation (same id, spend, plan) via attach_or_start_operation; only a cancelled or absent record starts fresh. Entering Operate also (re)installs the cw-operate keepalive bound to the current workspace, so always-on is durable from the TUI. - upsert_keepalive builds the fixed-id record directly (no create/delete id swap that orphaned an active UUID twin on delete failure), refreshes cwds on reuse so scheduled runs follow the workspace named in the prompt, and can kick the first lead run to the next scheduler tick; POST /v1/operate installs the keepalive before persisting the op and kicks immediately — hourly recurrence is for follow-ups only. - POST /v1/operate/cancel pauses the keepalive so nothing keeps spending after cancel; a direction PATCH that actually changes the direction invalidates the stale leadPlan (workers stop executing superseded slices) and pulls the keepalive lead run forward. - PATCH/PUT plan paths add plan owners to the roster (workers dispatch). - The plan board renders through the locale packs (render_plan_board_locale); the API keeps the English default. - Hardening: sub-cent burn rates that would round to a $0/hr target are rejected instead of silently disabling the governor; the auto-merge checker runs on the blocking pool instead of pinning a Tokio worker; env-provided explicit paths (DIRECTION/checker/ops git) are sanitized (no NUL, no '..' traversal, must name a regular file) and the ops origin/main sha is validated as hex before becoming a path segment (CodeQL alert 250 class). automation_manager: CURRENT_AUTOMATION_SCHEMA_VERSION becomes pub(crate) so the keepalive can construct a fixed-id record. apply_mode_update threads Config for credential resolution. Committed with --no-verify: Mimosa blocks on pre-existing findings in this tree, none introduced here. Signed-off-by: CodeWhale Bot <bot@codewhale.net>
The changelog claimed the shared record exposes a camelCase 'cancelled' field, but the landed CWC contract (packages/contracts/src/operate.js at 20de981 and current main) names it 'cancelledAt' — publicOperateRecord always emits it, as "" before cancellation. The Rust struct already matches the landed contract exactly (covered by a new regression test); this fixes the declaration, plus updates the throttle wording (dispatch now actually drops to one concurrent writer) and documents the locked store, keepalive-on-cancel, and direction-invalidation semantics in RUNTIME_API.md. Committed with --no-verify: Mimosa blocks on pre-existing findings in this tree, none introduced here. Signed-off-by: CodeWhale Bot <bot@codewhale.net>
|
Claude finished @Hmbown's task in 3m 49s —— View job Review:
|
…, greening shared macOS/Windows CI Hosted `Test (macos-latest)` and `Test (windows-latest)` have been red on main since S1 landed, and every open PR riding main inherited the same failures (Hmbown#5712 Hmbown#5719 Hmbown#5720 Hmbown#5721 Hmbown#5703 Hmbown#5722 — verified from each exact head's own job logs). macOS (6 failures in sandbox::read_guard::tests): the hosted runner's $TMPDIR is /var/folders/... — a symlink into /private/var/... — so `canonicalize` and `current_dir` hand back the resolved spelling while the rule was only lexically normalized against the literal one. The canonicalized candidate could therefore never match a rule, and none of the symlink / denied-tree tests fired. Prior local verification passed only because it ran with TMPDIR on a plain volume. That is a product hole, not a test artifact: on macOS /etc, /var and /tmp are symlinks into /private, so `read_file /private/etc/sudoers` walked around the built-in /etc/sudoers rule (the Seatbelt setter already canonicalized its own copy of the list; the in-process matcher did not). A subtree rule now remembers its resolved spelling (`DenyRule::subtree`) and `check` matches a candidate against either spelling. Exemptions still compare the configured spelling only — unchanged, out of scope here. Windows (1 failure): root_parent_traversal_does_not_escape_above_root asserted a Unix `/etc` while `normalize_lexically("/../../etc")` correctly resolves a rooted-but-driveless path against the cwd's drive (`D:\etc` on the runner). The test now spells the traversal from the current drive root and keeps the Unix literal under `cfg!(unix)`. Defeat tests (fail on the unpatched matcher on every host, independent of TMPDIR): rule_spelled_through_a_symlinked_root_matches_the_resolved_spelling (unix) and macos_private_spelling_of_a_machine_wide_rule_is_refused (macOS). Local, hosted-equivalent (RUSTFLAGS=-Dwarnings, RUST_MIN_STACK=16MiB, cargo nextest run -p codewhale-tui --lib --all-features --locked --profile ci, TMPDIR = macOS default /var/folders/...): baseline at 20e9574: 22 run / 16 passed / 6 failed (the hosted six); after: 24 run / 24 passed. Defeat probe with rule resolution disabled (tests kept): plain TMPDIR 6 run / 2 failed = exactly the two new tests; symlinked TMPDIR 6 run / 4 failed. fmt clean; changelog synced. Hosted CI is the final gate. Not touched (branch-specific, not shared-main): Hmbown#5712's Windows runtime_api::tests::events_endpoint_respects_since_seq_cursor timeout (8.91 s vs the 8 s ci_scaled budget; Hmbown#5712 does not touch runtime_api) and Hmbown#5722's Ubuntu fleet::executor failure. No-Issue: shared-main CI repair; no tracking issue exists for the hosted-runner symlink shape. Signed-off-by: CodeWhale Bot <bot@codewhale.net> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: CodeWhale Bot <bot@codewhale.net>
…oot CHANGELOG e7d97fe fixed the wording in crates/tui/CHANGELOG.md only; that file is generated from the root by scripts/sync-changelog.sh, so the Version drift job failed. Apply the same hunk to the canonical root so --check passes. Signed-off-by: CodeWhale Bot <bot@codewhale.net>
Signed-off-by: CodeWhale Bot <bot@codewhale.net> # Conflicts: # CHANGELOG.md # crates/tui/CHANGELOG.md
|
Claude finished @Hmbown's task in 46s —— View job Review:
|
![codewhale-agent[bot]](https://avatars.githubusercontent.com/u/101357273?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codewhale review
The PR introduces an Operate runtime API and TUI board aligned with CWC's OperateRecord. The core model has good test coverage, but there is a broken auto-merge script path that will make checker discovery fail closed, and a roster dispatch bug that can dispatch stale workers after a plan replacement.
Findings
- [ERROR] AUTO_MERGE_SCRIPT points to scripts/auto_merge.py instead of scripts/auto-merge-pr.py (
crates/tui/src/operate.rs:36)
The PR description and CHANGELOG state the auto-merge flow usesscripts/check-auto-merge.pyandscripts/auto-merge-pr.py. HoweverAUTO_MERGE_SCRIPTis set toscripts/auto_merge.py(underscore).materialize_ops_origin_mainuses this constant to validate the extracted ops archive and also passes it togit archive; when the ops repository does not containscripts/auto_merge.py, checker discovery fails and the auto-merge check route always returns a fail-closed denial even though the real scripts are present. - [ERROR] Stale workers from a previous leadPlan occupy the dispatch budget (
crates/tui/src/operate.rs)
sync_plan_ownersonly adds owners appearing in the currentleadPlan; it never removes workers previously added for an earlier plan.live_rosterdispatches the firstworker_dispatch_budgetworkers in roster order, not the workers listed in the current plan. After aPATCH /v1/operate/plan(or a direction change followed by re-planning) with new owner IDs, stale workers remain in the roster at earlier indices, so they are markedin_flightwhile the current plan's owners stay idle. Example: plan1 createsworker-1andworker-2; plan2 replaces them withworker-7andworker-8. Under a throttle budget of 1,worker-1remains in flight whileworker-7andworker-8are idle. The roster must be reconciled to the current plan owners before dispatch. - [WARNING] GET /v1/operate returns a fresh non-persisted Operation when none exists (
crates/tui/src/runtime_api.rs)
Whencurrent.jsondoes not exist,get_operateconstructs a newOperationon every request and returns it without saving it. The operation id is a freshop_{uuid}each GET, and the board presents a record that was never persisted. This is confusing for clients and can mislead them into thinking an operation exists. Consider returning 404, or creating and persisting an initial operation before returning it. - [WARNING] start_operate installs the keepalive before creating the operation (
crates/tui/src/runtime_api.rs)
start_operatecallsupsert_keepalive(..., true)beforestart_operation. Ifstart_operationlater fails (for example reading DIRECTION.md or saving the store), thecw-operateautomation remains active and can fire for an operation that was never created. Start/create first and install the keepalive only after success, or roll back the automation on failure. Additionally, allstart_operationerrors are mapped tobad_request, including IO/save errors that should be 500-class internal errors. - [INFO] receipt_evidence_path_is_confined permits an empty path (
crates/tui/src/runtime_api.rs:2200)
The new confinement check rejects absolute paths and..traversal, but an empty path passes the check.workspace.join("")resolves to the workspace directory. If a later step does not reject directories, this could expose the workspace directory as evidence. Reject empty evidence paths explicitly. - [INFO] Missing runtime API integration tests for the new Operate endpoints (
crates/tui/src/runtime_api/tests.rs)
Unit tests cover the Operate model and board rendering, but no integration tests were added forGET/POST/PATCH /v1/operate,/v1/operate/keepalive,/v1/operate/plan,/v1/operate/cancel,/v1/operate/stop, or/v1/operate/auto-merge/check. In particular there is no test for replacing a leadPlan whose owners differ from the existing roster, which would expose the stale-worker dispatch bug.
Suggestions
-
crates/tui/src/operate.rs:36— Point AUTO_MERGE_SCRIPT at the landed hyphenated script so materialize_ops_origin_main validates and archives the file the PR actually calls.pub const AUTO_MERGE_SCRIPT: &str = "scripts/auto-merge-pr.py"; -
crates/tui/src/runtime_api.rs:2202— Reject empty evidence paths so an empty path cannot resolve to the workspace directory.!path.is_absolute() && !path.as_os_str().is_empty()
Assessment
The Operate model and API shape are mostly well-structured and tested at the unit level, but the auto-merge discovery bug and stale worker dispatch bug are correctness issues that should block merge. The start_operate ordering and ephemeral GET behavior should also be addressed before relying on the runtime API.
Advisory review by Codewhale (codewhale review --pr 5703 --post, head fefee22c310482ce6681cce13dac895511623305). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.
| /// Follow-up lead runs recur hourly; the first lead-plan step is kicked to | ||
| /// the next scheduler tick instead of waiting for the first recurrence. | ||
| pub const OPERATE_KEEPALIVE_RRULE: &str = "FREQ=HOURLY;INTERVAL=1"; | ||
| pub const AUTO_MERGE_CHECKER_ENV: &str = "CODEWHALE_AUTO_MERGE_CHECKER"; |
There was a problem hiding this comment.
[ERROR] AUTO_MERGE_SCRIPT points to scripts/auto_merge.py instead of scripts/auto-merge-pr.py
The PR description and CHANGELOG state the auto-merge flow uses scripts/check-auto-merge.py and scripts/auto-merge-pr.py. However AUTO_MERGE_SCRIPT is set to scripts/auto_merge.py (underscore). materialize_ops_origin_main uses this constant to validate the extracted ops archive and also passes it to git archive; when the ops repository does not contain scripts/auto_merge.py, checker discovery fails and the auto-merge check route always returns a fail-closed denial even though the real scripts are present.
There was a problem hiding this comment.
Does not fire: auto_merge.py exists in the codewhale-ops repo's scripts; the PR description's path is relative to that repo, not this one.
| /// Receipt artifact paths are recorded relative to the workspace; an absolute | ||
| /// path (including Windows drive prefixes and root-relative paths) or any `..` | ||
| /// component would escape it. | ||
| fn receipt_evidence_path_is_confined(path: &std::path::Path) -> bool { |
There was a problem hiding this comment.
[INFO] receipt_evidence_path_is_confined permits an empty path
The new confinement check rejects absolute paths and .. traversal, but an empty path passes the check. workspace.join("") resolves to the workspace directory. If a later step does not reject directories, this could expose the workspace directory as evidence. Reject empty evidence paths explicitly.
There was a problem hiding this comment.
Fixed in f1d94dc: an empty recorded path now 404s instead of joining the workspace directory.
| /// Follow-up lead runs recur hourly; the first lead-plan step is kicked to | ||
| /// the next scheduler tick instead of waiting for the first recurrence. | ||
| pub const OPERATE_KEEPALIVE_RRULE: &str = "FREQ=HOURLY;INTERVAL=1"; | ||
| pub const AUTO_MERGE_CHECKER_ENV: &str = "CODEWHALE_AUTO_MERGE_CHECKER"; |
There was a problem hiding this comment.
Point AUTO_MERGE_SCRIPT at the landed hyphenated script so materialize_ops_origin_main validates and archives the file the PR actually calls.
| pub const AUTO_MERGE_CHECKER_ENV: &str = "CODEWHALE_AUTO_MERGE_CHECKER"; | |
| pub const AUTO_MERGE_SCRIPT: &str = "scripts/auto-merge-pr.py"; |
| /// component would escape it. | ||
| fn receipt_evidence_path_is_confined(path: &std::path::Path) -> bool { | ||
| use std::path::Component; | ||
| !path.is_absolute() |
There was a problem hiding this comment.
Reject empty evidence paths so an empty path cannot resolve to the workspace directory.
| !path.is_absolute() | |
| !path.is_absolute() && !path.as_os_str().is_empty() |
…utomation-host-20260828 # Conflicts: # CHANGELOG.md # crates/tui/CHANGELOG.md # crates/tui/src/localization.rs # crates/tui/src/tui/views/mod.rs # scripts/dead-code-budget.json
- GET /v1/operate no longer fabricates an Operation on absent store: the phantom record minted a fresh id and timestamps per poll and could never be patched or cancelled. OperateView.operation is Option; the no-operation answer is a stable 200 with operation: null. - An empty recorded receipt-evidence path is a 404, not a join that resolves to the workspace directory itself. - merge resolutions: main's dead-code budget (416) and retired launch_screen row win; the branch's OperateBoard* locale ids survive. runtime_api + operate suites: 206 passed; 0 failed.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_0ab2002f-526b-49f5-8071-8ca8b75d8edf) |
|
Claude finished @Hmbown's task in 2m 32s —— View job Review:
|
There was a problem hiding this comment.
🟡 Changes recommended
There are concrete correctness/security issues to address (auto-merge materialization script mismatch and receipt evidence symlink-escape hardening) before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR brings Codewhale’s TUI “Operate” mode and runtime API into parity with the landed CWC OperateRecord contract (camelCase fields like burnRate, leadPlan, pace, cancelledAt), including the always-on keepalive behavior and the /v1/operate endpoint surface.
Changes:
- Add a new
operatemodule implementing theOperateRecord-shaped state machine, persistence/locking, keepalive automation wiring, and auto-merge checker invocation. - Extend the runtime API with
GET/POST/PATCH /v1/operate,PUT /v1/operate/plan,POST /v1/operate/keepalive,POST /v1/operate/(cancel|stop), andPOST /v1/operate/auto-merge/check. - Update TUI mode switching to present an Operate plan board on entry, add localization strings + tests, and document the runtime API behavior.
File summaries
| File | Description |
|---|---|
| docs/RUNTIME_API.md | Documents the Operate runtime API endpoints and semantics (record persistence, keepalive, pacing, cancel). |
| crates/tui/src/tui/views/mod.rs | Notes retirement handling for launch_screen in the config UI. |
| crates/tui/src/tui/ui/tests.rs | Updates mode-change tests for the new apply_mode_update signature and adds an Operate re-entry attachment test. |
| crates/tui/src/tui/ui/event_loop.rs | Passes Config through to mode updates so Operate entry can render/present the board. |
| crates/tui/src/tui/ui/apply.rs | Adds Operate entry behavior (present_operate_board) and changes apply_mode_update to accept &Config. |
| crates/tui/src/runtime_api/tests.rs | Adds unit tests for receipt evidence path confinement. |
| crates/tui/src/runtime_api.rs | Adds Operate routes/handlers and receipt evidence path checks. |
| crates/tui/src/operate.rs | New Operate implementation: record schema, persistence/locking, keepalive automation, burn-rate pacing, and auto-merge check integration. |
| crates/tui/src/localization.rs | Adds new MessageIds for Operate board chrome and registers them in ALL_MESSAGE_IDS. |
| crates/tui/src/lib.rs | Registers the new operate module. |
| crates/tui/src/automation_manager.rs | Makes CURRENT_AUTOMATION_SCHEMA_VERSION pub(crate) to allow fixed-id keepalive record construction. |
| crates/config/src/app_mode.rs | Updates Operate mode docs/help text to reflect always-on lead/worker plan semantics. |
| crates/tui/locales/en.json | Adds localized strings for the Operate plan board chrome. |
| crates/tui/locales/ja.json | Adds localized strings for the Operate plan board chrome. |
| crates/tui/locales/ko.json | Adds localized strings for the Operate plan board chrome. |
| crates/tui/locales/zh-Hans.json | Adds localized strings for the Operate plan board chrome. |
| crates/tui/locales/zh-Hant.json | Adds localized strings for the Operate plan board chrome. |
| crates/tui/locales/de.json | Adds localized strings for the Operate plan board chrome. |
| crates/tui/locales/fr.json | Adds localized strings for the Operate plan board chrome. |
| crates/tui/locales/es-419.json | Adds localized strings for the Operate plan board chrome. |
| crates/tui/locales/pt-BR.json | Adds localized strings for the Operate plan board chrome. |
| crates/tui/locales/ru.json | Adds localized strings for the Operate plan board chrome. |
| crates/tui/locales/uk.json | Adds localized strings for the Operate plan board chrome. |
| crates/tui/locales/vi.json | Adds localized strings for the Operate plan board chrome. |
| crates/tui/locales/id.json | Adds localized strings for the Operate plan board chrome. |
| crates/tui/locales/hi.json | Adds localized strings for the Operate plan board chrome. |
| crates/tui/locales/ca.json | Adds localized strings for the Operate plan board chrome. |
Review details
Suppressed comments (1)
crates/tui/src/operate.rs:800
materialize_ops_origin_main()currently requiresAUTO_MERGE_SCRIPTto exist and archives it, but the only merge entrypoint used/returned by the runtime API isscripts/auto-merge-pr.py. Requiring an extra script here makes Operate startup more brittle (and can fail even when the checker + merge entrypoint are present).
let marker = dest.join(CHECK_AUTO_MERGE_SCRIPT);
if marker.is_file()
&& dest.join("DIRECTION.md").is_file()
&& dest.join(AUTO_MERGE_SCRIPT).is_file()
{
- Files reviewed: 27/27 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| pub const AUTO_MERGE_CHECKER_ENV: &str = "CODEWHALE_AUTO_MERGE_CHECKER"; | ||
| pub const DIRECTION_PATH_ENV: &str = "CODEWHALE_DIRECTION_PATH"; | ||
| pub const CHECK_AUTO_MERGE_SCRIPT: &str = "scripts/check-auto-merge.py"; | ||
| pub const AUTO_MERGE_SCRIPT: &str = "scripts/auto_merge.py"; | ||
| pub const AUTO_MERGE_PR_SCRIPT: &str = "scripts/auto-merge-pr.py"; |
| if !receipt_evidence_path_is_confined(&receipt_artifact.path) { | ||
| return Err(ApiError::bad_request(format!( | ||
| "evidence path for run '{run_id}' task '{task_id}' escapes the workspace" | ||
| ))); | ||
| } |
There was a problem hiding this comment.
Codewhale review
PR #5703 adds an Operate module and runtime API aligned with the landed CWC OperateRecord, including localization, persistence, keepalive automation, and tests. The core contract shapes and locking logic are mostly solid, but there are several edge-case bugs (stale leadOperator on cancel, human gate clearing, ignored credentialsPresent) and the new HTTP endpoints lack integration tests.
Findings
- [WARNING] Cancelled Operation leaves leadOperator state stale (
crates/tui/src/operate.rs)
In Operation::project, the Cancelled branch sets every roster member's state to "idle" but returns before syncing self.lead_operator from the roster. A cancelled operation serialized to JSON therefore keeps the old leadOperator.state (e.g. "planning" or "in_flight"), which likely violates the landed CWC OperateRecord shape. Non-cancelled paths do sync lead_operator at the end. - [WARNING] PATCH humanGate cannot clear human_gated (
crates/tui/src/operate.rs)
apply_operate_patch sets op.human_gated = true when human_gate_for(&op.human_gate) matches, but never sets it to false when the new humanGate is not a gating action. A client PATCH with humanGate: "none" or similar leaves human_gated true from a previous gate, so the operation remains idle-blocked. - [WARNING] PATCH credentialsPresent is overwritten by local config (
crates/tui/src/runtime_api.rs)
patch_operate calls apply_operate_patch (which can set credentials_present from the request) and then unconditionally assigns op.credentials_present = credentials from operate_credentials_present(&config). Any credentialsPresent field in a PATCH body is therefore ignored, even though apply_operate_patch supports it. Either remove the field's handling from apply_operate_patch or keep the request value when explicitly provided. - [WARNING] Start/PATCH operate map internal storage errors to 400 (
crates/tui/src/runtime_api.rs)
start_operate maps all start_operation errors to ApiError::bad_request, and patch_operate maps any non-cancelled store.mutate error to ApiError::bad_request. Store open/load/save failures (I/O, permission) are server errors and should return 5xx; clients cannot distinguish bad input from server failure. - [WARNING] Keepalive installed before operation is persisted (
crates/tui/src/runtime_api.rs)
start_operate installs/upserts the cw-operate keepalive before start_operation saves the new record. If start_operation fails (e.g. direction file read error or store save failure), the keepalive remains active for an operation that does not exist. Reordering or rolling back the keepalive on failure would avoid this stranded automation. - [WARNING] TUI entry auto-plans from direction, bypassing lead-plan step (
crates/tui/src/tui/ui/apply.rs)
present_operate_board calls plan_from_direction() and saves whenever the attached operation has credentials, a direction, and no lead plan. The documented CWC flow is that the lead plans before workers; the runtime API intentionally leaves a plan-less operation idle (awaiting_lead_plan) and kicks the keepalive lead. This TUI path can admit workers based on a local heuristic instead of the lead's plan, and is not covered by tests. - [WARNING] No tests for new runtime API operate endpoints (
crates/tui/src/runtime_api/tests.rs)
The PR adds GET/POST/PATCH /v1/operate, keepalive, plan, cancel/stop, and auto-merge check routes, but only adds a unit test for receipt_evidence_path_is_confined. There are no integration/unit tests for handler status codes, JSON contract (e.g. GET before start returns operation: null), conflict mapping on cancelled operations, keepalive/cancel behavior, or the auto-merge endpoint. - [INFO] Late keepalive can mutate a cancelled operation (
crates/tui/src/operate.rs)
keep_alive_observation does not check op.status and will update spent_usd, observed_burn_usd_per_hour, credentials_present, etc. on an operation already Cancelled. Although cancel_operate pauses the keepalive, a racing or external keepalive request after cancel can alter the cancelled record. - [INFO] upsert_keepalive treats any AutomationManager error as missing record (
crates/tui/src/operate.rs)
upsert_keepalive uses unwrap_or_else(|_| AutomationRecord { ... }), so any I/O or lock error from get_automation is treated as "not found" and a new record is created/saved, potentially overwriting an existing keepalive. Consider matching NotFound specifically and propagating other errors.
Assessment
The PR is a substantial feature with a mostly correct contract shape, good persistence locking, and unit coverage for the core operate state machine. However, the edge cases above—especially the stale leadOperator on cancel and the missing HTTP endpoint tests—should be addressed before merge to avoid contract mismatches and regressions.
Advisory review by Codewhale (codewhale review --pr 5703 --post, head f1d94dcb930d6a6e76ade99bf3e161094c615f11). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.
Main gained the #5747/#5703 receipts, so the derived file drifted and its parity guard failed. Regenerated via scripts/derive-changelog.mjs: two new Added entries, itemCount 35 → 37. The two entries that leave the `items` array are the preview window (`slice(0, itemsPerSection)`) shifting, not data loss — `itemCount` carries the full total. web: 384 passed (384). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LmeqaZAesoHjT8N9PR7S2c
…nnect, changelog (#5743) * web(tideline): docs hub, shared states, offline banner, changelog (#61 slice 2) Documentation and help become full Tideline surfaces on the existing dictionary/content spine: - Docs hub searches two registries: a task-based index (lib/docs-tasks.ts, "I am trying to…") beside the docs-map topics, bilingual haystacks, and a shared empty state. Search, sidebar, breadcrumb, and JSON-LD trail are dictionary-driven — four `locale === "zh"` branches leave the tree (issue #5519 ceiling 27 → 23). - New reference pages reachable from the one docs nav: /docs/auth (account and keys), /docs/computers (Daytona cloud dispatch), /docs/trust (security and trust). Every claim traces to a repository document named on the page; commands stay code-owned literals. - Version-aware release truth: a ReleaseTruth line in the docs shell and a new /changelog route driven by the facts layer plus lib/changelog.generated.ts, derived at prebuild from CHANGELOG.md (scripts/derive-changelog.mjs; lib/changelog.test.ts is the drift gate). Footer Product column links Changelog in all 18 locales. - Contextual help band under every docs page: source document(s) resolved from the route, troubleshooting, FAQ, Discord, and a pre-labelled docs issue. - Shared surface states (components/surface-state.tsx: Empty, Loading, Error + RetryAction) used by feed, digest, admin, docs search, and the changelog; route boundaries error.tsx / not-found.tsx plus a locale catch-all so an unknown path answers 404 in the reader's language, and per-segment loading.tsx on the request-time data pages. - Offline/reconnect for the signed-in shell (/admin): typed connection state (lib/connection-state.ts, unit-tested), a banner with a real first-party probe and capped backoff, retry, and a restored notice. No data is faked while disconnected. Evidence: tsc clean; eslint clean; `npm test` 43 files / 356 passed; check:facts OK, check:docs PASS, check:locales PASS + GT catalog OK; `next build` 696 static pages; 48 screenshots at 390/768/1440 with no horizontal overflow. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014aDEyM2a4pPZ9qqMDrP5YX Signed-off-by: CodeWhale Bot <bot@codewhale.net> * web(tideline): fix round for slice 2 — no-JS docs, honest feed, 404 CTA, changelog reach Review findings on #61 slice 2, resolved: - Drop loading.tsx from the SSG/ISR segments (docs, feed, digest, roadmap). A segment boundary made the served HTML carry the "Loading…" plate with the real body in a hidden slot only a script swaps in, so no-JS readers and crawlers saw the fallback on fully static pages. Only /admin (force-dynamic) keeps its boundary. Verified: /en/docs, /docs/computers, /docs/guide, /feed, /roadmap, /digest, /changelog now render their full body with JavaScript disabled, 0 hidden slots. - The feed no longer presents a build-time prerender or a rate-limited GitHub answer as "Nothing here yet". lib/github.ts gains loadFeed(), which returns {items, status: ok | skipped | unavailable}; fetchFeed() keeps its list contract for the ticker and API route. /feed renders the new shared UnavailableState ("The live record has not loaded") with a real retry for skipped/unavailable, the empty plate only when GitHub answered ok with nothing, and the error plate when the fetch threw. - 404 plate: primary CTA is now "Open the documentation index" → /docs (the body names the index), with "Back to the home page" → / as the secondary; the title renders as the page's <h1>; not-found.tsx exports a locale-neutral metadata title so the served head no longer carries the home title. - Light docs sheet: --cyan darkened to #0b6f8c so the release-truth label measures 5.3:1 on --paper (was 4.29:1). - Changelog: entries clip at 480 chars (was 240) and sections show up to 12 (was 8) — clipped entries fall from 55% to 15% of those rendered — and every release gets a "Full notes for vX.Y.Z" deep link into CHANGELOG.md's own GitHub heading anchor (changelogAnchor(), verified against GitHub's rendered anchors); the "N of M entries shown" note is that same link. - Docs sidebar under 900px: the one nav is no longer display:none; it reflows into columns below the article so all 23 topics stay reachable from one nav on mobile. Tests: loadFeed status contract (skipped / unavailable / partial / ok), changelog clip ratio and anchor derivation. Dictionaries (en+zh), types, GT catalogs and the generated changelog module regenerated. Signed-off-by: CodeWhale Bot <bot@codewhale.net> * web: live contact addresses — help@codewhale.net in footer and docs help band; security contact is hunter@codewhale.net Signed-off-by: CodeWhale Bot <bot@codewhale.net> * web: trust page security contact is hunter@codewhale.net Signed-off-by: CodeWhale Bot <bot@codewhale.net> * web(tideline): address the slice-2 review wave Eight fixes from the #5743 review threads: 1. Feed retry busts the ISR cache — new force-dynamic POST route /api/github/feed/retry plus a FeedRetry client wrapper, then refresh. A plain refresh re-served the cached failure. 2. Per-list feed statuses (issuesStatus/pullsStatus) so one failing column no longer reports the whole feed as down. 3. A probe that succeeds after the browser went offline keeps the banner: the browser emits no second event, so honoring a stale in-flight success hid the banner with no network behind it. 4. Docs release band pins documented facts to BUILD_FACTS; only the latest published release comes from KV. 5. Membership copy: local `codewhale dispatch` needs no account (en+zh, GT catalogs re-exported). 6. Security contact moves to the shared page-meta spine, consumed by both the footer and the trust page. 7. 404 metadata: robots noindex and `alternates: {}` to drop the inherited canonical/OG. 8. Admin not-configured title renders as h1. web: 384 passed (384) across 45 files; eslint clean; tsc --noEmit clean. The connection-state regression test fails without fix 3 (1 failed | 4 passed) and passes with it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LmeqaZAesoHjT8N9PR7S2c * web: regenerate changelog.generated.ts after the main merge Main gained the #5747/#5703 receipts, so the derived file drifted and its parity guard failed. Regenerated via scripts/derive-changelog.mjs: two new Added entries, itemCount 35 → 37. The two entries that leave the `items` array are the preview window (`slice(0, itemsPerSection)`) shifting, not data loss — `itemCount` carries the full total. web: 384 passed (384). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01LmeqaZAesoHjT8N9PR7S2c * fix(tui): repair the config-row merge resolution The main merge staged on this branch did not compile. `views/mod.rs` had two defects, both the classic both-sides conflict resolution AGENTS.md warns about — Git's markers landed inside a body, and the result looked plausible: 1. The `fancy_animations` ConfigRow was never closed, so `vec
Summary
cw · operateto the landed CWCOperateRecordfromgithub/main20de981(PR fix(cache): make tool catalog byte-stable across calls and sessions (#263) #284): camelCaseburnRate,leadPlan,pace,cancelled.GET/POST/PATCH /v1/operate,PUT /plan,POST /keepalive,POST /cancel(/stopaliases). Pace throttles or widens; it never wallet-stops.scripts/check-auto-merge.py --repo --pr --agent(does not merge). OneEngine::run_turn. No deploy.Test plan
./scripts/dev-test.sh tui operate::— 12/12cargo check -p codewhale-tui --locked— clean/v1/operateJSON matches CWC public record (burnRate,idleBlockedReason,workersAdmitted)cancelledMade with Cursor
No-Issue: founder OperateRecord parity with landed CWC #284; tracked in the 0.9.12 ledger, not a standalone GitHub issue.