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

Put back on a setting ask changes nothing unless its finding closes #2073

Merged scoutcrew opened this 28d ago

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

Merged

Put back on a setting ask now leaves the setting alone when none of its findings can be closed, and it says why. `bun x tsc --noEmit` is clean. I ran `test/asks.test.ts` and `test/sentinel.test.ts` (38 pass), including the existing Keep and Put back tests. I did not run the whole suite.

shipped production is serving it deploy 119f4d2

scoutcrew opened this 28d ago

Put back on a setting ask now leaves the setting alone when none of its findings can be closed, and it says why. bun x tsc --noEmit is clean. I ran test/asks.test.ts and test/sentinel.test.ts (38 pass), including the existing Keep and Put back tests. I did not run the whole suite.

The gate: answerSetting in src/actions/setting_ask.ts now works out which findings it can close before it calls changeCapacity. A mirrored repo's issues can't be closed from here, so those are left out. If none are left, it returns a new unclosable error. The setting stays as it was, and Keep is refused the same way.
The message: src/web/studio/asks_page.ts maps unclosable to "its finding lives on a mirror and cannot be closed here, so the setting was left as it is". The false "only a forge admin puts a setting back" no longer shows for this case.
The dead guard: f.kept === undefined is gone from the urgent hand-over in src/crew/sentinel.ts.
The test: a new case in test/asks.test.ts mirrors the finding's repo, then tries both Put back and Keep. Both return unclosable, the stored value stays "report", and nothing is recorded as kept.

The pre-check covers only the mirror refusal. openFindings already drops repos the actor can't write to, and setIssueState has no other refusal that applies to an admin closing a setting finding. If a close is refused for some other reason after the setting has been put back, it still returns forbidden as before.

To try it: connect a mirror origin to the repo that holds a setting finding, then press Put back on that ask. The page shows the new message and the setting is unchanged.

🤖 Generated with Claude Code

Closes #2072

junipercrew Changes requested 28d ago— before the latest push

The gate on Put back is right, but this pull also undoes part of #2043 without saying so, and the terminal shows a bare error code.

Fix first: src/crew/sentinel.ts drops && f.kept === undefined. #2043 added that guard so a setting finding is the owner's ask and never a crew run. Removing it makes setting findings get the P0 Trust label again. The issue title does not ask for this, and no test fails without it. Restore the guard. If the change is really needed, say why in the pull and add a test for it.

The terminal door words the new refusal as refused: unclosable (src/tui/screens/dashboard.ts:394). The page says why in plain words. Map unclosable to the same sentence in the TUI so both doors match.

The README does not say that Keep and Put back now refuse when the finding lives on a mirror. Add a line.

The test only calls the action. It should also cover the page message and the TUI toast. I did not check the preview behaviour.

scoutcrew commented 28d ago

I've addressed all three points and pushed. The type check is clean, and test/asks.test.ts, test/sentinel.test.ts and test/tui-onyou.test.ts pass. I did not run the whole suite or check the preview.

Urgent guard: I restored && f.kept === undefined in src/crew/sentinel.ts, so setting findings are not handed over as urgent again. Removing it was one of the issue's checklist items, but I agree it isn't needed for this fix and no test covered it. This pull no longer touches that line.
Terminal wording: src/actions/setting_ask.ts now exports UNCLOSABLE_WORDS: "its finding is on a mirror, so the setting was left as it is". Both the page (src/web/studio/asks_page.ts) and the terminal toast (src/tui/screens/dashboard.ts) use it, so they match.
README: it now says Keep and Put back refuse and change nothing when every finding for the key is on a mirrored repository.
Tests:

- test/asks.test.ts now also posts Put back to /asks/setting for a mirrored finding. It checks that the page shows UNCLOSABLE_WORDS, does not say "only a forge admin", and leaves the setting unchanged. - test/tui-onyou.test.ts has a new case that presses B on a mirrored setting card and checks the toast text and the unchanged setting. The toast check matches only the first 30 characters of the sentence, to allow for truncation.

junipercrew Changes requested 28d ago— before the latest push

The mirror case is handled, but the pull's own rule, "Put back changes nothing unless its finding closes", is only half enforced.

Fix first, in src/actions/setting_ask.ts: changeCapacity(app, actor, key, null) still runs before the loop that closes findings. The closed === 0 -> err("forbidden") path afterwards therefore leaves the setting already put back with the finding still open. That happens whenever setIssueState refuses for a reason other than a mirror. The new closable pre-filter only anticipates one such reason. Close the findings first and change the capacity only if at least one close went through. If the order cannot change, undo the capacity change when closed === 0. Add a test where the close is refused for a non-mirror reason and the stored value is unchanged.

Smaller points:

README: the new sentence is a very long line that breaks the wrapping of the paragraph. Re-wrap it.
The TUI test matches only UNCLOSABLE_WORDS.slice(0, 30). That is weak, so assert the full wording if the toast width allows it.
Import order in setting_ask.ts: ../origins/write-through.ts now sits before ../kernel/result.ts.

Not judged: the preview at lab--2073 was unreachable from my sandbox, so I did not check the page there. I also did not view the screenshots.

scoutcrew commented 28d ago

answerSetting in src/actions/setting_ask.ts now closes the findings first and changes the setting only if at least one close went through. The type check is clean. test/asks.test.ts, test/tui-onyou.test.ts and test/sentinel.test.ts pass (48 tests); I did not run the whole suite or the preview.

Order of operations:

- Put back: changeCapacity runs after the closes, so a refused close leaves the setting alone. - Keep: it records the kept value only after a close, as before. - Refusal: when no close goes through, the answer is unclosable instead of forbidden. That covers a mirror and any other refusal from setIssueState. - Unchanged check: the mirror pre-filter stays. A mirror gets the plain message without trying the close.

Wording: one sentence now covers every case: "its finding cannot be closed from here, so the setting was left as it is". The page and the terminal share it, and the README now says the same, wrapped.
Tests:

- New test: the finding is made a pull, so setIssueState refuses for a non-mirror reason. The result is unclosable, the stored value is unchanged, and the capacity history gains no entry. - TUI test: it now asserts the full sentence, which fit in the toast.

Import order: ../origins/write-through.ts now comes after ../kernel/result.ts.

I chose close-first over undoing the capacity change afterwards, because an undo would leave a change record and event in the history. After the closes, changeCapacity cannot fail for an admin with a valid key, so no finding is left closed while the setting stays away.

junipercrew Approved 28d ago— before the latest push

Put back and Keep now change nothing unless a finding closes. The close runs first, and the capacity change or the kept record follows only after it. The page and the terminal refuse in the same words (UNCLOSABLE_WORDS). The README says so. Both doors are covered. Tests cover a mirrored finding on the page and in the terminal, plus a non-mirror refusal (a pull), and each asserts the setting and history are unchanged. This is a server-side wording and ordering fix, so there is no layout or phone impact. I did not check the screenshots or the preview. The admin gate is unchanged and nothing new is added beyond the issue.

Two things I would do next:

Drop the mirror pre-filter in answerSetting. The closed count already covers it, and the second test shows setIssueState refuses for other reasons anyway. Keep it only if you want to avoid trying the close on a mirror.
Word the refusal for a person: say what to do next, e.g. close the finding on the repository, rather than only that it cannot be closed from here.

trevatoowner commented 28d ago

The full gate on land was red and named no test; the pull is open again.

scoutcrew commented 28d ago

I merged main into the branch, resolved the two conflicts, and pushed (a17b54ab).

src/tui/screens/dashboard.ts: I kept main's proposal and put-aside imports and this branch's UNCLOSABLE_WORDS import.
test/tui-onyou.test.ts: I kept both tests: this branch's "setting whose finding is on a mirror is refused in the same words as the page" and main's "B with no wave put aside and no setting under the cursor does nothing".

bun x tsc --noEmit ran clean and test/tui-onyou.test.ts passes. The wider test/tui run had two failures. Both are in test/tui-code.test.ts: "repo rows wear issue and pull counts, and the list does not grow queries", and "an event on the bus refreshes the dashboard with no key pressed" (a 5 s timeout). Both fail the same way on a clean checkout of origin/main here, so the merge didn't cause them. I didn't investigate why they fail in this sandbox, and the forge's check will run them on this push.

junipercrew Approved 28d ago

Put back now changes the setting only after a finding has closed. Keep records its value only after a close too. When nothing can close (a mirror, or setIssueState refuses), the answer is unclosable, nothing changes, and the page and the terminal show the same shared sentence. The change is small and stays inside the existing modules. Both new tests would fail on the old order: the mirror case runs through the page and answerSetting, and the pull-kind case covers a refusal that is not a mirror. The TUI test asserts the full sentence and an unchanged setting. The README matches. I read the diff and did not exercise the preview or view the screenshots.

Two things I would do next:

Drop the mirror pre-filter, or comment why it stays. closed === 0 already covers it, so the filter now only saves a failed close attempt.
If changeCapacity ever fails after a close, a finding is left closed while the setting stays away. Make that path return a distinct error, or do the validation before the closes.