feat(github): close and reopen stories from Facility - #180
Conversation
adrian-lorenzo
left a comment
There was a problem hiding this comment.
Thanks for the contribution!
The marker-based retry needs a stronger identity. Any GitHub commenter can post the same marker, and this code will treat that comment as Facility-owned and edit it. It also reuses the original comment after a close, reopen, and second close, replacing the first rationale instead of preserving both decisions. Please scope retry recovery to a specific close attempt, verify that any reused comment belongs to this Facility app, and add regression tests for a forged marker, a second close after reopening, and cross-tenant access.
Please also complete the tracked behavior: show the closing actor and reason in the timeline, and ensure stories closed as not_planned are not classified as Shipped.
Addresses review on theam#102. The close-reason comment was matched by marker text alone, so any commenter could post the marker and have Facility edit their comment on the next retry. Recovery now requires three things at once: the marker naming this close attempt, a Bot author, and a login belonging to this app's configured slug. Without GITHUB_APP_SLUG the set is empty and no comment is ever reused, so the failure mode is a duplicate comment rather than an edit to someone else's. The marker also carries the attempt that wrote it, derived from the request's idempotency key. A retry of one attempt recovers its own comment; a later close after a reopen is a different attempt and writes a new one, so the issue thread keeps every decision instead of overwriting the previous rationale. Mirrors GitHub's state_reason (migration 0042) so a story abandoned as not_planned is no longer classified as Shipped — it leaves the board rather than joining delivered work — and surfaces the closing actor and reason on the timeline's issue_closed milestone, read back from the story.closed audit event. Adds regression tests for a forged marker, a second close after reopening, cross-tenant close and reopen, the pipeline classification, and the timeline attribution.
|
Both confirmed and fixed. Marker identity. Recovery now requires three things together: the Attempt scoping. The marker now names the attempt that wrote it — Two calls I'd rather you overrule now than after merge:
Also here: Regression tests cover the forged marker, close → reopen → close, |
adrian-lorenzo
left a comment
There was a problem hiding this comment.
Thanks for the contribution!
The browser generates a new Idempotency-Key inside every submit() call. If the reason comment succeeds but the GitHub close fails, clicking confirm again uses a different marker and posts a duplicate comment. Please keep the key stable across retries of the same close attempt and test that flow.
These endpoints also use repoId in the query to select the repository, while the current idempotency layer ignores query parameters. With the same story number, key, and body, closing a story in a second repository can replay the first response without closing the second story. Please include the repository in idempotency equivalence and add a two-repository regression test.
story.closed is currently recorded even when GitHub was already closed and changed is false. latestStoryClose then presents that event as the reason for the current closure without tying it to the current closedAt. This lets a no-op request claim an external closure and can resurrect an old Facility rationale after an external reopen and close. Please record or display attribution only when it belongs to the current transition, with regression coverage for both cases.
Stories closed as not_planned also disappear from every stage, making their history and the new reopen action undiscoverable after leaving the detail page. Please keep them reachable through a terminal stage or another explicit surface.
Finally, please update the branch: the current merge with main fails typechecking because state is declared twice, and the migration should be renumbered after the migrations now on main.
With those fixes, we can approve and merge it!
|
All five fixed. The first one is the important one — the stable key was the 1. Idempotency key stability. 2. Repository in idempotency equivalence. Fixed in the shared layer rather 3. Attribution. 4. Abandoned stories. They now land in a terminal 5. Branch. Merged Coverage for the new paths: same key across retries of one attempt; two API 599 passed / 1 skipped, web 74, SDK 23, typecheck 16/16, lint 428 files, |
feat(github): close and reopen stories from Facility
Closes #102.
What changed
Adds:
POST /v1/projects/:projectId/stories/:number/closePOST /v1/projects/:projectId/stories/:number/reopenBoth routes are gated by
repos:write, acceptIdempotency-Key, and emitstory.closed/story.reopenedaudit events.Closing a story now:
state_reason;Reopen is intentionally minimal: it reopens the issue, updates the mirror, and records the audit event without adding another comment.
In the web app, the story page gains a
Close storyaction for issue stories — a required reason and acompleted/not plannedchoice — which becomes a one-clickReopen storyonce the story is closed. Both are shown only to principals holdingrepos:write.Decisions
state_reasondefaults tonot_planned. This endpoint represents the abandon / won't-do path. Completed work normally closes through a merged pull request, where GitHub already records the issue as completed. Usingnot_plannedby default keeps the API and UI semantics aligned with that workflow.repos:write, not a new permission. #102 asked for this to be settled explicitly.issues:writegoverns platform watchtower issues, a different concept, while this verb mutates a connected repository in the same way as the pull-request link routes.A direct verb, not a HITL proposal. As proposed in the issue, the human closing the story is already the gate.
GitHub remains the source of truth. The local mirror is updated only after the GitHub mutation succeeds. This departs from the issue's suggested optimistic mirror update: waiting for GitHub's response ensures the mirror cannot claim a close that did not happen.
Retries are safe. If the closing-reason comment is written but the state transition fails, a retry updates the marked comment instead of posting a duplicate. If the issue is already closed on GitHub, Facility reconciles the mirror and returns
changed: false.Known limitation
Stories closed as
not_plannedstill appear under theShippedpipeline stage for seven days because the mirror does not currently store GitHub'sstate_reason. That requires a separate schema / classification change and is intentionally out of scope here.Verification
pnpm --filter @facility/api test— 478 passed, 1 skipped (Docker-backed sandbox E2E)pnpm --filter @facility/web test— 66 passedpnpm --filter @facility/sdk test— 23 passed (route manifest and committed-contract drift guards)pnpm lint,pnpm typecheck,pnpm build:clean— cleanpnpm guards— 2 guards, 0 failedpackages/sdk/openapi.jsonandsrc/schema.d.tsregenerated withpnpm --filter @facility/api openapipnpm verifyis currently blocked by a pre-existing timing flake inrunner/test/workspace.test.ts(.facility-hang.pidnot written within the 100 ms deadline). I reproduced the same failure with this change fully stashed, while the affected test file passes standalone.