Bind the preflight scope to pm-github's declared command set - #42

@greptileai
/gemini review
@coderabbitai full review

Summary by CodeRabbit

• Bug Fixes

  • Fixed GitHub command handling to ensure mutating operations consistently require the appropriate credential preflight.
  • Prevented discrepancies between command scope and mutation classification.

• Tests

  • Added automated checks confirming mutating and read-only GitHub commands are classified correctly.

• Documentation

  • Added an Unreleased changelog entry describing the command validation fix.

Walkthrough

The change adds bidirectional smoke tests for GitHub preflight scope and mutating-command classification, records issue creation through closure, updates the changelog, and removes extensions-related merge-driver rules.

Changes

GitHub scope alignment

Merge-driver cleanup

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: 🟡 Moderate · up to d6781

The PR adds a drift guard, but it does not yet verify the complete set of declared commands against the preflight scope. A future mutating command could be omitted while tests still pass, allowing it to run without the intended early credential warning; the test should be completed before merge.

Possibly related PRs

• unbraind/pm-github#28: Introduced related GitHub preflight and mutating-command consistency.
• unbraind/pm-github#36: Added the scoped GitHub preflight command list covered by these alignment tests.

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 @coderabbitai help to get the list of available commands.}

Reviewer's Guide

This PR strengthens the preflight credential gate for pm-github by binding the override scope directly to the mutating-command classifier, and adds tests plus changelog and pm agent artifacts to guard against future drift between declared commands and the scoped override.

Flow diagram for the new preflight scope drift guard

flowchart TD
  A[Collect declared github commands] --> B[Compute preflight_scoped_paths]
  A --> C[Filter commands where isMutatingGithubCommand is true]
  C --> D[Assert preflight_scoped_paths equals mutating_commands]
  A --> E[Find declared_but_omitted_paths]
  E --> F[Assert isMutatingGithubCommand is false for each omitted_path]
  D --> G[Fail test if mismatch]
  F --> G
  G --> H[Prevent drift between mutating command set and preflight override scope]

File-Level Changes

@unbraind I will perform a complete review of PR #42, including the declared command set and the preflight scope drift guard.

Greptile Summary

The PR extends the activation smoke test to derive command paths from real registrations and compare the preflight scope with classifier-derived mutating paths.

• Collects command, importer, and exporter registrations and cross-checks dispatch handlers.
• Partitions declared paths using the production mutation predicate.
• Adds the associated changelog and PM issue-history artifacts.

Confidence Score: 5/5

The PR appears safe to merge because no blocking failure remains.

No blocking failure remains.

_{Reviews (2): Last reviewed commit: "Derive the preflight drift guard from re..." | Re-trigger Greptile}

Pushed d85872a — re-review requested.

Both review findings adopted. The drift guard no longer compares against a hand-maintained list; it derives the declared command set from real activation (registerCommand / registerImporter / registerExporter) and asserts the mutating and read-only classes reconstruct that set exactly, so a declared command nobody classified fails the test.

Proven non-vacuous by declaring a scratch mutating command and confirming the suite fails naming it (1 of 251), then removing it — 251/251 green. Full output in the thread replies.

Note: the sibling pm-gantt-chart PR found a live instance of this exact class — gantt export, derived from registerExporter, was covered by the old global override by accident and would have been silently dropped. The finding was not hypothetical.

@greptileai
/gemini review
@coderabbitai full review

#!/bin/bash
set -euo pipefail

echo '== PR commit and changed files =='
git status --short
git show --stat --oneline --decorate --no-renames d85872a
git diff d85872a^ d85872a --no-ext-diff -- . ':(exclude)dist/**'

echo
echo '== Candidate file locations =='
fd -t f -a 'smoke\.test\.ts|github.*\.(ts|js)|.*\.(ts|js)$' . \
  | sed -n '1,240p'

echo
echo '== Symbol outlines =='
if [ -f test/smoke.test.ts ]; then
  ast-grep outline test/smoke.test.ts --items all --view expanded || true
fi

echo
echo '== Registration and classifier references =='
rg -n -C 4 -P '\b(registerCommand|registerImporter|registerExporter|isMutatingGithubCommand)\b' \
  -g '*.{ts,js,mjs,cjs}' \
  -g '!dist/**' .

#!/bin/bash
set -euo pipefail

echo '== Classifier and preflight implementation =='
sed -n '4555,4730p' index.ts

echo
echo '== Complete activation registrations =='
sed -n '4845,5095p' index.ts

echo
echo '== Registration model definitions available in the repository =='
rg -n -C 4 -P '(interface|type)\s+.*(Registration|Importer|Exporter|Command)|registrations:\s*\{|handlers:\s*\[' \
  -g '*.{ts,d.ts}' \
  -g '!dist/**' \
  -g '!test/**' .

echo
echo '== Tests that exercise classifier false/default behavior =='
rg -n -C 4 -P '\bisMutatingGithubCommand\s*\(' test index.ts

echo
echo '== Registration calls with their enclosing option blocks =='
ast-grep run --lang typescript \
  --pattern 'api.registerCommand({$$$})' index.ts
ast-grep run --lang typescript \
  --pattern 'api.registerImporter($_, $_, $_)' index.ts
ast-grep run --lang typescript \
  --pattern 'api.registerExporter($_, $_, $_)' index.ts

@unbraind I found one remaining gap.

• Major — The test does not fail for an unclassified mutating command. isMutatingGithubCommand() returns false by default in index.ts. At Line 214, every declared command for which the classifier returns false is placed in readOnlyPaths. Therefore, if a developer registers github new-write with a mutating handler but does not add it to isMutatingGithubCommand(), the command is treated as read-only, omitted from the preflight scope, and all current assertions pass.

  The non-vacuous check described in the PR updates both the registration and classifier. It proves that scope drift is detected after classification, but it does not prove that missing classification is detected.

  Define mutation intent at the command registration boundary, then derive both the classifier and preflight scope from that declaration. Alternatively, add a separate source-level guard that requires an explicit mutation classification for every declared command. The guard must distinguish read-only from unclassified; a boolean predicate with a false default cannot make that distinction.

_{You are interacting with an AI system.}