Skip to content

feat(appscript): add push with opt-in prune - #1091

Open
haosdent wants to merge 1 commit into
openclaw:mainfrom
haosdent:appscript-push
Open

feat(appscript): add push with opt-in prune#1091
haosdent wants to merge 1 commit into
openclaw:mainfrom
haosdent:appscript-push

Conversation

@haosdent

@haosdent haosdent commented Sep 6, 2026

Copy link
Copy Markdown

Second slice of #1005, after the read-only third landed in #1018. This is appscript push, reworked around the reason you closed #1005: the old push deleted remote files that were not present locally, silently and by default.

What it adds

Command Purpose
appscript push <scriptId> <dir> Upload a local directory into an Apps Script project

What changed since #1005

  • Nothing is deleted by default. Push merges: files the directory provides are updated or created, remote-only files are left alone. Removal happens only under --prune (alias --delete).
  • --prune names the removal set before it removes anything. The file list is printed first, then the call that drops them is made — so the operator sees the exact set rather than inferring it afterwards.
  • --dry-run reports the decision. Without --prune the request is the whole preview and the dry run stays auth-free. With --prune it reads the project first, because it cannot name what it would remove otherwise, and emits "prune": true with the removal set.
  • Reads are pinned to the requested directory. DirEntry.Type comes from the ReadDir snapshot and is false for a symlink's target, so each entry is re-checked with root.Lstat against the live filesystem; symlinks and other non-regular files are rejected rather than followed. This is the ClawSweeper finding from feat(appscript): add push, pull, deploy, and version management #1005 — push could otherwise upload a file from outside dir.

Notes

  • Extension mapping is the inverse of pull: Apps Script keeps a file's extension in its type, so Code.gs is sent as {name: "Code", type: SERVER_JS}. Files whose extension has no mapping are skipped rather than guessed at.
  • appsscript.json is mandatoryUpdateContent rejects a payload without it, so push fails early with a usage error instead of a provider error.
  • Ordering is stable without an explicit sort: os.ReadDir returns entries sorted by name.
  • Safety profiles. push is listed as false in both readonly.yaml and agent-safe.yaml, so a baked build rejects and hides it.

One drive-by, called out so you can veto it

appscript content printed file names into tab-separated output without sanitizeTab, unlike the rest of the repo (adsense, appscript_list, and others use it). A name containing a tab would corrupt that output. I extracted the shared printAppScriptFiles helper that content and push both use and routed it through sanitizeTab. Happy to drop it into its own PR if you would rather keep this one to push alone.

Live proof

Run against a real Apps Script scratch project (created for this, trashed afterwards). Script ID redacted as <SCRIPT_ID>.

1. Push into an empty project

$ ls ./work
appsscript.json  Code.gs  Helper.gs

$ gog appscript push <SCRIPT_ID> ./work
pushed	true
script_id	<SCRIPT_ID>
files	3
file	Code	SERVER_JS
file	Helper	SERVER_JS
file	appsscript	JSON
editor_url	https://script.google.com/d/<SCRIPT_ID>/edit

2. Remove a local file, push again without --prune — the remote-only file survives

$ rm ./work/Helper.gs && ls ./work
appsscript.json  Code.gs

$ gog appscript push <SCRIPT_ID> ./work
pushed	true
script_id	<SCRIPT_ID>
files	3
file	Code	SERVER_JS
file	appsscript	JSON
file	Helper	SERVER_JS
# Kept 1 remote-only file(s) that /tmp/appscript-proof/work does not provide: Helper.gs
# Use --prune to delete them instead.
editor_url	https://script.google.com/d/<SCRIPT_ID>/edit

$ gog appscript content <SCRIPT_ID>
script_id	<SCRIPT_ID>
files	3
file	Code	SERVER_JS
file	appsscript	JSON
file	Helper	SERVER_JS

3. Both dry runs, neither of which touches the project

$ gog appscript push <SCRIPT_ID> ./work --dry-run
Dry run: would appscript.push
{
  "dir": "/tmp/appscript-proof/work",
  "files": ["Code.gs", "appsscript.json"],
  "prune": false,
  "script_id": "<SCRIPT_ID>"
}

$ gog appscript push <SCRIPT_ID> ./work --prune --dry-run
Dry run: would appscript.push
{
  "dir": "/tmp/appscript-proof/work",
  "files": ["Code.gs", "appsscript.json"],
  "prune": true,
  "removed": ["Helper.gs"],
  "script_id": "<SCRIPT_ID>"
}

$ gog appscript content <SCRIPT_ID>     # unchanged, still 3 files
files	3

4. --prune non-interactively refuses without --force, naming the set first

$ gog appscript push <SCRIPT_ID> ./work --prune --no-input
# 1 remote file(s) will be deleted (not present in /tmp/appscript-proof/work):
#   Helper.gs
refusing to delete 1 remote file(s): Helper.gs without --force (non-interactive)
$ echo $?
2

5. --prune --force prints the removal set, then removes it

$ gog appscript push <SCRIPT_ID> ./work --prune --force
# 1 remote file(s) will be deleted (not present in /tmp/appscript-proof/work):
#   Helper.gs
pushed	true
script_id	<SCRIPT_ID>
files	2
file	Code	SERVER_JS
file	appsscript	JSON
removed	Helper.gs

$ gog appscript content <SCRIPT_ID>
script_id	<SCRIPT_ID>
files	2
file	Code	SERVER_JS
file	appsscript	JSON

6. A symlinked entry is refused and nothing is uploaded

Helper.gs was restored locally and pushed back first, so the project is at three files again before this step.

$ ln -s /tmp/appscript-proof/outside.txt ./work/Evil.gs && ls -la ./work
-rw-r--r--  appsscript.json
-rw-r--r--  Code.gs
lrwxr-xr-x  Evil.gs -> /tmp/appscript-proof/outside.txt

$ gog appscript push <SCRIPT_ID> ./work
Evil.gs is a symlink; push only uploads regular files inside /tmp/appscript-proof/work
$ echo $?
2

$ gog appscript content <SCRIPT_ID>     # untouched, no Evil
files	3
file	Code	SERVER_JS
file	Helper	SERVER_JS
file	appsscript	JSON

Verification

  • make ci green locally (fmt, lint, deadcode, Go and JavaScript tests, generated docs, agent skills).
  • go test ./internal/cmd -run 'AppScript|DryRun|SafetyProfile' -count=1.
  • 14 regressions in internal/cmd/appscript_sync_test.go. Behaviour: merge default keeps remote-only files, --prune removes them, the prune dry run reports the removal set, prune requires force when non-interactive, prune with nothing to remove skips the confirmation, JSON always emits both file lists. Directory reading: extension mapping, name ordering, non-manifest JSON skipped, missing manifest rejected, empty directory rejected, and symlinked sources and symlinked directory entries both rejected.

Second slice of openclaw#1005, after the read-only third landed in openclaw#1018. Reworked
around the reason openclaw#1005 was closed: push deleted remote files that were not
present locally, silently and by default.

- Push merges. Files the directory provides are created or updated, remote-only
  files are left alone. Removal happens only under --prune (alias --delete).
- --prune names the removal set before making the call that removes it, so the
  operator sees the exact list rather than inferring it afterwards.
- --dry-run reports the decision. Without --prune the request is the whole
  preview and the dry run stays auth-free; with --prune it reads the project
  first, because it cannot name what it would remove otherwise.
- Reads are pinned to the requested directory. DirEntry.Type comes from the
  ReadDir snapshot and is false for a symlink's target, so every entry is
  re-checked with root.Lstat against the live filesystem; symlinks and other
  non-regular files are rejected rather than followed. Without this, push could
  upload a file from outside dir — the ClawSweeper finding on openclaw#1005.
- push is listed false in readonly.yaml and agent-safe.yaml, so a baked build
  rejects and hides it.

Also extracts printAppScriptFiles, shared by content and push, and routes it
through sanitizeTab. appscript content wrote API-supplied names into
tab-separated output without it, unlike the rest of the repo.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@haosdent
haosdent requested a review from a team as a code owner September 6, 2026 09:13
@clawsweeper

clawsweeper Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 6, 2026
@clawsweeper

clawsweeper Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Codex review: blocked before merge. Reviewed September 6, 2026, 5:16 AM ET / 09:16 UTC.

ClawSweeper review

What this changes

Adds Apps Script directory uploads with remote-file preservation by default, opt-in pruning, dry-run previews, safety-profile restrictions, tests, and command documentation.

Merge readiness

Blocked before merge - 4 items remain

This is useful, distinct work absent from main and the latest release, with convincing live proof. One safety contract needs clarification: whole-project replacement can overwrite concurrent remote changes despite the preservation guarantee.

Priority: P2
Reviewed head: 35f675b4ec38206923ad851d611f6e952acf19d4
Owner decision: Required. See Decision needed.

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused implementation with strong real-project evidence; the remaining material question is the accuracy and acceptance of its concurrency safety contract.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (terminal): The captured PR body exercises the new push owner through the real CLI against a scratch Apps Script project, with content readbacks showing preservation and pruning plus dry-run, confirmation-refusal, and symlink-refusal results.
Patch quality 🐚 platinum hermit (4/6) 1 actionable review finding remain.

Verification

Check Result Evidence
Real behavior Verified Sufficient (terminal): The captured PR body exercises the new push owner through the real CLI against a scratch Apps Script project, with content readbacks showing preservation and pruning plus dry-run, confirmation-refusal, and symlink-refusal results.
Evidence reviewed 9 items Repository policy and review scope: Read the complete root AGENTS.md; no nested AGENTS.md files or maintainer-notes directory were found. The policy requires Unreleased entries with references and contributor thanks.
Unique capability remains absent from main: Main's command tree provides get, content, run, create, pull, deployments, and versions, but no push. A main-tree search found no Apps Script UpdateContent implementation.
Latest release check: The v0.39.1 Apps Script command tree also lacks push; the feature is not already shipped.
Findings 1 actionable finding [P3] [P3] Add the required changelog reference and contributor thanks
Security None None.

How this fits together

The Apps Script CLI sends local source files to Google's project-content API using the selected account. Push combines local files with a remote snapshot, then replaces the project's working content.

flowchart TD
  A[Local project directory] --> B[Validate and read files]
  C[Selected account and script] --> D[Fetch remote content]
  B --> E{Prune enabled?}
  D --> E
  E -->|No| F[Preserve remote-only files]
  E -->|Yes| G[Preview deletions and confirm]
  F --> H[Replace project content]
  G --> H
Loading

Decision needed

Question Recommendation
May push ship with explicitly documented single-writer semantics, given that Google's whole-project update cannot preserve changes arriving after the initial read? Document exclusive editing: Retain the implementation but require exclusive editing during push and clearly qualify preservation and deletion-preview guarantees in user-facing guidance.

Why: Choosing whether this residual remote-data-loss risk is acceptable changes the advertised safety contract and requires maintainer intent.

Before merge

  • [P3] Add the required changelog reference and contributor thanks (P3) - The new Unreleased entry describes the feature but omits both a PR/issue reference and contributor thanks, which this repository's AGENTS.md explicitly requires. Add the reference and credit to this entry before landing.
  • Resolve merge risk (P1) - Concurrent editor changes or another push between the initial GET and final PUT can be lost; pruning can also delete files absent from the displayed confirmation set. The supplied proof covers sequential operations, so the unconditional preservation wording exceeds the demonstrated guarantee.
  • Complete next step (P2) - Approve and document the single-writer limitation or require concurrency protection, then add the changelog reference and contributor thanks.
  • Resolve maintainer decision - Resolve the maintainer decision shown above before merge.

Findings

  • [P3] [P3] Add the required changelog reference and contributor thanks — CHANGELOG.md:7
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test growth Production +315/-4; tests +407/-0 The growth implements one new upload command with directory validation, merge/prune handling, and focused regression coverage.

Merge-risk options

Maintainer options:

  1. Accept an explicit single-writer contract (recommended)
    Approve the residual concurrent-write risk only after documentation and safety claims explain that remote content is preserved from a snapshot.
  2. Pause for concurrency protection
    Hold the PR if preservation must remain guaranteed while other editors or uploaders modify the project.

Technical review

Best possible solution:

Keep merge-by-default and explicit pruning, with an approved single-writer contract or a provider-supported concurrency safeguard that makes the preservation promise accurate.

Do we have a high-confidence way to reproduce the issue?

Not applicable as an existing-behavior bug: this adds a command. The supplied real-project transcript demonstrates the sequential workflows; the concurrent-write limitation follows from the source and provider replacement contract.

Is this the best way to solve the issue?

Yes for sequential uploads: reusing the existing service, rooted filesystem helper, and confirmation path is appropriately scoped. Its safety claims need to acknowledge the snapshot replacement boundary.

Full review comments:

  • [P3] [P3] Add the required changelog reference and contributor thanks — CHANGELOG.md:7
    The new Unreleased entry describes the feature but omits both a PR/issue reference and contributor thanks, which this repository's AGENTS.md explicitly requires. Add the reference and credit to this entry before landing.
    Confidence: 0.99

Overall correctness: patch is correct
Overall confidence: 0.86

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against 981ca4a163e8.

Labels

Label changes:

  • add P2: This is a bounded Apps Script workflow improvement with a remote-write safety decision.
  • add merge-risk: 🚨 other: Whole-project replacement can lose concurrent remote file additions or edits, a data-integrity risk outside the more specific PR risk labels.
  • add proof: sufficient: Contributor real behavior proof is sufficient. The captured PR body exercises the new push owner through the real CLI against a scratch Apps Script project, with content readbacks showing preservation and pruning plus dry-run, confirmation-refusal, and symlink-refusal results.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The captured PR body exercises the new push owner through the real CLI against a scratch Apps Script project, with content readbacks showing preservation and pruning plus dry-run, confirmation-refusal, and symlink-refusal results.

Label justifications:

  • P2: This is a bounded Apps Script workflow improvement with a remote-write safety decision.
  • merge-risk: 🚨 other: Whole-project replacement can lose concurrent remote file additions or edits, a data-integrity risk outside the more specific PR risk labels.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (terminal): The captured PR body exercises the new push owner through the real CLI against a scratch Apps Script project, with content readbacks showing preservation and pruning plus dry-run, confirmation-refusal, and symlink-refusal results.
  • proof: sufficient: Contributor real behavior proof is sufficient. The captured PR body exercises the new push owner through the real CLI against a scratch Apps Script project, with content readbacks showing preservation and pruning plus dry-run, confirmation-refusal, and symlink-refusal results.

Evidence

What I checked:

  • Repository policy and review scope: Read the complete root AGENTS.md; no nested AGENTS.md files or maintainer-notes directory were found. The policy requires Unreleased entries with references and contributor thanks. (AGENTS.md:41, 35f675b4ec38)
  • Unique capability remains absent from main: Main's command tree provides get, content, run, create, pull, deployments, and versions, but no push. A main-tree search found no Apps Script UpdateContent implementation. (internal/cmd/appscript.go:14, 981ca4a163e8)
  • Latest release check: The v0.39.1 Apps Script command tree also lacks push; the feature is not already shipped. (internal/cmd/appscript.go:14, 6895139ab1b4)
  • Snapshot replacement boundary: Push fetches content once, retains remote-only files from that snapshot, optionally waits for confirmation, and sends an unconditional whole-project update. A file added after the GET is absent from the payload; an intervening edit to a retained file is replaced by its earlier source. (internal/cmd/appscript_sync.go:117, 35f675b4ec38)
  • Authoritative provider contract: The patch directly calls Google's projects.updateContent API. Its official documentation states that this replaces all project files and updates HEAD; the documented request has no revision precondition: Google API contract.
  • Real after-change proof: The supplied complete PR body, captured under sourceRevision 7bbf4c6fceb88a691fd6bead3ecf16f42c160b281dfee8974335a25a96a463a8, contains redacted terminal output from a real scratch project. It exercises AppScriptPushCmd through gog, verifies remote-only preservation with content readback, previews both modes, refuses unconfirmed pruning, confirms deletion after forced pruning, and rejects an outside symlink without changing the project. (35f675b4ec38)

Likely related people:

  • haosdent: Raw commit d8fa2de adds internal/cmd/appscript_pull.go:20 relative to its recorded parents. This identifies author metadata, not feature responsibility or a PR merger. (role: source-line author; confidence: high; commits: d8fa2de4d652; files: internal/cmd/appscript_pull.go)
  • Peter Steinberger: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Resolve and document the concurrent-editing contract before advertising unconditional preservation.
  • Add the changelog reference and contributor thanks required by repository policy.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

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

Labels

merge-risk: 🚨 other 🚨 Merging this PR has meaningful risk outside the owned taxonomy. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant