Skip to content

fix(cmux-rebuild): resolve code review findings for PR #114 - #115

Open
DJRHails wants to merge 1 commit into
mainfrom
skill/cmux-rebuild-review-114
Open

fix(cmux-rebuild): resolve code review findings for PR #114#115
DJRHails wants to merge 1 commit into
mainfrom
skill/cmux-rebuild-review-114

Conversation

@DJRHails

Copy link
Copy Markdown
Owner

Follow-up to the post-merge review of #114 (the SKILL.md rewrite). Fixes the two P3 findings posted inline there:

  • F1 — $CACHE undefined: the pane-title cache snippet referenced $CACHE but the rewrite dropped the assignment line the original had. The assignment is back inside the code block (macOS value, linux path as a comment), and the now-redundant path listing is out of the prose.
  • F2 — serialization config dropped: restored (session_serialization true + viewport in config.kdl) to the macOS-reboot bullet — it is the config that makes EXITED skeletons exist at all, so dropping it left the "no skeletons after reboot" failure mode undebuggable from this doc.

P4 noted but not restored (respecting the rewrite's compression intent): the picker Esc-fallback binding gap — recovery is already covered by the "--bind backfills" line.

Verified: both pi facts in the rewrite were checked against pi 0.84.2 source (renames append session_info entries carrying name; latest wins) and the linux cache path against zellij's consts.rs — both correct as written. prek hooks pass (gitleaks, trufflehog, glassine checks, all local test suites).

via gantry

Two P3 findings from the post-merge review of the SKILL.md rewrite:

- F1: the pane-title cache snippet referenced $CACHE without defining it
  (the rewrite dropped the assignment line). Restored the macOS assignment
  inside the block with the linux path as a comment, and dropped the now-
  redundant paths from the prose.
- F2: restored the enabling config for zellij session-serialization
  ('session_serialization true' + viewport in config.kdl) — without it no
  EXITED skeletons exist after a reboot and the doc gave no debugging handle.

P4 noted, not restored: the picker Esc-fallback binding gap (covered by the
general '--bind backfills' line).

Verified: prek hooks pass (gitleaks, trufflehog, glassine, test suites).
@DJRHails

Copy link
Copy Markdown
Owner Author

Review Summary

Direct review (docs-only follow-up delta, +6/−5 in one file) — no agent fan-out.

Findings

No P1–P3 findings. Both changes verified against the pre-rewrite text (git show fd447030^):

# Severity Finding Resolution
1 P4 The rg -o '^\s+title "(.*)"' snippet's capture group is inert without -r '$1' — output includes the title " prefix. Pre-existing (unchanged since before #114), harmless for human-read inventory. Noted only; out of scope for this delta.
  • F1 ($CACHE undefined): the assignment restored inside the code block matches old line 159 exactly; the linux path rides as a comment. No other reference to the removed prose paths remains in the doc.
  • F2 (serialization config): (session_serialization true + viewport in config.kdl) matches old line 250's wording verbatim.

Verification

Commit

No fix commit needed — the PR is correct as submitted (head ab3bb29).

Verdict: approve

via gantry

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant