fix(mt#1366): Add acceptStaleReviewerSilence waiver to session_pr_merge for bot PR self-approval deadlock

[minsky-ai]

Summary

Addresses minsky-reviewer[bot] round-3 BLOCKING (B1) and two NON-BLOCKING findings on PR #844.

Changes

B1 (BLOCKING) - CLI flag description enumerates all four enforced constraints

Updated the acceptStaleReviewerSilence parameter description in src/adapters/shared/commands/session/session-parameters.ts to explicitly document all four conditions that must hold for the waiver to apply:

1. PR author must be minsky-ai[bot] -- waiver never applies to human-authored PRs.
2. At least one COMMENTED review from the same identity as the PR author must exist.
3. No non-DISMISSED CHANGES_REQUESTED review may exist (DISMISSED reviews are excluded from this check).
4. No review from minsky-reviewer[bot] may exist -- waiver is inapplicable when the reviewer bot has already acted.

NON-BLOCKING - Misleading hint for human-authored PRs

In src/domain/session/session-merge-operations.ts, the default rejection error message previously appended the acceptStaleReviewerSilence hint whenever hasReviewerBotReview was false, even for human-authored PRs where the waiver can never apply. Gated the hint on isPrAuthorBot && !hasReviewerBotReview so it only appears when the waiver is plausible.

NON-BLOCKING - Centralize bot identity constant

Moved BOT_IDENTITY_LOGIN = "minsky-ai[bot]" from session-merge-operations.ts to a new src/domain/constants.ts shared module. The constant is re-exported from session-merge-operations.ts for backward compatibility with any existing consumers.

Testing

All changes are behavioral refinements to error messages and parameter descriptions. The merge logic itself is unchanged. Pre-commit hooks (lint, typecheck) validate the structural correctness of the edits.

[minsky-reviewer]

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: unknown

---

Findings

[BLOCKING] Spec-mismatch: Waiver not restricted to self-authored bot PRs

• Evidence: src/domain/session/session-merge-operations.ts: lines around 205–310 (waiverEligible computation)
  • The PR description explicitly states the waiver is “for self-authored bot PRs” and references “same-App-identity self-approval rule.” The implementation never checks the PR author identity at all before applying the waiver.
  • Code only checks:
    • acceptStaleReviewerSilence flag is true
    • No CHANGES_REQUESTED review
    • No review from “minsky-reviewer”
    • Presence of at least one COMMENTED review
  • There is no guard verifying the PR author is a bot (e.g., minsky-ai[bot]) or any bot user, nor that it is the same App identity creating the PR.
  • Consequence: An operator could set acceptStaleReviewerSilence=true and merge any PR that has a COMMENTED review and no reviewer-bot review, regardless of author identity — this is beyond the stated safety boundary.
• Required fix: Enforce that the PR is authored by the expected bot/same App identity before applying the waiver (e.g., compare sessionRecord.pullRequest.github?.author against the configured bot identity) and document this constraint explicitly.

[BLOCKING] Spec-mismatch: “Same-identity COMMENTED review” is not enforced

• Evidence: src/domain/session/session-merge-operations.ts: lines around 241–248 (hasCommentedReview)
  • The design says “At least one COMMENTED review exists (same-identity review confirming no blockers).” The implementation only checks for any COMMENTED review from any reviewer.
  • There is no check tying the COMMENTED review’s reviewerLogin to the PR author identity or same-App identity.
  • Consequence: A COMMENTED review from any third party can satisfy the waiver condition, which weakens the safety assumption of “same-identity.”
• Required fix: Ensure that the COMMENTED review is from the same App identity as the PR author (or, minimally, the intended “self” identity), and that this is asserted in code and covered by tests.

[NON-BLOCKING] Over-broad reviewer-bot detection via substring match

• Evidence: src/domain/session/session-merge-operations.ts: lines around 233–238
  • hasReviewerBotReview uses r.reviewerLogin.includes("minsky-reviewer")
  • Risks:
    • False positives if a different user contains this substring.
    • Potential false negatives if the exact bot login format differs (e.g., missing “[bot]” or if future renames occur).
  • Suggestion: Compare exact login "minsky-reviewer[bot]" (and/or configurable list), case-normalized. Consider excluding dismissed bot reviews if they should not count as “exists.”

[NON-BLOCKING] Dismissed/obsolete reviews not accounted for; stale CHANGES_REQUESTED can block indefinitely

• Evidence:
  • src/domain/repository/github-pr-approval.ts: lines around 108–152 (reviews extraction)
  • src/domain/session/session-merge-operations.ts: lines around 226–232 (hasChangesRequested)
  • listReviews returns historical reviews (including REQUEST_CHANGES) that may later be dismissed or superseded by approval. The implementation treats the presence of any CHANGES_REQUESTED review as blocking the waiver, even if it is obsolete or dismissed.
  • Suggestion: Use the most recent state per reviewer and/or filter out dismissed reviews (Octokit review objects include state "DISMISSED"). Alternatively, resolve current effective state via the reviews timeline logic you already use for isApproved, but extended to non-approval states for the waiver path.

[NON-BLOCKING] Flag surfaced on “session.approve” parameters but not used there

• Evidence:
  • src/adapters/shared/commands/session/session-parameters.ts: lines 86–99 (acceptStaleReviewerSilence added to sessionApproveCommandParams)
  • src/adapters/shared/commands/session/workflow-commands.ts: createSessionApproveCommand still uses sessionApproveCommandParams but never consumes the flag.
  • Result: CLI help for session.approve includes acceptStaleReviewerSilence, which is irrelevant for that command. This is confusing UX.
  • Suggestion: Split parameter sets or document that this flag is only used by session.pr.merge.

[NON-BLOCKING] “No reviewer-bot review exists” criterion may be too strict for non-APPROVE reviews

• Evidence: src/domain/session/session-merge-operations.ts: lines around 233–238
  • Any review from minsky-reviewer[bot] (COMMENTED, DISMISSED, etc.) blocks the waiver. The PR description says “waiver only applies to the webhook-miss class,” which seems to indicate the bot hadn’t fired at all. If the bot left a harmless COMMENT, the waiver would be suppressed even though approval is still missing.
  • This may be intended, but it’s worth confirming. If the intended constraint is “no APPROVED review by the reviewer-bot,” then the current check is too broad. If the intent is literally “no review at all by the bot,” this is fine — consider documenting explicitly and testing COMMENT-only and DISMISSED cases for clarity.

Spec verification

• Waiver applies only to “self-authored bot PRs” (same-App identity): Not Met
  • Implementation doesn’t check author identity.
• Waiver requires no CHANGES_REQUESTED: Met
  • hasChangesRequested check implemented.
• Waiver requires no reviewer-bot review: Met (but broad)
  • Any review by minsky-reviewer blocks waiver; see NON-BLOCKING note.
• Waiver requires at least one COMMENTED review (same-identity): Not Met
  • Only checks for any COMMENTED; no same-identity enforcement.
• INFO-level audit entry includes PR number, author identity, COMMENT reviewers, and waiver used: Met
  • src/domain/session/session-merge-operations.ts: lines ~252–270.
• Default-blocked error names the acceptStaleReviewerSilence flag: Met
  • src/domain/session/session-merge-operations.ts: lines ~292–310.
• Acceptance scenarios covered by tests:
  • Default-blocked: Met
  • Flag-allowed under intended conditions: Partially Met (tests cover flag path but do not assert author identity/same-identity commenter; reflects the implementation gap)
  • CHANGES_REQUESTED still blocked: Met
  • Normal happy path merges with reviewer-bot approval: Met

Documentation impact

• CLI help and operator docs should be updated:
  • Document the new acceptStaleReviewerSilence flag on session.pr.merge, its exact constraints, and the audit log behavior.
  • If keeping the flag in session.approve parameter set, document that it only affects session.pr.merge to avoid confusion.
• Security/operations notes should explicitly state the author-identity and same-identity COMMENTED review constraints once implemented (currently missing).

Conclusion

REQUEST_CHANGES

Primary blockers are spec mismatches that widen the waiver applicability:

• Missing restriction to self-authored bot PRs.
• Missing enforcement that the COMMENTED review is from the same identity as the PR author.

Please add those guards, tighten reviewer-bot detection as appropriate, and extend tests to cover:

• Non-bot PR author should not be eligible for the waiver.
• COMMENTED review by a third party should not satisfy the “same-identity” condition.
• Optional: clarify behavior when reviewer-bot left non-APPROVE reviews (COMMENTED or DISMISSED) and add tests accordingly.

[minsky-reviewer]

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: unknown

---

Findings

[BLOCKING] Waiver gate relies on an incomplete (non-paginated) reviews list — risk of false eligibility

• Evidence:
  • src/domain/repository/github-pr-approval.ts: lines ~66–116 (getPullRequestApprovalStatus) call octokit.rest.pulls.listReviews without pagination and then derive both approvals and rawReviews from that single page.
  • src/domain/session/session-merge-operations.ts: lines ~200–290 use approvalStatus.rawReviews to enforce waiver conditions, specifically:
    • hasReviewerBotReview = rawReviews.some(r => r.reviewerLogin.toLowerCase() === "minsky-reviewer[bot]")
    • hasCommentedReview = rawReviews.some(r => r.state === "COMMENTED" && r.reviewerLogin === prAuthor)
• Failure mode:
  • listReviews returns paginated results (default per_page ~30). If minsky-reviewer[bot] has reviewed on a later page, hasReviewerBotReview will incorrectly be false, allowing the waiver to fire despite an existing reviewer-bot review (contradicting the PR’s “No minsky-reviewer[bot] review exists” condition).
  • Similarly, a same-identity COMMENT review (or a CHANGES_REQUESTED review) could be on a later page and be missed, leading to an incorrect block/allow decision.
• Consequence:
  • Silent behavior change: merges can be allowed under the waiver when they should not be (or blocked when they should be allowed) due to incomplete review data.
• Requested change:
  • Implement pagination for pulls.listReviews (loop until no more pages or use Octokit’s paginate) before computing approvals, rejections, and rawReviews. Recompute approvalStatus fields from the full set.

[NON-BLOCKING] Case-sensitive match for “same-identity COMMENT review” is brittle

• Evidence: src/domain/session/session-merge-operations.ts: lines ~240–250 uses r.reviewerLogin === prAuthor while other identity checks normalize case (e.g., isPrAuthorBot).
• Failure mode:
  • If GitHub returns casing variations for the author login (unlikely for bots but possible in generalized use), the same-identity COMMENT could be missed and the waiver incorrectly denied.
• Suggested improvement:
  • Normalize both sides to lower case for the same-identity comparison, consistent with the minsky-reviewer[bot] check.

[NON-BLOCKING] CLI parameter exposure is broader than its effect, which may confuse users

• Evidence:
  • src/adapters/shared/commands/session/session-parameters.ts: acceptStaleReviewerSilence added to sessionApproveCommandParams.
  • src/adapters/shared/commands/session/workflow-commands.ts: session.approve and session.pr.approve reuse sessionApproveCommandParams but do not thread acceptStaleReviewerSilence anywhere in those flows — only session.pr.merge passes it to mergeSessionPr.
• Failure mode:
  • Users invoking session.approve or session.pr.approve may see or use the flag with no effect. The description says “Only used by session.pr.merge,” but the flag appears in the other commands’ schemas/help.
• Suggested improvement:
  • Either remove the flag from parameter sets used by non-merge commands, or explicitly hide/ignore it in help output for those commands and document its scope clearly.

[NON-BLOCKING] Hard-coded bot identity string reduces configurability and may drift from actual bot login

• Evidence: src/domain/session/session-merge-operations.ts: lines ~228–234 set botIdentityLogin = "minsky-ai[bot]".
• Risk:
  • If the bot login changes (org rename, reinstallation, environment differences), the waiver will never apply even when intended.
• Suggested improvement:
  • Source the bot identity from configuration (the same source used elsewhere for service identity), and/or reuse a single constant from a central module to avoid hard-code drift.

[PRE-EXISTING] GitHub approval pre-check swallows API errors and proceeds to attempt merge

• Evidence: src/domain/session/session-merge-operations.ts: lines ~296–312 catch-all around getApprovalStatus logs “Skipping pre-merge approval check due to API error. Proceeding with merge attempt.”
• Context:
  • This predates the waiver logic but remains relevant: merge proceeds relying on sessionRecord.prApproved rather than current GitHub approval state if the API call fails.
• Note:
  • Calling this out for completeness; not introduced by this PR. If that behavior is intended, consider adding an INFO audit entry when the pre-check is skipped due to API errors.

Spec verification

• Add explicit operator-override waiver path (boolean flag default false): Met
  • Flag added (acceptStaleReviewerSilence) with default false; threaded into session.pr.merge only.
• Waiver applies only when ALL conditions hold:
  1. No CHANGES_REQUESTED review: Met (filters out DISMISSED; checks for CHANGES_REQUESTED).
  2. No minsky-reviewer[bot] review: Met (any review by that login blocks waiver).
  3. At least one COMMENTED review exists (same identity as PR author): Met.
  4. Self-authored bot PR: Met (restricts to author === "minsky-ai[bot]").
• Populate rawReviews from full listReviews response: Partially Met
  • Implemented, but listReviews not paginated, so “full” is only the first page.
• Audit log entry at INFO level when waiver used: Met
  • log.info emits a structured message with PR number, author, and COMMENT reviewers.
• Default-blocked error names acceptStaleReviewerSilence as the operator path: Met
  • Message includes note about the flag when minsky-reviewer[bot] review is absent.

Documentation impact

• CLI behavior and operator procedure should be updated:
  • Document the new acceptStaleReviewerSilence flag, its strict conditions, its scope (session.pr.merge only), and the audit log emission.
  • Clarify that the flag appearing in approve-related commands has no effect, or better, limit its exposure to merge-only in the CLI docs/help.
• Architecture notes:
  • ApprovalStatus now includes rawReviews; repository adapters must ensure full review enumeration (pagination) to keep waiver logic dependable.

Conclusion

REQUEST_CHANGES

The pagination gap in getPullRequestApprovalStatus introduces a real risk that the waiver will trigger (or be denied) based on an incomplete review set, undermining the safety constraints described in the PR. Please address pagination for reviews before relying on rawReviews to gate merges. The other items are advisable improvements but non-blocking.

[minsky-reviewer]

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: unknown

---

Findings

[BLOCKING] Spec-doc mismatch: CLI flag description omits enforced constraints (bot-identity + same-identity COMMENT requirement)

• Evidence: src/adapters/shared/commands/session/session-parameters.ts: acceptStaleReviewerSilence parameter description currently reads:
  • "Operator-override waiver: allow merge for self-authored bot PRs when minsky-reviewer[bot] is absent (webhook-miss class). Requires no CHANGES_REQUESTED review and at least one COMMENTED review. Emits an audit log entry when used. Default: false."
• The implementation in src/domain/session/session-merge-operations.ts explicitly requires:
  • PR author is minsky-ai[bot] (BOT_IDENTITY_LOGIN)
  • No CHANGES_REQUESTED review (with DISMISSED excluded)
  • No review from minsky-reviewer[bot] present
  • At least one COMMENTED review from the same identity as the PR author
• Failure mode: The CLI help text does not document the “same-identity” requirement for the COMMENTED review, and it also doesn’t clearly state that the waiver is restricted to minsky-ai[bot] authored PRs (it hints “self-authored bot PRs” but lacks explicit same-identity review constraint and can be read as looser than the code). The PR description claims “Flag JSDoc updated … documents bot-identity requirement, DISMISSED exclusion, same-identity COMMENT requirement,” but the in-repo param doc does not reflect all of that. This is a sensitive, operator-override flag — ambiguity in CLI docs is unsafe and can lead to misuse.
• Ask: Update the acceptStaleReviewerSilence description in session-parameters.ts to explicitly state:
  • Only for PRs authored by minsky-ai[bot]
  • Requires at least one COMMENTED review from the same identity as the PR author
  • Excludes DISMISSED from CHANGES_REQUESTED consideration
  • Waiver does not apply if any review by minsky-reviewer[bot] exists

[NON-BLOCKING] Misleading default rejection hint for human-authored PRs

• Evidence: src/domain/session/session-merge-operations.ts: around the default “no waiver” path, the error message conditionally appends:
  • Note: minsky-reviewer[bot] has not reviewed this PR. If the reviewer bot is silent (webhook-miss), you may use acceptStaleReviewerSilence=true as an operator-override waiver.
• This hint is displayed whenever hasReviewerBotReview is false, regardless of whether the PR author is the bot. For human-authored PRs, the waiver will never apply (and will error out with a different message), so the hint encourages a dead-end attempt.
• Ask: Gate the hint on isPrAuthorBot to avoid suggesting the waiver for human-authored PRs, or add a clarifying clause “…only for PRs authored by minsky-ai[bot].”

[NON-BLOCKING] Missing targeted test coverage for pagination edge-case

• Evidence: src/domain/repository/github-pr-approval.ts now uses octokit.paginate for listReviews, addressing the prior blocking review about incomplete review pages.
• There is no test that simulates >100 reviews or a scenario where minsky-reviewer[bot] appears on a non-first page to verify the waiver gate correctly sees it.
• Ask: Add a unit test stubbing octokit.paginate to return at least two pages and ensure hasReviewerBotReview detection works when the reviewer-bot review is only present on page 2+.

[NON-BLOCKING] Bot identity constant is locally scoped; centralization would reduce drift risk

• Evidence: src/domain/session/session-merge-operations.ts defines export const BOT_IDENTITY_LOGIN = "minsky-ai[bot]".
• Other modules or future features might need the same constant. Keeping it local invites drift and redefinition.
• Ask: Move BOT_IDENTITY_LOGIN to a shared constants module (e.g., src/domain/constants.ts or similar) and import where needed.

[PRE-EXISTING] Approval pre-check swallows API errors and proceeds to merge

• Evidence: src/domain/session/session-merge-operations.ts: in the GitHub pre-merge approval check, any non-ValidationError is caught and logs “Skipping pre-merge approval check due to API error. Proceeding with merge attempt.”
• While validateSessionApprovedForMerge enforces a local sessionRecord.prApproved === true, bypassing the GitHub review/state fetch on API failure can still allow merging without the additional reviewer-bot gate. This behavior predates the PR and is not introduced here, but is security-sensitive and worth a follow-up.
• Ask: Consider documenting this explicitly (and potentially adding a config to hard-fail on API errors for protected repos).

Spec Verification

• No formal task spec provided. Using PR description as intent.

Intent from PR Description vs Diff:

• B1 (waiver restricted to self-authored bot PRs): Met — session-merge-operations.ts enforces prAuthor === minsky-ai[bot]; tests cover B1.
• B2 (same-identity COMMENTED review required): Met — hasCommentedReview now matches reviewerLogin === prAuthor; tests cover B2.
• Reviewer-bot detection exact match: Met — .toLowerCase() === "minsky-reviewer[bot]" used.
• Stale CHANGES_REQUESTED (DISMISSED excluded): Met — filter excludes DISMISSED before CHANGES_REQUESTED; tests cover dismissed case.
• Pagination for reviews list (prior blocking review): Met — github-pr-approval.ts switched to octokit.paginate for listReviews.

Documentation Impact

• Requires updates:
  • CLI parameter help for acceptStaleReviewerSilence in src/adapters/shared/commands/session/session-parameters.ts (BLOCKING).
  • Optionally, user/ops docs and CHANGELOG to explain the new waiver, its constraints (bot-authored only, same-identity comment, DISMISSED handling, reviewer-bot absence), audit logging behavior, and risks.
  • Consider updating error hint strings to avoid suggesting the waiver for human-authored PRs.

Conclusion

REQUEST_CHANGES

• Address the BLOCKING CLI doc mismatch for acceptStaleReviewerSilence so the operator-facing description matches the enforced constraints.
• The remaining items are non-blocking but recommended for clarity, test robustness, and maintainability.

[minsky-reviewer]

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: unknown

---

Findings

[BLOCKING] Misleading success message after waiver path

• Evidence: src/domain/session/session-merge-operations.ts: in mergeSessionPr(), after the waiver eligibility block inside the GitHub approval check, the code logs “✅ PR is approved and mergeable” unconditionally when params.json is false. This log happens even when approvalStatus.isApproved is false and the merge proceeds only because waiverEligible was true.
  • Snippet: after if (!approvalStatus.isApproved) { … waiver logic … } the code falls through to:
    if (!params.json) {
    log.cli(✅ PR is approved and mergeable);
    }
  • Failure mode: Users invoking session.pr.merge with acceptStaleReviewerSilence=true and meeting waiver conditions will see a success message that states the PR is “approved and mergeable,” which is false. This is a silent UX/observability bug and can cause operators to believe branch protection/approval conditions were met when they were not (they were explicitly waived).
  • Required change: Condition the success message on approvalStatus.isApproved (and possibly canMerge), and/or emit a different message for the waiver path.

[BLOCKING] Waiver path ignores additional merge blockers (draft/mergeable_state/status checks) yet presents normal flow

• Evidence: src/domain/session/session-merge-operations.ts: the waiver applies solely based on rawReviews and author identity; it does not check approvalStatus.canMerge (which considers pr.mergeable and PR open) or any status checks. After a waiver, the code proceeds to attempt merge with repositoryBackend.pr.merge(). If the token has elevated permissions (or repo settings allow), this may merge despite failing checks or draft state; if not, it fails later with a backend error. Either way, the CLI messaging (see previous finding) suggests a normal green state.
  • Specific lines: same region as above — waiverEligible bypasses any consideration of approvalStatus.canMerge and then falls through to the generic “approved and mergeable” message.
  • Failure mode: Operator override is documented as addressing reviewer-bot silence, but the current implementation can also bypass other gating conditions without warning (e.g., draft PR, failing checks, mergeable=false). This is a system-level incoherence risk versus documented intent.
  • Required change: If the waiver is intended only for reviewer-bot silence, guard against other blockers by checking approvalStatus.canMerge and surface blockers (or explicitly document and log that other blockers will be ignored and merge will attempt anyway). At minimum, adjust messaging to reflect that a waiver bypass is being used and not imply approvals/checks are satisfied.

[NON-BLOCKING] DISMISSED handling asymmetry for reviewer-bot detection

• Evidence: src/domain/session/session-merge-operations.ts: hasChangesRequested filters out state === "DISMISSED" before treating a review as blocking, but hasReviewerBotReview treats any review by “minsky-reviewer[bot]” as present, regardless of state. If the reviewer bot’s review was dismissed, the waiver remains inapplicable per current logic.
  • Snippet:
    • hasChangesRequested = rawReviews.filter(r => r.state !== "DISMISSED").some(r => r.state === "CHANGES_REQUESTED")
    • hasReviewerBotReview = rawReviews.some(r => r.reviewerLogin.toLowerCase() === "minsky-reviewer[bot]")
  • Concern: This may be an intentional policy, but it’s inconsistent with the DISMISSED carveout for CHANGES_REQUESTED. If the reviewer-bot’s CHANGES_REQUESTED (or any review) is DISMISSED, should the waiver consider the bot “absent”? If yes, this logic prevents waiver application erroneously.
  • Action: Clarify policy and either document the current behavior in the flag description or align detection to exclude DISMISSED reviews for reviewer-bot presence as well.

[NON-BLOCKING] Potentially duplicated/confusing test invocations

• Evidence: src/domain/session/session-merge-security-validation.test.ts: multiple tests call mergeSessionPr twice in a row asserting rejects, e.g., around the “Acceptance 1 (default)” case. This duplicates work with no added coverage and can increase test runtime and flakiness.
  • Suggested change: Remove the duplicate second call or combine assertions on a single awaited rejection.

[NON-BLOCKING] Spec/UX docs likely need updates

• Evidence:
  • New CLI flag added: src/adapters/shared/commands/session/session-parameters.ts (acceptStaleReviewerSilence). The in-code parameter description is detailed, but there’s no update here to user-facing docs or release notes.
  • Constants file introduced: src/domain/constants.ts centralizes BOT_IDENTITY_LOGIN; architecture notes may need a pointer if other modules rely on the constant (and the re-export from session-merge-operations suggests there may be external imports to audit).
  • Action: Update docs/CLI help, CHANGELOG, and any architecture notes mentioning reviewer waiver, reviewer identity matching, and DISMISSED treatment.

[PRE-EXISTING] ApprovalStatus.prState may not align with GitHub API

• Evidence: src/domain/repository/github-pr-approval.ts and approval-types.ts: prState is typed as "open" | "closed" | "merged" | "draft", but GitHub’s PR “state” is "open"|"closed" and “merged” is a separate boolean flag; “draft” is a separate field. This existed prior to the change; the diff didn’t introduce it.
  • Not actionable for this PR, but note for future cleanup.

Spec verification table

• No standalone task spec provided. Evaluated against PR description.

1. Waiver restricted to self-authored bot PRs (minsky-ai[bot]) — Met

   • session-merge-operations.ts enforces prAuthor matches BOT_IDENTITY_LOGIN (case-insensitive). New tests cover B1.

2. Same-identity COMMENTED review required (from PR author) — Met

   • session-merge-operations.ts checks state === "COMMENTED" and reviewerLogin equals prAuthor (case-insensitive). New tests cover B2.

3. Reviewer-bot detection exact match — Met

   • session-merge-operations.ts uses reviewerLogin.toLowerCase() === "minsky-reviewer[bot]".

4. DISMISSED CHANGES_REQUESTED excluded — Met

   • hasChangesRequested filters out r.state === "DISMISSED". Test added.

5. CLI flag description updated — Met

   • session-parameters.ts description lists all four constraints and usage scope.

6. Reviews pagination for waiver correctness — Met

   • github-pr-approval.ts switched to octokit.paginate for listReviews.

Documentation impact

• CLI documentation/help: Add acceptStaleReviewerSilence to session.pr.merge and session.changeset.merge help. Ensure the constraints match code. Note explicitly what other blockers (draft, status checks, non-mergeable state) do when waiver is used.
• Release notes/CHANGELOG: Announce waiver flag and bot-only restriction.
• Architecture notes: Reference src/domain/constants.ts for BOT_IDENTITY_LOGIN and any planned future identity changes.

Conclusion

REQUEST_CHANGES

The waiver path currently emits a misleading “PR is approved and mergeable” success message even when proceeding under a waiver, and it does not account for (or clearly communicate about) other merge blockers beyond approvals. Please fix the messaging and, if intended, explicitly guard or clearly document behavior regarding other blockers when the waiver is applied. The remaining items are non-blocking but recommended for polish and clarity.

[minsky-reviewer]

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: unknown

---

Findings

[BLOCKING] Waiver logic is unattainable due to canMerge coupling to isApproved

• Evidence:
  • src/domain/repository/github-pr-approval.ts:216-220 computes canMerge as isApproved && !!pr.mergeable && pr.state === "open". This makes canMerge false whenever isApproved is false.
  • src/domain/session/session-merge-operations.ts:328-346 sets waiverEligible when isApproved is false (that’s the only context where the waiver applies) and then immediately refuses the waiver if (!approvalStatus.canMerge).
• Failure mode: The waiver is intended to allow merging when approval is missing solely because the reviewer bot is silent. However, since canMerge is defined as isApproved && mergeable && open, canMerge will always be false whenever isApproved is false. Therefore, the code will always reject the waiver with “Another merge blocker is active” even when there are no blockers other than the missing bot approval. The “flag-allowed” tests pass only because they stub approvalStatus.canMerge = true while isApproved = false (src/domain/session/session-merge-security-validation.test.ts:360-384), a state the real getPullRequestApprovalStatus implementation will never produce.

[BLOCKING] CLI flag description omits the fifth enforced constraint (canMerge)

• Evidence:
  • src/adapters/shared/commands/session/session-parameters.ts:302-313 acceptStaleReviewerSilence description lists four constraints only; it does not mention any requirement about canMerge/mergeability conditions.
  • src/domain/session/session-merge-operations.ts:114-131 documents an additional constraint in the JSDoc: “approvalStatus.canMerge is true (PR is not a draft, no merge conflicts, no failing checks).”
• Failure mode: The CLI help now diverges from actual enforcement behavior: the code enforces five conditions (including canMerge), but the flag’s CLI description only enumerates four. This is the exact spec-doc mismatch your prior review sought to correct, now reintroduced in a different place.

[BLOCKING] Draft detection and prState reporting are incorrect; leads to misleading error messages

• Evidence:
  • src/domain/repository/github-pr-approval.ts:239 sets prState to pr.state cast to "open" | "closed" | "merged" | "draft". On GitHub, pr.state is "open" for both normal and draft PRs; draft status is not represented via pr.state, but via a separate draft flag/mergeable_state.
  • src/domain/session/session-merge-operations.ts:309-323 uses prState in the waiver rejection message (“Another merge blocker is active (PR state: ${prState}).”), implying it will show “draft” when in draft. It will not; it will show "open" for draft PRs.
• Failure mode: Operators will see “PR state: open” even when the actual blocker is the Draft state. This degrades diagnostics and conflicts with the intent of making the waiver path informative. If you intend to surface draft, you need to either carry a draft boolean from GitHub API, or map mergeable_state to a human-readable reason.

[NON-BLOCKING] Bot reviewer login is hard-coded in waiver logic

• Evidence:
  • src/domain/session/session-merge-operations.ts:301 compares reviewerLogin.toLowerCase() === "minsky-reviewer[bot]".
• Impact: Using a literal rather than a centralized constant risks drift if the reviewer bot login changes (you did centralize BOT_IDENTITY_LOGIN for the author, not for the reviewer). Consider extracting a REVIEWER_BOT_LOGIN constant in src/domain/constants.ts for consistency and future-proofing.

[NON-BLOCKING] Case-insensitive compare can be made symmetric for clarity

• Evidence:
  • src/domain/session/session-merge-operations.ts:301 lowercases the left side but compares to a lowercased literal on the right. While functionally fine, lowercasing both sides or centralizing the normalized constant would make intent clear.

[NON-BLOCKING] Test realism gap: waiver “happy path” relies on impossible ApprovalStatus combinations

• Evidence:
  • src/domain/session/session-merge-security-validation.test.ts:360-384 sets isApproved: false and canMerge: true to validate the waiver path.
  • src/domain/repository/github-pr-approval.ts:216-220 guarantees canMerge cannot be true when isApproved is false for real GitHub data.
• Impact: Unit tests validate behavior against mocked states the production provider won’t emit, masking the core logic bug noted above. Consider adding an integration test that wires through the real getPullRequestApprovalStatus or adjust canMerge semantics so that it does not include isApproved if you intend to use it as “no other blockers beyond approvals.”

Spec verification table

• Enumerate all waiver constraints in CLI flag description: Not Met
• Show accurate and non-misleading operator messages for waiver outcomes: Not Met (prState misuse; waiver success message fix is good, but draft/open mislabel remains)
• Implement waiver gating only for bot-authored PRs with same-identity COMMENT and no CHANGES_REQUESTED or reviewer-bot review: Met (logic present; but blocked by canMerge coupling)
• Centralize bot identity constant: Met (BOT_IDENTITY_LOGIN added and re-exported)

Documentation impact

• Command help (session.pr.merge parameters) must be updated to reflect all enforced constraints, including the current canMerge requirement, unless the implementation is changed to decouple canMerge from isApproved.
• If you change canMerge semantics, update architecture notes and comments in github-pr-approval.ts to clarify what canMerge means (e.g., “mergeability aside from approvals”).

Conclusion

REQUEST_CHANGES

Proposed remediation directions (non-prescriptive):

• Decouple canMerge from isApproved in getPullRequestApprovalStatus (e.g., compute mergeability blockers excluding approvals: draft, merge conflicts, failing checks), or adjust the waiver check to use a new field like hasNonApprovalMergeBlockers instead of canMerge.
• Update the CLI flag description in src/adapters/shared/commands/session/session-parameters.ts to include the mergeability constraint if you keep enforcing it.
• Fix prState reporting to accurately reflect draft and other blockers (e.g., include a draft boolean or map mergeable_state to a human-readable status that you surface in the message).
• Consider centralizing the reviewer bot login alongside BOT_IDENTITY_LOGIN to avoid future drift.

[minsky-reviewer]

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: unknown

---

Findings

[BLOCKING] Silent platform config bypass in GitHub approval status client

• Evidence: src/domain/repository/github-pr-approval.ts: in getPullRequestApprovalStatus(), Octokit is constructed directly with new Octokit({ auth: githubToken, log: … }) rather than via the repo’s createOctokit factory used elsewhere in the same file (approvePullRequest() uses createOctokit(githubToken)). This silently bypasses any custom baseUrl, user agent, retry/throttle, or enterprise config that createOctokit may apply.
• Failure mode: On GitHub Enterprise or any environment where createOctokit injects critical options (proxy, previews, timeouts), getPullRequestApprovalStatus will use defaults and may fail to reach the correct API host or lose retry behavior. This can cause false negatives (no reviews found), misclassification of draft state, or total failure when the rest of the system works.
• Impacted code: src/domain/repository/github-pr-approval.ts, Octokit instantiation in getPullRequestApprovalStatus() (near the top of the function body).
• Requested change: Replace new Octokit(...) with createOctokit(githubToken) to remain consistent with the rest of the GitHub client usage.

[BLOCKING] Incorrect “merge conflicts” diagnosis when mergeable is null (unknown)

• Evidence: src/domain/repository/github-pr-approval.ts: after computing isDraft, code sets nonApprovalBlockerDescription to "merge conflicts" when !pr.mergeable is true. However, GitHub returns mergeable: null while computing mergeability; !!pr.mergeable coerces null to false, and !pr.mergeable treats null identically to false, mislabeling unknown mergeability as “merge conflicts.”
  • Snippet: else if (!pr.mergeable) { nonApprovalBlockerDescription = "merge conflicts"; }
• Failure mode: During the short “unknown” window, waiver attempts will be rejected with “Another merge blocker is active (merge conflicts)” even though there may be no conflicts. This is both misleading and can block legitimate waiver paths.
• Requested change: Check explicitly for pr.mergeable === false, not falsy. Optionally, handle pr.mergeable === null with a distinct “mergeability unknown” description.

[BLOCKING] CLI command description is now misleading vs. behavior

• Evidence: src/adapters/shared/commands/session/workflow-commands.ts:392-400 (createSessionPrMergeCommand) — description remains "Merge an approved session pull request", but the implementation now allows merges under the acceptStaleReviewerSilence waiver when approvalStatus.isApproved is false.
• Failure mode: Operator confusion and incorrect mental model; the command can succeed without formal approval under documented waiver conditions. The description implies otherwise.
• Requested change: Update the command description to reflect waiver behavior (e.g., “Merge a session pull request (requires approval or waiver)”).

[NON-BLOCKING] Parameter doc count inconsistency with PR summary and earlier reviews

• Evidence: src/adapters/shared/commands/session/session-parameters.ts: the acceptStaleReviewerSilence description says “All five constraints must hold,” listing the extra “no other merge blockers” condition. The PR summary “B1” framing mentioned enumerating “all four conditions.” The code enforces the fifth (hasNonApprovalMergeBlockers).
• Impact: Minor documentation drift in PR summary; the CLI parameter description is accurate. No code change requested; just note the mismatch in the PR narrative.

[NON-BLOCKING] Potential false positives in canMerge computation due to null coalescing

• Evidence: src/domain/repository/github-pr-approval.ts: canMerge = isApproved && !!pr.mergeable && pr.state === "open" && !isDraft. While acceptable, !!pr.mergeable will make canMerge false for the transient null state. You mitigated this with hasNonApprovalMergeBlockers for waiver checks, but canMerge itself may still misinform other consumers if used elsewhere.
• Suggestion: Consider documenting that canMerge may be false during mergeability recalculation (mergeable === null) and that callers should use diagnoseMergeBlocker() for user-facing reasons or wait/retry paths.

[NON-BLOCKING] Test double reuse risks cross-test contamination

• Evidence: src/domain/session/session-merge-security-validation.test.ts: top-level mockSessionProvider is spread into fakeDeps and partially overridden. The same underlying mocks (and their call history) are reused across suites; beforeEach clears some, but overriding methods inside fakeDeps may cause subtle cross-test interactions if future tests mutate the shared instance.
• Suggestion: Instantiate a fresh session provider mock per test or per describe to eliminate shared state.

[NON-BLOCKING] Observability: reviewer list in waiver audit log might include third-party COMMENTED reviews

• Evidence: src/domain/session/session-merge-operations.ts: within the waiver path, commentReviewers is derived from all COMMENTED reviews, not only the same-identity review required by the waiver. This might blur the primary evidence trail.
• Suggestion: Consider logging both the required same-identity COMMENTED review and a separate list of other COMMENTED reviewers.

Spec verification

No formal task spec was provided. Verifying against the PR description’s stated intent:

• B1 (BLOCKING) - CLI flag description enumerates all enforced constraints
  • Met. session-parameters.ts updates acceptStaleReviewerSilence with five constraints; implementation enforces the fifth via hasNonApprovalMergeBlockers.
• NON-BLOCKING - Misleading hint for human-authored PRs
  • Met. In session-merge-operations.ts, the rejection message hint for using the waiver is gated by isPrAuthorBot && !hasReviewerBotReview.
• NON-BLOCKING - Centralize bot identity constant
  • Met. Introduced src/domain/constants.ts with BOT_IDENTITY_LOGIN and REVIEWER_BOT_LOGIN, imported and re-exported by session-merge-operations.ts.

Additional issues addressed beyond the PR description (from prior reviews):

• Misleading success message after waiver path
  • Addressed. Success CLI message is conditioned on waiverApplied.
• Waiver unattainable due to canMerge coupling
  • Addressed. Introduced hasNonApprovalMergeBlockers and independent blocker description, and waiver checks it instead of canMerge.

Documentation impact

• CLI help/docs: Update session.pr.merge description to reflect the waiver behavior and the new acceptStaleReviewerSilence flag including the five constraints and audit logging behavior.
• Architecture/ops notes: Document that getPullRequestApprovalStatus paginates all reviews to avoid page-1 omissions, and that hasNonApprovalMergeBlockers is the correct signal for non-approval blockers (draft/conflicts/closed) while mergeable may be null transiently.
• Constants: Note the new domain constants module and re-export in session-merge-operations for compatibility.

Conclusion

REQUEST_CHANGES

Blocking issues remain:

• Use of raw new Octokit() in getPullRequestApprovalStatus bypassing createOctokit (platform config risk).
• Misclassification of mergeability unknown as “merge conflicts” when pr.mergeable is null.
• Misleading command description for session.pr.merge now that waiver-enabled merges are possible without approval.

[minsky-reviewer]

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: unknown

---

Findings

[BLOCKING] Silent platform config bypass persists in GitHub approval status client

• Evidence: src/domain/repository/github-pr-approval.ts:96-117 constructs Octokit via new Octokit({...}) inside getPullRequestApprovalStatus(), whereas approvePullRequest() in the same file uses the repository factory createOctokit(token) (see src/domain/repository/github-pr-approval.ts:41-54 and 71-90). The prior independent review (iteration 6) flagged this exact issue and it remains.
• Failure mode: Bypassing createOctokit silently drops any global Octokit configuration the repo may apply (baseUrl for GHE, UA, retry/throttle plugins, previews, proxy). In Enterprise or proxied environments, getPullRequestApprovalStatus can call the wrong host or miss retry behavior, causing false negatives (no reviews detected), incorrect draft detection, or outright failures while other calls succeed.
• Required change: Replace the direct new Octokit(...) with the repo-standard createOctokit(githubToken) and keep the debug logger wiring consistent through the factory (or extend the factory to accept a log adapter).

[BLOCKING] Over-broad “merge conflict” classification maps unrelated blockers to CONFLICT

• Evidence: src/adapters/shared/commands/session/workflow-commands.ts:140-154 (isMergeConflictError). The classifier returns true if the error message contains “mergeable”. This string appears in many non-conflict scenarios (e.g., “PR is not mergeable because it is a draft” or “mergeable=false due to required checks”), so any such error would be wrapped as MCP CONFLICT.
• Failure mode: Non-conflict blockers (draft PRs, failing checks, blocked by protection rules) are surfaced to MCP clients as CONFLICT, causing clients to take conflict-resolution flows that do not apply (e.g., “resolve conflicts” guidance when the PR is draft). This is a silent behavior change from previous unstructured errors and breaks machine consumers now relying on code=CONFLICT.
• Required change: Narrow the classifier to true textual/semantic conflict signals (e.g., “merge conflict”, “CONFLICT”, Git’s “Automatic merge failed; fix conflicts”) and remove generic “mergeable” matching. Consider surfacing a separate structured code (e.g., VALIDATION_ERROR) for non-conflict mergeability failures.

[NON-BLOCKING] PR description vs code mismatch: “four” vs “five” waiver constraints

• Evidence:
  • PR description says: “CLI flag description enumerates all four enforced constraints.”
  • Code documents five constraints: src/adapters/shared/commands/session/session-parameters.ts:321-333 (“All five constraints must hold … (5) no other merge blockers …”). The implementation enforces this via approvalStatus.hasNonApprovalMergeBlockers in src/domain/session/session-merge-operations.ts:327-354.
• Impact: Reviewers/readers of the PR summary receive outdated information. Please update the PR description and any CLI/docs references to state five constraints.

[NON-BLOCKING] Command description is now misleading for session.pr.merge

• Evidence: src/adapters/shared/commands/session/workflow-commands.ts:164 still describes the merge command as “Merge an approved session pull request”, but the waiver path allows merging an otherwise-unapproved PR under narrow constraints.
• Impact: Misleading UX/documentation. Suggest updating the command description to note the optional operator-override waiver path.

[NON-BLOCKING] CLI surface change for session.pr.merge parameters may be breaking for consumers

• Evidence:
  • Before: createSessionPrMergeCommand used sessionApproveCommandParams (src/adapters/shared/commands/session/workflow-commands.ts: previously).
  • After: it uses sessionMergeCommandParams including new flags skipCleanup and acceptStaleReviewerSilence (src/adapters/shared/commands/session/workflow-commands.ts:166-171; src/adapters/shared/commands/session/session-parameters.ts:303-336).
• Impact: This is an intentional expansion, but it changes the parameter schema. Any external tooling/scripts generating usage or validating params may need updates. Please document the new flags in CLI help/changelogs and ensure MCP tool schemas (if mirrored) are updated consistently.

[NON-BLOCKING] Potential downstream impact from reclassifying prState to include “draft”

• Evidence:
  • ApprovalStatus.prState now includes “draft” and getPullRequestApprovalStatus maps GitHub draft=true to prState="draft" (src/domain/repository/github-pr-approval.ts:203-215 and 222-236).
• Impact: If any callers previously assumed prState==="open" for drafts, they may diverge. The type union was updated (src/domain/repository/approval-types.ts:116-120) which helps at compile time, but please audit call sites and update docs/help output to reflect the expanded state.

[NON-BLOCKING] Inconsistent import path usage for structured MCP errors

• Evidence: src/adapters/shared/commands/session/pr-create-command.ts:16-17 import mcp-structured-errors and codes directly from errors/* files, while errors/index.ts re-exports those symbols (src/errors/index.ts:79-92).
• Impact: Minor style/consistency issue; centralizing imports via errors/index keeps module boundaries consistent.

Spec verification

(No explicit task spec was provided; PR description is the only intent source.)

• Enumerate all waiver constraints in the CLI flag help: Met (src/adapters/shared/commands/session/session-parameters.ts:321-333 lists five constraints; PR text mentions four but code lists five).
• Gate the “acceptStaleReviewerSilence” hint to bot-authored PRs only: Met (src/domain/session/session-merge-operations.ts:368-385).
• Centralize bot identity constants: Met (src/domain/constants.ts; re-exported from session-merge-operations.ts:24-26).
• Address waiver-can-never-trigger coupling (B1): Met (use of hasNonApprovalMergeBlockers, independent from isApproved/canMerge; src/domain/repository/github-pr-approval.ts:214-236; src/domain/session/session-merge-operations.ts:341-354).
• Do not bypass Octokit factory in status client (iteration 6 blocking): Not Met (see first BLOCKING finding).

Documentation impact

• CLI changes:
  • New merge flags: acceptStaleReviewerSilence (with five constraints) and skipCleanup. Update CLI help/docs and any usage examples.
  • Update command description for session.pr.merge to reflect the waiver path, not just “approved”.
• Behavior changes:
  • Approval status prState now can be “draft” (not simply “open”). Update any docs that surface PR states.
  • MCP structured error mapping for merge conflicts added; after fixing the over-broad classifier, document the structured error codes (CONFLICT, PRE_COMMIT_FAILED, etc.) and when they are emitted.
• PR summary correction: state “five constraints” for the waiver.

Conclusion

REQUEST_CHANGES

Blocking issues remain:

• getPullRequestApprovalStatus still constructs Octokit directly and bypasses the repository’s createOctokit factory.
• Over-broad merge conflict classifier maps unrelated merge blockers to CONFLICT.

Please address the blocking items and update the PR description/docs per the non-blocking notes.

[minsky-reviewer]

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: unknown

---

Findings

[BLOCKING] Silent platform config bypass persists in GitHub approval status client

• Evidence: src/domain/repository/github-pr-approval.ts: the function getPullRequestApprovalStatus() still constructs a raw Octokit via:
  • const octokit = new Octokit({ auth: githubToken, log: … });
• Contrast: approvePullRequest() in the same file uses the repository factory createOctokit(githubToken), which is imported from ./github-pr-operations and presumably centralizes baseUrl, UA, retry/throttle plugins, proxy, etc.
• Failure mode: This bypass silently drops any global Octokit configuration (e.g., GitHub Enterprise baseUrl, retry/throttle, previews, proxy). In Enterprise or proxied environments, getPullRequestApprovalStatus may call the wrong host, miss retry behavior, or behave inconsistently with the rest of the client. This is the same platform-config inconsistency flagged in prior independent reviews and remains unaddressed in this diff.
• Action: Replace new Octokit({ … }) with the repository factory createOctokit(githubToken) to maintain consistent configuration across all GitHub API calls.

[NON-BLOCKING] CLI flag description and PR summary are inconsistent about “four” vs “five” constraints

• Evidence: PR description “B1 (BLOCKING) - CLI flag description enumerates all four enforced constraints” vs implementation: src/adapters/shared/commands/session/session-parameters.ts adds acceptStaleReviewerSilence description enumerating five constraints (adds “no other merge blockers” as (5)).
• Impact: Documentation mismatch in the PR summary could confuse reviewers/users; the code and CLI description are consistent with five constraints. Consider updating the PR summary or commit message to reflect “five.”

[NON-BLOCKING] Potentially confusing success message on waiver path is improved but may still benefit from stronger audit context

• Evidence: src/domain/session/session-merge-operations.ts:305-352 logs an INFO “WAIVER” and a CLI warning when the waiver is applied, then later prints a waiver-specific CLI message (“PR proceeding via acceptStaleReviewerSilence waiver…”).
• Impact: You’ve improved messaging and audit logging. Consider including the PR number and author identity in the final CLI waiver message as well (not just the log.info) for parity with the audit line and to aid CLI-only operators.

[NON-BLOCKING] Test coverage gap for the Octokit factory usage regression

• Evidence: No tests in this PR (or existing tests in src/domain/session/session-merge-security-validation.test.ts) exercise getPullRequestApprovalStatus() making real Octokit calls or asserting usage of the configured Octokit factory. Tests mock review.getApprovalStatus in multiple places, which masks the factory bypass.
• Impact: The platform-configuration regression persists without test coverage to catch it.
• Action: Add a unit/integration test for github-pr-approval.getPullRequestApprovalStatus() that verifies it obtains an Octokit via createOctokit() (e.g., spy on createOctokit) and/or honors a non-default baseUrl from configuration.

[PRE-EXISTING] Minor formatting bug in merge blocker diagnosis message

• Evidence: src/domain/repository/github-pr-approval.ts: around the “Failing status checks” string interpolation, there is an extra literal “$” before a template expression:
  • Failing status checks: ${list}$${ … }
• Impact: The emitted message includes an unintended “$” character. This appears unrelated to the current PR changes and likely predates it. Consider cleaning it up in a follow-up.

Spec verification table

• No task spec provided. Using PR description as intent.

• Enumerate all constraints for acceptStaleReviewerSilence in CLI help:

  • Met (the CLI description lists five constraints; the PR summary mentions four, see non-blocking note).

• Gate hint for human-authored PRs:

  • Met (src/domain/session/session-merge-operations.ts conditions the hint on isPrAuthorBot && !hasReviewerBotReview).

• Centralize bot identity constant:

  • Met (src/domain/constants.ts defines BOT_IDENTITY_LOGIN and REVIEWER_BOT_LOGIN; re-exported from session-merge-operations.ts).

• Preserve platform config via Octokit factory in approval status client:

  • Not Met (still uses new Octokit in getPullRequestApprovalStatus).

Documentation impact

• Yes:
  • The CLI parameter description now includes five explicit constraints; ensure any user-facing docs or help text elsewhere reflect the same set.
  • New structured MCP errors introduced (CONFLICT, PRE_COMMIT_FAILED, SUBPROCESS_FAILED, VALIDATION_ERROR) and server passthrough of McpError: developer docs for MCP tools should mention branching on error.data.code and the existence of StructuredMcpError.
  • New shared constants module src/domain/constants.ts: update architecture notes to point consumers to this central source for bot identity strings.

Conclusion

REQUEST_CHANGES

The previously flagged blocking issue (Octokit factory bypass) remains in getPullRequestApprovalStatus(). This is a system-level configuration risk and should be corrected by switching to createOctokit(githubToken). The other findings are non-blocking/documentation/observability concerns.

[minsky-reviewer]

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: unknown

---

Findings

[BLOCKING] Silent platform config bypass persists in GitHub approval status client

• Evidence: src/domain/repository/github-pr-approval.ts:69-93 constructs Octokit via new Octokit({ auth: githubToken, log: … }) inside getPullRequestApprovalStatus(), bypassing the repository factory createOctokit(token) used elsewhere (e.g., approvePullRequest at src/domain/repository/github-pr-approval.ts:41-54 and 71-90).
• Failure mode: This bypass silently drops any global Octokit configuration (e.g., GitHub Enterprise baseUrl, UA, retry/throttle plugins, previews, proxy). In Enterprise/proxied environments, getPullRequestApprovalStatus can call the wrong host or behave inconsistently, causing false negatives and flaky behavior. This exact issue was previously raised and remains unaddressed in the current diff.

[BLOCKING] Misclassification of mergeability: null mergeable treated as “merge conflicts”

• Evidence: src/domain/repository/github-pr-approval.ts:212-221 sets nonApprovalBlockerDescription = "merge conflicts" when !pr.mergeable. GitHub’s mergeable can be null while being (re)computed; treating null as false will mislabel transient/unknown mergeability as conflicts.
• Failure mode: Waiver logic in src/domain/session/session-merge-operations.ts:302-321 blocks merges when approvalStatus.hasNonApprovalMergeBlockers is true, whose description here can be incorrectly set to “merge conflicts” due to mergeable === null. This yields false blocker messages and can incorrectly refuse a valid waiver path or mislead operators.

[NON-BLOCKING] Spec/description inconsistency: “four constraints” vs “five constraints”

• Evidence: PR Description “Changes” section claims the CLI flag description “enumerates all four enforced constraints,” while the actual CLI parameter description in src/adapters/shared/commands/session/session-parameters.ts:311-327 states “All five constraints must hold” and includes an additional constraint about “no other merge blockers.”
• Risk: Confusing release notes and operator expectations. Align the PR description with the implemented five constraints.

[NON-BLOCKING] Potential over-broad conflict detection for MCP error mapping

• Evidence: src/adapters/shared/commands/session/workflow-commands.ts:442-456 isMergeConflictError() returns true if the message includes “mergeable”. This can capture non-conflict mergeability conditions (e.g., blocked by checks, draft).
• Impact: Some non-conflict merge failures may be surfaced as McpErrorCode.CONFLICT to MCP clients, leading to inaccurate client branching. Consider narrowing conditions or delegating to structured reasons from the backend.

[NON-BLOCKING] Cross-backend coherence risk: waiver relies on hasNonApprovalMergeBlockers which is optional

• Evidence: approval-types.ts:112-135 marks hasNonApprovalMergeBlockers and nonApprovalBlockerDescription as optional; session-merge-operations.ts:302-321 relies on hasNonApprovalMergeBlockers to gate the waiver. If a non-GitHub backend (or an older implementation) returns undefined for these fields, the check evaluates false and the waiver may proceed despite unreported blockers.
• Suggestion: Default to conservative behavior when hasNonApprovalMergeBlockers is undefined (treat as possibly blocked or derive blockers from prState), or assert backend type is GitHub before enabling the waiver. At minimum, document the assumption and add runtime assertions.

[NON-BLOCKING] CLI parameter set reshaping: potential downstream impact

• Evidence: src/adapters/shared/commands/session/workflow-commands.ts:467 changes session.pr.merge parameters from sessionApproveCommandParams to sessionMergeCommandParams (adds skipCleanup and acceptStaleReviewerSilence). While additive, any tight coupling in external tooling expecting the smaller schema may need to be validated.
• Action: Note in changelog; ensure command help/docs are updated and any consumers parsing the schema are compatible.

[NON-BLOCKING] Test suite duplication of internal helpers and magic strings

• Evidence: src/errors/mcp-structured-errors.test.ts duplicates classifyPreCommitFailure logic; session-merge-security-validation.test.ts introduces numerous long inline fixtures.
• Impact: Future refactors risk tests diverging from implementations. Consider factoring helpers behind an exported test utility or keeping string constants centralized to reduce drift.

Spec verification

• B1 (BLOCKING) - CLI flag description enumerates all four enforced constraints: Not Met

  • The code enumerates five constraints (includes “no other merge blockers”); PR description claims four. Behavior is fine; documentation consistency is not.

• NON-BLOCKING - Misleading hint for human-authored PRs: Met

  • src/domain/session/session-merge-operations.ts:338-356 now gates the hint on isPrAuthorBot && !hasReviewerBotReview.

• NON-BLOCKING - Centralize bot identity constant: Met

  • Introduced src/domain/constants.ts and re-export from session-merge-operations.ts at src/domain/session/session-merge-operations.ts:45-47.

Additional notes on prior reviews

• The previously flagged platform-config inconsistency for Octokit instantiation in getPullRequestApprovalStatus remains. This is re-raised as BLOCKING with concrete line references (see first finding).

Documentation impact

• CLI help text changed (new acceptStaleReviewerSilence flag and expanded description). Update user-facing docs and changelog to:
  • Document the five constraints clearly and consistently.
  • Note the new structured MCP error behaviors for session.commit (PRE_COMMIT_FAILED) and PR create/merge (CONFLICT).
  • Note that session.pr.merge now accepts skipCleanup and acceptStaleReviewerSilence parameters.

Event

REQUEST_CHANGES

[minsky-reviewer]

Independent adversarial review (Chinese-wall)
Reviewer: minsky-reviewer[bot] via openai:gpt-5
Tier: unknown

---

Findings

[BLOCKING] Platform config bypass persists in GitHub approval status client

• Evidence: src/domain/repository/github-pr-approval.ts: around lines 65–95, getPullRequestApprovalStatus constructs an Octokit via:
  const octokit = new Octokit({ auth: githubToken, log: … });
  This bypasses the repository factory createOctokit(token) used elsewhere (e.g., approvePullRequest in the same file).
• Failure mode: This silently drops any global Octokit configuration (GitHub Enterprise baseUrl, UA, retry/throttle plugins, proxies, previews, telemetry). In Enterprise/proxied environments, getPullRequestApprovalStatus can call the wrong host or behave inconsistently, causing false negatives and flaky behavior. This exact issue was raised in prior iterations and remains unaddressed here.
• Request: Replace new Octokit(...) with createOctokit(githubToken) to ensure consistent platform configuration.

[BLOCKING] Mergeability misclassification when GitHub returns mergeable = null

• Evidence: src/domain/repository/github-pr-approval.ts: non-approval blocker classification sets:
  else if (!pr.mergeable) { nonApprovalBlockerDescription = "merge conflicts"; }
  but mergeable is typed as boolean | null (see interface and GitHub semantics). null means “unknown, GitHub is still computing,” not “conflicts.”
• Failure mode: PRs in the “unknown” phase are incorrectly flagged as “merge conflicts,” misleading users and blocking the waiver path with an inaccurate reason.
• Request: Treat mergeable === false as “merge conflicts” and mergeable === null as “mergeability unknown (retry shortly).” Consider threading the “unknown” result into diagnoseMergeBlocker or aligning with mergeable_state when available.

[BLOCKING] Silent API/CLI schema regression: removal of range comments support from session.pr.review.submit

• Evidence: src/adapters/shared/commands/session/session-parameters.ts:682–704 (per diff context). The z.object for inline comments previously accepted:
  • startLine?: number
  • startSide?: "LEFT" | "RIGHT"
    with corresponding detailed behavior docs. These properties are removed entirely, and the parameter description is reduced to “Optional inline line-level comments.”
• Failure mode: This is a breaking API change for clients posting multi-line range comments. It is unrelated to the PR’s stated goal and is not documented in the PR description. Users relying on range comments will see validation failures or silently lose functionality.
• Request: Restore startLine/startSide support and the associated documentation, or clearly justify and document the breaking change in a dedicated PR with migration notes.

[NON-BLOCKING] PR description vs code mismatch on waiver conditions count (four vs five)

• Evidence:
  • PR description “Changes → B1” says “enumerates all four conditions.”
  • Code changes update the CLI help to “All five constraints must hold” in src/adapters/shared/commands/session/session-parameters.ts (acceptStaleReviewerSilence description), adding “no other merge blockers” as the fifth.
• Impact: Documentation inconsistency can confuse operators following PR notes versus CLI help.
• Request: Align the PR description with the implemented five conditions or adjust the help text if that fifth condition is not intended to be part of the CLI-specified constraints.

[NON-BLOCKING] Potential scope creep: unrelated CLI help simplification

• Evidence: src/adapters/shared/commands/session/session-parameters.ts: the detailed inline comments documentation for side defaulting and range semantics was removed, not just adjusted, as part of this PR which is scoped to merge waiver and reviewer-bot silence handling.
• Impact: Even if functionality is restored per the BLOCKING finding above, reducing the help text weakens usability. If the functionality is intentionally unchanged (and restored), keep concise but sufficient guidance to avoid user confusion.
• Request: Either keep the original detailed help or provide a brief but clear summary of side/startSide defaulting rules in the CLI help. If functionality is intentionally removed, this belongs in a dedicated, clearly-scoped PR.

[NON-BLOCKING] New approval fields without explicit consumer validation

• Evidence: src/domain/repository/approval-types.ts introduces:
  • hasNonApprovalMergeBlockers?
  • nonApprovalBlockerDescription?
  • rawReviews?
    and github-pr-approval.ts populates them.
• Impact: The expansion is additive and likely safe, but consider validating downstream consumers for assumptions about “unknown” mergeability (see BLOCKING above). Also consider adding unit coverage for the null mergeable case to ensure nonApprovalBlockerDescription is correct.

Spec verification table

• No explicit task spec provided. Using PR description as intent.

1. B1 (BLOCKING) - CLI flag description enumerates all four enforced constraints

• Code: Updated description enumerates five constraints, adding “no other merge blockers.”
• Status: Not Met (as-written vs described). The implementation + CLI now specify five. The PR description claims four. Needs reconciliation.

2. NON-BLOCKING - Misleading hint for human-authored PRs

• Code: src/domain/session/session-merge-operations.ts gates the hint on isPrAuthorBot && !hasReviewerBotReview.
• Status: Met.

3. NON-BLOCKING - Centralize bot identity constant

• Code: Added src/domain/constants.ts and updated imports; re-exported for compatibility.
• Status: Met.

Additional prior-review concern (persisting):

• Silent platform config bypass in getPullRequestApprovalStatus (constructing raw Octokit)
• Status: Not Met (see BLOCKING finding #1).

Documentation impact

• CLI help changed:
  • session.pr.merge: new acceptStaleReviewerSilence help with five conditions and an audit log note. Requires updating any user docs that enumerate conditions.
  • session.pr.review.submit: help text substantially simplified and range comment fields removed; this is a breaking change and must be documented or reverted. If intended, add a changelog entry and migration guidance.
• Architecture notes:
  • ApprovalStatus now includes hasNonApprovalMergeBlockers, nonApprovalBlockerDescription, rawReviews, and a ‘draft’ prState; update any docs that describe ApprovalStatus and mergeability evaluation to cover draft state and the independent blockers field.

Conclusion

REQUEST_CHANGES

Rationale: The PR retains a previously flagged platform configuration bypass for Octokit client construction; misclassifies mergeability when mergeable=null; and introduces an undocumented, out-of-scope breaking change removing range comment support from the review submit parameters. These require correction or explicit, documented justification before merge. The remaining issues are non-blocking but should be addressed or documented.