Skip to content

docs(adr): propose ADR-0006 PR-comment delivery and App permission evolution - #414

Merged
rowkav09 merged 5 commits into
mainfrom
docs/adr-0006
Sep 26, 2026
Merged

rowkav09 merged 5 commits into
mainfrom
docs/adr-0006

Conversation

@rowkav09

Copy link
Copy Markdown
Member

What

ADR-0006, the explicit superseding decision reviewer-2 required on #406 before any activation of the PR-comment delivery path. Status Proposed — acceptance is owner-gated (Activation section).

It supersedes only the permission set, event list and "rare PR comments" clauses of ADR 0003 for this feature. Everything else in ADR 0003 stands; ADR 0004 is untouched; ADR 0005's rollout conditions are unchanged.

Decisions carried

  • One-time ceiling raise (contents: write, issues: write, issue_comment) as the opt-in change ADR 0003 anticipated. No actions: write.
  • Broker/child credential split: every operational path mints explicitly narrowed tokens (scan worker unchanged one-repo contents: read + checks: write; webhook lookup narrowing; narrow dispatch token; comment editor issues: write only; no ambient context.octokit on untrusted-input paths).
  • Decline-preserves-scans and installation effective-rights detection as coded acceptance properties.
  • One maintained comment per PR; ADR 0003 check behaviour unchanged; no gating.
  • Activation requires owner acceptance (this ADR + live App permission update separately), slice-2 evidence gates, M3 stability, reviewer security sign-off at exact head, and the credential split (Isolate untrusted analysis from App credentials and network #326).

Also fixes the stale ADR index (0005 was never listed; both entries added in the second commit).

Refs: #406 (gated manifest delta), #411 (token narrowing hardening), #326 (credential split), reviewer-2 gate review on #406 (2026-09-26).

@rowkav09

Copy link
Copy Markdown
Member Author

CHANGES REQUESTED for exact head 51df563cca551e510cd56901c7b6ceb0009f4f03.

The proposed ADR draws the right boundaries for this feature: it limits the ADR 0003 supersession to permissions/events/comment delivery, excludes actions: write, makes path-specific token narrowing and declined-reapproval scan continuity testable properties, and leaves owner acceptance, live App update, M3, slice-2 evidence, exact-head security review and #326 credential split as separate activation gates. It does not itself approve #406 or activation.

Two document fixes are needed before this head lands:

  1. docs/adr/index.md joins ADR 0004 and ADR 0005 on one line: ...0004-analysis-sandboxing.md)- [0005.... Put ADR 0005 on its own list line so the index renders and navigates correctly.
  2. docs/adr/0006-pr-comment-delivery-and-app-permission-evolution.md fails the repository's Prettier check. The sentence after the broker/child token list needs the list-continuation indentation shown by Prettier. Please format and check both changed docs.

The document cites earlier owner delegation as background, but that citation does not establish owner acceptance of this proposed ADR or the future live App permission update; the Activation section correctly keeps both as separate gates. A changed head needs another review.

@rowkav09

Copy link
Copy Markdown
Member Author

APPROVE this proposed ADR document at exact head a454add120b729ad7c8ee654998c9fd906219ce6. The revision puts ADR 0005 on its own index line and fixes the ADR 0006 list-continuation formatting; both changed documents pass Prettier and the diff has no whitespace errors.

The substantive scope from my prior review is unchanged: this document narrowly proposes superseding ADR 0003's permission set, event list and rare-comment clause for the PR-comment feature; it excludes actions: write, requires per-path narrowed tokens and declined-reapproval scan continuity as tested properties, and keeps owner ADR acceptance, a separate live App permission decision, M3, slice-2 evidence, exact-head security sign-off and #326 credential separation as activation gates. Approval of the document's quality is not owner acceptance of the ADR, permission to re-register the App, or approval to activate #406. The hosted checks were still running at review time and definitive mergeability remains a separate gate. A push resets this verdict.

@rowkav09
rowkav09 merged commit fa873fc into main Sep 26, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant