Add CONTRIBUTING.md (#138); 3.4.4 - #146
Conversation
The contributor-facing half of #87 criterion 6: the read-before-write model (guardMutation, the fifteen Docs tools that open a lease through beginDocsMutation, the Sheets and Drive tools that call the guard directly, how a read handle differs from the in-process tracker), the seeding rule for create/copy tools ("trustworthy or not at all", with the #135 gap as the reason it is written down), the working-copy lifecycle, the error boundary, how to read a test run (Test Suites: is the line that matters), the inventory snapshot, the per-PR changelog and version bump, and the live-testing safety boundary. README.md and docs/README.md link to it. Every file, function, anchor, test, and PR number named in the page was checked against main before this commit. CHANGELOG 3.4.4; package.json / package-lock.json 3.4.4; SESSION-STATE.md updated with the merged #144 state, the issue triage outcomes, and the live runs on 665a605. Co-Authored-By: Claude Code <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2bd6ed73b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| ### What the guard is | ||
|
|
||
| `dist/readTracker.js` keeps a map of `fileId -> { readAt, modifiedTime, content, revisionId }`. Every read tool calls `trackRead`; every mutating tool calls `guardMutation` before its first side effect. `guardMutation` throws a caller-visible error when the file was never read in this scope, when a previous write marked it `requiresReread`, or when Drive's `modifiedTime` moved since the read (with a unified diff and rebase instructions when the entry holds a content snapshot and the tool passed a `contentFetcher`). The tracked `revisionId` becomes `WriteControl.requiredRevisionId` on the Docs batchUpdate, so Google itself refuses a write against a revision that moved. |
There was a problem hiding this comment.
This statement is false for both reads and mutations: listHeadings explicitly avoids trackRead because its structural response must not authorize a content edit, while existing mutators such as appendSpreadsheetRows do not call guardMutation. A contributor following “every read tool” could accidentally authorize a write from an incomplete metadata read, and a reviewer following “every mutating tool” could incorrectly assume unlisted write tools are already protected. Describe only the reads and mutations that participate in this guard.
Useful? React with 👍 / 👎.
|
|
||
| **Staleness.** Document staleness is the guard's job (revision and handle validation above). Local staleness is `backupIfLocallyModified` in `dist/workspace.js`: before any overwrite of an editable file, if its content is not what this process last wrote there (or there is no record, because this is the first write or the process restarted), the current bytes are copied to `<path>.bak` and the read reports it (issue #122). The overwrite still proceeds; recovery is from the `.bak`. `writeLocalFile=false` on `readDocument` skips the mirror write for a staleness check that must not touch it. | ||
|
|
||
| **Cleanup and retention.** `cleanupHandleWorkspaces` runs on every mint and at shutdown, never on a timer, and removes only the exact paths recorded in each workspace's ownership manifest, never a glob. A workspace whose editable file diverged from its baseline hash is dirty: it is retained, reported in the shutdown log, and never deleted. A baseline is removed when its last referencing workspace is gone. The store side (`dist/readHandles.js`) keeps a failed write's dirty workspace for recovery instead of reclaiming it. Nothing in `dist/workspace.js` removes legacy mirrors or `.bak` files. |
There was a problem hiding this comment.
A dirty workspace is not “never deleted”: in the normal edit-and-push flow the predecessor differs from its baseline, but after the Google write succeeds docsHandles.complete() calls safeDiscardWorkspace and removes it. Only dirty workspaces encountered by expiry/shutdown cleanup or retained after a failed write are preserved. The absolute wording gives contributors the wrong lifecycle invariant and could lead cleanup changes to retain successfully pushed workspaces indefinitely.
Useful? React with 👍 / 👎.
|
|
||
| The rule: **caught error text never reaches a caller-visible error or a persisted diagnostic.** Do not interpolate `error.message` into `publicError`, `UserError`, a `warnings` field, or anything written to disk. The redactor removes registered secrets and labeled credential patterns; it cannot recognize a filesystem path, a resolved host, or a project number as sensitive, so the only way those stay server-side is never to put them in a public string in the first place. | ||
|
|
||
| The shape every catch block in `dist/tools/` follows (`dist/tools/drive/copyFile.js` is a clean example): rethrow when `isPublicError(error)`, `log.error` the caught text server-side, map known status codes to fixed sentences, and `wrapOperationError` for the rest. `tests/publicErrorBoundary.test.js` pins the rule with a path-shaped diagnostic; a new catch block that maps errors gets a case there. |
There was a problem hiding this comment.
The repository does not currently follow this shape in every tool catch. For example, createDocument interpolates colorError.message and seedError.message into caller-visible contentWarnings, directly violating the rule immediately above, while publicErrorBoundary.test.js covers only selected paths. Present this as a requirement for new or modified catches (and identify existing debt), or bring the existing call sites under the boundary; otherwise contributors and reviewers are told that known raw-error exposure has already been audited away.
Useful? React with 👍 / 👎.
1 participant
Closes #138.
What
CONTRIBUTING.md, covering every bullet under "Required outcome" in #138 plus the repo conventions that were only in people's heads:guardMutationchecks, how to settle the tracker after a write (trackMutation,refreshRevision,requireRereadBeforeMutation), the fifteen Docs tools that open a lease throughbeginDocsMutation, the three Sheets tools anddeleteFilethat call the guard directly, how a read handle differs from the in-process tracker, and the five steps for adding a guarded Docs mutation.createDocument/createFromTemplate/createSpreadsheet/copyFileseeds and what is deliberately left unseeded, and why a false seed is worse than a rejection. The Seed read state after createSpreadsheet and copyFile (#87) #135 gap is named as the reason the rule is written down.backupIfLocallyModified), cleanup and retention.publicError/wrapOperationError/getApiErrorDetail/redactDiagnostic, and the rule that caught error text never reaches a caller-visible error or a persisted diagnostic.Test Suites:is the line to read, the mocking pattern, and the inventory snapshot regeneration command.package.jsonbump.scripts/live-smoke/guard.mjs.README.md("Development / Contributing") anddocs/README.md(index) link to it. Where a topic already has a canonical page (docs/architecture.mdfor layout and adding a tool,docs/http-mode.mdfor the client-facing handle contract,RELEASING.mdfor the release flow), the new page links rather than restates.Verification
Every file path, exported function, markdown anchor, test file, environment variable, and PR number named in
CONTRIBUTING.mdwas checked againstmainwith grep/ls before this went up (the list of guarded Docs tools is the exact set of files importingbeginDocsMutation; the TTL constants,BINDING_KEYS,cleanupHandleWorkspacestriggers, the "starts empty on purpose" successor workspace, and the guard's Gmail allowlist were each confirmed in source). Relative links and anchors resolve. No em dashes.npm run test:ci:Test Suites: 96 passed, 96 total/Tests: 2 skipped, 1419 passed, 1421 totalnpm audit --omit=dev: 0 vulnerabilitiesnpm pack --dry-run: 185 files, 396.7 kB (CONTRIBUTING.md is not in thefilesallowlist, so the tarball is unchanged)No runtime code changes. CHANGELOG 3.4.4,
package.json3.4.4.🤖 Generated with Claude Code