Release v0.51.303 — Release JS (stage-p1a — cron toggle + config var expansion + git-discard hardening)#3756
Conversation
… open (#3732) _loadRunContent() only ever expanded, so clicking an already-open cron run row re-fetched its content pointlessly. It now toggles: an open row collapses (clears the expansion state + resets the toggle button) and returns early, avoiding the redundant API call. Co-authored-by: mysoul12138 <mysoul12138@users.noreply.github.com>
…3736) hermes-agent already expands ${ENV_VAR} in config.yaml, but the WebUI's own loader stored the raw dict, leaving literal ${...} strings. Recursively expand ${VAR} against os.environ on both config load paths (reload_config and _load_yaml_config_file); unset vars are left untouched (${VAR} preserved). Co-authored-by: Carry00 <Carry00@users.noreply.github.com>
git_discard(delete_untracked=True) used raw shutil.rmtree / Path.unlink after a separate safe_resolve_ws validation, leaving a validation-to-use symlink-swap window. Route untracked deletes through the anchored helpers (rmtree_anchored / unlink_anchored) so a swapped path component is rejected at delete time; preserve the prior missing_ok tolerance for benign concurrent-removal races. Adds regression coverage for both the symlink-swap block and the concurrent-missing case. Co-authored-by: Hinotoi-agent <Hinotoi-agent@users.noreply.github.com>
|
| Filename | Overview |
|---|---|
| api/workspace_git.py | Replaces raw shutil.rmtree/Path.unlink with rmtree_anchored/unlink_anchored for untracked file deletion; correctly preserves missing_ok tolerance for files only, closing the validation-to-use symlink-swap window. |
| api/config.py | Adds _expand_env_vars helper and applies it to both YAML load paths; expansion is values-only, unset vars preserved as literal ${VAR}, logic is correct. |
| static/panels.js | Adds early-return collapse branch to _loadRunContent; contains one dead null guard (item ?) that is harmless but misleading. |
| tests/test_workspace_git.py | Adds two regression tests for the anchored-deletion fix; the symlink-swap test's swap trigger relies on an implicit two-call invariant in _repo_rel that is undocumented. |
| CHANGELOG.md | Changelog entry accurately describes all three changes with correct issue references. |
Sequence Diagram
sequenceDiagram
participant UI as Browser UI
participant JS as panels.js
participant API as /api/crons/run
Note over UI,API: New toggle behaviour
UI->>JS: click open row
JS->>JS: item.classList.contains('open') → true
JS->>JS: remove 'open', clear expansionState, reset button
JS-->>UI: row collapsed (no API call)
Note over UI,API: git_discard untracked delete (security fix)
participant GD as git_discard
participant SA as safe_resolve_ws
participant RA as rmtree/unlink_anchored
GD->>SA: validate path (call 1, via _repo_rel)
SA-->>GD: resolved target
GD->>SA: validate path (call 2, pre-delete)
SA-->>GD: resolved target
GD->>RA: delete via anchored helper
RA->>RA: re-resolve at delete time (dir_fd)
alt symlink swapped after validation
RA-->>GD: ValueError: Path traversal blocked
else path still valid
RA-->>GD: deleted OK
end
Reviews (1): Last reviewed commit: "docs(changelog): stamp v0.51.303 — Relea..." | Re-trigger Greptile
| item.classList.remove('open'); | ||
| body.classList.remove('expanded'); | ||
| _cronExpansionSet(_cronRunExpandKey(jobId, filename), false); | ||
| const btn = item ? item.querySelector('.detail-expand-toggle') : null; |
There was a problem hiding this comment.
item is guaranteed non-null at this point — item.classList.contains('open') on line 728 would have already thrown a TypeError if item were null. The ternary item ? item.querySelector(…) : null is dead code that adds noise without any safety benefit.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| # git_discard validates once for the Git pathspec and once immediately | ||
| # before deletion. Race the second validation-to-use window. | ||
| if requested == "d/f" and state["calls"] == 2 and not state["swapped"]: | ||
| shutil.rmtree(repo / "d") |
There was a problem hiding this comment.
Swap trigger relies on
_repo_rel calling safe_resolve_ws as call #1
The test comment ("validates once for the Git pathspec and once immediately before deletion") is accurate — _repo_rel internally calls safe_resolve_ws(ctx.workspace, workspace_rel) before the explicit call at the deletion site — but this coupling is invisible in the test. If a future refactor inlines _repo_rel without that intermediate safe_resolve_ws call, state["calls"] would never reach 2, the swap would never trigger, and the test would fail on assert state["swapped"] is True rather than surfacing the regression in the production code. Adding a comment or an explicit assertion like assert state["calls"] >= 2 would make the precondition explicit and guard against silent drift.
Release v0.51.303 — Release JS (stage-p1a)
Three low-risk contributor fixes, each rebased onto fresh master (original branches were 9 commits stale — revert-guard would have rolled back releases on a direct squash). Rebase fidelity verified byte-identical to each PR head; dependencies confirmed present on master.
Fixed
Added
${VAR}references fromos.environ(both load paths); unset vars left as literal${VAR}.Security
git_discard(delete_untracked=true)now deletes untracked files via the anchored helpers (rmtree_anchored/unlink_anchored), closing a validation-to-use symlink-swap window; preserves missing_ok tolerance. Regression tests for both the swap-block and concurrent-missing-file cases.Gates
Closes #3732, closes #3736, closes #3702.