feat(hardened): Agent SDK CLI Wizard and TAC commands - #404
Merged
POWERFULMOVES merged 1 commit intoJan 3, 2026
Conversation
…365) * feat(cli): rebrand Crush CLI to PMOVES CLI Update user-facing branding from "Crush CLI" to "PMOVES CLI" while maintaining backward compatibility with existing Crush infrastructure. Changes: - Update crush_app help text: "Crush CLI integration" → "PMOVES CLI integration" - Update crush_configurator.py docstring to emphasize PMOVES deployment - Update command help texts for setup/status/preview commands - Update user-facing documentation in .claude/commands/crush/ Rationale: The "Crush" name originated as an internal codename but the production CLI should reflect the PMOVES brand for consistency with the broader PMOVES.AI ecosystem. The underlying "crush" command name and file paths are preserved for backward compatibility. Modified Files: - pmoves/tools/mini_cli.py - pmoves/tools/crush_configurator.py - .claude/commands/crush/setup.md - .claude/commands/crush/status.md 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> * feat(cli): add PMOVES Agent SDK commands to mini CLI Implement agent-sdk sub-commands for creating and managing PMOVES Agent instances with full ecosystem access via interactive CLI wizard. Features Implemented: - `pmoves agent-sdk create` - Interactive wizard for agent creation - 5 agent roles: researcher, code-reviewer, media-processor, knowledge-manager, general - Role-based tool and subagent configuration - Automatic NATS, TensorZero, and Hi-RAG connection - Unique agent ID generation with timestamps - Beautiful formatted output with configuration summary - `pmoves agent-sdk run` - Execute tasks with existing agents - Task execution with streaming output - Model override support - Session resumption capability - `pmoves agent-sdk list` - List agent instances - Status filtering - Configurable limit (placeholder for SessionManager integration) - `pmoves agent-sdk status` - Check agent status - NATS heartbeat monitoring - Active agent information (placeholder for SessionManager) Technical Details: - Integrated with PMOVES-BoTZ Agent SDK - Async/await pattern for agent lifecycle management - Interactive role selection with graceful Ctrl+C handling - Comprehensive error handling for missing dependencies - Auto-discovery of PMOVES-BoTZ submodule Usage Examples: ```bash # Interactive agent creation pmoves agent-sdk create # Pre-select role pmoves agent-sdk create --role researcher # Execute task pmoves agent-sdk run pmoves-researcher-1735123456 "Analyze architecture" # List agents pmoves agent-sdk list --status active --limit 50 ``` Related Documentation: - .claude/commands/agent-sdk/create.md - .claude/commands/agent-sdk/run.md - .claude/commands/agent-sdk/resume.md 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> * docs(agent-sdk): update CLI documentation for run and resume commands Update user-facing documentation for agent-sdk CLI commands to reflect the new PMOVES CLI integration pattern. Changes: - `.claude/commands/agent-sdk/run.md` - Updated from skill-based to CLI command documentation - Added usage examples with `pmoves agent-sdk run` - Documented arguments and options - Added troubleshooting section - `.claude/commands/agent-sdk/resume.md` - Updated from skill-based to CLI command documentation - Added session management workflow - Documented session states and storage backends - Added troubleshooting section Documentation Pattern: All agent-sdk command documentation now follows a consistent pattern: - Usage section with use cases - Implementation section with CLI examples - Arguments and options tables - What It Does checklist - Related commands section - Notes and troubleshooting This aligns with the create.md documentation updated in the previous implementation phase. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> * fix(agent-sdk): address all PR #365 review comments Fix all 14 issues from comprehensive PR review across error handling, documentation, and code quality improvements. Critical Fixes (4): - Make NATS connection mandatory with ConnectionError on failure - Add two-layer error handling to task execution - Replace generic Exception catches with specific error types - Exit with code 1 on all failure paths Documentation (5): - Correct NATS event subjects (remove non-existent events) - Add prerequisites sections to all agent-sdk docs - Fix example code placeholders with runnable examples - Update model IDs (remove date suffixes) - Document storage backends and timeouts Improvements (5): - Add Google-style docstrings to key functions (≥80% coverage) - Enhance Crush configurator docstrings - Improve list/status placeholders with NATS monitoring guidance - Fix context manager usage pattern - Add comprehensive timeout documentation All syntax checks pass. Docstring coverage ≥80%. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> --------- Co-authored-by: Codex Agent <codex-agent@example.com> Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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 |
POWERFULMOVES
added a commit
that referenced
this pull request
Jan 18, 2026
…365) (#404) * feat(cli): rebrand Crush CLI to PMOVES CLI Update user-facing branding from "Crush CLI" to "PMOVES CLI" while maintaining backward compatibility with existing Crush infrastructure. Changes: - Update crush_app help text: "Crush CLI integration" → "PMOVES CLI integration" - Update crush_configurator.py docstring to emphasize PMOVES deployment - Update command help texts for setup/status/preview commands - Update user-facing documentation in .claude/commands/crush/ Rationale: The "Crush" name originated as an internal codename but the production CLI should reflect the PMOVES brand for consistency with the broader PMOVES.AI ecosystem. The underlying "crush" command name and file paths are preserved for backward compatibility. Modified Files: - pmoves/tools/mini_cli.py - pmoves/tools/crush_configurator.py - .claude/commands/crush/setup.md - .claude/commands/crush/status.md 🤖 Generated with [Claude Code](https://claude.com/claude-code) * feat(cli): add PMOVES Agent SDK commands to mini CLI Implement agent-sdk sub-commands for creating and managing PMOVES Agent instances with full ecosystem access via interactive CLI wizard. Features Implemented: - `pmoves agent-sdk create` - Interactive wizard for agent creation - 5 agent roles: researcher, code-reviewer, media-processor, knowledge-manager, general - Role-based tool and subagent configuration - Automatic NATS, TensorZero, and Hi-RAG connection - Unique agent ID generation with timestamps - Beautiful formatted output with configuration summary - `pmoves agent-sdk run` - Execute tasks with existing agents - Task execution with streaming output - Model override support - Session resumption capability - `pmoves agent-sdk list` - List agent instances - Status filtering - Configurable limit (placeholder for SessionManager integration) - `pmoves agent-sdk status` - Check agent status - NATS heartbeat monitoring - Active agent information (placeholder for SessionManager) Technical Details: - Integrated with PMOVES-BoTZ Agent SDK - Async/await pattern for agent lifecycle management - Interactive role selection with graceful Ctrl+C handling - Comprehensive error handling for missing dependencies - Auto-discovery of PMOVES-BoTZ submodule Usage Examples: ```bash # Interactive agent creation pmoves agent-sdk create # Pre-select role pmoves agent-sdk create --role researcher # Execute task pmoves agent-sdk run pmoves-researcher-1735123456 "Analyze architecture" # List agents pmoves agent-sdk list --status active --limit 50 ``` Related Documentation: - .claude/commands/agent-sdk/create.md - .claude/commands/agent-sdk/run.md - .claude/commands/agent-sdk/resume.md 🤖 Generated with [Claude Code](https://claude.com/claude-code) * docs(agent-sdk): update CLI documentation for run and resume commands Update user-facing documentation for agent-sdk CLI commands to reflect the new PMOVES CLI integration pattern. Changes: - `.claude/commands/agent-sdk/run.md` - Updated from skill-based to CLI command documentation - Added usage examples with `pmoves agent-sdk run` - Documented arguments and options - Added troubleshooting section - `.claude/commands/agent-sdk/resume.md` - Updated from skill-based to CLI command documentation - Added session management workflow - Documented session states and storage backends - Added troubleshooting section Documentation Pattern: All agent-sdk command documentation now follows a consistent pattern: - Usage section with use cases - Implementation section with CLI examples - Arguments and options tables - What It Does checklist - Related commands section - Notes and troubleshooting This aligns with the create.md documentation updated in the previous implementation phase. 🤖 Generated with [Claude Code](https://claude.com/claude-code) * fix(agent-sdk): address all PR #365 review comments Fix all 14 issues from comprehensive PR review across error handling, documentation, and code quality improvements. Critical Fixes (4): - Make NATS connection mandatory with ConnectionError on failure - Add two-layer error handling to task execution - Replace generic Exception catches with specific error types - Exit with code 1 on all failure paths Documentation (5): - Correct NATS event subjects (remove non-existent events) - Add prerequisites sections to all agent-sdk docs - Fix example code placeholders with runnable examples - Update model IDs (remove date suffixes) - Document storage backends and timeouts Improvements (5): - Add Google-style docstrings to key functions (≥80% coverage) - Enhance Crush configurator docstrings - Improve list/status placeholders with NATS monitoring guidance - Fix context manager usage pattern - Add comprehensive timeout documentation All syntax checks pass. Docstring coverage ≥80%. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Codex Agent <codex-agent@example.com> Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
POWERFULMOVES
added a commit
that referenced
this pull request
Aug 15, 2026
* feat(tools): pmoves standard branch protection spec + tool
The PMOVES standard branch-protection tool. Single source of truth for
how a PMOVES org repo's main branch is protected. The tool reads a
canonical JSON spec (pmoves_standard.json) and either audits a repo
against the spec, applies the spec to a repo, or drift-checks the
whole org.
What this slice lands:
- pmoves/configs/branch_protection/pmoves_standard.json - the
canonical spec. 2 profiles (monorepo + fork) + per_repo_overrides
for the 3 PMOVES repos in the org. The shape mirrors the GitHub
REST API 1:1, so a profile maps to actual API calls without
intermediate transformation.
- pmoves/tools/branch_protection.py - the tool. Pure-stdlib Python
(urllib.request + json), no new deps. Three public functions:
audit(repo, profile) - diff actual state vs spec
apply(repo, profile, dry_run=True) - apply the spec; dry-run by default
drift_check(org) - audit every repo in the org's overrides
8 dataclasses for structured results (AuditResult, DriftItem,
ApplyResult, DriftReport, etc.) so the orchestrator can consume
the output. CLI surface: `python -m pmoves.tools.branch_protection
{audit,apply,drift-check}` with structured JSON output.
Why this is the next slice after the harness v0:
The 3-PR review pass (PMOVES.AI #2477 + PMOVES-hermes-agent #4 +
PMOVES-pinokio #1) shipped the CGP bootstrap contract. The contract
ties 3 repos together, but the SECURITY POSTURE is wildly
asymmetric: PMOVES.AI is heavily protected (4 required status
checks, reviews with code owner enforcement, linear history,
signatures, 3 rulesets), but the 2 forks have NO protection at
all. The Hermes PR #4 was admin-merged only because there's no
required gate to be met - that's a bug-as-feature, not a
designed protection.
This tool makes the asymmetry visible (drift-check) and
correctable (apply). The Mavis cron can call drift-check daily
and publish on pmoves.branch_protection.drift.v1 (the NATS
subject lands in a follow-up slice).
Design notes (codified in the docstring + tests):
- The tool shells out to `gh api` instead of using urllib directly
for HTTP. Reason: the PMOVES GitHub App token + the operator's
PAT both flow through gh's auth, and wrapping gh gives the
operator free auth-state inspection via `gh auth status`.
Tradeoff: the tool requires `gh` installed and authenticated.
Documented in BRANCH_PROTECTION_BASELINE.md (follow-up docs).
- dry-run is the default. The tool never issues a PUT/POST without
`--no-dry-run`. The dry-run output is a JSON list of would-be
API calls; the operator reviews the list before issuing live
apply. The 2 forks in the spec will be applied manually after
this PR merges (slice 2, separate PRs per repo).
- The per_repo_overrides section is the per-repo customization
point. New forks add themselves here; the spec's strict shape
(additionalProperties: false on the profile keys) catches
typos before the tool hits the network.
- The diff function separates block (must-fix) from warn
(advisory) severity. required_status_checks + required_review
count are block; dismiss_stale_reviews + require_code_owner
are warn. A compliant repo has zero block-level drift;
warn-level drift is logged but doesn't fail the audit.
Three-body: delivery=Mavis (this PR), control=DARKXSIDE (operator
reviews the spec + the drift report, then runs apply manually for
each repo), memory=this trail + the spec + the LEARNINGS file.
CHIT trail unsigned-local (no CHIT_PASSPHRASE loaded in this Mavis
session).
* test(tools): 44 tests for branch_protection across 8 groups
Eight test groups cover the spec loader, the diff logic, the
apply body builder, audit + apply + drift-check end-to-end (with
mocked `gh api`), the CLI surface, and 2 error paths (gh missing
+ gh 404 for unprotected branch).
Test groups (each is a unittest.TestCase class):
- A. SpecLoaderTests (5) - load + resolve_repo_profile default +
with override + unknown repo + unknown profile
- B. DiffLogicTests (14) - 4 status-check scenarios, 3 review-policy
scenarios, 4 boolean-field scenarios (including the nested
{"enabled": bool} shape the real API returns), 3 rulesets
scenarios
- C. ApplyBodyTests (4) - classic body has all 9 required keys,
preserves values, ruleset body has 6 required keys, preserves
rules
- D. AuditTests (6) - compliant repo, no-protection-is-block,
missing-check-is-block, extra-check-is-warn, explicit profile,
unknown-repo-raises
- E. ApplyTests (5) - dry-run-no-calls, live-calls, skips-existing-
ruleset, includes-per-repo-override-checks, unknown-repo-raises
- F. DriftCheckTests (3) - one-report-per-repo, only-org-repos,
surfaces-audit-error-as-synthetic-drift
- G. CLITests (4) - audit-exits-0-when-compliant, audit-exits-1-
when-drift, drift-check-exits-2-when-any-drift, apply-default-
is-dry-run (the dry-run does ONE read for existing rulesets
so the output can accurately report skip-vs-create; no PUT or
POST is issued; the test asserts both)
- H. ErrorPathTests (2) - gh-missing-raises, gh-404-for-unprotected-
returns-none (the real GitHub API returns 404 + "Branch not
protected" stderr for an unprotected branch; the tool
recognizes this and returns None, not an error)
All 44 tests pass. The mocked subprocess calls use a lambda
side_effect keyed on (method, path) so the test surface is
explicit and the failure mode is "unmocked gh call" rather than
a silent no-op.
Test design notes (codified in the test docstring):
- The mocks use the SAME shape as the real GitHub API
response: required_status_checks.checks is a list of
{"context": "name"} objects; required_linear_history etc are
nested as {"enabled": bool}; rulesets is a list of dicts with
name + id + rules + bypass_actors. The diff logic was updated
to handle both the spec's bare-boolean shape and the API's
{"enabled": bool} shape, so a real audit + a unit test give
the same answer.
- The drift-check test (F3) ensures that an audit error (e.g.,
gh subprocess failure) doesn't crash the whole drift report -
the erroring repo appears with a synthetic DriftItem so the
operator can see which repos failed and why. This is the
pattern the Mavis cron relies on.
Three-body: delivery=Mavis (this), control=DARKXSIDE (operator
can run the tests locally with `python -m unittest
pmoves.tools.tests.test_branch_protection` before applying the
spec to the 2 forks), memory=this trail. CHIT trail unsigned-local.
* docs(agnote): branch protection v0 CLAIM row + 2-fork apply record
Records the Slice 2 fan-out: the PMOVES standard branch protection
tool landed (PR #2490) + both unprotected forks now have
the fork profile applied.
Real-run evidence (this commit's author):
- python -m pmoves.tools.branch_protection apply --repo
POWERFULMOVES/PMOVES-pinokio --no-dry-run → created classic
protection (CodeRabbit required, 1 reviewer, linear history,
conversation resolution) + [main] ruleset (id=20589542)
- python -m pmoves.tools.branch_protection apply --repo
POWERFULMOVES/PMOVES-hermes-agent --no-dry-run → created
classic protection (9 required status checks, 1 reviewer,
linear history, conversation resolution) + [main] ruleset
(id=20589548)
- python -m pmoves.tools.branch_protection drift-check --org
POWERFULMOVES (post-apply) → both repos compliant, zero drift
What's NOT in this slice (intentional follow-up):
- PMOVES.AI migration to rulesets-only (Option A approved by
operator; needs a separate migration script because the
current tool's `apply` doesn't support "delete classic +
consolidate rulesets"). The bypass_actor list from the
[main] ruleset (RepositoryRole id=5, Integration id=1144995,
Integration id=1236702) must be re-registered in the new
ruleset.
- NATS subject pmoves.branch_protection.drift.v1 (Slice 3)
- Mavis cron that calls drift-check daily (Slice 3)
- BRANCH_PROTECTION_BASELINE.md + pair-review skill update
(Slice 4)
Three-body: delivery=Mavis, control=DARKXSIDE, memory=this
trail + PR #2490. CHIT trail unsigned-local.
* feat(tools): PMOVES.AI branch-protection migration (Option A)
The one-off migration script that consolidates PMOVES.AI's
classic + 3-ruleset layered state into a single ruleset
([ main ]) with the status check + review requirements +
copilot_code_review + the 3 bypass_actors preserved.
What this slice lands:
- pmoves/tools/branch_protection_migrate_pmai.py - the
migration script. Pure-stdlib (no new deps), uses the existing
branch_protection.py helpers (the same `gh api` wrapper, the
same spec loader, the same dataclass patterns). 2 public
functions:
plan() - reads the current state, computes the new
[ main ] ruleset body, returns a MigrationPlan
apply(plan, dry_run=True) - issues DELETE classic + PUT
[ main ] ruleset; dry-run is the default
- pmoves/tools/tests/test_branch_protection_migrate_pmai.py -
15 tests across 4 groups (compute_main_ruleset,
capture_state, plan, apply). All 59 tests pass across
both the tool + the migration.
- pmoves/configs/branch_protection/pmoves_standard.json -
added `submodule-gitlink-gate` to the monorepo profile's
required status checks. The actual state has 5 required
checks; the original spec had 4. This aligns the spec
with reality.
The migration is destructive (DELETE classic + PUT ruleset
in a different shape), so dry-run is the default. The
operator reviews the call sequence + the captured state
before --no-dry-run is issued.
Design notes (codified in the LEARNINGS file):
- The list endpoint /rulesets returns a SUMMARY without
bypass_actors. The migration re-fetches the per-ruleset
body to get the full bypass_actors list. Without this
re-fetch, the migration would silently drop the operator's
preauthorized --admin bypass. Captured in LEARNINGS lesson 1.
- The pull_request rule in a ruleset uses different field
names than the classic required_pull_request_reviews
block. The migration explicitly maps the spec's
required_pull_request_reviews keys to the ruleset
pull_request parameters. Captured in LEARNINGS lesson 2.
- The spec's monorepo profile hard-codes RepositoryRole id=5
as the default bypass_actor. The migration OVERRIDES this
with the captured bypass_actors from the existing ruleset
(3 actors: RepositoryRole id=5, Integration id=1144995,
Integration id=1236702). The spec is the source of truth
for new repos; the migration preserves the operator's
actual escape hatch for this repo. Captured in LEARNINGS
lessons 5 + 6.
- The migration is a one-off. After it runs, the canonical
branch_protection.py apply tool keeps the [ main ]
ruleset in sync with the spec. The migration script is
archived in the tool's directory; the spec + the tool are
the source of truth going forward.
Migration call sequence (dry-run, current state):
1. DELETE /repos/POWERFULMOVES/PMOVES.AI/branches/main/protection
2. PUT /repos/POWERFULMOVES/PMOVES.AI/rulesets/10887588 with:
- name: [ main ]
- rules: deletion, non_fast_forward, pull_request (1 reviewer
+ code owner + dismiss stale + review thread resolution),
copilot_code_review, required_status_checks (5 checks),
- bypass_actors: 3 (preserved)
Three-body: delivery=Mavis, control=DARKXSIDE (operator
reviews the dry-run output before --no-dry-run), memory=this
trail + the LEARNINGS file + the BRANCH_PROTECTION_BASELINE.md
doc. CHIT trail unsigned-local.
* docs(operations+learnings): branch protection baseline + 5-class LEARNINGS
Two companion docs for the branch protection fan-out.
- pmoves/docs/operations/BRANCH_PROTECTION_BASELINE.md - the
human-readable version of pmoves_standard.json. Covers:
- Why a baseline (the 3-PR review pass surfaced the
asymmetric protection state; this doc is the fix)
- The 2 profiles (monorepo + fork) with field-level
rationale + the GitHub doc citation for each
- Current state per repo (PMOVES.AI: not yet migrated;
PMOVES-hermes-agent: applied; PMOVES-pinokio: applied)
- How to apply / audit / drift-check (with the exact
`python -m pmoves.tools.branch_protection` invocations)
- How to add a new repo or a new profile
- The PMOVES.AI migration plan (Option A, the next apply)
- Wire-up to the harness (load_bootstrap CGP, Mavis cron,
orchestrator dispatch)
- 5 references to the official GitHub docs (rulesets,
protected branches, troubleshooting, MergeStateStatus
enum, the LEARNINGS file)
- pmoves/tools/LEARNINGS/branch-protection-v0_LEARNINGS.md -
the 5-class taxonomy + 4-bucket learning signal per the
pr-trim convention. Populated with 13 already-fixed, 5
out-of-scope, 0 pre-existing observations. The "Pattern
update" section adds 6 new lessons to the pmoves-pair-review
skill's step 7:
1. The list endpoint /rulesets returns a SUMMARY without
bypass_actors. Re-fetch the per-ruleset body when
bypass_actors is needed.
2. The pull_request rule in a ruleset uses different field
names than the classic required_pull_request_reviews
block. Map explicitly.
3. additionalProperties: false is the right default for
required objects, but bypass_actors and status_checks
should stay open (extending pair-review lesson 3 to
nested arrays).
4. UNSTABLE = mergeable + bypass_actors re-fetch = mandatory.
Both are silent-corruption traps.
5. The spec is the source of truth for fresh repos, but
the migration captures the existing bypass_actors to
preserve the operator's escape hatch.
6. Migrate the operator's preauthorized bypass list
explicitly; don't rely on the spec's defaults.
Three-body: delivery=Mavis, control=DARKXSIDE, memory=this
trail + the spec + the migration script. CHIT trail
unsigned-local.
* refactor(tools): collapse branch_protection to ruleset-only per operator ratification
Per the operator's 2026-08-10 ratification (PR #2490 review id 4893614185) grounded
in GitHub's "About rulesets" docs: classic branch protection is NOT deprecated,
rulesets LAYER with it ("the most restrictive version of the rule applies"),
and "start using rulesets without overriding any of your existing protection rules"
is the intended adoption path. This collapses the original migration script
(N1/N2/N3 delete) + the classic-PUT body builder (P1-A/P1-C/N8 delete) into
the new ownership split:
- .github/workflows/branch-protection-sync.yml owns CLASSIC protection
- pmoves/tools/branch_protection.py owns RULESETS only
- The two writers layer additively (most-restrictive-wins)
- Additive adoption is monotonic - the tool can only make a branch stricter
What changes:
- pmoves/tools/branch_protection.py: dropped _build_classic_body +
_diff_required_status_checks + _diff_review_policy + _diff_boolean_field;
added SpecValidator (validates at load, per P1-B); added resolve_branch()
(per_repo_overrides -> .gitmodules -> spec default -> "main", per N4);
deep-diff _ruleset_matches() (rules + conditions + bypass_actors, per N6);
apply() now creates missing AND updates existing rulesets; _gh_api has
GH_TIMEOUT_SECONDS=30
- pmoves/configs/branch_protection/pmoves_standard.json: upgraded to v2
(pmoves.rulesets/v2); profiles only have rulesets: []; monorepo profile
carries 8 ruleset rules; fork profile has required_approving_review_count=0
(matches workflow default, per N5); per_repo_overrides includes PMOVES.AI +
PMOVES-hermes-agent + PMOVES-pinokio + PMOVES-nats-server (the new fork)
- pmoves/docs/operations/BRANCH_PROTECTION_BASELINE.md: rewritten with the
ownership split documented at the top
- pmoves/tools/LEARNINGS/branch-protection-v0_LEARNINGS.md: 5-class taxonomy
updated (15 already-fixed / 6 owner / 5 out-of-scope / 4 pre-existing);
2 new pair-review lessons (#7 merge-by-type ruleset overrides; #8
~DEFAULT_BRANCH sentinel in conditions.ref_name.include); ratification
documented (5 of 6 P1s collapse to deletions)
Bugs caught and fixed during the refactor:
- spec had require_linear_history typo (correct: required_linear_history)
- VALID_RULESET_RULE_TYPES was missing required_conversation_resolution
- _ruleset_matches() was running the conditions.ref_name.include comparison
after stripping the sentinel (should have skipped it entirely)
- resolve_repo_profile() was REPLACING the rules array in ruleset_overrides
(should have MERGED by type)
CHIT trail unsigned-local. Three-body: delivery=Mavis, control=DARKXSIDE,
memory=this commit + the spec + the LEARNINGS file.
* test(tools): rewrite 55 tests for ruleset-only branch_protection
The 44 old tests targeted the old classic+ruleset tool shape
(_build_classic_body, _diff_required_status_checks, _diff_review_policy,
_diff_boolean_field). Replaced with 55 tests across 9 groups for the
post-ratification ruleset-only API:
A. SpecValidatorTests (13 tests)
- spec shape, rule type validation, target/enforcement validation,
override->profile cross-check, multi-error collection, load_spec
path + skip-validation flag, validator set completeness
(required_conversation_resolution, required_linear_history)
B. ResolveRepoProfileTests (7 tests)
- default + ruleset_override merge, unknown repo/profile raise,
no-input-mutation, MERGE-BY-TYPE semantics (B6, B7)
C. ResolveBranchTests (4 tests)
- override wins, .gitmodules lookup matches workflow logic,
no-override-no-gitmodules -> main, slug extraction for PMOVES.AI
D. RulesetDiffTests (7 tests)
- compliant, missing rule, extra rule, drifted parameters,
drifted bypass_actors, drifted enforcement, ~DEFAULT_BRANCH
sentinel handling (D7)
E. AuditTests (5 tests)
- compliant repo, no rulesets drift, explicit profile, unknown
repo raise, per-ruleset re-fetch for bypass_actors (lesson #1)
F. ApplyTests (7 tests)
- dry-run creates, live creates, update existing with drift,
skip in-sync, per_repo ruleset_overrides, unknown repo raise,
strip ~DEFAULT_BRANCH sentinel
G. DriftCheckTests (3 tests)
- one report per repo, org filter, audit error surface
H. CLITests (4 tests)
- exit 0 compliant, exit 1 drift, exit 2 any-repo drift,
default dry-run
I. GHErrorPathTests (4 tests)
- gh missing, gh timeout, gh nonzero stderr, "Branch not protected"
returns None (404 is expected state)
Two new pair-review lessons (B6/B7 merge-by-type + D7 ~DEFAULT_BRANCH
sentinel) are codified in the LEARNINGS file.
CHIT trail unsigned-local.
* docs(agnote): Mavis::BRANCH-PROTECTION-V0-RATIFICATION-REFACTOR trail row
Records the 2026-08-10 refactor of the branch_protection tool to
ruleset-only per the operator's ratification (PR #2490 review id
4893614185). Captures the 5-of-6 P1s collapse to deletions, the
ownership split (tool = rulesets, workflow = classic), the spec v2
upgrade, the 4 additional bugs caught and fixed during the refactor
(require_linear_history typo, missing rule type, ~DEFAULT_BRANCH
sentinel handling, merge-by-type ruleset overrides), and the 55-test
rewrite.
CHIT trail unsigned-local. Three-body: delivery=Mavis, control=DARKXSIDE,
memory=this trail.
* refactor(tools): actually delete branch_protection_migrate_pmai.py
The 2026-08-10 ratification said the migration script goes away: classic
branch protection is not deprecated, rulesets layer with it, and "the most
restrictive version of the rule applies" — so there is nothing to migrate
away from and no reason to DELETE classic protection before a replacement
exists.
The refactor commit b0fbf68 rewrote branch_protection.py to ruleset-only
but left the script and its 15 tests on disk, while the PR comment reported
them as deleted. Verified against the tree: both files were still tracked at
78157b7. This makes the reported state the real state.
That closes the four findings that only existed because of the script:
N1 DELETE fires before any replacement (with a test asserting it should)
N2 signed commits + linear history silently dropped by the migration
N3 captured_required_status_checks captured, printed, never used
#20 migration test docstrings
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(tools): make the resolved branch load-bearing in the ruleset writer
Five defects, all in the path between "the spec says which branch" and
"the ruleset GitHub actually stores". Verified against the live org, not
just the diff.
1. ~DEFAULT_BRANCH was stripped, never substituted (CRITICAL)
_build_ruleset_body removed the sentinel from conditions.ref_name.include
and put nothing back, so a created ruleset carried an EMPTY include list
and matched no ref. It now takes the resolved branch and writes
refs/heads/<branch>. The old test asserted only assertNotIn(sentinel),
which an empty list satisfies — that is how it shipped.
2. The diff SKIPPED the include comparison whenever the spec used the
sentinel, so a ruleset pinned to the wrong branch reported compliant.
_ruleset_matches now takes the branch and resolves the sentinel on the
EXPECTED side only. A live ~DEFAULT_BRANCH stays unresolved on purpose:
it means "whatever GitHub currently calls default", which is not the
branch we mean.
Live evidence for why both matter: PMOVES-hermes-agent's default branch
is main, but the monorepo consumes PMOVES.AI-Edition-Hardened. The
ruleset applied in Slice 2 targets ~DEFAULT_BRANCH -> main. The branch
that actually ships has no ruleset, and audit called it compliant. It
now reports drift. This is N4 in production, not in theory.
3. .gitmodules lookup used `slug in section`, a substring match. The slug
PMOVES-nats matched submodule "PMOVES-nats-server" and would write that
repo's branch. Now matches the exact section name or the url basename.
4. resolve_branch step 3 looped over EVERY profile and returned the first
branch it found, so one profile declaring a branch would leak it onto
every repo without an override. It now takes the resolved profile name
and reads only that profile.
5. apply crashed on `created.get` when a POST returned an empty body
(_gh_api returns None). The write had already happened, so the repo was
left changed with no entry in `applied`. Now records it with a fallback id.
Also: rule parameters were compared by strict equality, but GitHub echoes
back its own defaults (required_reviewers, allowed_merge_methods) that the
spec never declares — every audit reported permanent drift and every apply
re-PUT a correct ruleset. Comparison is now a subset over spec-declared keys
only, which is what makes drift_check trustworthy enough to run on a cron.
Tests: 64 pass (was 55). New: C5-C7 (exact + url-basename match, profile
scoping), D8-D11 (include drift on a non-default branch, live sentinel not
silently matched, API-defaulted params not drift, declared mismatch still
reported), F7-F9 (substitution, no spec mutation, empty POST body).
Fixed A11, which patched read_text but not exists() and so passed on the
"spec not found" error without ever reaching validation; and I2-I4, which
depended on a real gh binary being on PATH.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* docs(branch-protection): correct the baseline + LEARNINGS against the tree
- NATS catalog link was ../nats-subjects.md, which resolves to
pmoves/docs/nats-subjects.md — a file that does not exist. Repointed at
the canonical .claude/context/nats-subjects.md. (A prior comment marked
this fixed; it was not.)
- require_linear_history -> required_linear_history everywhere. The v2
contract and the GitHub rule type both use the required_ prefix, and an
operator copying the table into pmoves_standard.json would fail validation.
- Lesson count 6 -> 8, and "5 of 6 P1s" -> "6 of 6" (the section lists six:
N1, N2, N3, P1-A, P1-C, N8).
- Rewrote lesson 8. It codified "skip the include check when the spec uses
the sentinel" as the right behavior; that was the bug. Replaced with the
general form: a sentinel a builder strips but never substitutes is a
silent no-op — resolve it, and assert on what replaced it rather than on
its absence.
- Lesson 5 notes that the migration script it was learned on is gone.
- Documented the release gate on --no-dry-run: claim -> work (dry-run,
confirm the resolved branch) -> sign -> release, with post-apply evidence.
If signing is unavailable, the release stays pending.
Two corrections that came out of reading the workflow rather than the diff:
- The doc said the monorepo profile "layers on top of whatever classic
protection branch-protection-sync.yml writes". It does not.
The workflow derives its scope from .gitmodules and PMOVES.AI is not a
submodule of itself, so on the monorepo there is no second writer and
required_approving_review_count: 1 is the only review gate in play. That
also means the N5 layering deadlock cannot apply to this profile — the
fork profile already resolves it at 0.
Flagged for the operator instead: apply --no-dry-run on PMOVES.AI would
newly enforce required_signatures and required_linear_history on main.
Both are real behavior changes to the merge flow, so they are called out
as decisions rather than defaults.
- Added the 2026-08-10 audit finding: the Slice 2 rulesets on
PMOVES-hermes-agent and PMOVES-pinokio target ~DEFAULT_BRANCH, so the
hardened gitlink branch is ungated. Remediation is a re-apply behind the
release gate.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* docs(agnote): 4090-CLAUDE::PR2490-TRIM-16-THREADS trail row
Appended as a correction rather than an edit to the preceding row: that
row recorded the migration script as deleted and the ~DEFAULT_BRANCH
include-skip as correct behavior, and both were wrong against the tree.
The historical row stays as written; this one records what was actually
found and what changed.
Also records the two decisions left to the operator (the PMOVES.AI
--no-dry-run, and the re-apply that remediates the wrong-branch rulesets
on the two Slice 2 forks) rather than taking them.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* docs(learnings): lesson 9 — a test that can only assert absence cannot say no
Promotes the root cause of lesson 8 to its own entry, at the team lead's
request, because the fix is a habit and the failure mode is silent.
The guard on the sentinel substitution was
`assertNotIn("~DEFAULT_BRANCH", includes)`, which passes on an empty list.
It therefore held green across exactly the two states it existed to
distinguish: sentinel correctly replaced by a real ref, and sentinel
deleted with nothing put back. A test that could only report success,
guarding a ruleset whose empty include list matched no ref while `apply`
printed "applied".
The tell is structural rather than domain-specific, so the lesson is
written to generalize: an assertion whose predicate is satisfied by the
empty/null/absent case is not a gate. assertNotIn, assertNotEqual,
assertFalse, "no error raised", an empty `grep -v`, `rc == 0` on a command
that no-ops when misconfigured — each admits a degenerate state alongside
the intended one. When a transform removes something, assert on what
replaced it.
With three checks in cost order: ask what the empty case does; mutate the
implementation to the degenerate state and confirm the test goes red (a
guard that survives its own sabotage was never a guard); and for tools
that write to an external system, verify against live state once. Here a
single `gh api ... --jq .default_branch` collapsed the whole question.
Lesson count 8 -> 9 in both this file and the baseline doc.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
---------
Co-authored-by: Mavis <Mavis@pmoves.local>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Changes
Test Plan