feat(docs): add sheet find and replace command

[luoshu0211]

Summary

• expose docs sheet replace as a generated CLI command
• support workbook, worksheet, and inclusive 0-based selection scopes
• document value/formula search, case sensitivity, whole-cell matching, empty replacements, baseVersion concurrency, and result counters
• ship the workflow in the embedded octo-docs skill for cold-start agents

Linked Spec

No matching octo-cli issue was found. Backend contract: https://codex.mlamp.cn/dmwork/octo-docs-backend/-/merge_requests/132
Frontend surface: https://codex.mlamp.cn/dmwork/octo-web-enterprise/octo-docs-module/-/merge_requests/89

How verified

• go test ./...
• go vet ./...
• go build ./cmd/octo-cli
• real CLI to Docker backend replacement returned matchedCells=1, replacedCells=1, replacements=1
• a no-context Agent loaded the embedded skill, performed the scoped replacement, read it back, and cleaned the test cell

COMPREHENSION

1. What does this change actually do to the load-bearing path?
   It registers the backend sheet replacement endpoint in the embedded command schema and documents the concurrency-safe workflow, so agents send one guarded server-side replacement request instead of reconstructing a workbook locally.

2. What could break because of it?
   Registry generation could expose the wrong path or flags, the If-Match token could be omitted, or the embedded skill could omit required scope semantics. The backend endpoint must land before this command is usable outside the local paired environment.

3. How do you know it works?
   Unit and schema tests cover command generation and request shape; a real Docker-backed CLI run and an independent cold-start Agent both completed scoped replace and readback successfully.

[yujiawei]

Code Review — PR #167 (octo-cli)

Verdict: APPROVED

This PR adds a server-side docs sheet replace find-and-replace command. It is a purely spec-driven addition — the command is generated from a new OpenAPI entry in internal/registry/specs/docs.json; there is no hand-written per-endpoint Go logic. Validation (required body fields, findBy enum, the required If-Match header) all flows through the existing generic registry loader + schema walker, exactly as the sibling docs.sheet.edit command does. This is the right architectural choice for a thin CLI and keeps the surface consistent.

Spec / scope compliance

• No over-build. The diff is limited to: one new spec operation, an enum-vocab pin, four focused tests, and doc/skill text. No unrelated flags, endpoints, or behavior changes.
• No under-build. The stated goal (expose the backend replace endpoint, document scopes/concurrency, ship the workflow in the embedded skill) is fully realized.
• Contract fidelity. POST /v1/bot/docs/{docId}/sheet/replace, required: [findString, replaceString], findBy enum [value, formula], and the required If-Match (base-version) header all match the linked backend contract as described. The enum-vocab test (enum_vocab_test.go) pins findBy to value|formula with a reference to the paired backend parseSheetReplaceBody, which guards against future drift.

Security review (flagged security-sensitive)

• Optimistic concurrency is enforced client-side. --base-version maps to the If-Match header and is marked required; TestDocsSheetReplace_RequiresBaseVersion confirms the server is never called when it is missing. This prevents blind overwrites of a stale workbook — the correct behavior for a destructive, whole-workbook mutation.
• baseVersion is not leaked into the request body — the test explicitly asserts it travels only as If-Match. Good.
• No secrets, tokens, or credentials are introduced. x-octo-risk: write is set correctly.
• Empty replaceString (delete semantics) is intentionally allowed per the contract and documented in the skill.
• Point for the human verifier (non-blocking): the range-requires-logicalId rule and the "trim findString, treat replacement literally including $" semantics are enforced server-side only; the CLI does not pre-validate them. This is consistent with how the CLI defers cell-level validation to the backend elsewhere, and the skill documentation states these rules clearly, so it is acceptable — but it is worth a human confirming the backend rejects range without logicalId (a client that sends one relies entirely on the server's 400).

Code quality

• Tests are strong and behavior-focused: atomic request shape + If-Match propagation, required-flag guard, kebab-case flag mapping, and required-body / enum client-side rejection (replaceString required, findBy ENUM_NOT_ALLOWED) with assertions that invalid requests never reach the server.
• Operation counts reconciled. The loader count (docs: 32 → 33) and the tree doc (docs (31) → (33)) now converge on 33; the command-tree doc was previously drifted at 31 and this PR corrects it. CLAUDE.md/README.md op totals updated consistently (322→323 / 32→33).
• Skill docs cover scope rules, value vs formula matched-vs-replaced asymmetry, empty-replacement, and the 412 base_version_stale concurrency path — enough for a cold-start agent to use the command safely.

Verification performed

• go build ./... — clean
• go vet ./cmd/service ./internal/registry ./skills — clean
• go test ./... — all pass, including the four new TestDocsSheetReplace_* cases

Nits (non-blocking)

• The endpoint is unusable until the paired backend MR (octo-docs-backend !132) ships, as the PR body already notes. No action needed here; just a rollout-ordering reminder.

No blocking issues. Approving.

[Jerry-Xin]

Review of head b50e7643c8c67d35abe3098adc64d37bd969a254 (pinned at submit; merge-base with main is 47f4c5f0).

Verdict: APPROVE

The PR registers docs.sheet.replace (POST /v1/bot/docs/{docId}/sheet/replace) in the embedded docs spec and documents it across every doc surface. No hand-written production Go changes — the command is generated from the registry — so the review focused on the spec contract, registry/doc-surface parity, generated behavior, and byte-level cross-verification against the paired backend change.

Contract verification

1. Family parity with docs.sheet.edit — internal/registry/specs/docs.json:580 declares the same optimistic-concurrency shape as the sibling edit op: x-octo-risk: write, required If-Match header promoted to --base-version, docId path param, 200-only responses. The body schema is stricter than the sibling (which has no required list): required: ["findString","replaceString"]. Generated help confirms both flags render with (required) markers and --find-by shows one of: value, formula; range is --data-only, consistent with object-valued fields elsewhere in the family.

2. Backend cross-verification — every semantic claim in the spec description and skills/octo-docs/sheet.md:45-78 was checked against the paired backend merge request (currently open, revision 0bc5f8c0; src/api/routes/docSheet.ts route/parse layer + src/api/services/replaceDocSheet.ts):

   • findString trimmed and required non-empty → 400 invalid_body on the backend; matches the flag description verbatim.
   • findBy accepts exactly value|formula (default value); caseSensitive/matchesTheWholeCell are booleans defaulting false. The provenance note on the enum-vocabulary pin (cmd/service/enum_vocab_test.go:164) is accurate against parseSheetReplaceBody.
   • range without logicalId rejected at the route → matches "range without logicalId is rejected".
   • Value-mode matching of formula cells via cached display value (counted in matchedCells, replaced only in formula mode so replacedCells can be smaller) — exactly the asymmetry sheet.md documents.
   • Literal replacement including $ sequences (function replacer), true→1 / false→0 coercion mirroring the UI provider, rich-text metadata preservation, empty replaceString allowed.
   • Zero-replacement path creates no version/Yjs update and returns the original baseVersion, omitting bytes/newDocVersionSeq — matching the response schema's "present when the sheet changed" notes on both fields.
   • Error envelopes are real: 412 base_version_stale (state-vector compare), 403 protected_range (forwarded from the shared batch-edit service), 413 too_many_cells / cell_too_large (write-limit config), plus 400/422 fail-closed paths.
   • Backend range validation (validateSheetRange, export-only change in the MR) enforces integers, non-negative bounds, start ≤ end, inside the 10000×100 grid — consistent with the cell-grid contract already documented in sheet.md (rows 0..9999, columns 0..99).

3. Registry and count parity — docs.json now carries 33 ops; pinned consistently at internal/registry/loader_test.go:48 (docs=33, cross-domain total 323), contributor guide heading (323 total, line 44) and its command tree (line 83), README domain table (docs 33 + "paged read and atomic replace", line 40), and the search-command-tree snapshot (claim 33, line 14). Raw op total across all specs moves 332→333 merge-base→head (+1, hidden ops untouched). A repo-wide sweep found no stale 322/31/32 count claims.

4. Doc-surface census — repo-wide greps for the new command and field names hit exactly the expected six files; every enumerating surface was updated: tree-shape pin (cmd/service/docs_test.go:42), method+path pin (docs_test.go:83), embedded-skill refCheck (skills/skills_test.go:59), skill seed routing + schema list (skills/octo-docs/SKILL.md:60,116), sheet.md section + schema footer (sheet.md:45-78,607). The routing-table row SKILL.md:24 does not add find/replace to its capability list, but that row is already selective (it also omits hyperlinks, merges, tabs, comments) — consistent with the existing convention, non-blocking.

Test and mutation evidence

• Baselines: at merge-base 47f4c5f0 and at head, go build ./..., go vet ./..., go test -race -count=1 ./... are green in all 12 packages. Top-level test-function delta in cmd/service is exactly +4 (the new TestDocsSheetReplace_* family); internal/registry and skills counts unchanged (pin edits only).
• New tests verified executing: atomic request shape + If-Match header + baseVersion kept out of the body; missing --base-version → required-flag error with zero server calls; kebab-case flag promotion with an exact-body assertion (no default injection); missing replaceString and out-of-enum findBy rejected pre-transport with zero server calls (docs_test.go:1236-1237).
• Mutation battery, 7 mutations, each applied then reverted: loader pin 33→32 → red; drop findBy enum from the spec → enum-vocabulary pin red; If-Match required→false → base-version-required test red; drop replaceString from body required → validation test red; rename spec path → registry-shape red; strip the "docs sheet replace" phrase from sheet.md → embedded-skill refCheck red. The 7th (weakening the tree-shape group map from {get,edit,replace} to {get,edit}) survived at TestDocs_TreeShape because that comparison is subset-only by pre-existing design; exactness lives one layer up: TestDocs_RegistryShape asserts the exact docs op count against its case map, and deleting the op from the spec turns four tests red at once (tree shape, registry shape, loader count, enum vocabulary). No guard gap for this change.
• Runtime probes against the built binary (dry-run, no network): empty replacement is accepted and forwarded as "replaceString": "" through both the flag path and the --data path, so the advertised delete-by-empty-replacement recipe works end to end; empty --base-version "" is loudly rejected (the concurrency gate cannot be silently defeated); unknown body fields and negative range coordinates pass through the CLI and are ignored/rejected fail-closed by the backend (400) — both are the pre-existing docs-family looseness (no docs op declares additionalProperties:false; the generator does not enforce numeric bounds client-side for any docs op), not introduced here.

💬 Non-blocking

• 🔵 Test comment at cmd/service/docs_test.go:1183 labels the pinned range (startColumn:2..endColumn:4) as "B2:E4"; 0-based column 2 is C, so the comment should read C2:E4. The sheet.md C2:E10 example (rows 1..9, columns 2..4) is correct. Comment-only; assertions unaffected.
• 🔵 The search-command-tree snapshot's claim (33) now matches the real registry count, but its enumeration lists 32 leaves because docs search has been missing from the "直接" row since before this PR (pre-existing; at merge-base the claim was 31, stale in the other direction). Worth adding search to that row in a follow-up.
• 🔵 Consider pinning transport coverage for an empty --replace-string "" (behavior verified by probe) so the documented empty-replacement recipe stays guarded against future gate changes.
• 🔵 caseSensitive/matchesTheWholeCell render with empty help descriptions (precedent exists: summary.create's include_archived); one-line descriptions would polish the generated help.
• 🔵 Merge ordering: the paired backend MR is still open (targeting the backend's test branch); until it lands, this command 404s outside paired environments. The PR description already flags this — keep the ordering discipline previous sheet PRs declared ("merge after the backend MR").

Highlights

• The spec description is unusually precise: every claim — including the subtle value-mode formula-cell asymmetry and the zero-replacement no-version-bump behavior — was verifiable against backend bytes.
• Zero-HTTP-call rejection tests follow the established fail-closed CLI-boundary discipline.
• The enum-vocabulary entry carries accurate, checkable provenance.

[mochashanyao]

[Octo-Q · automated review]

Verdict: Request changes — blocking findings below (data-flow traced).

---

Code Review — PR #167 (octo-cli)

Reviewer: Octo-Q (automated review)

Summary

This PR adds one generated command, octo-cli docs sheet replace, by declaring POST /v1/bot/docs/{docId}/sheet/replace (docs.sheet.replace) in the embedded docs spec, plus the agent-facing documentation for it in the embedded octo-docs skill and the usual index/count updates. There is no engine code change: the leaf, its promoted flags (--find-string, --replace-string, --find-by, --case-sensitive, --matches-the-whole-cell, --logical-id), the required --base-version → If-Match header, the findBy enum gate and the findString/replaceString required gate all come from the existing metadata-driven path, and four new tests drive the real cobra tree against a stub server. The spec work itself is careful and symmetric with docs.sheet.edit; my one blocker is about rollout, not about the wire contract.

Verification

Static analysis only at head b50e7643; build and tests not executed in this environment.

• ✅ Operation counts reconcile — 33 docs.* operationIds in internal/registry/specs/docs.json, matching internal/registry/loader_test.go:48 (32→33) and README.md:40; CLAUDE.md:44's 322→323 equals the sum of the per-domain counts asserted in that test.
• ✅ Flag names are what the docs and tests claim — every promoted body property carries an explicit x-octo-flag, which matters because the derivation in cmd/service/flags.go only dashifies underscores, so findString would otherwise surface as --findString. None collides with reservedFlagNames, and cmd/service/flagcollision_test.go walks the whole registry, so the new op is covered without edits.
• ✅ No schema default leaks into the request body — findBy/caseSensitive/matchesTheWholeCell declare defaults, but body flags register with zero values and applyBodyFlags (cmd/service/run.go:729) merges only Changed flags. TestDocsSheetReplace_UsesKebabCaseFlags pins this with reflect.DeepEqual on the exact six-key body, and the --data-only test proves caseSensitive:true survives unset flags.
• ✅ Empty replaceString is not rejected locally — validateRequiredProperties (cmd/service/run.go:997) checks presence/non-nil rather than non-empty, and rejectEmptyRequiredValue is wired to query and header params only, so the documented delete-by-empty-replacement flow does reach the wire.
• ✅ Concurrency guard is symmetric with docs.sheet.edit — identical If-Match header param shape (x-octo-flag: base-version, required: true), so --base-version is cobra-required, --base-version "" is refused, and the token stays out of the JSON body.
• ✅ Retry cannot double-apply a replacement — isRetryableStatus (internal/client/client.go:1302) retries only 429/502/503/504 and is method-agnostic, but a successful replace mints a new baseVersion, so a replayed request carries a stale If-Match and 412s instead of applying twice.
• ✅ minimum: 0 on the four range fields is inert by design — schemaInfoFromNode (internal/registry/loader.go:903) has no minimum/maximum field and extended constraints are enforced only for Loop / x-octo-strict-request-schema services, the same convention as file.json and loop.json. The bound is backend-enforced and the parent description still says "inclusive 0-based".
• ✅ Nested range is validated on the --data path — range is an object, so promotableKind gives it no flag and it can only arrive via --data, which the merged-body walker checks at depth including its four required coordinates.
• ✅ Error gates are already documented for agents — 412 base_version_stale, 403 protected_range, 413 too_many_cells / cell_too_large are carried by sheet.md's shared "Concurrency / errors" section (skills/octo-docs/sheet.md:552), so the new op needs no separate gate list, and backendErrorMapping maps none of the docs sheet codes today — no parity gap in internal/output/errors.go.
• ✅ Skill embedding and schema lookup — skills/octo-docs/sheet.md already rides the //go:embed */*.md glob, skills/skills_test.go:59 gained the "docs sheet replace" needle, and octo-cli schema docs.sheet.replace was added to the lookup block.

Findings

One P1 blocker; four P2 items and one nit.

P1 — New route ships with no rollout gate and no recorded backend dependency (internal/registry/specs/docs.json:580)

The leaf is generated unconditionally from this spec and released through npm/goreleaser the moment a tag is cut, while skills/octo-docs/sheet.md:47 now tells agents to use the server-side replace "instead of downloading the sheet and rewriting cells locally" — i.e. the embedded skill actively retires the working docs sheet get + docs sheet edit path for this task. Nothing in the repo records that the paired backend route must be live first. If it is not live in an environment, every call answers 404, and internal/output/errors.go:337 + :357 classify that as type=api_error / code=NOT_FOUND with the mapped hint "resource not found" (internal/output/errors.go:122), so the agent is pointed at a bad docId rather than at a CLI-newer-than-backend mismatch, with the fallback already talked out of it. The repo has both halves of the fix already: CHANGELOG.md:199 records a "Minimum rollout dependency: release only after … merged and deployed" for exactly this class of change, and the loader honours a per-op "x-octo-cli-hidden": true (internal/registry/loader.go:442, used throughout loop.json) that keeps octo-cli schema docs.sheet.replace introspectable while withholding the leaf.

// smallest gate, if the backend route is not yet deployed everywhere:
"/v1/bot/docs/{docId}/sheet/replace": {
  "post": {
    "operationId": "docs.sheet.replace",
    "x-octo-cli-hidden": true,
    "x-octo-risk": "write"
  }
}

Fix: either add the [Unreleased] → Added CHANGELOG entry naming the backend MR as a minimum rollout dependency, or set x-octo-cli-hidden until the route is deployed and flip it in a one-line follow-up; and add one fallback sentence to sheet.md so a 404 on this route sends the agent back to docs sheet get + docs sheet edit instead of failing the task. If the route is already deployed everywhere the CLI ships, stating that with the deployed version reduces this to the CHANGELOG note — the note is still the in-repo signal the next release cutter needs.

P2 — Documented empty-replaceString delete path is untested on both input paths (cmd/service/docs_test.go:1230)

The spec says "An empty replaceString is allowed" (internal/registry/specs/docs.json:584) and skills/octo-docs/sheet.md:65 makes --data '{"findString":"obsolete","replaceString":""}' the canonical way to delete matching text workbook-wide, but no test sends an empty replaceString through either --data or the promoted --replace-string flag. It works today only because the required-field gate checks presence/non-nil and because the empty-value refusal at cmd/service/run.go:360 is applied to query and header params but not to body flags — and cmd/service/run.go:344 documents that this guard has been widened before (for --space-id and --base-version). If it is ever extended to required body fields, the documented delete flow starts failing locally. Fix: add two cases pinning that replaceString:"" passes the local gate and reaches the wire as "" via --data and via --replace-string "".

P2 — New capability is missing from the agent-facing routing surface (skills/octo-docs/SKILL.md:60)

The routing row that decides whether a cold-start agent opens sheet.md (skills/octo-docs/SKILL.md:24) still enumerates only "cells, formulas, styles, layout, floating images, freeze panes, shared filters, sorting, data validation/dropdowns, paged reads, xlsx export", and the sheet.md intro (skills/octo-docs/sheet.md:3) omits find/replace too, so a task phrased as "find and replace text in this spreadsheet" has no keyword to route on; the capability needles at skills/skills_test.go:49 were not extended either, so nothing pins it the way freeze panes / shared filters / sorting are pinned. Separately, sheet.md's new section omits a rule the spec documents at internal/registry/specs/docs.json:613 — findString is trimmed and must be non-empty after trimming — so --find-string " " yields a bare 400 invalid_body with no local explanation. Fix: add "find & replace" to the routing row, the sheet.md intro and the skills_test capability list, and one clause to sheet.md's scope-rules paragraph stating the trim/non-empty rule.

P2 — Command-tree count no longer matches its own enumeration (docs/octo-cli-search-command-tree.md:14)

The header was raised from (31) to (33), which is correct against the registry, but the leaves listed on :15-:23 sum to 32 because the 直接 row still omits docs search. Before this change header and list agreed (both stale by one); after it the document contradicts itself, and it is the one place a reader cross-checks the per-domain count. Fix: add search to the 直接 row so the enumeration sums to 33.

P2 — Formula-mode example is a substring-rewrite hazard with no caveat (skills/octo-docs/sheet.md:57)

The canonical example pairs "findBy":"formula" with "matchesTheWholeCell":false and SUM → AVERAGE, which is a substring match over formula source: on a real sheet it also rewrites SUMIF → AVERAGEIF and SUMPRODUCT → AVERAGEPRODUCT (the latter is not a function), so a copied example can corrupt formulas. Whole-cell matching cannot narrow this, because in formula mode it would require the entire formula text to equal SUM. sheet.md warns about precisely this class of hazard elsewhere (skills/octo-docs/sheet.md:148 — the server stores f verbatim and does not re-anchor relative references), so the omission reads as an oversight. Fix: add one sentence noting that formula-mode matching is substring matching over formula text, so a short find string also hits longer function names — pick an unambiguous find string or keep the selection narrow, and read the formulas back afterwards.

Nit — Assertion message says B2:E4 for column-C coordinates (cmd/service/docs_test.go:1183)

startColumn: 2 is column C under the 0-based convention this PR documents (skills/octo-docs/sheet.md:57 renders startRow:1, startColumn:2, endRow:9, endColumn:4 as C2:E10), so {1,2,3,4} is C2:E4, not "B2:E4". Harmless at runtime, but it is the text a maintainer reads when the assertion fires and it contradicts the skill doc's own worked example. Fix: s/B2:E4/C2:E4/.

Human-verify

1. Whether the paired backend route POST /v1/bot/docs/{docId}/sheet/replace is merged and deployed to every environment a released octo-cli talks to. Not verifiable from this checkout; the P1 is written so that a "yes, deployed since <version>" answer downgrades it to the CHANGELOG note. Not a merge blocker beyond the P1 itself.
2. Backend semantics the spec asserts and this repo cannot prove: findString trimming, matchedCells vs replacedCells divergence for cached formula display values, true/false searching as 1/0, cell-metadata preservation, literal treatment of $ sequences in replaceString, and the no-op case returning the original baseVersion. Not a merge blocker — flagging the epistemic scope; cmd/service/enum_vocab_test.go:164 already pins findBy to value|formula against the backend parser.

数据流回溯 (data-flow trace)

• --base-version → headerFlag{apiName:"If-Match"} registered from the spec's header param (cmd/service/flags.go:168) → buildHeaders (cmd/service/run.go:541) emits it only when Changed, after rejectEmptyRequiredValue → request header → backend concurrency guard. Really flows (test asserts If-Match == "BV_ABC==" and that baseVersion is absent from the body).
• --data → cmdutil.ParseInput (inline / @file / @-) → Decoder.UseNumber → base map[string]any → applyBodyFlags merges Changed promoted flags over it → applyGeneratedIdempotencyKey (no-op: the op declares no x-octo-auto-idempotency-key) → validateRequiredBodyFields → recursive enum/required walk → client.Request body → POST. Really flows; the two edges (empty string, out-of-vocabulary findBy) were traced to their gates rather than assumed.
• Promoted flags → bodyFlag.apiName is the wire property name regardless of the flag alias, so --find-by formula lands as "findBy":"formula". Really flows (pinned by reflect.DeepEqual on the exact body).
• range → not promotable → --data-only → nested required enforced by the walker; its minimum: 0 reaches no consumer (dropped at internal/registry/loader.go:903) → backend-only, consistent with repo convention.
• Response counters (matchedCells / replacedCells / replacements / baseVersion / bytes / newDocVersionSeq) → generic envelope, no x-octo-response-fields or lossless-id declaration, identical to docs.sheet.edit; nothing in this repo reshapes them, so the documented semantics are backend-truth claims (Human-verify 2).
• Skill text (skills/octo-docs/sheet.md:45) → //go:embed */*.md → octo-cli skills octo-docs writes it out (cmd/skills.go:232) → consumed by cold-start agents. This is the one consumed datum whose upstream is a deployment state the checkout cannot prove → the P1.

盲点 checklist (C1–C6)

• C1 双路径 parity — hit (1 P1, 3 P2). Pairs checked: get↔edit↔replace (replace reuses get's token and edit's If-Match shape ✅); spec↔generated flags ✅; spec description↔sheet.md ✗ (P2 above); count indexes — CLAUDE.md / README.md / loader_test.go ✅, command-tree enumeration ✗ (P2); skill routing↔capability ✗ (P2); rollout gate↔new backend route ✗ (P1). Guard self-equivalence was verified by reading the guards, not trusting the claim: flagcollision_test.go and enum_vocab_test.go are registry-wide/table-driven so they cover the new op unedited, whereas skills_test.go needles are hand-listed and were extended for the command but not for the capability.
• C2 control-flow ordering / 等价写法 — clear. No new ordering; the op reuses the single generic path. Non-canonical forms tried against the local gates: --find-string " old " (passes locally, trimmed server-side — documented in the spec, missing from sheet.md), --find-by VALUE / Formula (rejected locally, ENUM_NOT_ALLOWED, case-sensitive canonical compare, matching the pinned backend vocabulary), --data with a missing range coordinate (rejected by the nested required walk), --data null / trailing garbage (rejected by the generic parser), --replace-string "" (accepted → the documented delete), --base-version "" (refused). No regex/escape/sanitize control is added, so there is no new canonicalization-bypass surface.
• C3 授权边界 ≠ 能力边界 — clear, with a note. The new capability is the widest-blast-radius write in the docs domain: sheet edit requires naming explicit cell keys, while replace defaults to every match in every worksheet when logicalId and range are both omitted. Reachability is unchanged from the rest of /v1/bot/docs/* — docs declares no x-octo-allowed-token-kinds / x-octo-mount-by-token-kind, so the CLI makes no authorization decision; writer-role and protected-range enforcement are server-side and both are documented ((needs writer) in the summary, 403 protected_range in the description). The checks are not absent, they are simply not in this repo (Human-verify 1).
• C4 授权生命周期 / 容器-成员级联 — N/A. No membership, status, or container query changes and the CLI holds no authorization state. The one cascade that does apply — a protected range inside the target scope gating the whole write — is documented as 403 protected_range and is already carried by sheet.md's shared errors section (skills/octo-docs/sheet.md:552).
• C5 build/note ≠ 运行期路径 — hit → P1. "Tests pass" and the PR description's own warning were not accepted as mitigation. Runtime推演: the leaf is built at startup from the embedded spec (//go:embed specs/*.json, no per-op build step) and the request is POST {OCTO_API_BASE_URL}/v1/bot/docs/<docId>/sheet/replace with If-Match, proven by a test that executes the real cobra tree. The unproven link is the server end of that path, and the failure surfaces as NOT_FOUND / "resource not found", which misdirects the agent to the docId.
• C6 治理/策略/文档自洽性 — hit → 3 P2 + 1 nit. Cross-checked the new text against every in-repo index and the sibling spec: counts reconcile (33 / 323), the tree doc's enumeration no longer sums to its own header, the routing row and sheet.md intro omit the capability, the formula example contradicts the substring semantics documented in the same section, and the assertion message contradicts sheet.md's worked example. No SECURITY.md / disclosure / label-policy surface is touched.

跨轮 blocker 复检 (R6)

N/A — first review round against head b50e7643. This issue has no prior comments (Octo-Q issue comment list returned []), and prior GitHub review threads were deliberately not read (checkout-only rule). If an earlier round exists on GitHub, its unresolved blockers still need re-verification against this head by the final reviewer.

Verdict: CHANGES_REQUESTED

One P1: a newly exposed workbook-wide write command ships with no rollout gate and no in-repo record of its backend dependency, while the embedded skill steers agents off the docs sheet get + docs sheet edit path that works today — and the resulting 404 is reported as NOT_FOUND / "resource not found", pointing at the wrong cause. Everything else about the change is solid: the wire contract, flag surface, enum and required gates, If-Match symmetry, retry safety and the count/index updates all check out, and the four P2s plus the nit are one-line doc and test fixes. Recording the rollout dependency (CHANGELOG entry, or x-octo-cli-hidden until the route is live) plus one fallback sentence in sheet.md is sufficient to clear the blocker; if the backend route is already deployed everywhere the CLI ships, say so with the version and the P1 reduces to the CHANGELOG note.

[Octo-Q] verdict: REQUEST_CHANGES — 存在 1 个 P1：新命令 docs sheet replace 无 rollout gate / 无仓库内后端依赖记录，同时 skill 文案把 agent 从可用的 sheet get+sheet edit 路径引开；后端未部署时 404 被 CLI 归类为 NOT_FOUND / "resource not found"，误导排查方向（R1：让本来能工作的路径在生产中不可用 + C5：note 不是修复）。按 R4，有 P1 即 REQUEST_CHANGES，不因修复成本低而降级。另有 4 个 P2 + 1 nit，均为文档/测试一行级修复。P1 置信度：中-高（仓库内先例证据充分：CHANGELOG.md:199 的 "Minimum rollout dependency" 与 x-octo-cli-hidden；唯一不可从 checkout 验证的是后端路由是否已在所有环境部署）。

[yujiawei]

Follow-up (non-blocking) — confirmed suggestions

My approval stands: the wire contract, flag surface, enum/required gates, If-Match symmetry, retry safety, and count/index updates all verify. The items below are confirmed improvements but are doc/test one-liners, not merge blockers — none is a correctness, security, or build defect.

1. Skill retires the working path with no fallback (recommended). skills/octo-docs/sheet.md:47 now tells agents to use replace "instead of downloading the sheet and rewriting cells locally." Until the paired backend route is deployed, a call 404s and the CLI (like every docs op) reports it as a generic NOT_FOUND / "resource not found", which points at a bad docId rather than a CLI-newer-than-backend mismatch. Add one sentence: on 404, fall back to docs sheet get + docs sheet edit. Also worth an [Unreleased] → Added CHANGELOG note recording the backend MR as the minimum rollout dependency (there's precedent in this file for the canonical-create change).

2. Command-tree doc now self-contradicts. docs/octo-cli-search-command-tree.md:14 header is correctly bumped to (33), but the enumerated leaves sum to 32 — the 直接 row omits docs search (a pre-existing gap that the header bump now makes visible). Add search to the 直接 row so it sums to 33.

3. Formula-mode example is a substring-rewrite hazard. sheet.md:57 pairs findBy:formula + matchesTheWholeCell:false with SUM→AVERAGE; on a real sheet that also rewrites SUMIF→AVERAGEIF and SUMPRODUCT→AVERAGEPRODUCT. Add a caveat that formula matching is substring-based, so a short find string also hits longer function names.

4. Documented empty-replaceString delete flow is untested. The spec allows it and the skill makes it canonical, but no test sends replaceString:"" via either --data or --replace-string. Add two cases pinning that it passes the local gate and reaches the wire as "".

5. Capability not on the routing surface. SKILL.md:24 routing row and the sheet.md intro don't mention find/replace, and skills_test.go capability needles weren't extended — a "find and replace text" task has no keyword to route on. Also add the spec's trim/non-empty findString rule to sheet.md's scope paragraph.

Nit: cmd/service/docs_test.go:1183 assertion message says B2:E4 but startColumn:2 is column C under the 0-based convention the PR documents — should read C2:E4.

[Jerry-Xin]

Review of head fb408024193194a4cb2829f00414cf52936aec56 (pinned at submit; merge-base with main is 47f4c5f0, unchanged from the prior round).

Verdict: APPROVE

Delta characterization (vs previously approved head b50e7643c8c6)

The new head is an amendment of the same single commit on the same base (both have parent 47f4c5f0), so this round is a pure review-response delta: 7 files, +70/−19, zero production-Go changes, no rebase noise. Registry contract bytes, flag surface, and wire shape verified last round are unchanged except two added description fields in internal/registry/specs/docs.json (caseSensitive, matchesTheWholeCell); all other prior-round adjudications carry by construction (unchanged bytes).

What the delta does:

1. CHANGELOG.md:11-16 — new [Unreleased] → Added entry recording the Minimum rollout dependency (release only after octo-docs-backend MR !132 is merged and deployed; 404 fallback on mixed-version environments), following the established precedent wording at CHANGELOG.md:205 (canonical-create).
2. skills/octo-docs/sheet.md — rewrites the find-and-replace guidance: "Prefer … when the endpoint is available" replaces "Use … instead of"; adds the 404 fallback sentence (docs sheet get + explicit docs sheet edit batch); adds the trim/non-empty rule, formula substring-matching caveat, value-mode raw-value note, whole-cell vs rich-text behavior, and literal $ semantics; fixes the formula example from SUM→AVERAGE to =SUM(→=AVERAGE(.
3. cmd/service/docs_test.go — adds TestDocsSheetReplace_AllowsEmptyReplacement (pins explicit-empty replaceString reaching the wire via both --data and --replace-string ""); fixes the B2:E4→C2:E4 assertion message.
4. internal/registry/specs/docs.json — help descriptions added to the two boolean flags.
5. docs/octo-cli-search-command-tree.md — adds search to the 直接 row so the enumeration sums to 33, matching the (33) header.
6. skills/octo-docs/SKILL.md + skills/skills_test.go — "find & replace" routing keyword added; new pins for "mixed-version environment", "trims findString", "substring matching over formula source text".

Adjudication of the outstanding CHANGES_REQUESTED review (5165466714, at b50e7643, mochashanyao)

• P1 (rollout gate / no recorded backend dependency / skill retires the working path / 404 surfaces as NOT_FOUND) — ADDRESSED. The review stated its own clearance bar: a CHANGELOG entry naming the backend MR as a minimum rollout dependency plus one fallback sentence in sheet.md "is sufficient to clear the blocker". The delta implements both halves: CHANGELOG.md:13-16 (matching the precedent format at :205 byte-for-byte in style) and sheet.md:47-51 ("Prefer … when the endpoint is available" + explicit 404→fallback mapping), the latter pinned by skills_test.go:59 ("mixed-version environment") so it cannot silently regress. x-octo-cli-hidden was not used — it was offered as the alternative branch, not both. The misleading-404 concern (internal/output/errors.go classifying it as NOT_FOUND/"resource not found") is mitigated as far as this repo can: the agent-facing doc now states the 404→fallback rule explicitly. The "already deployed everywhere" downgrade branch does not apply: the backend MR is still open (see process note).
• P2 (empty replaceString untested on both input paths) — ADDRESSED. TestDocsSheetReplace_AllowsEmptyReplacement covers both --data and the promoted flag and asserts an explicit "" in the request body. Executed: both subcases pass; mutation-killed (below).
• P2 (capability missing from routing surface + trim rule missing from sheet.md) — ADDRESSED. SKILL.md description + routing row, sheet.md intro, and the skills_test capability needle all gained "find & replace"; the trim/non-empty rule is at sheet.md:76, pinned by skills_test.
• P2 (command-tree enumeration summed to 32 vs header 33) — ADDRESSED. search added; enumeration hand-verified to sum exactly 33 (7+2+3+3+3+2+4+6+3), and docs.search genuinely exists (docs.json:100). Note: this snapshot doc is not pinned by any test (no Go file references it) — verified manually this round.
• P2 (formula-mode example substring-rewrite hazard) — ADDRESSED. Caveat added at sheet.md:76-79 and the example itself is now unambiguous: =SUM( is not a substring of =SUMIF( or =SUMPRODUCT(, so the canonical example can no longer corrupt longer function names.
• Nit (B2:E4 → C2:E4) — ADDRESSED (docs_test.go:1183).

yujiawei's non-blocking follow-up comment (5165623424) listed the same six items (fallback sentence + CHANGELOG note, tree sum, formula caveat, empty-replacement test, routing keyword, B2:E4 nit) — all addressed per the above; their approval at the old head was dismissed by the push.

New-byte verification (fresh eyes on the delta)

Backend cross-validation against the paired MR's CURRENT head (!132 @ 9e9a1194, force-updated 2026-09-10, still OPEN, targets test): every semantic claim added to sheet.md/docs.json was checked against backend bytes:

• trim + non-empty: route parse (docSheet.ts:705-706) → 400 invalid_body ✓ ("The server trims findString, which must remain non-empty").
• value mode = stored raw values, not formatted display text: searchableValue() returns rich-text dataStream / String(v) for numbers / '1'/'0' for booleans / string v ✓ ("booleans are 1 and 0").
• formula mode = substring matching over formula source: candidate is cell.f, matched via substring regex ✓ (the SUM→SUMIF hazard is real; caveat accurate).
• formula cells in value mode count into matchedCells via the cached value but are never replaced (replacedCell returns null unless findBy=formula) ✓ (kept asymmetry sentence still accurate).
• whole-cell vs rich-text: ^…$-anchored equality against a stream that contains paragraph terminators → does not match ✓ (docs.json description accurate).
• literal $: rebuild joins parts rather than String.replace, keeping $&/$1/$$ literal ✓.
• zero replacements: early return with the original baseVersion and no edit call → no version/Yjs bump; response omits bytes/newDocVersionSeq ✓.
• caseSensitive/matchesTheWholeCell default false ✓ (route ?? false).

Executable evidence (at head; local go1.26.0 darwin/arm64, go.mod pins go 1.24.0, CI ran 1.24.x):

• gofmt clean; go vet ./... clean; go build ./cmd/octo-cli OK.
• go test -race -count=1 ./... — 12/12 packages green at head. Merge-base suite carries from the prior round (same base 47f4c5f0, green there).
• Test delta exactly accounted: 796 → 801 test functions repo-wide vs merge-base (+5 = four from the prior round + one new this round).
• Mutation battery 5/5 killed: (1) rephrase "mixed-version environment" in sheet.md → skills test red; (2) drop the "trims findString" phrase → red; (3) rename "find & replace" in SKILL.md → capability check red; (4) widen the required-body gate to reject empty strings (the exact regression the review warned about) → both new-test subcases red; (5) make applyBodyFlags drop empty-string values → promoted-flag subcase red with precise fault localization (--data path unaffected).
• Dry-run probes: empty --base-version still loudly rejected (concurrency gate intact); --replace-string "" lands on the wire as explicit "replaceString": ""; whitespace-only --find-string " " passes the local gate and is forwarded for the server to trim→400, matching the documented division of responsibility.
• Registry/doc-surface census re-run: 33 docs operations (docs.json = loader pin loader_test.go:48 = README:40); the contributor guide's 323 total intact; repo-wide grep for the new names hits exactly the 7 expected files (prior 6 + CHANGELOG); zero stale counts.

💬 Non-blocking observations

1. matchesTheWholeCell help says "require the complete stored value to match", while the backend trims leading/trailing spaces (not line breaks) before whole-cell comparison (Univer parity). Consider "ignores leading/trailing spaces" in a follow-up.
2. range without logicalId is not refused locally (backend 400s it). Fine for a forwarding CLI; an optional local pin test would guard a future strict-schema switch.
3. CI: check-sprint fails (no Closes #<issue> reference — the PR body states none was found, while the sprint gate requires one; maintainer process decision) and scan-pr/osv-scan fails — both reproduce identically at the prior head and on unrelated open PR #165, and go.mod/go.sum are untouched by this PR → org-wide process gates, not introduced by this delta. build & test and npm test pass at head.
4. SKILL.md keeps version 0.2.0 (the pre-PR value); no documented bump convention and no test impact.

Maintainer process note

• Backend MR !132 is still OPEN (squashed/force-updated today; its own review line is being re-adjudicated separately). The in-repo rollout-dependency record now exists, which is exactly what P1 asked for — release cutters have the signal. Do not cut a release containing this command until !132 is merged and deployed; the CHANGELOG entry says so verbatim.
• Peer vote state after the push: both approvals at b50e7643 (yujiawei, Jerry-Xin) were dismissed by the push; the CHANGES_REQUESTED review at b50e7643 is the only live stateful peer review. This head implements that review's own stated clearance bar; clearing or re-reviewing it at this head is its author's call — not dismissed from this side.

This review pins head fb408024; the verdict covers the delta plus the carried-forward contract verification itemized above.

[yujiawei]

Code Review — PR #167 (octo-cli)

Verdict: APPROVED

This PR adds a server-side octo-cli docs sheet replace find-and-replace command. It is a purely spec-driven addition: the command is generated from a new OpenAPI entry in internal/registry/specs/docs.json, with no hand-written per-endpoint Go logic. Validation (required body fields, the findBy enum, the required If-Match header) flows through the existing registry loader + schema walker, exactly as the sibling docs.sheet.edit command does — the right architecture for a thin CLI.

This head resolves every blocking and non-blocking item raised in the prior review round (against the now-superseded head). Details below.

Spec / scope compliance

• No over-build. The diff is limited to one new spec operation, an enum-vocab pin, five focused tests, and doc/skill text. No unrelated flags, endpoints, or behavior changes.
• No under-build. The stated goal (expose the backend replace endpoint, document scopes/concurrency, ship the workflow in the embedded skill) is fully realized.
• Contract fidelity. POST /v1/bot/docs/{docId}/sheet/replace, required: [findString, replaceString], findBy enum [value, formula], and the required If-Match (base-version) header all match the paired backend contract. enum_vocab_test.go pins findBy to value|formula with a reference to the backend parseSheetReplaceBody, guarding against future drift.
• Count/index parity. docs op count reconciles at 33 across loader_test.go, README.md, CLAUDE.md (322→323), the command-tree doc header, and the search-command-tree snapshot. The command-tree 直接 row now includes search, so its enumeration sums to 33 (a self-contradiction flagged previously and now fixed).

Prior-round findings — all addressed

• Rollout gate / backend dependency (was P1). CHANGELOG.md now records a "Minimum rollout dependency: release only after octo-docs-backend MR !132 is merged and deployed" entry, and skills/octo-docs/sheet.md documents the 404-in-mixed-version fallback to docs sheet get + docs sheet edit. The working path is no longer retired without an escape hatch.
• Empty-replaceString delete flow (was P2). TestDocsSheetReplace_AllowsEmptyReplacement now pins that replaceString:"" passes the local gate and reaches the wire as "" via both --data and --replace-string.
• Routing surface (was P2). SKILL.md's routing row, the sheet.md intro, and the skills_test.go capability needles all now advertise "find & replace"; the findString trim/non-empty rule is documented in the scope paragraph.
• Formula-mode substring hazard (was P2). sheet.md now warns that formula matching is substring-based, so SUM also hits SUMIF/SUMPRODUCT; it advises an unambiguous find string or a narrow selection and a readback.
• Assertion-message nit (was nit). The range test comment now reads C2:E4 (0-based column 2 = C), matching the documented convention.

Security review (flagged security-sensitive)

• Optimistic concurrency is enforced client-side. --base-version maps to the required If-Match header; TestDocsSheetReplace_RequiresBaseVersion confirms the server is never called when it is missing, and an empty --base-version "" is refused. This prevents blind overwrites of a stale workbook — correct for a destructive, potentially whole-workbook mutation.
• baseVersion does not leak into the body — asserted to travel only as If-Match.
• Retry safety. A successful replace mints a new baseVersion, so a replayed request carries a stale If-Match and 412s rather than double-applying. Retry classification is limited to 429/5xx.
• No secrets, tokens, or credentials are introduced; x-octo-risk: write is set.
• Blast radius, for the human verifier (non-blocking): with both logicalId and range omitted, replace defaults to every match in every worksheet — the widest write in the docs domain. This is documented ((needs writer) summary, 403 protected_range), and writer-role + protected-range enforcement are server-side. The CLI makes no authorization decision here, consistent with the rest of /v1/bot/docs/*. Worth a human confirming the backend enforces writer role and protected-range refusal on this route, and that range-without-logicalId is rejected server-side (the CLI does not pre-validate this).

Test & verification

• CI: build & test (1.24.x) and npm test are green at this head; the five TestDocsSheetReplace_* cases cover atomic request shape + If-Match propagation, required-flag guard, kebab-case flag mapping with an exact-body assertion, empty-replacement on both input paths, and client-side required-body / enum rejection with zero server calls on invalid input.
• Two CI checks are red — scan-pr / osv-scan and check-sprint. Neither is attributable to this diff: the PR changes no dependency manifest (go.mod/go.sum/package*.json untouched), so the OSV finding is a pre-existing repo-wide vulnerability, and check-sprint is a process/metadata gate unrelated to the code. These should be triaged separately and are not a reason to block this change; a human should confirm the OSV finding is tracked independently.

Nits (non-blocking)

• caseSensitive / matchesTheWholeCell render with empty help descriptions in generated help (precedent exists elsewhere); one-line descriptions would polish the UX. No functional impact.

No blocking correctness, security, or build defects in this diff. Approving. The endpoint remains unusable until the paired backend MR ships, which the CHANGELOG and skill now correctly gate.

[mochashanyao]

[Octo-Q · automated review]

Verdict: Approve — no blocking findings; notes below (data-flow traced).

---

Code Review — PR #167 (octo-cli)

Summary

This PR exposes the backend's sheet find-and-replace endpoint as a generated CLI leaf: one new operation in internal/registry/specs/docs.json (docs.sheet.replace, POST /v1/bot/docs/{docId}/sheet/replace), five new tests plus tree/shape/vocabulary updates, the docs operation count bumped 32→33 (total 322→323), and a new "Find and replace" section in the embedded octo-docs skill. No production Go code changes — the leaf is entirely spec-driven, so the review surface is the contract, its generated wiring, and the agent-facing prose that ships with it. I traced that contract from spec to wire and found it sound; the two items below are guidance and metadata completeness, not correctness.

Verification

Static analysis only at head fb408024193194a4cb2829f00414cf52936aec56; build and tests not executed in this environment.

• ✅ --base-version really reaches the POST as If-Match — registerHeaderFlags (cmd/service/flags.go:169) binds the in: header param and marks it required; buildHeaders (cmd/service/run.go:541) is method-agnostic and emits it whenever the flag was set, and rejectEmptyRequiredValue refuses --base-version "". The block is shape-identical to docs.sheet.edit and docs.scene.edit.
• ✅ Promoted flags and --data merge correctly, including the empty replacement — applyBodyFlags (cmd/service/run.go:729) writes only Changed flags, so --replace-string "" lands as an explicit replaceString: "" rather than being dropped as a zero value, and baseVersion never enters the body.
• ✅ range is --data-only by construction, and its four keys are still gated locally — promotableKind (cmd/service/flags.go:408) admits no object kind, so no --range flag exists (matching the documented usage), while validateRequiredProperties (cmd/service/run.go:991) rejects a typo'd startCol before any HTTP.
• ✅ Required-body and enum gates fire pre-flight — requestBody.required plus RequestBodyRequired mean a bare docs sheet replace d1 --base-version BV fails locally naming findString/replaceString, and findBy: "style" fails as ENUM_NOT_ALLOWED; the missing-field message carries the wire key the new test asserts on.
• ✅ Operation counts are self-consistent — docs.json holds exactly 33 operationIds, internal/registry/loader_test.go:41 sums to the 323 claimed in CLAUDE.md:44, and the leaves in docs/octo-cli-search-command-tree.md:14 sum to 33 (also repairing a stale 31 and a missing search).
• ✅ Every asserted skill string exists — all 36 sheet.md and 5 SKILL.md tokens required by skills/skills_test.go:49 and :59 are present verbatim.
• ✅ Response shape is parallel to docs.sheet.edit — docId/bytes/baseVersion/newDocVersionSeq plus the three counters; docs.json declares no x-octo-lossless-id-fields, so no new uint64-rounding surface appears.

Findings

No P0/P1 issues; one P2 and one Nit below.

P2 — Value-mode substring hazard is cautioned only for formula mode (skills/octo-docs/sheet.md:79)

The section establishes at :54 that default value mode is a "case-insensitive substring match" scoped to the whole workbook, but the "use an unambiguous string or a narrow selection and read … back afterwards" caution at :77–:79 is attached to the formula-mode sentence; the value-mode sentence at :79–:80 only adds that raw stored values are searched and that "booleans are 1 and 0". A cold-start agent reading just this section can run a whole-workbook value replace of a digit-bearing string with no warning that it is substring-matched: --find-string 0 --replace-string "" turns 2024 into 224 and 100.5 into 1.5, and also lands on the checkbox/boolean cells this same section documents as 1/0. Nothing gates it locally — validateByType (cmd/service/run.go:830) has no integer/number case — and the backend faithfully applies what was asked, so the prose is the only defence. Extend the substring caution to value mode, name the numeric/boolean case, and point digit-bearing or boolean searches at matchesTheWholeCell: true or a narrow logicalId + range.

Nit — range declares minimum: 0 that no gate reads, and inverted/out-of-grid selections are undocumented (internal/registry/specs/docs.json:649)

schemaInfoFromNode (internal/registry/loader.go:902) carries no Minimum/Maximum field and validateByType (cmd/service/run.go:830) has no integer case, so these four minimum: 0 declarations are documentation-only — consistent with the rest of the specs, but they read like an enforced bound. An inverted range (startRow > endRow) or one past the documented grid (skills/octo-docs/sheet.md:19 — rows 0..9999, columns 0..99) satisfies all four required keys and is forwarded with no documented outcome. Either drop the unenforced minimum, or state in the operation description that ordering and grid bounds are backend-checked and which code an inverted range returns.

Human-verify

1. Backend contract fidelity: the wire keys, the value|formula vocabulary, and the 412 base_version_stale / 403 protected_range / 413 too_many_cells|cell_too_large codes are asserted from octo-docs-backend MR !132, which is not readable from this checkout. Not a merge blocker for this PR.
2. Matching semantics: "booleans are 1 and 0" in value mode, and a formula cell counting in matchedCells via its cached display value while only being writable in formula mode (so matchedCells ≥ replacedCells), are backend behaviours this repo only documents. Worth one confirmation against the paired MR. Not a merge blocker.
3. Rollout ordering: the leaf ships enabled while MR !132 is unmerged, and the only available control is the note at CHANGELOG.md:11 — x-octo-disabled is service-level (internal/registry/loader.go:187), so using it would withhold all 33 docs ops. On an un-upgraded backend the failure is a visible 404 with the documented get + edit fallback and no mutation, and the note matches precedent (CHANGELOG.md:205); a human should still confirm the release gates on !132. Not a merge blocker.

Things I checked that are fine

• Retry / duplicate apply: no x-octo-retry override, so the transport default applies, but a lost-response retry resends the same stale If-Match and the backend answers 412 base_version_stale — the guard makes a duplicate replace fail closed instead of double-applying. A no-match replace creates no version, so its retry is a genuine no-op.
• Flag namespace: none of base-version, find-string, replace-string, find-by, case-sensitive, matches-the-whole-cell, logical-id collides with the engine or root-persistent reserved names (cmd/service/flags.go:209), and no enum is declared on a header or path param.
• Non-canonical input: --data null and trailing content after the JSON object are refused (cmd/service/run.go:600, :613); a whitespace-only findString is not refused locally but the server trims and rejects it, which :75–:76 documents.
• Identity routing and risk: docs.json declares no x-octo-allowed-token-kinds / x-octo-mount-by-token-kind, so the op inherits the same routing as its 32 siblings and keeps the /v1/bot/docs/ prefix; x-octo-risk: write with "(needs writer)" matches docs.sheet.edit.
• I deliberately did not suggest expressing "range requires logicalId" via x-octo-body-variants: validateBodyVariants (cmd/service/run.go:711) hardcodes an html-specific hint about slugs and republish, so adopting it here would emit misleading guidance. The backend rejects the combination, so it fails closed.
• No golden/help fixtures or alias tables need regenerating (cmd/service/aliases.go is matter-only), docs/octo-cli-design.md carries no sheet enumeration, and the shared gate list at skills/octo-docs/sheet.md:559–:573 already covers the replace error codes.

Verdict: APPROVED

Both items are non-blocking. The contract, its generated wiring, the pre-flight gates, and every documented count are internally consistent, and the destructive-by-default whole-workbook scope is guarded by the same If-Match token as its sibling write ops. The P2 is a documentation-completeness fix in the embedded skill and the Nit is metadata hygiene; neither changes runtime behaviour. The rollout dependency in CHANGELOG.md:11 is still worth honouring at release time.