Skip to content

refactor(claws): progressive-disclose ox install into references/#511

Merged
galexy merged 2 commits into
mainfrom
claws-install-refactor
Apr 14, 2026
Merged

refactor(claws): progressive-disclose ox install into references/#511
galexy merged 2 commits into
mainfrom
claws-install-refactor

Conversation

@galexy
Copy link
Copy Markdown
Contributor

@galexy galexy commented Apr 14, 2026

Summary

  • Shrinks SKILL.md by ~40% in sageox-distill (435→251) and ~31% in sageox-summary (587→403) by pulling the one-time ox install flow out of every invocation's resident context. On every run, each skill now invokes bash scripts/update-ox.sh — a short state check that handles the git auto-update path non-fatally, or exits 2 to signal "no install yet, read references/INSTALL.md."
  • Extracts the deterministic shell into three helpers, mirroring the split fix(claws): enforce 24h window and dedupe state in sageox-summary #504 did for sageox-summary's 24h-window math:
    • scripts/install-ox-curl.sh — pinned curl install (OX_INSTALL_REF=v0.6.1), head/tail preview, persists the memory file.
    • scripts/install-ox-git.sh <clone_path> <auto_update> — defensive path re-validation (same rules as SKILL.md § 3), Go ≥ 1.24 check, clone + make install, Go-path detection, ~/.openclaw/.env guidance on stderr when ox is not on the subprocess PATH.
    • scripts/update-ox.sh — reads the memory file; curl mode = silent no-op; git mode = re-validate recorded clone path, run git pull --ff-only && make build && make install, fall back to existing binary on any failure with a 10-line tail of the build log on stderr; missing state file = exit 2.
  • Relocates the interactive install prose to references/INSTALL.md per the Agent Skills spec § Directory structure (references/ is the spec's bucket for on-demand docs; assets/ is reserved for templates/images/data, which is why sageox-summary's SUMMARIZE.md stays under assets/). Progressive disclosure is the spec default for references/ and scripts/, so there's no prose explaining why INSTALL.md isn't preloaded — that's just how the runtime works.

Why

The install flow is a one-time decision that gets persisted to ~/.openclaw/memory/sageox-ox-install.json and reused forever. Until now, ~218 lines of interactive-prompt prose + both option walkthroughs + the auto-update flow sat in the skill's resident context on every invocation, even though the actual install work runs once per user per machine. Extracting to references/INSTALL.md means the install prose only loads on the single run where it actually runs, and extracting the deterministic shell to scripts/ means the executing agent no longer has to parse bash, handle BSD-vs-GNU date portability, or get atomic path validation right — it just invokes three helpers with documented contracts.

This is the same pattern #504 applied to the 24h-window math in sageox-summary.

Script contracts

Script Args Exit codes
update-ox.sh none 0 ready (continue); 2 no install state — read references/INSTALL.md
install-ox-curl.sh none 0 success; 3 curl missing / install failed / ox not on PATH
install-ox-git.sh <clone_path> <auto_update> 0 success; 2 usage / bad arg; 3 Go missing or too old, build failed

All three scripts treat the persisted state file (~/.openclaw/memory/sageox-ox-install.json) as untrusted and re-validate every path read from it — the file is user-writable and may have been edited externally between runs.

Test plan

  • bash scripts/update-ox.sh with no state file → exit 2, stderr points at references/INSTALL.md
  • curl-mode state file → silent exit 0
  • git mode with auto_update=false → silent exit 0
  • git mode with clone_path=/tmp/escape (outside $HOME) → stderr warning, exit 0, falls back
  • git mode with clone_path containing .. → stderr warning, exit 0, falls back
  • git mode with clone_path containing each of 17 shell metacharacters → stderr warning, exit 0, falls back
  • git mode with clone_path missing .git subdirectory → stderr warning, exit 0, falls back
  • Unknown install_method value → stderr warning, exit 0
  • Malformed state JSON → stderr warning, exit 0
  • install-ox-git.sh with no args → exit 2 usage
  • install-ox-git.sh with auto_update=maybe → exit 2 bad arg
  • install-ox-git.sh with clone_path=/tmp/escape → exit 2 defensive re-check
  • install-ox-git.sh with clone_path containing ; → exit 2 defensive re-check
  • install-ox-git.sh with Go missing from PATH → exit 3
  • Metachar loop iterates all 17 characters (verified via bash -x | grep -c "for ch in")
  • Duplicated scripts + INSTALL.md are byte-identical between sageox-distill and sageox-summary (diff -q clean)
  • clawhub-skill-lint PASS on both skills — 0 critical, 0 warnings, 6 files / 24 KB and 9 files / 38 KB respectively
  • End-to-end first-run install against a throwaway ~/.openclaw/memory/ — to be run by the reviewer before publish

Follow-ups (not in this PR)

  • Bump OX_INSTALL_REF in install-ox-curl.sh whenever a newer sageox/ox release lands. Right now it's pinned to v0.6.1 matching the prior inline value.
  • Throwaway-slug publish test before the next clawhub skill publish — see claws/openclaw/PUBLISHING.md.

Summary by CodeRabbit

  • New Features

    • Simplified per-run readiness check that verifies the CLI and performs git auto-updates when enabled
    • Two install options exposed to users: curl-based release install or git source build
    • Installer now records a single remembered install choice for future checks
  • Documentation

    • New interactive installation guide covering both install methods, prerequisites, PATH guidance, and re-entry commands
  • Chores

    • Added requirement: jq is used for state handling and improved validation flows

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Apr 14, 2026

📝 Walkthrough

Walkthrough

This change consolidates ox installation into scripted, per-run checks: new installer scripts (install-ox-curl.sh, install-ox-git.sh), a runtime state checker/auto-updater (update-ox.sh), and interactive install guidance (references/INSTALL.md). Installation state is persisted to ~/.openclaw/memory/sageox-ox-install.json; missing or broken state causes update-ox.sh to exit 2 and direct users to the interactive INSTALL.md flow.

Changes

Cohort / File(s) Summary
Top-level Skill Docs
claws/openclaw/sageox-distill/SKILL.md, claws/openclaw/sageox-summary/SKILL.md
Replaced embedded interactive install flow with a per-run bash scripts/update-ox.sh contract; removed prior persisted-choice UI and inline install steps; document now defers to scripts and new INSTALL.md.
Interactive Install Guides
claws/openclaw/sageox-distill/references/INSTALL.md, claws/openclaw/sageox-summary/references/INSTALL.md
Added interactive installation instructions invoked when update-ox.sh exits 2; defines two options (curl vs git), validation rules (Go ≥1.24, clone path under $HOME, input validation), expected scripts/contracts, and PATH guidance.
Curl Installer Scripts
claws/openclaw/sageox-distill/scripts/install-ox-curl.sh, claws/openclaw/sageox-summary/scripts/install-ox-curl.sh
New strict bash scripts that download pinned install.sh (v0.6.1) via curl, execute it, verify ox on PATH, persist install metadata to memory file, and exit with code 3 on missing curl or install failures.
Git Installer Scripts
claws/openclaw/sageox-distill/scripts/install-ox-git.sh, claws/openclaw/sageox-summary/scripts/install-ox-git.sh
New strict bash scripts to clone/build sageox/ox from source. Enforce exact CLI args, validate clone path under $HOME, require jq/go and Go ≥1.24, canonicalize paths (pwd -P), ensure ownership, run make build/make install, persist state JSON, and emit PATH guidance if ox absent.
State Checker & Auto-Update
claws/openclaw/sageox-distill/scripts/update-ox.sh, claws/openclaw/sageox-summary/scripts/update-ox.sh
New per-run readiness check that exits 2 if memory file missing; if install_method=git and auto_update=true performs guarded git pull --ff-only && make build && make install (non-fatal, captures logs); for curl installs no per-run update; always verifies ox on PATH before exiting 0.
Added Memory File Usage
~/.openclaw/memory/sageox-ox-install.json (written by installers)
New persisted install metadata JSON schema: install_method, clone_path/ox_install_ref, auto_update (bool), detected Go bin paths, installed_at timestamp. Stored/consumed by scripts and docs.

Sequence Diagram

sequenceDiagram
    participant User as Agent/User
    participant Update as update-ox.sh
    participant Memory as Memory File\n~/.openclaw/memory/sageox-ox-install.json
    participant Installer as install-ox-{curl,git}.sh
    participant GitRepo as ox Git Repo (when git)
    participant Builder as make build/install
    participant Ox as ox Binary

    User->>Update: bash scripts/update-ox.sh
    Update->>Memory: stat/read
    alt memory missing
        Memory-->>Update: not found
        Update->>User: exit 2 (stderr guidance)
        User->>INSTALL: open references/INSTALL.md (interactive)
        User->>Installer: run chosen `install-ox-curl.sh` or `install-ox-git.sh`
        Installer->>Installer: validate env (curl/go/jq), clone/build as needed
        Installer->>Memory: write sageox-ox-install.json
        Installer->>Ox: verify on PATH (or emit PATH guidance)
        Installer-->>User: exit 0 (or non-zero on failure)
        User->>Update: run update-ox.sh again
    else memory exists
        Memory-->>Update: returns state
        Update->>Update: inspect install_method, auto_update
        alt install_method = git and auto_update = true
            Update->>GitRepo: validate clone_path & run git pull
            GitRepo->>Builder: run make build && make install
            Builder-->>Update: success or non-fatal failure (log)
        else install_method = curl or auto_update = false
            Update->>Update: no-op update
        end
        Update->>Ox: verify on PATH
        alt Ox missing
            Update->>User: exit 2 with PATH guidance
        else Ox present
            Update->>User: exit 0
        end
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

Poem

🐰 I hopped through scripts and mem'ry files,
Pinned versions, checks, and careful trials;
When state is missing I lead the way,
Install or build — then run away!
A nimble rabbit keeps oxs at play.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'refactor(claws): progressive-disclose ox install into references/' clearly and specifically describes the main change: extracting and reorganizing ox installation logic from inline SKILL.md documentation into dedicated scripts and reference files.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claws-install-refactor

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (1)
claws/openclaw/sageox-distill/scripts/install-ox-git.sh (1)

113-122: Potential JSON injection if CLONE_PATH contains a double quote.

While the validation rejects many shell metacharacters, it does not reject " (double quote). If a user provides a path like /home/user/foo"bar, the heredoc will produce malformed JSON. This is unlikely in practice but could cause silent failures or unexpected behavior.

♻️ Option: Add `"` to the rejected characters list
-for ch in ';' '$' '`' '|' '&' '<' '>' '(' ')' '{' '}' '*' '?' '[' ']' '!' '\'; do
+for ch in ';' '$' '`' '|' '&' '<' '>' '(' ')' '{' '}' '*' '?' '[' ']' '!' '\' '"'; do

Alternatively, use jq to safely construct the JSON if it's available.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@claws/openclaw/sageox-distill/scripts/install-ox-git.sh` around lines 113 -
122, The heredoc JSON can be broken if CLONE_PATH contains a double-quote;
update the script that writes "$HOME/.openclaw/memory/sageox-ox-install.json" to
either 1) sanitize/validate CLONE_PATH (and other string vars) to reject or
escape double quotes before writing, or 2) prefer generating the JSON via a safe
tool like jq (if available) by building the JSON object from CLONE_PATH,
AUTO_UPDATE, GO_BIN_DIR, GO_INSTALL_DIR, and INSTALLED_AT so values are properly
quoted/escaped; ensure you reference the variables CLONE_PATH, AUTO_UPDATE,
GO_BIN_DIR, GO_INSTALL_DIR, and INSTALLED_AT when implementing the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@claws/openclaw/sageox-summary/scripts/install-ox-git.sh`:
- Around line 47-74: The clone-path checks only validate the textual prefix of
CLONE_PATH and still allow symlink escapes; resolve CLONE_PATH to its
canonical/physical path (e.g., via readlink -f/realpath) and validate that the
resolved path is under the user's $HOME and that its owner matches the current
user (compare stat/chown UID to $(id -u) or use test -O semantics) before
proceeding to clone or run make install; update the checks that reference
CLONE_PATH (the existing validation block and the similar block at lines
~97-103) to use the resolved path for all prefix, metacharacter, and newline
validations and to abort with a clear error if the path is outside $HOME or not
owned by the user.
- Around line 110-122: The install state is written by interpolating shell
variables directly into a here-doc which can produce invalid JSON if values
contain quotes; replace the heredoc with a JSON encoder call (use jq -n) to
safely emit the object using --arg for CLONE_PATH, GO_BIN_DIR, GO_INSTALL_DIR
and INSTALLED_AT and --argjson for AUTO_UPDATE (or convert it to true/false),
and write the output to "$HOME/.openclaw/memory/sageox-ox-install.json" after
ensuring mkdir -p "$HOME/.openclaw/memory" still runs; reference the existing
variables CLONE_PATH, AUTO_UPDATE, GO_BIN_DIR, GO_INSTALL_DIR and INSTALLED_AT
and the target file path to locate where to change the heredoc into a jq -n
construct.

In `@claws/openclaw/sageox-summary/scripts/update-ox.sh`:
- Around line 65-107: Resolve the recorded CLONE_PATH to its physical path
(e.g., via readlink -f/realpath) into a new variable like REAL_CLONE_PATH and
use that for all subsequent checks and the git/build invocation instead of the
raw CLONE_PATH; after resolving, verify ownership (compare the owner UID/GID
from stat against the current user UID/GID or ensure REAL_CLONE_PATH is under
$HOME) and treat it as unsafe if owned by another user or outside the trusted
location, then skip the auto-update; finally switch the git/pull/make/install
invocation to operate on REAL_CLONE_PATH (e.g., cd "$REAL_CLONE_PATH") so you
never build from a symlinked or redirected path supplied by an editable state
file.
- Around line 43-57: The curl branch and the AUTO_UPDATE=false branch exit with
status 0 without ensuring the ox CLI is available; add a check using command -v
ox (or attempting `ox status`) before returning success: in the
INSTALL_METHOD="curl" block and the AUTO_UPDATE check (variables INSTALL_METHOD,
STATE_FILE, AUTO_UPDATE), if ox is not found print an error to stderr and exit
non-zero (or try `ox status` and fail if it errors); otherwise allow the
existing exit 0 behavior.

---

Nitpick comments:
In `@claws/openclaw/sageox-distill/scripts/install-ox-git.sh`:
- Around line 113-122: The heredoc JSON can be broken if CLONE_PATH contains a
double-quote; update the script that writes
"$HOME/.openclaw/memory/sageox-ox-install.json" to either 1) sanitize/validate
CLONE_PATH (and other string vars) to reject or escape double quotes before
writing, or 2) prefer generating the JSON via a safe tool like jq (if available)
by building the JSON object from CLONE_PATH, AUTO_UPDATE, GO_BIN_DIR,
GO_INSTALL_DIR, and INSTALLED_AT so values are properly quoted/escaped; ensure
you reference the variables CLONE_PATH, AUTO_UPDATE, GO_BIN_DIR, GO_INSTALL_DIR,
and INSTALLED_AT when implementing the fix.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 02eeb40e-8d63-4f95-a405-489e6e847cd3

📥 Commits

Reviewing files that changed from the base of the PR and between e34daf5 and 6d58bd1.

📒 Files selected for processing (10)
  • claws/openclaw/sageox-distill/SKILL.md
  • claws/openclaw/sageox-distill/references/INSTALL.md
  • claws/openclaw/sageox-distill/scripts/install-ox-curl.sh
  • claws/openclaw/sageox-distill/scripts/install-ox-git.sh
  • claws/openclaw/sageox-distill/scripts/update-ox.sh
  • claws/openclaw/sageox-summary/SKILL.md
  • claws/openclaw/sageox-summary/references/INSTALL.md
  • claws/openclaw/sageox-summary/scripts/install-ox-curl.sh
  • claws/openclaw/sageox-summary/scripts/install-ox-git.sh
  • claws/openclaw/sageox-summary/scripts/update-ox.sh

Comment thread claws/openclaw/sageox-summary/scripts/install-ox-git.sh
Comment thread claws/openclaw/sageox-summary/scripts/install-ox-git.sh Outdated
Comment thread claws/openclaw/sageox-summary/scripts/update-ox.sh Outdated
Comment thread claws/openclaw/sageox-summary/scripts/update-ox.sh Outdated
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (1)
claws/openclaw/sageox-summary/scripts/update-ox.sh (1)

43-131: Solid auto-update function with proper fallback behavior.

The function correctly:

  • Falls through to the readiness gate on any validation failure (using return, not exit)
  • Resolves paths physically before operating on them
  • Captures build output to a temp file and shows tail on failure
  • Uses --ff-only to avoid problematic merge situations

One minor cleanup opportunity:

♻️ Remove duplicate local declaration

Line 48 already declares real_clone_path as local, so line 91's re-declaration is redundant:

-  local real_clone_path
   if ! real_clone_path="$(cd "$clone_path" 2>/dev/null && pwd -P)"; then
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@claws/openclaw/sageox-summary/scripts/update-ox.sh` around lines 43 - 131,
The function try_git_update has a redundant local variable declaration for
real_clone_path (declared in the initial "local clone_path real_clone_path ch
log" and again later as "local real_clone_path"); remove the second "local
real_clone_path" line so real_clone_path is only declared once, leaving the rest
of the function unchanged (locate the duplicate inside try_git_update near the
physical path resolution block and delete that extra local declaration).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@claws/openclaw/sageox-distill/scripts/install-ox-git.sh`:
- Around line 117-123: The script currently runs mkdir -p on PARENT_DIR before
validating it, which can create directories via symlink escape; change the flow
in the block that uses PARENT_DIR/REAL_HOME/REAL_PARENT so that you first find
the deepest existing ancestor of PARENT_DIR (walk up from PARENT_DIR toward /
until you find an existing directory), compute its physical path with cd ... &&
pwd -P into REAL_PARENT (or a new temp var), verify that REAL_PARENT is within
REAL_HOME, and only after that create the missing parent directories (mkdir -p
"$PARENT_DIR"); keep references to CLONE_PATH, PARENT_DIR, REAL_HOME and
REAL_PARENT so the validation happens against the real physical ancestor before
any filesystem mutation.
- Around line 140-156: Validate the existing checkout before skipping git clone
by resolving REAL_CLONE_PATH to its canonical location (use cd
"$REAL_CLONE_PATH" and pwd -P or equivalent) and verifying its git remote
matches the expected repo; specifically, after detecting
"$REAL_CLONE_PATH/.git", run a check using git remote get-url origin (or git -C
"$REAL_CLONE_PATH" remote get-url origin) and compare to the expected
"https://github.com/sageox/ox.git" (or
"git@github.com:sageox/ox.git"/"sageox/ox" normalized); if the origin does not
match, treat it as not a valid checkout (delete/rename or exit with error) and
perform git clone into REAL_CLONE_PATH as before, and only then run the
build/install step (cd "$REAL_CLONE_PATH" && make build && make install) so you
never execute build/install on an unrelated or symlink-escaped tree.

In `@claws/openclaw/sageox-distill/scripts/update-ox.sh`:
- Around line 41-42: The prefix comparison that decides auto-update currently
compares stored REAL_CLONE_PATH (physical path) against the textual $HOME,
causing mismatch when $HOME is a symlink; change the check to compare the clone
path prefix against REAL_HOME (the physical $HOME from REAL_HOME="$(cd "$HOME"
&& pwd -P)") instead of $HOME so the stored REAL_CLONE_PATH and the runtime
check use the same canonical paths; update the conditional in the prefix-check
block that references $HOME to use REAL_HOME (or otherwise canonicalize the
left-hand prefix using REAL_HOME) so auto-update is not skipped.

---

Nitpick comments:
In `@claws/openclaw/sageox-summary/scripts/update-ox.sh`:
- Around line 43-131: The function try_git_update has a redundant local variable
declaration for real_clone_path (declared in the initial "local clone_path
real_clone_path ch log" and again later as "local real_clone_path"); remove the
second "local real_clone_path" line so real_clone_path is only declared once,
leaving the rest of the function unchanged (locate the duplicate inside
try_git_update near the physical path resolution block and delete that extra
local declaration).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e83b1374-ded1-493a-b76e-bc79963c86a8

📥 Commits

Reviewing files that changed from the base of the PR and between 6d58bd1 and bd7147f.

📒 Files selected for processing (6)
  • claws/openclaw/sageox-distill/SKILL.md
  • claws/openclaw/sageox-distill/scripts/install-ox-git.sh
  • claws/openclaw/sageox-distill/scripts/update-ox.sh
  • claws/openclaw/sageox-summary/SKILL.md
  • claws/openclaw/sageox-summary/scripts/install-ox-git.sh
  • claws/openclaw/sageox-summary/scripts/update-ox.sh
🚧 Files skipped from review as they are similar to previous changes (2)
  • claws/openclaw/sageox-distill/SKILL.md
  • claws/openclaw/sageox-summary/SKILL.md

Comment on lines +117 to +123
PARENT_DIR="$(dirname "$CLONE_PATH")"
mkdir -p "$PARENT_DIR"
REAL_HOME="$(cd "$HOME" && pwd -P)"
if ! REAL_PARENT="$(cd "$PARENT_DIR" && pwd -P)"; then
echo "error: could not resolve parent directory: $PARENT_DIR" >&2
exit 2
fi
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Don't create parent directories before the symlink-escape check.

Line 118 mutates the filesystem before the physical-path validation. If a component inside "$PARENT_DIR" is a symlink out of $HOME, mkdir -p can create directories outside the allowed tree and only then fail at Lines 120-128. Resolve the deepest existing ancestor first, validate it against $REAL_HOME, and only then create missing parents.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@claws/openclaw/sageox-distill/scripts/install-ox-git.sh` around lines 117 -
123, The script currently runs mkdir -p on PARENT_DIR before validating it,
which can create directories via symlink escape; change the flow in the block
that uses PARENT_DIR/REAL_HOME/REAL_PARENT so that you first find the deepest
existing ancestor of PARENT_DIR (walk up from PARENT_DIR toward / until you find
an existing directory), compute its physical path with cd ... && pwd -P into
REAL_PARENT (or a new temp var), verify that REAL_PARENT is within REAL_HOME,
and only after that create the missing parent directories (mkdir -p
"$PARENT_DIR"); keep references to CLONE_PATH, PARENT_DIR, REAL_HOME and
REAL_PARENT so the validation happens against the real physical ancestor before
any filesystem mutation.

Comment on lines +140 to +156
REAL_CLONE_PATH="$REAL_PARENT/$(basename "$CLONE_PATH")"

if [ ! -d "$REAL_CLONE_PATH/.git" ]; then
git clone https://github.com/sageox/ox.git "$REAL_CLONE_PATH"
fi

# After clone, the target itself must also be owned by the current
# user. `git clone` inherits parent-directory ownership by default but
# this catches the case where someone pre-created the target as a
# symlink to a foreign tree.
if [ ! -O "$REAL_CLONE_PATH" ]; then
echo "error: clone target not owned by current user: $REAL_CLONE_PATH" >&2
exit 2
fi

# Build and install in a subshell so the cd doesn't leak.
( cd "$REAL_CLONE_PATH" && make build && make install )
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Fully validate an existing checkout before skipping git clone.

Lines 142-156 accept any existing .git directory at REAL_PARENT/$(basename "$CLONE_PATH") as a valid sageox/ox checkout. That still allows a leaf symlink to hop outside $HOME, and it lets a caller point this script at an unrelated repo under $HOME, which then gets make build && make install executed. Re-resolve the final target with pwd -P and verify its origin is sageox/ox before building.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@claws/openclaw/sageox-distill/scripts/install-ox-git.sh` around lines 140 -
156, Validate the existing checkout before skipping git clone by resolving
REAL_CLONE_PATH to its canonical location (use cd "$REAL_CLONE_PATH" and pwd -P
or equivalent) and verifying its git remote matches the expected repo;
specifically, after detecting "$REAL_CLONE_PATH/.git", run a check using git
remote get-url origin (or git -C "$REAL_CLONE_PATH" remote get-url origin) and
compare to the expected "https://github.com/sageox/ox.git" (or
"git@github.com:sageox/ox.git"/"sageox/ox" normalized); if the origin does not
match, treat it as not a valid checkout (delete/rename or exit with error) and
perform git clone into REAL_CLONE_PATH as before, and only then run the
build/install step (cd "$REAL_CLONE_PATH" && make build && make install) so you
never execute build/install on an unrelated or symlink-escaped tree.

Comment on lines +41 to +42
REAL_HOME="$(cd "$HOME" && pwd -P)"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Use REAL_HOME for the prefix check too.

claws/openclaw/sageox-distill/scripts/install-ox-git.sh writes clone_path as the physical REAL_CLONE_PATH on Lines 170-184 there. When $HOME is symlinked, that stored path begins with REAL_HOME, not the textual $HOME, so Lines 57-63 skip auto-update on every run before the physical validation below can approve it.

Suggested fix
-  case "$clone_path" in
-    "$HOME"/*) ;;
+  case "$clone_path" in
+    "$HOME"/*|"$REAL_HOME"/*) ;;
     *)
       echo "warning: clone_path not under \$HOME; skipping auto-update: $clone_path" >&2
       return
       ;;
   esac

Also applies to: 57-63

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@claws/openclaw/sageox-distill/scripts/update-ox.sh` around lines 41 - 42, The
prefix comparison that decides auto-update currently compares stored
REAL_CLONE_PATH (physical path) against the textual $HOME, causing mismatch when
$HOME is a symlink; change the check to compare the clone path prefix against
REAL_HOME (the physical $HOME from REAL_HOME="$(cd "$HOME" && pwd -P)") instead
of $HOME so the stored REAL_CLONE_PATH and the runtime check use the same
canonical paths; update the conditional in the prefix-check block that
references $HOME to use REAL_HOME (or otherwise canonicalize the left-hand
prefix using REAL_HOME) so auto-update is not skipped.

Comment on lines +116 to +126
if [ ! -d "$real_clone_path/.git" ]; then
echo "warning: resolved clone_path missing .git subdirectory; skipping auto-update: $real_clone_path" >&2
return
fi

# Run the update from the RESOLVED path. Non-fatal on failure — fall
# back to the existing binary. Capture output so we can show a useful
# tail-of-log on failure without polluting stdout on the success path.
log="$(mktemp "${TMPDIR:-/tmp}/ox-update.XXXXXXXX")"
trap 'rm -f "$log" 2>/dev/null' RETURN
if ! ( cd "$real_clone_path" && git pull --ff-only && make build && make install ) > "$log" 2>&1; then
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Reject non-sageox/ox repos before running make.

The state file is explicitly treated as untrusted, but Lines 116-126 only require .git before git pull --ff-only && make build && make install. A hand-edited clone_path pointing at any user-owned Git repo under $HOME will execute that repo's Makefile on every invocation. Validate the checkout identity, e.g. by checking remote.origin.url, before updating.

Suggested fix
   if [ ! -d "$real_clone_path/.git" ]; then
     echo "warning: resolved clone_path missing .git subdirectory; skipping auto-update: $real_clone_path" >&2
     return
   fi
+  case "$(git -C "$real_clone_path" remote get-url origin 2>/dev/null || true)" in
+    https://github.com/sageox/ox.git|git@github.com:sageox/ox.git)
+      ;;
+    *)
+      echo "warning: clone_path is not a sageox/ox checkout; skipping auto-update: $real_clone_path" >&2
+      return
+      ;;
+  esac
 
   # Run the update from the RESOLVED path. Non-fatal on failure — fall
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if [ ! -d "$real_clone_path/.git" ]; then
echo "warning: resolved clone_path missing .git subdirectory; skipping auto-update: $real_clone_path" >&2
return
fi
# Run the update from the RESOLVED path. Non-fatal on failure — fall
# back to the existing binary. Capture output so we can show a useful
# tail-of-log on failure without polluting stdout on the success path.
log="$(mktemp "${TMPDIR:-/tmp}/ox-update.XXXXXXXX")"
trap 'rm -f "$log" 2>/dev/null' RETURN
if ! ( cd "$real_clone_path" && git pull --ff-only && make build && make install ) > "$log" 2>&1; then
if [ ! -d "$real_clone_path/.git" ]; then
echo "warning: resolved clone_path missing .git subdirectory; skipping auto-update: $real_clone_path" >&2
return
fi
case "$(git -C "$real_clone_path" remote get-url origin 2>/dev/null || true)" in
https://github.com/sageox/ox.git|git@github.com:sageox/ox.git)
;;
*)
echo "warning: clone_path is not a sageox/ox checkout; skipping auto-update: $real_clone_path" >&2
return
;;
esac
# Run the update from the RESOLVED path. Non-fatal on failure — fall
# back to the existing binary. Capture output so we can show a useful
# tail-of-log on failure without polluting stdout on the success path.
log="$(mktemp "${TMPDIR:-/tmp}/ox-update.XXXXXXXX")"
trap 'rm -f "$log" 2>/dev/null' RETURN
if ! ( cd "$real_clone_path" && git pull --ff-only && make build && make install ) > "$log" 2>&1; then

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant