[Epic] 1647-epic-github-app-11i - #1659
Conversation
…file shows the identical warning), and `eslint .` reports no errors. Everything is clean. Summary When I investigated issue #1655, I found that the `propr github-app manifest` command was already fully implemented in the base branch (`origin/1647-epic-github-app-11i`, built earlier under PR #1651). My branch had no commits beyond the base. I verified the existing implementation against every acceptance criterion and found the one genuine gap: there was no dedicated test exercising the manifest generator. Verification of the existing implementation (all acceptance criteria met) I ran the command end-to-end and confirmed: - `propr github-app manifest --root <dir> --public-url https://propr.example.com` writes `github-app-manifest.json` and `github-app.env` into the target dir. - The manifest contains the correct webhook URL (`https://host/webhook`), all required permissions (`contents:write`, `issues:write`, `pull_requests:write`, `metadata:read`, `actions:read`, plus `checks:read`/`statuses:read` which GitHub requires to deliver `check_run`/`status` events), and all 7 webhook events — matching core's `SUPPORTED_WEBHOOK_EVENTS` exactly. - `github-app.env` contains `GH_AUTH_MODE=app`, `GITHUB_EVENT_INTAKE_MODE=direct_webhook`, and the same generated secret as the manifest. - A second run without `--force` exits 1 with "Refusing to overwrite…"; `--force` overwrites and exits 0. - The command is exported from the barrel (`commands/index.ts`) and registered in the CLI entrypoint (`index.ts`). What I added `packages/cli/src/commands/githubAppCommands.test.ts` — a 14-case test suite (following the project's `node:test` + `assert/strict` convention) that locks in the acceptance criteria and guards against regressions: - File creation, manifest contents (URL/permissions/events), and the direct-webhook `.env` snippet sharing the manifest's secret. - Strong secret generation (64 hex chars), supplied-secret passthrough, and trailing-slash URL normalization. - Overwrite protection — including the case where only one of the two files pre-exists (the preflight refuses so no half-pair is written) — plus `--force` overwrite. - URL validation (rejects non-http(s) and missing URL), custom webhook URL override, org-scoped create URL, `redactSecret` non-mutation, and an event-list parity check against core. Results: 14/14 tests pass, `tsc -p tsconfig.test.json` typechecks clean, and `eslint .` reports no errors.
Summary Key finding: Issue #1656 is a verbatim duplicate of issue #1648, whose implementation is already complete and merged into the base branch `1647-epic-github-app-11i`. I verified this rigorously rather than assuming: - Issue #1648 has the identical title, body, requirements, and acceptance criteria to #1656. - Its implementation landed in commit `8d3a418e` ("...for issue #1648"), which `git merge-base --is-ancestor` confirms is an ancestor of this branch's base. - The diff between this branch and the base touches none of the relevant setup/manifest files. The functional work the issue asks for is already present and passing tests in the base: | Requirement | Where it lives | Status | |---|---|---| | Setup action for manifest generation | `SetupActions.inspectGithubAppManifest` / `generateGithubAppManifest` (engine.ts) | ✅ | | Prompt on `app` auth + `direct_webhook` | `maybeGenerateAppManifest()` (engine.ts:662), wired into intake step (engine.ts:1135) | ✅ | | Public URL from `API_PUBLIC_URL`/`FRONTEND_URL`, else ask | engine.ts:678 + both renderers' `configureGithubAppManifest` | ✅ | | Writes same files as `propr github-app manifest` | delegates to `generateGithubAppManifest` | ✅ | | Next steps surfaced (GH_APP_ID, etc.) | folded into intake `settle` (engine.ts:1172) | ✅ | | Re-runnable, no overwrite without confirm | `filesExist` → regenerate confirm (default no) | ✅ | | Sequential + Ink renderers | `sequential.ts:432`, `SetupApp.tsx:365` | ✅ | | Existing relay/polling unchanged | guarded by `direct_webhook && app` early-return | ✅ | I ran the relevant suites — all green: engine (33), sequential (24), Ink (16), manifest command (14), and the end-to-end `cliSetupWizard` integration test (30, including "writes the same files as `propr github-app manifest`" and the re-run warning case). What I changed: Since fabricating redundant code would be wrong, I closed the one genuine gap #1648 left — documentation. The docs described the standalone `propr github-app manifest` command but never mentioned that `propr setup` now offers to generate the manifest during the direct-webhook flow (the user-facing behavior this issue added). I added accurate, verified notes to: - `docs/docs/features/propr-cli.md` — in the "Own GitHub App (direct webhook mode)" section. - `docs/docs/tutorials/setup-server.md` — in the manifest-scaffolding section. Both notes precisely match the implementation (when it's offered, URL detection from `.env`, no-overwrite re-run safety, and the next-steps folded into the intake step). If the intent was net-new functional work, there is none to do here — the feature is already shipped in the base branch via #1648. I'd recommend the planner deduplicate #1656 against #1648.
I analyzed issue #1657 ("Document and validate the automated own-GitHub-App direct webhook workflow") and audited the base branch `1647-epic-github-app-11i` against every requirement and acceptance criterion. Finding: Most of the issue was already satisfied in the base branch — `.env.example` references `propr github-app manifest` and explains the generated files + manual values; the setup, deployment, CLI, and GitHub-auth docs all document the command (all cross-reference anchors verified to resolve); routing WebSocket is still framed as the recommended default; and `propr check` already appends a manifest hint to missing own-App values in direct-webhook mode. All 34 existing tests pass. The one genuine gap was in this issue requirement: > `propr check` should also recognize generated-but-incomplete own-App setup files and provide better next actions. The existing hint was static — it always told users to *"Generate a ready-to-fill manifest"*, even when they'd already run the command and just needed to finish. Since users run `propr check` precisely *between* generating the manifest and filling in values, this was misleading. Changes made: 1. `packages/cli/src/commands/checkCommands.ts` — Made the direct-webhook guidance context-aware: - Added `manifestFilesPresent()` to detect `github-app-manifest.json` / `github-app.env` in the stack root. - Added a "generated" hint variant that points at the existing files and the remaining GitHub-side steps (create/install the App, fill in `GH_APP_ID` / `GH_INSTALLATION_ID` / `HOST_GH_PRIVATE_KEY`), and mentions `propr github-app manifest --force` only for regeneration — so the command stays discoverable. - Threaded the stack `rootDir` through `checkGithubAuth()` and `checkGithubIntakeMode()` (backward-compatible optional param) and the `runChecks` call sites. 2. `test/cliCheckGithubGuidance.test.ts` — Added 2 tests covering the generated-files branch for both auth and intake-mode checks (assert it names the files + stack root, still mentions the command, and no longer says "Generate a ready-to-fill manifest"). 3. Docs (`propr-cli.md`, `setup-server.md`, `deployment.md`) — Refined the three sentences describing `propr check`'s behavior so they accurately reflect file recognition rather than "repeats the command." Verification: CLI package typechecks clean (`tsc --noEmit`, exit 0); all 36 guidance/manifest tests pass. The one unrelated failure in `checkRunHandler.test.ts` is pre-existing and environmental — it fails at import time on `mock.module is not a function` (the installed Node v20.20.2 predates that API), before any of my code runs.
|
/merge |
|
🔀 Resolved merge conflicts from Conflicting Files
Resolution SummaryAll conflicts are resolved. Here is the detailed summary. Merge Conflict Resolution Summary — PR #1659 (
|
|
/ultrafix |
|
🔄 Ultrafix loop started (goal: 8/10, max cycles: 10) First action:
|
![propr-dev[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
Automatically resolved merge conflicts after merging target branch into PR branch. Resolved conflicts in: - docs/docs/features/propr-cli.md - docs/docs/operations/deployment.md - docs/docs/tutorials/setup-server.md - packages/cli/src/commands/checkCommands.ts - test/cliCheckGithubGuidance.test.ts PR: #1659 Model: claude-opus-4-8
|
✅ AI Code Review Complete requested by @integry Posted 1 review: |
🔍 AI Code Review — codex:gpt-5.5Overall EvaluationThis PR adds focused test coverage for the GitHub App manifest generator, covering file output, manifest shape, webhook secrets, overwrite behavior, URL validation, org create URLs, redaction, and event-list drift. Overall quality is good and the tests are readable. I would consider this ready to merge, with a couple of test-strengthening improvements worth considering. Findings🟡 Overwrite test does not verify disk contents — In 🟢 Manifest event assertion allows extra events — In 🟢 Custom webhook URL test could cover validation parity — In ✅ Good coverage of acceptance criteria — The tests cover the main user-facing behavior: generated files, manifest permissions/events, direct webhook env mode, secret consistency, overwrite refusal, ✅ Temp directory helper is clean and safe — ✅ Redaction immutability is explicitly tested — ScoreStrong test-only PR with clear coverage of the intended behavior. The main remaining gaps are around making a couple of assertions stricter so regressions cannot slip through with superficially correct return values. Score: 8/10 🤖 Review Details
|
1 participant
Epic PR
This PR aggregates all changes from child PRs merged to the
1647-epic-github-app-11ibranch.Created automatically by ProPR