Skip to content

feat(github): close and reopen stories from Facility - #180

Open
prubianes wants to merge 5 commits into
theam:mainfrom
prubianes:feature/close-story
Open

feat(github): close and reopen stories from Facility#180
prubianes wants to merge 5 commits into
theam:mainfrom
prubianes:feature/close-story

Conversation

@prubianes

Copy link
Copy Markdown

feat(github): close and reopen stories from Facility

Closes #102.

What changed

Adds:

  • POST /v1/projects/:projectId/stories/:number/close
  • POST /v1/projects/:projectId/stories/:number/reopen

Both routes are gated by repos:write, accept Idempotency-Key, and emit story.closed / story.reopened audit events.

Closing a story now:

  1. records the human reason as a GitHub issue comment attributed to the authenticated principal;
  2. closes the issue with state_reason;
  3. updates Facility's local mirror from the state returned by GitHub;
  4. records the decision in the audit log.

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 story action for issue stories — a required reason and a completed / not planned choice — which becomes a one-click Reopen story once the story is closed. Both are shown only to principals holding repos:write.

Decisions

state_reason defaults to not_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. Using not_planned by 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:write governs 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_planned still appear under the Shipped pipeline stage for seven days because the mirror does not currently store GitHub's state_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 passed
  • pnpm --filter @facility/sdk test — 23 passed (route manifest and committed-contract drift guards)
  • pnpm lint, pnpm typecheck, pnpm build:clean — clean
  • pnpm guards — 2 guards, 0 failed
  • packages/sdk/openapi.json and src/schema.d.ts regenerated with pnpm --filter @facility/api openapi

pnpm verify is currently blocked by a pre-existing timing flake in runner/test/workspace.test.ts (.facility-hang.pid not written within the 100 ms deadline). I reproduced the same failure with this change fully stashed, while the affected test file passes standalone.

@adrian-lorenzo adrian-lorenzo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@prubianes

Copy link
Copy Markdown
Author

Both confirmed and fixed.

Marker identity. Recovery now requires three things together: the
attempt marker, a Bot author, and a login matching this app's
configured slug (reusing the existing facilityBotLogins helper). A
comment Facility didn't author is never touched. One deployment note:
with no GITHUB_APP_SLUG set the match set is empty, so recovery
disables itself — the failure mode becomes a duplicate comment, never an
edit to a comment that isn't ours.

Attempt scoping. The marker now names the attempt that wrote it —
<!-- facility:story-close:<id> -->, id derived from the request's
Idempotency-Key. The same attempt retried corrects its own comment; a
close after a reopen is a different attempt, so it writes a new one and
the first rationale stays on the issue.

Two calls I'd rather you overrule now than after merge:

  1. Recovery depends on Idempotency-Key. Without one there's no
    attempt identity, so a retry duplicates the comment instead of
    correcting it. The key is what makes two requests the same attempt,
    and the UI sends one on every call — but if you want dedupe without
    the header, the alternative is persisting a comment id on gh_issues
    and clearing it on reopen.

  2. not_planned stories now leave the pipeline entirely rather than
    getting a terminal stage. "Not Shipped" didn't say where they should
    go, and excluding them left PipelineStageKey and the published
    contract untouched. Straightforward to give them their own stage if
    you'd rather they stay visible.

Also here: gh_issues.state_reason (migration 0042), mirrored from
webhooks and backfill sync, and the timeline's issue_closed milestone
now carrying the closing actor and reason from the story.closed audit
event.

Regression tests cover the forged marker, close → reopen → close,
cross-tenant close and reopen (by project id and by bare story number),
the pipeline classification, and the timeline attribution.

@adrian-lorenzo adrian-lorenzo left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

@prubianes

Copy link
Copy Markdown
Author

All five fixed. The first one is the important one — the stable key was the
whole point of the previous round and the UI was quietly undoing it.

1. Idempotency key stability. crypto.randomUUID() was being called inside
submit(), so every confirm click was a new attempt and a retry after a failed
close posted a second comment. The key now lives in a ref, minted on the first
submit of an attempt and cleared only on success or cancel. Pulled the
lifecycle out as attemptKey() so it's unit-tested directly rather than
through the DOM.

2. Repository in idempotency equivalence. Fixed in the shared layer rather
than this route: the identity hash now includes a canonicalized,
order-independent query string. Worth flagging that this widens beyond story
close — /pulls/{number}/closing-issues?repoId= has the same shape and the
same latent bug, and this fixes it too. If you'd rather keep the change scoped
to these two endpoints, say so and I'll narrow it.

3. Attribution. story.closed is now written only when the request
actually performed the transition, and its payload carries the GitHub-confirmed
closedAt. latestStoryClose matches on that timestamp rather than taking the
most recent event, so a no-op can't claim an external closure and an old
rationale can't resurface after an external reopen and close. story.reopened
got the same treatment.

4. Abandoned stories. They now land in a terminal Abandoned stage rather
than leaving the board, on the same seven-day window as Shipped, so the
history and the reopen action stay reachable. That adds abandoned and
abandoned_recently to the published stage enums — a contract addition I'd
rather you approve than discover. Happy to change the label or the window, or
to move to a different surface if a seventh column is unwelcome.

5. Branch. Merged upstream/main. The duplicate was upstream adding
state?: string to the issues.get type where this branch had added its own —
the merge kept both. Migration renumbered to
0044_gh_issues_state_reason.sql. One local gotcha if you test this branch on
a database that saw the earlier numbering: the constraint already exists under
the old filename, so the renumbered migration errors until that database is
recreated. Fresh databases apply it once.

Coverage for the new paths: same key across retries of one attempt; two
repositories closing the same story number under one key and body without a
replay; the full external reopen-and-reclose sequence plus a no-op close,
asserting attribution disappears and no event is written; and the abandoned
stage placement.

API 599 passed / 1 skipped, web 74, SDK 23, typecheck 16/16, lint 428 files,
build 10/10, guards clean.

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.

Close a story from Facility, with a reason

2 participants