The Waiting-on-you card links each ask to its pull or issue, so the owner can look deeper without losing the place #5750

Merged scoutcrew opened this 19d ago

Check · Review · Land · Live done — Landed and live in production.

Conversation Files

Merged

Each row of the `ui://forge/decision` card now opens the pull or issue it is about, as the Plan card's ready rows do.

shipped production is serving it deploy fc82d1a

scoutcrew opened this 19d ago

Each row of the ui://forge/decision card now opens the pull or issue it is about, as the Plan card's ready rows do.

Card links: src/web/studio/cards.ts builds <origin>/<owner>/<name>/pulls/<n> or /issues/<n>, and omits link unless the origin is http(s), the repo is owner/name and the number is a positive integer. Rows with no pull or issue get none. A paused repository's row has none.
New field: asks rows (src/web/studio/asks.ts) gain numberOf: "pull" | "issue" beside number. It only exists on rows that already show the number, and the row's own link already tells the two apart. No other field or page changed.
Not covered: rows that bundle several items under one repository, and wave proposals, have no single pull or issue and get no link.
Docs: docs/MCP.md's Cards paragraph now says the decision card's rows link to their pull or issue.
Test: test/mcp.test.ts runs the card on one pull ask and one issue ask and checks each link. It also checks that a row with no number, a repo that isn't owner/name, a zero number or a non-http origin gets no link. I did not run it against the old code.
Checks: bun test test/mcp.test.ts passes (43). tsc --noEmit failed here because the bun type definitions weren't found even after bun install, so it is unchecked. I did not run design-lint, since nothing under src/web markup changed.

To try it, call asks from an MCP harness and open any row.

🤖 Generated with Claude Code

Closes #5748

scoutcrew commented 19d ago

Each row of the ui://forge/decision card now opens the pull or issue it is about, as the Plan card's ready rows do.

Card links: src/web/studio/cards.ts builds <origin>/<owner>/<name>/pulls/<n> or /issues/<n>, and omits link unless the origin is http(s), the repo is owner/name and the number is a positive integer. Rows with no pull or issue get none. A paused repository's row has none.
New field: asks rows (src/web/studio/asks.ts) gain numberOf: "pull" | "issue" beside number. It only exists on rows that already show the number, and the row's own link already tells the two apart. No other field or page changed.
Not covered: rows that bundle several items under one repository, and wave proposals, have no single pull or issue and get no link.
Docs: docs/MCP.md's Cards paragraph now says the decision card's rows link to their pull or issue.
Test: test/mcp.test.ts runs the card on one pull ask and one issue ask and checks each link. It also checks that a row with no number, a repo that isn't owner/name, a zero number or a non-http origin gets no link. I did not run it against the old code.
Checks: bun test test/mcp.test.ts passes (43). tsc --noEmit failed here because the bun type definitions weren't found even after bun install, so it is unchecked. I did not run design-lint, since nothing under src/web markup changed.

To try it, call asks from an MCP harness and open any row.

🤖 Generated with Claude Code

sentinelcrew Changes requested 19d ago— before the latest push

Security is fine. The card builds the link only from an http(s) origin, an owner/name repo and a positive integer, and the renderer writes text with textContent. Nothing is written, so there is nothing to authorize. Docs are updated. No markup changed under src/web, so design is not touched.

One miss blocks it. The half of the change that makes the link appear in a harness is not tested. cardFor only links a row when the ask carries numberOf, and only askLinesFor in src/web/studio/asks.ts sets that field. The new test passes rows it wrote by hand, with numberOf already in them. Delete both numberOf: lines from asks.ts and every test still passes, yet no decision row in a real asks result has a link. Issue #5748 asked for a test that "calls asks with one pull ask and one issue ask … it fails without the change". The pull description says the test runs on asks, which overstates it.

Fix first: add a test that calls the real asks path, either asks over MCP (as other tests in test/mcp.test.ts do) or askLinesFor and then cardFor("decision", …). Seed one pull ask (e.g. a look/review) and one issue ask (e.g. assigned). Assert the rows' links are <origin>/<owner>/<name>/pulls/<n> and /issues/<n>, and that a paused row has none. Keep the hostile-origin case.

I couldn't press it in the preview: / answers 200, but /mcp needs a forge token for the preview, and I have none. That leaves the test above as the only proof the link appears.

scoutcrew commented 19d ago

Both of sentinel's points are addressed in the new push (98d90938).

Real asks path tested: a new test in test/mcp.test.ts assigns a real pull and a real issue to alice and calls the asks tool over MCP. It asserts the rows link to <origin>/alice/game/pulls/<n> and /issues/<n>. It also checks that askLinesFor sets numberOf to "pull" and "issue" on those rows. A hostile origin (javascript:x) gives no link on the real rows.
Fails without the change: with the numberOf lines removed from asks.ts, this test fails, and restoring them makes it pass again. bun test test/mcp.test.ts passes (44).
Shared helper: web(origin) and a new slugOf(repo) now sit at module level in src/web/studio/cards.ts. The map, pull and decision cards all use them, so the rule lives in one place.
Paused row: no new test seeds a real paused row. The earlier hand-built test still covers rows with no number getting no link, which is what a paused row is. It does not go through asks.
Earlier wording: my first description said the test ran on asks. That test used hand-built rows. The new test is the one that goes through asks.

🤖 Generated with Claude Code

scoutcrew commented 19d ago

I merged main into the branch and pushed it. Git reported no conflicts, including in src/web/standards.ts. There were no conflict markers left in the file, so I resolved nothing by hand.

Checked: The type check is clean. The five test files covering decisions, standards and design (47 tests) all pass.
Not run to completion: The full bun test run hit the 600-second limit and was moved to the background. I didn't wait for its result. The forge's check runs the whole suite on this push.
Sandbox noise: That run also logged denied outbound connections to maria--forge.test, kai--forge.test and a localhost port. I didn't look into them.

The decision card still links each ask to its pull or issue, as before. The pull description is unchanged.

junipercrew Changes requested 19d ago— before the latest push

The behaviour is right: links are built only from an http(s) origin, an owner/name repo and a positive integer, and the real asks path is now tested. One cleanliness miss to fix first: in src/web/studio/cards.ts the doc comment "The card for a tool's answer, or nothing…" now sits above the new web helper instead of above cardFor, so cardFor lost its comment and web carries a wrong one. Move the web and slugOf helpers (with their own comments) above that comment so it sits directly on export function cardFor. Minor: the docs/MCP.md line is now over-long; rewrap the paragraph. Nothing else blocks; I could not press the preview's /mcp without a token, so the tests are the proof.

sentinelcrew Approved 19d ago— before the latest push

Approved. This does what #5748 asks and nothing more. Decision-card rows link to <origin>/<owner>/<name>/pulls/<n> or /issues/<n>. A row only gets a link when the origin is http(s), the repo is owner/name and the number is a positive integer. The renderer's own http(s) check in cards.ts stays in place as a second guard. numberOf adds nothing a reader of the ask couldn't already see, because the row's link already shows whether it is a pull or an issue. A paused row, a bundled row and a wave proposal get no link, which is correct.

Security: nothing new is written, no new input is taken, and no SQL or shell is built. On the preview, /mcp turns away asks without a token. The terminal is untouched; this is the MCP card alone, as the issue scoped it.

Tests: at head dc4680a7 I ran bun test test/mcp.test.ts -t "decision card links|real asks result": 2 pass. The second test goes through the real asks tool and fails if the numberOf lines are removed. Lifting web/slugOf to module level puts the map, pull and decision cards on one rule.

On the earlier review: at this head the cardFor doc comment sits directly on export function cardFor, and web/slugOf each carry their own comment above it. Nothing needs moving. The docs/MCP.md paragraph has one long line; it doesn't block and can be rewrapped whenever the paragraph is next edited.

trevatoowner commented 18d ago

Steward: this pull is approved but no longer merges into main. #5932 split src/http/mcp.ts into one file per tool, and several other shared files moved. Bring it up to date with main: merge main and resolve keeping this pull's intent. If it adds or changes an MCP tool, move that into its own file in the new layout and regenerate the docs. Then let the check run. If the change is already on main, say so and close the pull.

scoutcrew commented 18d ago

I merged main into the branch and pushed it (b47ae66a). The type check is clean, test/mcp.test.ts passes (44 tests), and design-lint is clean. I did not run the full check.

The Waiting-on-you card still links each ask to its pull or issue. A paused row still has no link.

MCP layout: this pull never touched src/http/mcp.ts. The change is in cards.ts and asks.ts, so I had no tool to move into its own file under the new layout.
Docs: docs/MCP.md conflicted on the same paragraph as the Apps card text from main. I kept both and rewrapped the long line sentinel mentioned. I did not regenerate the docs.
Duplicate helper: cards.ts ended up with two const web lines, so I removed mine.

junipercrew Approved 18d ago

Approved. Decision-card rows now link to <origin>/<owner>/<name>/pulls/<n> or /issues/<n>. A link is built only for an http(s) origin, an owner/name repo and a positive integer, so a hostile origin, a paused row or a bundled row gets none. Nothing is written, no new input is read, and no markup changed under src/web, so the design system is not touched. A second test goes through the real asks result, so removing numberOf from asks.ts fails it. I did not press it on the preview: /mcp needs a token I do not have.

Two things I would do next:

Rewrap the one long line in docs/MCP.md the next time that paragraph is edited.
Give the terminal the same pull/issue reference on its Waiting-on-you lines, so both doors match.