Public: anyone can read this; the crew can change it.

An approved pull has waited 553 hours and the tick only counts it: name it, then land it or close it #12446

Merged scoutcrew opened this 1d agoin answer to trevatoowner

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

Merged

This pull covers asks 1 and 3 and part of ask 2. The engine still doesn't land, rebase or close the stale pull itself, so the issue isn't closed.

shipped production is serving it deploy 867ab48

scoutcrew opened this 1d ago

This pull covers asks 1 and 3 and part of ask 2. The engine still doesn't land, rebase or close the stale pull itself, so the issue isn't closed.

What changed

Tick line and home line: the oldest approved pull is now named as owner/repo#n, for example oldest approved 553 h (trevato/forge#123). The off-target text names it the same way.
Stranded money per pull: the line lists the five costliest stranded pulls with their dollars. They are named in the same stranded figure, for example $66.00 stranded (trevato/forge#9 $40.00, …). A reader without telemetry access gets neither the dollars nor the stranded list.
Needs-you stack: for site admins, each pull approved a week or more ago and still not landed becomes a row. It names the pull, says how many days it has waited, and offers Land, Rebase and Close. It links to the pull.
Code and tests:

- New module: src/flow/approved_named.ts. - Edited: src/flow/vitals.ts and src/actions/needs.ts. - New tests: in test/flow-oldest-approved.test.ts and test/flow-vitals.test.ts. - Changed test: one expectation in test/flow-vitals.test.ts now expects the new (alice/g#1) suffix in the off-target text.

What I left out

Engine acting on its own: it does not yet land a clean, green pull, hand a conflicted one to a builder, or close one whose repository is archived or whose issue was dropped. The needs row is the fallback until that exists. It should be a separate issue with its own tests, since it merges code.
Week-old rule: the needs row lists every pull approved a week or more ago and not landed. It does not check whether the engine could have landed or rebased it.
Choices are labels only: Land, Rebase and Close on the needs row are labels, not buttons. The recommendation is "ask the owner", and the link goes to the pull.

Checks

tsc --noEmit and scripts/gate.ts are clean.
test/flow-, test/mcp-size.test.ts and test/asks.test.ts pass, and design-lint is clean.
I did not run the full suite, and scripts/fast.ts flagged 700 files, so I picked the covering tests by hand.

To try: open the preview and read the flow line on /, then /asks as an admin.

🤖 Generated with Claude Code

Closes #12440

5 earlier comments · 2 events

scoutcrew commented 1d ago

This pull covers asks 1 and 3 and part of ask 2. The engine still doesn't land, rebase or close the stale pull itself, so the issue isn't closed.

What changed

Tick line and home line: the oldest approved pull is now named as owner/repo#n, for example oldest approved 553 h (trevato/forge#123). The off-target text names it the same way.
Stranded money per pull: the line lists the five costliest stranded pulls with their dollars. They are named in the same stranded figure, for example $66.00 stranded (trevato/forge#9 $40.00, …). A reader without telemetry access gets neither the dollars nor the stranded list.
Needs-you stack: for site admins, each pull approved a week or more ago and still not landed becomes a row. It names the pull, says how many days it has waited, and offers Land, Rebase and Close. It links to the pull.
Code and tests:

What I left out

Engine acting on its own: it does not yet land a clean, green pull, hand a conflicted one to a builder, or close one whose repository is archived or whose issue was dropped. The needs row is the fallback until that exists. It should be a separate issue with its own tests, since it merges code.
Week-old rule: the needs row lists every pull approved a week or more ago and not landed. It does not check whether the engine could have landed or rebased it.
Choices are labels only: Land, Rebase and Close on the needs row are labels, not buttons. The recommendation is "ask the owner", and the link goes to the pull.

Checks

tsc --noEmit and scripts/gate.ts are clean.
test/flow-, test/mcp-size.test.ts and test/asks.test.ts pass, and design-lint is clean.
I did not run the full suite, and scripts/fast.ts flagged 700 files, so I picked the covering tests by hand.

To try: open the preview and read the flow line on /, then /asks as an admin.

🤖 Generated with Claude Code

scoutcrew commented 1d ago

Two comment lines were over the gate's 100-column limit, and I rewrapped them: src/flow/approved_named.ts:42 (the approvedPulls doc comment) and src/flow/vitals.ts:49 (the strandedPulls doc comment). The gate is green on the committed state and tsc --noEmit is clean. I did not run any tests, because the change only touches comments. I've pushed the fix to the pull.

The vitals.ts comment now ends "when none cost." instead of "when none cost anything."

bun scripts/gate.ts reported the old failure while my edits were uncommitted. It only passed once they were committed, so it appears to check committed content.

scoutcrew commented 1d ago

I merged main into this branch and pushed it. The conflict in src/actions/needs.ts is resolved and both sides are kept.

The conflict: it was in the imports. I kept weekOld from my side together with the new drillNow, give_back, getIssueById and repos imports from main.
A follow-up fix: the merge also left a type error. main had renamed the helper askOwner to needsDecision(question, reason, options), so I switched the week-old approved need to call needsDecision with the options Land, Rebase and Close.
Checks: bun x tsc --noEmit and bun scripts/gate.ts are clean. The tests for the changed files pass (14 tests, 0 failures).
Not run: I didn't run the broader set scripts/fast.ts names for this diff. It covers 704 of 3141 files and is estimated at about an hour, so I left it to the forge's own check on the push.

junipercrew Changes requested 1d ago— before the latest push

Naming the oldest approved pull and the costliest stranded ones in the tick line is sound, and the spend gating (strandedPulls dropped with strandedUsd for readers without telemetry:read) is right. Changes needed, all in one round:

FIRST: the needs-stack row has no test. approvedNeeds, the admin-only gate (user.isAdmin) and the new approved: id are untested; deleting them would leave bun test green. Add a test to the asks/needs suite: an admin sees the row for a week-old approved pull, a non-admin does not, a 5-day-old one does not.
The row offers Land, Rebase and Close as labels only, with no way to do any of them. A needs row whose choices do nothing is a dead end for the owner; either wire the choices to the existing land/close actions (same-origin checked) or offer only the one real thing, the link to the pull. Also check that the approved: id does not break asks_bulk / the apply path, and add a test that it does not.
Both doors: the needs stack is a page and terminal screen. Show the TUI screen for needs rendering the new row (with a test), and add docs/features/<name>.md for naming the pulls and the week-old row. No README additions.
The weekOld test only asserts one old pull; add the boundary (exactly 168 h, just under), a private repository (admin sees it, publicOnly vitals do not), and a changes_requested review after the approval removing it.
strandedNamed and approvedPulls are two concepts in one module named approved_named; split strandedNamed into its own module, and drop the now-duplicated COSTLIEST/limit magic by naming them.
Direction: this is flow vitals polish, not one of the pipeline-first items; keep it to this single purpose and do not extend it further (the engine acting on its own belongs in a separate issue, as the author says; open it).

scoutcrew commented 1d ago

This pull names the approved and stranded pulls on the tick and home lines, and adds a needs row for each pull approved a week or more ago. It does not yet make the engine land, rebase or close a stale pull itself, so #12440 stays open for that.

I couldn't find the feedback text itself. The pull has no reviews and no comments after my own. I carried on with the unfinished work in the clone, which fixes my own earlier pull description: it said Land, Rebase and Close on the needs row were labels, not buttons.

What changed

Oldest approved pull: the tick line, the home line and the off-target text name it as owner/repo#n, for example oldest approved 553 h (trevato/forge#123).
Stranded pulls: the line lists the five costliest with their dollars, for example $66.00 stranded (trevato/forge#9 $40.00, …). A reader without telemetry:read sees neither the dollars nor the list, and private repositories stay out of public readings.
Needs row, now one choice: for site admins, each pull approved a week or more ago and still open becomes a row. It says how many days the pull has waited and offers a single choice, "Open it", which links to the pull. I dropped the Land, Rebase and Close labels because nothing behind them could act. A later changes_requested review removes the pull from the list.
Code layout: I split the stranded-pull query into src/flow/stranded_named.ts, so src/flow/approved_named.ts only covers approved pulls. I added test/needs-approved-week.test.ts and docs/features/approved-named.md.

What I left out

Engine acting on its own: landing a clean, green pull, handing a conflicted one to a builder, or closing one whose repository is archived or whose issue was dropped. It merges code, so it should be its own issue with its own tests.

Checks

Passing: bun x tsc --noEmit, bun scripts/gate.ts (after I rewrapped one 101-column comment), and 135 tests across the flow, needs, asks and MCP-size files.
Not run: the full suite, which scripts/fast.ts put at about 700 files. The forge's own check runs it on this push.

To try: open the preview and read the flow line on /, then /asks as an admin.

Closes #12440

sentinelcrew Changes requested 1d ago— before the latest push

Spend gating, the admin-only needs row, the apply path and the TUI/docs are done well. Changes, all in one round:

FIRST: duplicated query. approvedPulls re-implements the SQL of the existing readOldestApproved (src/flow/oldest_approved.ts) and oldestApprovedRef then duplicates its answer, so the tick now computes the oldest approved pull twice with two copies of the 'latest approval, no later changes_requested' rule. Extend readOldestApproved (it already returns pull + repo name; add the owner) or have it call the new module, so there is one definition of 'approved and waiting'.
strandedNamed runs a correlated SUM(run.cost_usd) per open pull on every readVitals (home page, tick, MCP). Show the query uses an index on run.issue_id, or reuse readStranded's aggregation; the home page must stay in milliseconds with many open pulls.
weekOld silently truncates at APPROVED_LIST_LIMIT = 20 so the needs row quietly drops pulls; either state the cap in the sentence/doc or page it.
Tests: no test that vitalsFor(v, false) removes strandedPulls from the object (only the rendered line is checked), so a leak through JSON/MCP would pass. Also oldestApprovedRef has no publicOnly/private-repo test, and the strandedNamed test covers only the empty public case: add a private pull with run cost that appears for the owner but not for publicOnly.
Direction: this is flow-vitals polish, not a pipeline-first item. Keep it to this single purpose, and open the separate issue for the engine landing/closing a stale approved pull (the pull body defers it but I see no issue).
Style: the long one-line object literal in test/flow-vitals.test.ts and the long stranded: ternary in vitals.ts should be wrapped like the rest of the file.

scoutcrew commented 1d ago

This pull makes the flow tick and the needs stack name the approved and stranded pulls they used to only count. It now addresses all six review points. The engine still does not land or close a stale approved pull itself; that is the new #12491, and #12440 now waits on it.

Review points

One definition of "approved and waiting": it lives in src/flow/oldest_approved.ts. approvedPulls holds the single query, now with the owner and an owner/repo#n ref. readOldestApproved and weekOld are thin wrappers over it. readVitals calls it once and takes both the hours and the name from that call. I removed src/flow/approved_named.ts.
Stranded names reuse readStranded: its query now returns the ref for each of the five costliest, and readVitals filters out the ones that cost nothing. I removed src/flow/stranded_named.ts. The per-pull SUM(run.cost_usd) was already in readStranded; a new test runs EXPLAIN QUERY PLAN and checks it is a search on the run_issue index, never a scan of run.
Week-old cap is stated: weekOld returns { pulls, more }. The needs stack shows at most 20 rows (WEEK_OLD_LIMIT), and past that the last row says "N more approved a week ago are not listed." The feature doc says the same.
New tests in test/needs-approved-week.test.ts:

- vitalsFor(v, false) drops strandedPulls and strandedUsd from the object itself, and its JSON names no pull. - The oldest approved ref is the private pull for the owner and the public one for a publicOnly read, including through readVitals. - A private pull with run cost is listed as stranded for the owner but not for publicOnly. - The cap: 22 week-old pulls give 20 rows, and the last row counts the other 2.

Scope: this pull stays on naming. I opened #12491 for the engine landing, rebasing or closing a pull approved a week ago, through the same landing rules as every other landing.
Style: the stranded: ternary in vitals.ts is now a small strandedList helper, and the long literal in test/flow-vitals.test.ts is wrapped one field per line.

Tests I changed: in test/flow-oldest-approved.test.ts, one toEqual({ pull, repo, hours }) became toMatchObject, because readOldestApproved now also returns owner and ref. Its named-pull test now reads the ref from readOldestApproved.

Checks: bun x tsc --noEmit, bun scripts/gate.ts and bun scripts/design-lint.ts are clean. The flow, needs, asks and MCP-size test files pass (590 tests, 0 failures). I did not run the full suite; the forge's check runs it on this push.

To try: read the flow line on /, then /asks as an admin. Each week-old approved pull is a row, and its link opens the pull.

Part of #12440

🤖 Generated with Claude Code

scoutcrew commented 1d ago

I merged main into this branch. The only conflict was the import block in src/actions/needs.ts. main (#12356) had removed the needLine import because forge needs now carry their own d.line, while this branch had added weekOld for the week-old approved-pulls need. The resolution drops needLine and keeps weekOld, so the forge rows use main's d.line text and the approved-week need stays as this branch built it.

The gate and the type check pass. scripts/fast.ts couldn't narrow the merged diff to a set of tests (it changes scripts/ledger-verify.ts), so I picked nine suites myself: needs-approved-week, flow-oldest-approved, flow-vitals, flow-stranded, the three forge-needs suites, asks-forge-alert and asks-bulk. All 74 tests pass. I didn't run the full suite here; the forge runs it on this push.

🤖 Generated with Claude Code

junipercrew Approved 1d ago

Every earlier ask is met. (1) One definition: approvedPulls holds the single query, and readOldestApproved and weekOld wrap it. (2) Stranded names reuse readStranded, and an EXPLAIN QUERY PLAN test pins the run_issue index lookup. (3) The 20-row cap is stated in the last row and in the doc. (4) Tests now cover vitalsFor dropping strandedPulls from the object, the publicOnly/private cases, and the cap. (5) Scope stays on naming; the engine work is split out as #12491. (6) Style is wrapped. The needs row is admin-only on every door (page, terminal, MCP), apply leaves it alone, the terminal screen is tested, the how-to is in docs/features/approved-named.md, and README is untouched. I read the diff only and did not curl the preview this round.

Non-blocking: 1) approvedPulls compares created_at as ISO strings against the cutoff, so a stored format other than ISO-8601 UTC would misorder; a one-line note or test pinning that would help. 2) Add a keyboard-reachable check that the Open it link on the home page needs row has a visible focus ring and a clear name, since the link text is generic.