Add Plans tab: ordered cross-repo checklists from GitHub issues - #4
Open
woodwardmw wants to merge 5 commits into
Open
Add Plans tab: ordered cross-repo checklists from GitHub issues#4woodwardmw wants to merge 5 commits into
woodwardmw wants to merge 5 commits into
Conversation
A Plan is a `plan`-labelled issue in the repo where most of its work happens,
holding a flat checklist of ordered Steps with per-Step metadata in trailing
HTML comments. The dashboard gathers Plans across the org, shows live PR state
beside each Step, and lets people tick Steps off.
Done is asserted by a person, never inferred: a merged PR is displayed as live
state next to an unticked Step. The Steps that make a rollout dangerous —
migrations, deploys, verifications — leave no trace the dashboard can observe,
so inferring some ticks and not others would make "ticked" ambiguous.
- GET /api/plans list, parse, resolve refs
- POST /api/plans/{repo}/{number}/steps/{index} tick, with line verification
- POST /api/plans/{repo}/{number}/close close a finished Plan
Plan bodies are never cached (a stale checklist can get a migration run twice);
only the search that finds them is. Ticking re-reads the issue and verifies the
target line before writing, returning 409 rather than clobbering a concurrent
edit, and uses GH_WRITE_TOKEN so writes need only Issues: Read+Write rather
than whatever the read token happens to carry.
PLAN-FORMAT.md is the spec agents author against; CONTEXT.md carries the
vocabulary and docs/adr/ the reasoning.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012p8sz3UKvdpbPzzJ8AP1gF
`by:`/`at:` were documented as dashboard-only. An agent acting on a person's explicit instruction is that person's hand, not an inferrer, so it may write them — stamped with the person's login, never the agent's. Inferring Done from a merged PR remains forbidden. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012p8sz3UKvdpbPzzJ8AP1gF
…observed
Four criticals, all reproduced before fixing and re-verified after.
Critical
- `by` was unvalidated free text, which enabled three separate attacks: stored
XSS (it reaches `displayName()`, which interpolates raw), fabricated Step
injection (an embedded newline splices a new checklist line into a live Plan),
and metadata takeover (`by:"eve repo:evil ref:1"` redirects that Step's
live-state lookup, since `_parse_meta` is last-token-wins). Now validated
against the GitHub login charset, and `displayName()` escapes unconditionally.
- A negative `index` slipped past `index >= len(steps)` and Python's wraparound
silently ticked the *last* Step — in testing, "cut over production traffic".
Correctness
- Both write routes now verify the target issue carries the `plan` label, so
they cannot rewrite or close arbitrary issues in the token's scope. Closing
also re-checks server-side that every Step is Done.
- Per-Plan asyncio.Lock around the read-verify-write cycle. The `expect` check
only ever closed the same-line race; two ticks on different Steps could both
pass it and the second write would drop the first. Single-worker deployment
makes a process-local lock sufficient.
- `expect` is now required, so the guarantee holds for every caller rather than
only the bundled JS.
- Typed request body, so `{"done": "false"}` no longer coerces to True and a
malformed body returns 422 instead of 500.
- Write-path and plan-search `gh` calls are wrapped; failures log and return
502, or fall back to the cached Plan list, instead of a bare 500.
Silent data loss
- `_META_RE` is anchored and excludes angle brackets, so a literal `<!--` in a
Step's own text can no longer shadow its real metadata.
- `*` and `+` task-list bullets are recognised; GitHub renders them, and they
were being silently reclassified as prose. The bullet is preserved on rewrite.
- A malformed `ref` surfaces as a warning instead of being indistinguishable
from no ref at all.
- Ticking preserves the body's line endings and trailing newline, so editing one
line no longer reformats the whole issue.
Done vs observed
- Emerald meant "done" everywhere else in this app, and was being used for
"merged" beside an *unticked* Step — undercutting ADR 0002 exactly where it
matters. Emerald is now reserved for Done; an unticked Step whose PR has
merged gets an accent dot in its checkbox and "not yet confirmed", so the
control itself distinguishes observed from asserted.
- Warning states get a bordered pill; "next up" gets a pill matching its
importance.
Frontend
- `canWrite` reaches the controls, so read-only mode disables them rather than
offering a click that only fails.
- A 409 no longer erases its own message: the recovery reload re-renders
synchronously from cache, so the error is set after it, not before.
- The identity error scrolls into view and focuses the picker.
- Checkbox is a 28px target (was 16px, under the WCAG 2.2 floor) with
role=checkbox, aria-checked and a real label.
- Long Plan titles truncate instead of forcing horizontal scroll.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012p8sz3UKvdpbPzzJ8AP1gF
There was a problem hiding this comment.
Pull request overview
Adds a new Plans feature to the FastAPI PM Dashboard, representing ordered cross-repo rollout/migration/deploy checklists backed by plan-labelled GitHub issues. This extends the existing dashboard pattern of /api/* JSON endpoints consumed by static/app.js, with optional write capability gated by a dedicated GH_WRITE_TOKEN.
Changes:
- Adds a new Plans tab in the UI with rendering, per-step live GitHub state, and tick/close actions.
- Introduces new backend endpoints to list plans and perform verified write-backs (tick/un-tick steps, close plan), including cache handling for plan discovery.
- Adds documentation/specs and ADRs for plan format, vocabulary, and design rationale; updates README and Copilot instructions accordingly.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
templates/index.html |
Adds the Plans tab button and IDs range controls so the UI can hide date-range navigation on Plans. |
static/app.js |
Implements Plans UI rendering, ticking/closing interactions, and improves display-name escaping. |
dashboard.py |
Adds GH_WRITE_TOKEN support in run_gh, implements /api/plans read endpoint plus write endpoints for ticking steps and closing plans. |
README.md |
Documents the Plans tab and adds configuration notes for GH_WRITE_TOKEN. |
PLAN-FORMAT.md |
Adds the canonical authoring specification for Plan issues/steps. |
CONTEXT.md |
Adds shared vocabulary and definitions for reporting + Plans concepts. |
docs/adr/0001-plans-stored-as-github-issues.md |
ADR: why plans are stored as GitHub issues and implications of that design. |
docs/adr/0002-done-is-asserted-not-observed.md |
ADR: why “Done” is asserted by a person and never inferred from GitHub state. |
.github/copilot-instructions.md |
Updates repository guidance to reflect the new Plans/write-token design constraints. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+1028
to
+1043
| m = _STEP_RE.match(step["raw"]) | ||
| comment = _META_RE.search(m.group(4)) | ||
| meta = _parse_meta(comment.group(1)) if comment else {} | ||
| if done: | ||
| meta["by"] = by | ||
| meta["at"] = datetime.now(timezone.utc).strftime("%Y-%m-%dT%H:%M:%SZ") | ||
| else: | ||
| meta.pop("by", None) | ||
| meta.pop("at", None) | ||
|
|
||
| rendered = _fmt_meta(meta) | ||
| text = _META_RE.sub("", m.group(4)).strip() | ||
| line = f"{m.group(1)}{m.group(2)} [{'x' if done else ' '}] {text}" | ||
| if rendered: | ||
| line += f" <!-- {rendered} -->" | ||
| lines[step["line"]] = line |
Copilot correctly caught that the "round-trips byte-for-byte" claim held only for canonically-formatted lines: `- [ ]Text` gains a space, extra spaces before the metadata comment collapse, and trailing whitespace is dropped. Keeping the normalisation — it touches only the edited line and moves it toward the documented format — but saying so, and correcting the PR description. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012p8sz3UKvdpbPzzJ8AP1gF
Contributor
Author
|
Addressed the Copilot review comment on the step-rewrite path. Accepted — the claim was wrong, not the code. Verified the round-trip against five line shapes:
So "un-ticking restores the original line byte-for-byte" was only true for canonically-formatted lines. The PR description has been corrected. Keeping the normalisation. It touches only the line being ticked, and moves it toward the format Worth noting the whole-body case was a real bug and is fixed separately in 03b4b71: ticking used to rejoin with |
…tion exact Verification pass confirmed 8 of 9 fixes but found one regression and one gap. Regression I introduced: adding `escHtml` inside `displayName()` turned a harmless prototype lookup into a render-aborting TypeError. `displayNames` is a plain object, so a login of "constructor", "toString", "valueOf" or "hasOwnProperty" returns an inherited function instead of undefined, skipping the `||` fallback — and `escHtml` then called `.replace` on a function. Before the escaping it merely stringified to cosmetic garbage. It is reachable two ways: `_ACTOR_RE` accepts all four as a valid `by`, and any real GitHub user with such a login would have broken Summary, PR Status, Repo Status and My Tasks, not just Plans, since `displayName()` is shared. Now uses `Object.hasOwn` to skip the prototype chain, and `escHtml` coerces with `String()` so no caller can abort a render this way. `kindStyles[kind]` had the same shape — benign, but fixed alongside. Gap: preserving line endings by detecting one style for the whole body still rewrote a mixed-ending body, and converted a lone-CR body to LF. Now each line keeps its own terminator via `splitlines(keepends=True)`, so every byte outside the ticked line is untouched. Verified across LF, CRLF, mixed, lone-CR, and no-trailing-newline bodies. This is also simpler than the version it replaces. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012p8sz3UKvdpbPzzJ8AP1gF
This was referenced Aug 22, 2026
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What
A Plans tab for the ordered, often cross-repo sequences we have to get right — stacked PRs, a migration that must land between two merges, a deploy that follows both. Today those lists live in PR descriptions, issues and stray markdown files, and it's easy to lose track of which one is current.
A Plan is a
plan-labelled GitHub issue in the repo where most of its work happens, holding a flat checklist of ordered Steps. Per-Step metadata rides in a trailing HTML comment, so the issue stays a normal tickable checklist on github.com while the dashboard reads it as structured data:Key design decisions
Recorded in
docs/adr/, vocabulary inCONTEXT.md, authoring spec inPLAN-FORMAT.md.done. Migrations, deploys and verifications leave no trace the dashboard can observe, so auto-ticking merges would make "ticked" mean two different things.gh. In-repo markdown was rejected because a tick would become a git commit — and a container rebuild once auto-deploy lands.Endpoints
GET /api/plansPOST /api/plans/{repo}/{number}/steps/{index}POST /api/plans/{repo}/{number}/closeSecurity
Writes go through
run_gh(..., token=GH_WRITE_TOKEN)— a fine-grained PAT needing only Issues: Read and write. The dashboard has one shared password, so whatever its token can do, anyone with the password can do; keeping the write credential separate from the read path means a leak can't reach code. Write routes return 503 when the token is unset, mirroringapi_summarize_commitswithoutOPENAI_API_KEY.Ticking re-reads the issue and verifies the target line before writing, returning 409 rather than clobbering a concurrent edit.
Test plan
Verified end-to-end against a real plan (
sil-ai/aero-api#408, 14 steps across two repos):], extra spaces before the comment, trailing whitespace) is normalised toward the spec on first tick — documented in PLAN-FORMAT.md, and caught by Copilot as an overstatement in an earlier version of this descriptionGH_WRITE_TOKEN→ 200, correctby:/at:stampexpectline → 409 with no writecanWrite: falseNo automated tests — this repo has no test framework or linter configured.
🤖 Generated with Claude Code
https://claude.ai/code/session_012p8sz3UKvdpbPzzJ8AP1gF