feat(appscript): add push with opt-in prune - #1091
Conversation
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>
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: blocked before merge. Reviewed September 6, 2026, 5:16 AM ET / 09:16 UTC. ClawSweeper reviewWhat this changesAdds 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 Review scores
Verification
How this fits togetherThe 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
Decision needed
Why: Choosing whether this residual remote-data-loss risk is acceptable changes the advertised safety contract and requires maintainer intent. Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest 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:
Overall correctness: patch is correct AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 981ca4a163e8. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
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
appscript push <scriptId> <dir>What changed since #1005
--prune(alias--delete).--prunenames 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-runreports the decision. Without--prunethe request is the whole preview and the dry run stays auth-free. With--pruneit reads the project first, because it cannot name what it would remove otherwise, and emits"prune": truewith the removal set.DirEntry.Typecomes from theReadDirsnapshot and is false for a symlink's target, so each entry is re-checked withroot.Lstatagainst 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 outsidedir.Notes
pull: Apps Script keeps a file's extension in itstype, soCode.gsis sent as{name: "Code", type: SERVER_JS}. Files whose extension has no mapping are skipped rather than guessed at.appsscript.jsonis mandatory —UpdateContentrejects a payload without it, so push fails early with a usage error instead of a provider error.os.ReadDirreturns entries sorted by name.pushis listed asfalsein bothreadonly.yamlandagent-safe.yaml, so a baked build rejects and hides it.One drive-by, called out so you can veto it
appscript contentprinted file names into tab-separated output withoutsanitizeTab, 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 sharedprintAppScriptFileshelper thatcontentandpushboth use and routed it throughsanitizeTab. 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
2. Remove a local file, push again without
--prune— the remote-only file survives3. Both dry runs, neither of which touches the project
4.
--prunenon-interactively refuses without--force, naming the set first5.
--prune --forceprints the removal set, then removes it6. A symlinked entry is refused and nothing is uploaded
Helper.gswas restored locally and pushed back first, so the project is at three files again before this step.Verification
make cigreen locally (fmt, lint, deadcode, Go and JavaScript tests, generated docs, agent skills).go test ./internal/cmd -run 'AppScript|DryRun|SafetyProfile' -count=1.internal/cmd/appscript_sync_test.go. Behaviour: merge default keeps remote-only files,--pruneremoves 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.