Skip to content

ci(contribution): switch trigger to pull_request_target so check fires on every PR - #89

Merged
ai-hpc merged 1 commit into
mainfrom
fix/contribution-check-pull-request-target
May 18, 2026
Merged

ci(contribution): switch trigger to pull_request_target so check fires on every PR#89
ai-hpc merged 1 commit into
mainfrom
fix/contribution-check-pull-request-target

Conversation

@ai-hpc

@ai-hpc ai-hpc commented May 18, 2026

Copy link
Copy Markdown
Contributor

Summary

Switches the Contribution / PR body checklist workflow trigger from pull_request to pull_request_target so the check fires on every PR — including PRs whose head branch was created before the workflow itself existed.

Fixes the user-reported "ci only check 5" symptom on PRs #78 / #83 / #87.

Changes

  • .github/workflows/contribution.ymlon: pull_request:on: pull_request_target:. Same event types (opened, edited, synchronize, reopened). Same branches: [main] filter. Added an inline rationale comment that calls out the security caveat and why it doesn't apply here.
  • CHANGELOG.md — one-line addendum to the existing Unreleased entry explaining the trigger choice.

Real Behavior Proof

  • I have built and run the affected code locally (or noted why I could not).
  • I have verified the change end-to-end on Jetson hardware OR explained the equivalent verification path I used.

What I ran

Diagnosis path:

gh pr list --state open --limit 5 --json number,headRefOid --jq '.[] | "#\(.number) [\(.headRefOid[0:7])]"'
# PR #87 [a26a25a]: core: preserve genie runtime prompt context
# PR #83 [f6ccece]: fix(telegram): use AtomicU64 nonce ...
# PR #78 [6225417]: fix(telegram): spawn handle_update ...

for pr in 87 83 78; do
  gh pr view $pr --json statusCheckRollup --jq '[.statusCheckRollup[].name] | length, .'
done
# Each prints: 5  (and lists fmt / aarch64 / clippy / test / no-default-features only)
# — confirming "PR body checklist" is missing from all three.

Root cause: on: pull_request: resolves the workflow file from the PR's head ref. Those three PRs were opened before #88 landed, so their head refs don't contain .github/workflows/contribution.yml and the workflow never schedules.

pull_request_target reads the workflow file from the base ref instead. Once this lands on main, the check will appear on every existing PR the next time the PR is touched (push to head, edit body, reopen) — and on every new PR immediately. No rebase required on contributor branches.

What I observed

.github/workflows/contribution.yml diff is exactly one keyword change in the on: block plus a longer explanatory comment. The job body is unchanged — it still reads github.event.pull_request.body and never checks out the PR, so the well-known pull_request_target security caveat (untrusted code running with secret access) does not apply.

This PR itself will be the verification:

Test plan

Notes for reviewers

  • pull_request_target security caveat: this trigger runs in the context of the base branch, which means GitHub Actions grants it write access to secrets and GITHUB_TOKEN. The standard footgun is "workflow checks out PR code and runs it, exposing secrets to the contributor." This workflow does neither — actions/checkout is not used, no PR-controlled script is executed. The only PR-controlled data read is ${{ github.event.pull_request.body }} (used in a bash grep/awk pipeline, not eval'd as code) and ${{ github.event.pull_request.title }} (matched against a fixed glob allowlist). Both are safe to consume.
  • permissions: block stays contents: read + pull-requests: read (no write), so even though pull_request_target could grant more, we don't ask for it.

The original `pull_request` trigger meant the workflow only ran when
the PR's head branch contained the workflow file. PRs opened before
the contribution-checklist landed (#78, #83, #87, ...) therefore
didn't get the check — total stayed at 5 instead of 6.

`pull_request_target` runs the workflow from the base branch's copy
of this file, so the check fires regardless of when the PR head was
branched. Safe here because the job only reads
`github.event.pull_request.body` from the event payload — it never
checks out or executes PR-controlled code, so the standard
pull_request_target security caveat (untrusted code with secret
access) doesn't apply.
@ai-hpc
ai-hpc merged commit 8045d35 into main May 18, 2026
5 checks passed
hunnyboy1217 added a commit to hunnyboy1217/genie-claw that referenced this pull request May 19, 2026
…iePod#21)

Re-submission against current `main` per review feedback on the prior
revision. Drops the `.github/workflows/ci.yml` rewrite that conflicted
with the CI evolution landed across PRs GeniePod#37 / GeniePod#62 / GeniePod#89 / GeniePod#91, drops
the macOS matrix axis (Jetson is the deployment target), and drops
the coverage job (worth its own PR). Keeps the high-value content:

- `voice_loop::process_transcript` — extracted from `voice_cycle` so
  the post-record orchestration (intent gate, speaker identity,
  memory recall, quick-tool fast path, LLM streaming + TTS, tool
  dispatch, conversation persistence, latency banner, memory
  extract) can be driven with mocks. `ProcessTranscriptInputs`
  carries `wav_path: Option<&str>` and `tts_engine_override:
  Option<&TtsEngine>` so tests pass `None` / `Some(&silent_tts)`
  without leaking test-only ceremony into production. `voice_cycle`
  is now a thin wrapper around the audio-bound prelude + delegation.
- `TtsEngine::snapshot()` — config-only copy so a borrowed
  `&TtsEngine` can be wrapped in `Arc<TtsEngine>` for
  `streaming::stream_and_speak`, which is `Arc`-typed since PR GeniePod#61.
- `MockLlmBackend` (`LlmClient::mock(replies)`),
  `SttEngine::mock(transcripts)` + `MockTranscript`,
  `TtsEngine::silent()` — the test doubles the integration test
  needs. All three drop into the existing public surface.
- `crates/genie-core/tests/voice_loop_integration.rs` — ten
  `#[tokio::test]` cases. The canonical case
  `process_transcript_drives_full_voice_cycle_with_mocks` calls
  `process_transcript` with mock LLM (tool-call reply then summary
  reply), silent TTS, real `Memory`, real `ConversationStore`, and
  a `ToolDispatcher` wired to a tool-audit JSONL, then asserts on
  the three AC-B observables.
- `crates/genie-core/src/memory/mod.rs` — every test gets its own
  `${tmpdir}/geniepod-mem-${label}-${pid}-${id}-${nanos}/` parent
  dir so `Memory::open`'s `canonical_dir = path.parent().join("memory")`
  derivation no longer collides under parallel execution. Fixes the
  `promotion_redacts_person_memory_in_namespace_note` flake.
- `crates/genie-core/src/tools/parser.rs` — the
  `try_tool_call_executes_single_key_system_info_shape` test is now
  `#[cfg(target_os = "linux")]` because the assertion shape
  (`Memory available:`) only renders on Linux where
  `tegrastats::mem_available_mb()` can read `/proc/meminfo`.
@ai-hpc
ai-hpc deleted the fix/contribution-check-pull-request-target branch May 28, 2026 04:24
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