Skip to content

docs(playbooks): harden PR-slicing process with two-sided isolation#158

Merged
phuongnse merged 2 commits into
mainfrom
docs/pr-slicing-hardening
May 31, 2026
Merged

docs(playbooks): harden PR-slicing process with two-sided isolation#158
phuongnse merged 2 commits into
mainfrom
docs/pr-slicing-hardening

Conversation

@phuongnse

@phuongnse phuongnse commented May 31, 2026

Copy link
Copy Markdown
Owner

Summary

Adds the authoritative docs/playbooks/pr-slicing.md and tightens the slicing process. Splitting work into branches is not the same as isolating it — "sliced" PRs still ship coupled when they don't compile alone, claim green CI that was never run, duplicate files across siblings, or disagree on a shared constant. This doc is written as general, use-case-agnostic best practice so it applies to any future multi-PR feature.

Key additions:

  • Two-sided isolation test — a slice must both stand alone (fresh checkout compiles + full Gate 1 green; every referenced route/endpoint/contract/symbol exists on its merge target) and integrate (still green after rebasing onto current main).
  • Shared-seam ownership — one owner per shared file/symbol (barrels, generated files, aggregates + migrations, shared constants); others consume, never re-add.
  • One source for shared values/contracts — no second hardcoded copy.
  • Gate 1 honesty — a green claim means you ran the full verification; anything skipped is stated.
  • Merge order & rebase cadence — document order, require branches up to date, rebase + reconsolidate after each merge.
  • Failure-mode → rule table (generic) and a per-slice pre-push checklist.

This file was previously added as a new file in two open PRs at once (itself a slicing defect this doc now names). Landing it here makes it authoritative; those PRs drop their copies on rebase. Nav wired from docs/README.md, agent-checklist.md, and process.md.

Linked spec

Process/playbook change — no use-case AC.

Requirements

  • Authoritative, general pr-slicing.md (two-sided isolation, shared-seam ownership, single-source values, Gate-1 honesty, merge/rebase cadence, failure-mode table, checklist)
  • Nav back-links added (README playbooks table, agent-checklist, process.md Step 6)
  • ./scripts/check-doc-drift.sh green (link-targets + code-fences pass)

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Documentation
    • Added a comprehensive PR-slicing playbook detailing how to split large use cases into independently mergeable PRs, two-sided isolation verification, branching and merge/rebase cadence, and shared-seam ownership
    • Updated agent checklist with mandatory large-use-case workflow steps and isolation testing requirements
    • Clarified process guidance for multi-PR use cases and how to document per-slice ship/deferral items

Adds the authoritative pr-slicing.md (previously duplicated as a new file in
#153 and #154). Tightens "isolated" into a two-sided test (stands alone on the
branch AND integrates after rebase), adds shared-seam ownership, a single-source
rule for shared constants/contracts, a Gate-1-honesty section, merge/rebase
cadence, and a register-org retrospective mapping each real defect to the rule
that prevents it. Wires nav from docs/README, agent-checklist, and process.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented May 31, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 28f0bf4f-b36c-4fbc-b34a-3851a6cd185c

📥 Commits

Reviewing files that changed from the base of the PR and between 389183f and 96c5963.

📒 Files selected for processing (1)
  • docs/playbooks/pr-slicing.md

📝 Walkthrough

Walkthrough

Adds a new PR-slicing playbook requiring two-sided isolation verification for slice PRs, formalizes shared-seam ownership and merge/rebase cadence, and integrates the guidance into the agent checklist, README, and process docs with a per-slice pre-push checklist.

Changes

PR Slicing Playbook and Integration

Layer / File(s) Summary
PR Slicing Playbook: Core Concepts and Rules
docs/playbooks/pr-slicing.md
Playbook intro defines "isolated" via a two-sided test and enumerates P0 slicing rules (branch origin, proving both sides, vertical/horizontal slicing, single owner per shared seam, no duplicate new files).
PR Slicing Playbook: Ownership, Gate 1 Honesty, Merge Cadence, Decision Tree & Failures
docs/playbooks/pr-slicing.md
Describes shared-seams and ownership, Gate 1 honesty requirements, documented merge order and rebase cadence, decision tree for slicing choices, failure-modes mapping, agent workflow, and a per-slice checklist.
Agent Workflow Integration and Cross-Document Updates
docs/playbooks/pr-slicing.md, docs/playbooks/agent-checklist.md, docs/README.md, docs/playbooks/process.md
Adds "Large use cases" guidance to agent checklist, extends README playbooks table with Agents directive for PR slicing, and updates process.md to instruct multi-PR documentation handling per pr-slicing.md.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

Possibly related PRs

  • phuongnse/axis#56: Related updates to docs/playbooks/agent-checklist.md for agent workflow guidance.
  • phuongnse/axis#128: Similar tightening of agent Gate-status and pre-push checklist rules.
  • phuongnse/axis#51: Related playbook and agent/process checklist edits aligning PR-gating workflow.

Poem

🐰 I hop through branches, carrot in paw,
I split big tasks with careful law,
Two-sided checks, seams with one guide,
Rebase, retest, then merge with pride.
Tiny slices—big wins—🥕

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main change: documenting and formalizing the PR-slicing process with emphasis on two-sided isolation verification as a core hardening mechanism.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch docs/pr-slicing-hardening

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/playbooks/pr-slicing.md`:
- Around line 86-98: The fenced decision-tree block in the PR slicing doc is
missing a language tag causing MD040; edit the ASCII decision-tree fenced code
block (the triple-backtick block containing the "Does the change alter a
contract already used on main?" text) and add a language marker (e.g., change
``` to ```text) so markdownlint recognizes it, then save and re-run the linter
to confirm the MD040 error is resolved.
- Line 67: Update the Gate 1 checklist to match CI by requiring full-solution
tests: replace the "dotnet test for the affected module(s)" allowance with an
explicit requirement to run `dotnet test` against the full solution (Axis.sln)
after a successful `dotnet build Axis.sln` so Gate 1 mirrors CI; ensure the text
mentions `Axis.sln`, `dotnet build`, `dotnet test`, and the Gate 1 policy to
avoid permitting isolated-module testing.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 74d002c5-fde9-4d95-822b-3b363217a701

📥 Commits

Reviewing files that changed from the base of the PR and between d707d92 and 389183f.

📒 Files selected for processing (4)
  • docs/README.md
  • docs/playbooks/agent-checklist.md
  • docs/playbooks/pr-slicing.md
  • docs/playbooks/process.md

Comment thread docs/playbooks/pr-slicing.md Outdated
Comment thread docs/playbooks/pr-slicing.md
Rewrite pr-slicing.md as use-case-agnostic best practice: drop the
register-org-specific example/retrospective tables and PR numbers, keep the
general principles (two-sided isolation, shared-seam ownership, single-source
constants, Gate-1 honesty, merge/rebase cadence) and a generic failure-mode →
rule table so it applies cleanly to any future multi-PR use case.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@phuongnse phuongnse merged commit 3a84e52 into main May 31, 2026
8 checks passed
phuongnse added a commit that referenced this pull request May 31, 2026
Rebase resolution (seams now owned by merged slices):
- drop duplicate pr-slicing.md + nav lines (owned by #158)
- merge register FE surface with the journey slice already on main
  (api.ts authKeys, useRegister flow, register-page test, register-org callout)

Fix pre-existing CI-red issues surfaced by the rebase:
- AuthHelper: register via TestRegistrationPayload (includes required legal
  versions) so the now-mandatory Terms validator does not 400 every suite
- regenerate AddUserLegalAcceptance migration so it has its paired .Designer.cs
  (hand-written migration was unrecognized by EF and missing the designer)
- route GET /api/legal/versions through a GetLegalVersions query handler so the
  endpoint maps to a module (doc-drift discovery) and carries no inline logic
- add tests for GetLegalVersions and GetOrganizationSlugPreview handlers
- note Terms/slug register changes under identity-access (auth-frontend domain)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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