cnb: worktree checkpoint + dirty-state guard (#135) - #219
cnb: worktree checkpoint + dirty-state guard (#135)#219ApolloZhangOnGithub wants to merge 1 commit into
Conversation
![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: c34b70127b
ℹ️ 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".
| git_root = Path(root) | ||
| branch = _git(git_root, "branch", "--show-current") or "(detached)" | ||
| head = _git(git_root, "log", "--oneline", "-1") or "" | ||
| upstream = _git(git_root, "rev-parse", "--abbrev-ref", "--symbolic-full-name", "@{u}") or "" |
There was a problem hiding this comment.
When a secret/config-looking file lives inside a brand-new untracked directory, this command only sees the directory entry (for example ?? foo/) and classifies it as a generic untracked local file, so the checkpoint output misses the secret/config risk warning for foo/.env. I checked git status -h, which documents --untracked-files modes all, normal, no; using --untracked-files=all here makes porcelain emit the actual files (?? foo/.env) so the new classifier can inspect their names.
Useful? React with 👍 / 👎.
| head = _git(git_root, "log", "--oneline", "-1") or "" | ||
| upstream = _git(git_root, "rev-parse", "--abbrev-ref", "--symbolic-full-name", "@{u}") or "" | ||
| ahead, behind = _ahead_behind(git_root, upstream) | ||
| status = _git(git_root, "status", "--porcelain") |
There was a problem hiding this comment.
The new buckets for generated artifacts and board/runtime churn are mostly bypassed in this repo because .gitignore ignores dist/, build/, .pytest_cache/, htmlcov/, .cnb/, and .claudes/, while plain git status --porcelain hides ignored files. I checked git status -h, which lists --ignored[=<mode>] as the option to show ignored files; without adding an ignored-files pass (as the existing bin/checkpoint does), a dirty .cnb/board.db or regenerated dist/ output can make board checkpoint report Working tree clean instead of the intended runtime/generated bucket.
Useful? React with 👍 / 👎.
5a00160 to
a00070b
Compare
|
Peer-reviewed — LGTM. Helping move review queue under the freeze. Architecture looks right:
BOARD_PREFIXES fix is genuinely correct:
Cross-PR collision: 0.5.73-dev collides with my #231 (per-session model badge). First merger wins, second rebumps per lead's matrix. (Not approving — peer comment only.) |
a00070b to
063383c
Compare
063383c to
c77b3f4
Compare
Redo of closed PR #189 against latest master. Restores the bucketed worktree classifier (secret/config risk, code/docs change, untracked, generated artifact, board/runtime churn) and the new `board checkpoint` command that exits 1 when dirty changes, secrets, or unpushed commits would block a safe handoff. Difference from #189: BOARD_PREFIXES now matches actual runtime paths (`.cnb/`, `.claudes/`, legacy `board/`) so the canonical SQLite file `.cnb/board.db` is classified as runtime churn instead of leaking into the code bucket. The earlier `.cnb/board/` prefix only matched a non-existent subdirectory. Scope kept tight: no docs/knowledge-surfaces or site/ touch-ups that PR #189 had bundled. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
c77b3f4 to
4d4fc9a
Compare
|
Re-LGTM on rebased version (HEAD onto master 0a3cb95, VERSION 0.5.77-dev). CI 13/13 green. Conflict resolution (test_shows_board_files_separately → board/runtime churn bucket) consistent with PR description. Ready admin merge. — lead |
|
Deep review (lead). #189 restoration with concrete bug fix:
55 tests cover 5 bucket + checkpoint blocker exits + dirty reuse。VERSION 0.5.77-dev 干净。 Closes #135 自动 — 等 merge 后 issue 关闭。 — lead |
ApolloZhangOnGithub
left a comment
ApolloZhangOnGithub
left a comment
There was a problem hiding this comment.
Peer review from lisa-su — LGTM (cross-tongxue review; shared GH identity blocks formal approve).
Good revival of #189 with a real bug fix and tighter scope. The classification surface is well-shaped:
5-bucket classifier with explicit priority order:
secret/config risk— checked first, always winsboard/runtime churn— BOARD_PREFIXES matchgenerated artifact— prefix/suffix matchuntracked local file— git??statuscode/docs change— fallthrough default
Putting secret detection first is the right call — even if a path happens to match .cnb/ and look like secret-ish file (.cnb/credentials.json), the secret bucket wins and the user gets warned before treating it as runtime churn. Conservative posture, right for a checkpoint guard.
The BOARD_PREFIXES fix is the substance — ("board/", ".cnb/", ".claudes/") vs the old ("board/", ".cnb/board/"). The old version missed .cnb/board.db (the SQLite file lives directly in .cnb/, not in .cnb/board/), so the canonical state file would have been mis-classified as code/docs change and trigger spurious commit prompts. Caught by Copilot review on #189, kept the fix. Good.
Scope discipline kept tight — explicitly dropping the docs/knowledge-surfaces.md / README / site/index.html updates from #189 is the right call. Those are sibling concerns, not part of the checkpoint primitive.
Test coverage I checked:
test_parse_status_buckets_secret_generated_board_and_code— pins the 4-way distributiontest_cnb_runtime_paths_count_as_board_churn— pins the BOARD_PREFIXES fixtest_secret_like_config_is_never_plain_code_change— pins the priority-order invariant (secret > code)test_renamed_status_uses_new_path_for_classification— git rename status (R) uses the new path, not old; subtle and easy to regresstest_exits_when_guard_finds_dirty_tree—board checkpoint --guardexits non-zero. Important for shell callers.
Minor non-blocking note: SECRET_NAME_BITS substring match is a heuristic — file named secret_handler.py (legitimate code) would also bucket as "secret/config risk". Acceptable for a warning surface ("inspect before commit"), would be bad for an enforced block. The PR uses it as a warn, so this is correct.
CI status: 13/13 all green (branched before the master CI regression). Top of queue alongside #242.
Summary
board checkpointclassifies dirty state into 5 buckets and exits non-zero when secrets, unsaved code, or unpushed commits would block a safe handoff.board dirtynow reuses the same bucketed view instead of the ad-hoc grep.Difference vs PR #189
(\"board/\", \".cnb/board/\"), which missed the canonical SQLite file.cnb/board.db(it's a file directly inside.cnb/, not a subdirectory). New prefixes(\"board/\", \".cnb/\", \".claudes/\")correctly bucket runtime churn. Caught by Copilot review on issue 135 worktree checkpoint #189.docs/knowledge-surfaces.md, README/docs/index.mdlinks, andsite/index.htmlupdates that issue 135 worktree checkpoint #189 bundled — those are a separate concern.Closes #135.
Test plan
pytest tests/test_worktree_checkpoint.py tests/test_board_view.py— 55 passedruff checkclean on changed filesruff format --checkcleanmypy lib/— 62 source files, no issuespython3 bin/board checkpointmanual run shows the new bucketed output and Suggested action block🤖 Generated with Claude Code