-
Notifications
You must be signed in to change notification settings - Fork 0
Add the persona @-mention router #754
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
518ff2b
feat(personas): add the persona @-mention router
don-petry f3a6fb0
fix(reviews): address review comments [skip ci-relay]
donpetry-bot 1953ccd
fix(personas): close the fail-open paths and enforce the write gate
don-petry 277f6de
fix(personas): drop the dead review_requested trigger from the stub
don-petry 15df3d7
fix(personas): hash-lock the PyYAML install
don-petry 02ac387
Merge branch 'main' into feat/persona-mention-router
don-petry 9333997
fix(reviews): address review comments [skip ci-relay]
donpetry-bot e4e31f5
fix(personas): do NOT authenticate the manifest fetch — it adds a sil…
don-petry b022ed0
Merge branch 'main' into feat/persona-mention-router
don-petry File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,290 @@ | ||
| # Reusable Persona Mention Router. | ||
| # SOURCE OF TRUTH: petry-projects/.github/.github/workflows/persona-mention-reusable.yml | ||
| # Standard: https://github.com/petry-projects/.github/blob/main/standards/persona-standards.md (§4.1) | ||
| # | ||
| # Routes `@petry-projects/<role>` mentions to the addressed persona. ONE router | ||
| # for every persona — personas do NOT each ship a mention workflow (§4.1); that | ||
| # per-agent drift is exactly what the manifest exists to prevent. | ||
| # | ||
| # How a mention resolves, and why there is no index: | ||
| # `validate-personas.py` enforces address.handle's slug == id == the persona's | ||
| # directory name, so '@petry-projects/qa-lead' resolves to | ||
| # personas/qa-lead/persona.yml in .github-private by convention. The manifest | ||
| # IS the index-of-record (§1.1). A 404 means "not a persona" — which is also | ||
| # how a real, non-persona team like @petry-projects/org-leads falls through | ||
| # harmlessly. | ||
| # | ||
| # The manifest, not this file, decides: | ||
| # whether the mention surface is enabled, in what mode, behind which trust | ||
| # floor, and under which opt-out label. This router restates none of it. | ||
| # | ||
| # Requires: | ||
| # GH_PAT_WORKFLOWS — org secret, classic PAT (repo scope) for API calls and dispatch | ||
| name: Persona Mention Router (Reusable) | ||
|
|
||
| on: | ||
| workflow_call: | ||
| inputs: | ||
| tooling_ref: | ||
| description: | | ||
| Ref of petry-projects/.github to source scripts/lib/persona-mention.sh | ||
| from. Leave empty (the default) to source it from THIS reusable's own | ||
| commit (`github.job_workflow_sha`), which guarantees the routing logic | ||
| always matches the version of this workflow the caller pinned, with no | ||
| skew (the #465/#528 lesson: a fixed `v1` default predates the library | ||
| and fails every run with "No such file or directory"). Override only to | ||
| test a branch end-to-end. | ||
| required: false | ||
| default: '' | ||
| type: string | ||
| persona_ref: | ||
| description: | | ||
| Ref of petry-projects/.github-private to read persona manifests at. | ||
| Defaults to main — a persona's live contract is what is merged, not | ||
| what a branch proposes. Override only for end-to-end testing. | ||
| required: false | ||
| default: 'main' | ||
| type: string | ||
| secrets: | ||
| GH_PAT_WORKFLOWS: | ||
| description: 'Classic PAT with repo scope, used for API calls and dispatching the persona' | ||
| required: true | ||
|
|
||
| permissions: {} # no default permissions; each job grants only what it needs | ||
|
|
||
| concurrency: | ||
| # Serialise per comment, not per thread: two different personas addressed in | ||
| # one comment are dispatched by a single run, and cancelling an in-flight route | ||
| # would silently drop a mention. | ||
| group: persona-mention-${{ github.event.comment.id || github.run_id }} | ||
| cancel-in-progress: false | ||
|
|
||
| jobs: | ||
| route: | ||
| runs-on: ubuntu-latest | ||
| permissions: | ||
| contents: read # actions/checkout of the tooling repo | ||
| issues: read # read the item's labels for the opt-out / gate checks | ||
| pull-requests: read | ||
| # The CHEAP pre-filter, mirrored from pm_should_route (tests/persona_mention.bats). | ||
| # It runs BEFORE the job starts, so secrets are never exposed to a run that a | ||
| # bot, a marked comment, or an untrusted actor triggered. | ||
| # | ||
| # Two independent recursion axes, per §4.1 and the #860 postmortem (1,481 | ||
| # acks in 4.5h). Comments posted via a PAT re-trigger workflows, unlike | ||
| # GITHUB_TOKEN, and with N mutually addressable personas the cycles are | ||
| # combinatorial, not self-loops: | ||
| # 1. bot actor — a comment authored by an agent identity never routes | ||
| # 2. marker — a comment carrying '<!-- persona:' never routes | ||
| # Either alone closes the common loop; both together also close the case of | ||
| # an agent posting under a human's PAT. | ||
| # | ||
| # The trust check here is the conservative §4 default. A persona may TIGHTEN | ||
| # it in its own manifest (enforced below), never loosen it — so this gate is | ||
| # always at least as strict as the union of what the personas allow. | ||
| if: | | ||
| contains(github.event.comment.body, '@petry-projects/') && | ||
| github.event.comment.user.login != 'donpetry-bot' && | ||
| github.event.comment.user.login != 'github-actions[bot]' && | ||
| !contains(github.event.comment.body, '<!-- persona:') && | ||
| contains(fromJSON('["OWNER","MEMBER","COLLABORATOR"]'), github.event.comment.author_association) | ||
|
|
||
| steps: | ||
| - name: Checkout persona-mention tooling | ||
| uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 | ||
| with: | ||
| repository: petry-projects/.github | ||
| # Source the library from this reusable's own commit so the routing | ||
| # logic always matches the pinned workflow version (#465/#528). | ||
| ref: ${{ inputs.tooling_ref || github.job_workflow_sha }} | ||
| path: .persona-tooling | ||
| fetch-depth: 1 | ||
| persist-credentials: false # the tooling checkout needs no push creds | ||
|
|
||
| - name: Ensure PyYAML | ||
| # pm_manifest_query parses the manifest with python3 + yaml. PyYAML is NOT | ||
| # documented as preinstalled on the runner images, and this reusable runs | ||
| # on the CALLER's runner, so we cannot assume the caller installed it. | ||
| # Cheap no-op when already present. | ||
| # | ||
| # Hash- and binary-locked per the dependency-hardening gate | ||
| # (tests/dependency_hardening.bats). A bare `pip install pyyaml` is an | ||
| # unpinned, unverified supply-chain install running on every repo in the | ||
| # fleet — the gate caught exactly that here, and it was right to. | ||
| run: | | ||
| python3 -c 'import yaml' 2>/dev/null || \ | ||
| pip install --quiet --require-hashes --only-binary :all: \ | ||
| -r .persona-tooling/scripts/persona-mention-requirements.txt | ||
|
|
||
| - name: Route the mention | ||
| env: | ||
| GH_TOKEN: ${{ secrets.GH_PAT_WORKFLOWS }} | ||
| PERSONA_REF: ${{ inputs.persona_ref }} | ||
| # Every event field arrives via env, never interpolated inline into the | ||
| # run: body. A comment body and a login are attacker-controlled, and an | ||
| # inline `${{ }}` in a run: block is a script-injection vector. | ||
| COMMENT_BODY: ${{ github.event.comment.body }} | ||
| COMMENT_USER: ${{ github.event.comment.user.login }} | ||
| COMMENT_ASSOC: ${{ github.event.comment.author_association }} | ||
| COMMENT_URL: ${{ github.event.comment.html_url }} | ||
| EVENT_NAME: ${{ github.event_name }} | ||
| ITEM_NUMBER: ${{ github.event.issue.number || github.event.pull_request.number || github.event.discussion.number }} | ||
| SOURCE_REPO: ${{ github.repository }} | ||
| run: | | ||
| set -euo pipefail | ||
| # shellcheck source=scripts/lib/persona-mention.sh | ||
| source .persona-tooling/scripts/lib/persona-mention.sh | ||
|
|
||
| if ! pm_should_route "$COMMENT_USER" "$COMMENT_ASSOC" "$COMMENT_BODY"; then | ||
| echo "Not routable (bot actor, agent marker, trust floor, or no handle) — nothing to do." | ||
| exit 0 | ||
| fi | ||
|
|
||
| # Labels back BOTH the opt-out check and the write-mode gate, so an | ||
| # unreadable label set is never "no labels". `|| echo ""` here would let | ||
| # a transient API error bypass an opt-out and arm an ungated write — | ||
| # the fail-open pattern this framework already had four of (#755). | ||
| # | ||
| # Discussions genuinely have no labels API surface, so an empty set is | ||
| # the true answer there, not an error. That difference is load-bearing: | ||
| # write-mode on a discussion is refused below precisely because the gate | ||
| # cannot be verified. | ||
| LABELS="" | ||
| LABELS_KNOWN=false | ||
| if [ "$EVENT_NAME" = "discussion_comment" ]; then | ||
| echo "Discussion comment — no labels surface; opt-out/gate labels unavailable." | ||
| elif [ -z "${ITEM_NUMBER:-}" ]; then | ||
| echo "::error::no item number resolved for event '${EVENT_NAME}' — cannot check opt-out or gate labels" | ||
| exit 1 | ||
| else | ||
| if ! LABELS="$(gh api "/repos/${SOURCE_REPO}/issues/${ITEM_NUMBER}/labels" --jq '.[].name')"; then | ||
| echo "::error::could not read labels for ${SOURCE_REPO}#${ITEM_NUMBER} — refusing to route (an unreadable label set must never be read as 'no opt-out, no gate')" | ||
| exit 1 | ||
| fi | ||
| LABELS_KNOWN=true | ||
| fi | ||
|
|
||
| dispatched=0 | ||
| while IFS= read -r slug; do | ||
| [ -n "$slug" ] || continue | ||
| echo "::group::@petry-projects/${slug}" | ||
|
|
||
| url="$(pm_manifest_url "$slug")" | ||
| # Fetched UNAUTHENTICATED, on purpose. petry-projects/.github-private is | ||
| # PUBLIC despite its name (private: false), so no token is needed — and | ||
| # adding one is actively harmful. Measured against this very URL: | ||
| # | ||
| # unauthenticated -> 200 valid token -> 200 | ||
| # expired token -> 404 empty token -> 404 | ||
| # | ||
| # raw.githubusercontent answers a bad or MISSING token with 404, not 401. | ||
| # With an Authorization header, the moment GH_PAT_WORKFLOWS expires — or a | ||
| # repo adopts the stub before the secret exists, or a fork PR runs without | ||
| # secrets — every persona would 404 and be silently skipped as "not a | ||
| # persona". Unauthenticated cannot fail that way for a public repo, so the | ||
| # header only ADDS a silent-death path. (CodeRabbit asked for auth on the | ||
| # premise that the repo is private; it is not.) | ||
| # | ||
| # A 404 is a real ANSWER ("not a persona" — a plain team like | ||
| # @petry-projects/org-leads, or a typo). Anything else is a FAILURE to | ||
| # get an answer. `curl -f` collapses both into exit 22, which would | ||
| # make a raw.githubusercontent 5xx silently disable every persona | ||
| # fleet-wide with no error anywhere — indistinguishable from nobody | ||
| # being addressed. Separate them on the status code. | ||
| body_file="$(mktemp)" | ||
| # NO Authorization header, deliberately — see the note above the case. | ||
| http="$(curl -sSL --max-time 20 -o "$body_file" -w '%{http_code}' "$url" || echo 000)" | ||
| case "$http" in | ||
| 200) | ||
| manifest="$(cat "$body_file")" | ||
| ;; | ||
| 404) | ||
| rm -f "$body_file" | ||
| echo "No manifest at ${url} — not a persona (a plain team, or a typo). Skipping." | ||
| echo "::endgroup::" | ||
| continue | ||
| ;; | ||
| *) | ||
| rm -f "$body_file" | ||
| echo "::error::fetching ${url} returned HTTP ${http} — refusing to treat a fetch failure as 'not a persona'" | ||
| exit 1 | ||
| ;; | ||
| esac | ||
| rm -f "$body_file" | ||
|
|
||
| # The slug we routed on must equal the id the manifest claims. These | ||
| # cannot diverge while validate-personas.py holds, so a mismatch means | ||
| # the invariant broke — refuse rather than guess which one is right. | ||
| claimed="$(pm_persona_id "$manifest")" | ||
| if [ "$claimed" != "$slug" ]; then | ||
| echo "::warning::manifest at ${url} claims id '${claimed}' but was addressed as '${slug}' — refusing to route" | ||
| echo "::endgroup::" | ||
| continue | ||
| fi | ||
|
|
||
| read -r enabled mode opt_out <<<"$(pm_mention_decision "$manifest")" | ||
| if [ "$enabled" != "true" ]; then | ||
| echo "Persona '${slug}' does not enable the mention surface (mode=${mode}). Skipping." | ||
| echo "::endgroup::" | ||
| continue | ||
| fi | ||
|
|
||
| if [ -n "$opt_out" ] && printf '%s\n' "$LABELS" | grep -qxF "$opt_out"; then | ||
| echo "Item carries '${opt_out}' — persona '${slug}' is opted out here. Skipping." | ||
| echo "::endgroup::" | ||
| continue | ||
| fi | ||
|
|
||
| # A write-mode mention must be ARMED. §4 rule 2 makes gate_label | ||
| # schema-required for write surfaces, but the schema only enforces | ||
| # that it is DECLARED — declaring a lock is not locking the door. | ||
| # Without this, '@petry-projects/dev-lead do X' is an ungated write | ||
| # the moment the first write persona onboards (#755). | ||
| if [ "$mode" = "write" ]; then | ||
| gate="$(pm_mention_gate_label "$manifest")" | ||
| if [ -z "$gate" ]; then | ||
| echo "::error::persona '${slug}' declares mode=write with no gate_label — refusing (schema violation: §4 rule 2)" | ||
| exit 1 | ||
| fi | ||
| if [ "$LABELS_KNOWN" != "true" ]; then | ||
| echo "Persona '${slug}' is write-mode but labels are unavailable on this surface — cannot verify gate '${gate}'. Refusing." | ||
| echo "::endgroup::" | ||
| continue | ||
| fi | ||
| if ! printf '%s\n' "$LABELS" | grep -qxF "$gate"; then | ||
| echo "Persona '${slug}' is write-mode and NOT armed — '${gate}' absent. Skipping." | ||
| echo "::endgroup::" | ||
| continue | ||
| fi | ||
| echo "Write-mode armed by '${gate}'." | ||
| fi | ||
|
|
||
| # The persona's own floor, which may TIGHTEN the job-level default. | ||
| # An undeclared floor yields an empty set and pm_trust_ok denies — | ||
| # a persona that forgot to declare trust must not be more permissive | ||
| # than one that did. | ||
| floor="$(pm_mention_trust_floor "$manifest")" | ||
| # shellcheck disable=SC2086 # word-splitting is the point: floor is a set | ||
| if ! pm_trust_ok "$COMMENT_ASSOC" $floor; then | ||
| echo "@${COMMENT_USER} (${COMMENT_ASSOC}) is below persona '${slug}' floor [${floor}]. Skipping." | ||
| echo "::endgroup::" | ||
| continue | ||
| fi | ||
|
|
||
| echo "Routing to '${slug}' (mode=${mode})." | ||
| gh api \ | ||
| --method POST \ | ||
| --header "Accept: application/vnd.github+json" \ | ||
| /repos/petry-projects/.github-private/dispatches \ | ||
| --field event_type=persona-mention \ | ||
| --field "client_payload[persona]=$slug" \ | ||
| --field "client_payload[mode]=$mode" \ | ||
| --field "client_payload[source_repo]=$SOURCE_REPO" \ | ||
| --field "client_payload[item_number]=${ITEM_NUMBER:-}" \ | ||
| --field "client_payload[comment_url]=$COMMENT_URL" \ | ||
| --field "client_payload[requested_by]=$COMMENT_USER" | ||
| dispatched=$((dispatched + 1)) | ||
| echo "::endgroup::" | ||
| done < <(pm_extract_slugs "$COMMENT_BODY") | ||
|
|
||
| echo "Dispatched ${dispatched} persona(s)." | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| # Quality gates for the persona @-mention routing core. | ||
| # | ||
| # Standard: standards/persona-standards.md §4.1 (addressing). | ||
| # | ||
| # A bats suite with no CI runner is unenforced — the #613/#624 gap that | ||
| # canary-rollout-tests.yml exists to close. This is that runner for | ||
| # tests/persona_mention.bats, added with the suite rather than after it. | ||
| # | ||
| # Triggered on any PR (or push to main) that touches: | ||
| # - scripts/lib/persona-mention.sh (the pure routing core) | ||
| # - tests/persona_mention.bats (the suite) | ||
| # - .github/workflows/persona-mention-reusable.yml (the router that uses it) | ||
| # - .github/workflows/persona-mention-tests.yml (this file) | ||
| # | ||
| # Gates (all must pass before merge): | ||
| # 1. shellcheck — static analysis for the pure core | ||
| # 2. bats — the full routing suite, recursion guards included | ||
| # | ||
| # The core is side-effect-free and network-free by construction (fetching is the | ||
| # workflow's job, not the library's), so the suite pins routing behaviour — | ||
| # especially the two-axis recursion guard — with no API access at all. That | ||
| # matters here: .github-private#860 burned 1,481 acks in 4.5h from a single | ||
| # self-loop, and a regression in pm_should_route is the expensive kind. | ||
|
|
||
| name: Persona-mention Tests | ||
|
|
||
| on: | ||
| pull_request: | ||
| paths: | ||
| - 'scripts/lib/persona-mention.sh' | ||
| - 'tests/persona_mention.bats' | ||
| - '.github/workflows/persona-mention-reusable.yml' | ||
| - '.github/workflows/persona-mention-tests.yml' | ||
| push: | ||
| branches: | ||
| - main | ||
| paths: | ||
| - 'scripts/lib/persona-mention.sh' | ||
| - 'tests/persona_mention.bats' | ||
| - '.github/workflows/persona-mention-reusable.yml' | ||
| - '.github/workflows/persona-mention-tests.yml' | ||
|
|
||
| permissions: | ||
| contents: read | ||
|
|
||
| concurrency: | ||
| group: persona-mention-tests-${{ github.ref }} | ||
| cancel-in-progress: true | ||
|
|
||
| jobs: | ||
| test: | ||
| name: Lint and bats | ||
| runs-on: ubuntu-latest | ||
| timeout-minutes: 10 | ||
| steps: | ||
| - name: Checkout repository | ||
| uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 | ||
| with: | ||
| fetch-depth: 1 | ||
| persist-credentials: false | ||
|
|
||
| - name: Install bats, shellcheck, jq, and PyYAML | ||
| run: | | ||
| set -euo pipefail | ||
| sudo apt-get update -qq | ||
| sudo apt-get install -y --no-install-recommends bats shellcheck jq python3-yaml | ||
|
|
||
| - name: shellcheck | ||
| run: | | ||
| set -euo pipefail | ||
| shellcheck --severity=warning -x scripts/lib/persona-mention.sh | ||
|
|
||
| - name: Run bats suite | ||
| run: bats --print-output-on-failure tests/persona_mention.bats |
Oops, something went wrong.
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.