Repository navigation
CRW bundle trn-934cc56c29ff5de7cef04bd73c2b943f: #787 #797 - #813
thisisjun786 wants to merge 11 commits into
Conversation
…n operator command Fix the three post-merge P1s of PR #735 in the state backup routine: the store WAL is compared by its identity in the two-listing comparison, the store sidecars are recognised by the resolved database path rather than the top-level name, and an ENOENT from the walk Info on the store log is recorded as gone before its copy. Add the integrity_check gate: the copied store is duplicated into a scratch file beside the copy, PRAGMA integrity_check runs on the duplicate, and the manifest records integrityCheck and restoreCandidate (a failure refuses). The copy bytes are never opened. Add the operator command crw install backup-state --to <dir>, which takes the same artifact outside an install, refuses while the relay service runs, and holds the store write gate exclusively during the copy. Red-first tests for each behaviour; docs/runtime-install.md updated.
…y duplicate Review fixes for the operator command and the sidecar identity: - sidecarsFor resolves the database path even when store.InPlaceRead refuses it (a log with frames and no usable index after an unclean shutdown), so a linked store log is carried under the restore-compatible name rather than walked as an ordinary file; - listingDiff treats a log that went during a listing walk as absence, not a moved identity, so the same checkpoint race no longer refuses on timing; - the integrity duplicate is streamed with a fixed buffer instead of os.ReadFile, and the free-space preflight charges for it, so a store of any size is checked without holding it in memory and a destination that fits only the backup refuses up front; - the operator command locks the write gate beside the resolved database (holdGate), not the state directory root, and validates every path flag for non-UTF-8.
…t took the backup The standalone backup now records its own route: the manifest and the result say the copy was taken by crw install backup-state outside an install (takenBy: operator), with the words the operator command's checks support, rather than the swap gate's. PRAGMA integrity_check runs through QueryContext, so an interrupted backup stops the check instead of holding the write gate until a large scan finishes.
… reply improveReadDrafts read every .json file as a draft, keyed it on issue/identifier instead of the crw-issue-draft/1 fingerprint, took no time from seen[], and counted the drafts directory's index.json as a draft. Two different drafts therefore merged under an empty key. A draft is now a crw-issue-draft/1 document loaded through auditDraftLoad, keyed by its fingerprint, with first_at and last_at the earliest and latest seen[].at; index.json and any other schema are skipped and not counted, and a matching schema with an empty fingerprint is the source's improve_source_unreadable error. A child's blocked_needs_input receipt cannot carry a reason field (receiptFields in internal/relay/store/receipt_shape.go), so the reason of a blockage is now the note of the decision reply whose receipt answersEvent names that event, and a blockage no decision answered keeps the bare outcome name. The existing decision_reply reading rule is unchanged. improveAdd compared first_at and last_at as text, which reverses "2026-10-06T00:00:00.5Z" and "2026-10-06T00:00:00Z" because '.' sorts before 'Z'. Both are now compared as RFC3339Nano instants, and a value that is not an instant keeps its text and sorts after one that is.
The schema gate skips a candidate whose schema is not crw-issue-draft/1, which is what index.json and any foreign document need. A candidate that is not a JSON object at all carries no schema, and skipping it would report a complete bundle over a file the drafts directory could not be read from. It is refused instead, which is what this reader did before the schema gate and what improveReadJSONLines already does with a null line.
C1 says a draft is a crw-issue-draft/1 document and that index.json and any other schema are skipped and not counted. Only the listing was covered, so a reader that keyed on any JSON object would still have passed; a document of another schema is now pinned on its own. The configured path may also name one draft file rather than a directory, which the reader accepted before and still accepts, so that shape and its seen bounds are pinned too. Both tests fail on the baseline: the foreign document was read as a draft with an empty key, and the single file lost its seen times.
…d string The two-listing comparison joined its key fields with "|", a byte a file name and a link target may legally hold, so two different entries could join to one string and a retargeted link pass. The keys are now comparable structs used as map keys and compared with ==, and the "gone or changed" entry is reported from the struct Path rather than by cutting a string. Nothing else in listingDiff changes. Red-first tests reproduce the independent review pair with real files: the store log retargeted to a path that collides under the delimiter, and the same for an ordinary symlinked file in the state directory.
…parse-consistency The base refresh for CRW-873. One conflict, in internal/manage/improve_collect.go just above improveReadDag: this branch adds the type improveParseBlockedKey there, and dev's CRW-872 replaced the old improveReadDag header with its own comment and a new signature. The resolution keeps this branch's improveParseBlockedKey type and comment followed by dev's improveReadDag comment and signature, and drops the old "runs the relay's dag-measurements" comment, the e *Env signature and the coreDefaults lines that CRW-872 replaced. Nothing else was changed.
thisisjun786
added
the
crw-lane
|
You have reached your Codex usage limits for security reviews. Please try again later. |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![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: 9a3ef4e7d3
ℹ️ 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".
| continue | ||
| } | ||
| name := "relay.sqlite3" + suffix | ||
| if info, err := os.Lstat(resolved + "-wal"); err == nil { |
There was a problem hiding this comment.
When relay.sqlite3 resolves to another path, every error from os.Lstat(resolved + "-wal") is currently treated as if the WAL were absent. If that probe returns a non-ENOENT filesystem error (for example EIO or ESTALE) on both listings, the command copies only the database, the integrity check can pass, and the manifest incorrectly marks a restore candidate even though committed frames may have been omitted. Handle only fs.ErrNotExist as absence and refuse every other probe error.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed. The linked-store WAL probe in internal/runtime/install/zone.go treats any os.Lstat error as an absent log, so a non-ENOENT error can leave committed frames out of a backup that is still marked as a restore candidate. This is a data-loss finding on member #787 (CRW-862), so the bundle was abandoned and #787 was not merged.
The fix goes back to #787's own branch: only fs.ErrNotExist counts as absence, any other probe error refuses the backup, and a red test covers a real non-ENOENT probe error. #787 rejoins the merge lane after that correction is verified.
1 participant
CRW bundle merge (crw-run merge-readiness.md, Merge a bundle; CRW-768).
Each member was verified and accepted on its head by its own parent. The chain is one two-parent merge per member from D, each tree what git merges from its parents (no hand resolution). This pull request runs the full CI once and lands as one merge commit with its head checked. Do not push to this branch.