Skip to content

Commit f32f280

Browse files
authored
Put each open question on the first screen, label the finished review final, show the framing questions (#97)
The summary now lists each open decision as a linked line carrying its question, plus one sentence on what happens next, so the ask is on the first screen instead of below the fold (DD-5804). A finished review reads "Final recommendation" with "The review is complete. Nothing is waiting on you." (DD-5808), and the framing row shows each question beside the reviewer's answer (DD-5807). The final label is the part to check: it needs no qualifications, nothing owed, and, on a live page, the walk's `done` phase, so a page that has read every item but not finished still says "Recommendation". A static-only review can still be final, since final means complete, and the static caveat stays beside it. Phase 4 in SKILL.md now asks the agent to say whether each included finding blocks the merge. The flag question moved into `decision_text` so the beat card and the summary read from one place, and beats gained `id="item-N"` for the links. <!-- codesmith:footer --> --- <a href="https://app.blacksmith.sh/radkode/codesmith/underwrite/pr/97?autoLogin=true&ref=codesmith_pr_footer"><picture><source media="(prefers-color-scheme: dark)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-dark-v2.svg"><source media="(prefers-color-scheme: light)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-light-v2.svg"><img alt="View with [code]smith" src="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-dark-v2.svg"></picture></a> <a href="https://backend.blacksmith.sh/track/enable-autofix?expires=1793479410&installation_model_id=433329&pr_number=97&ref=codesmith_pr_footer&repository=radkode%2Funderwrite&return_to=https%3A%2F%2Fgithub.com%2Fradkode%2Funderwrite%2Fpull%2F97&signature=c0d42da250dcf21073abd0cba769df74aaafa23e4ee217c45baa34a90388a9f9"><picture><source media="(prefers-color-scheme: dark)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-light.svg"><img alt="Autofix with [code]smith" src="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-dark.svg"></picture></a> <sup>Need help on this PR? Tag <code>@codesmith-bot</code> with what you need. Autofix is disabled.</sup> <!-- codesmith:autofix:disabled --> <!-- /codesmith:footer -->
1 parent eac84e9 commit f32f280

5 files changed

Lines changed: 153 additions & 44 deletions

File tree

‎skills/underwrite/SKILL.md‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -722,6 +722,9 @@ handoff with the same assessment and count of decisions owed as the page. Say wh
722722
reviewed or verified; clean counts and included findings alone do not establish merge
723723
readiness. The renderer qualifies incomplete coverage, unverified items, and static
724724
inspection instead of turning a delivery-complete page into a verified review.
725+
For every included finding, say in the recommendation whether it blocks the merge and who
726+
acts on it; a finished page that says "safe to merge" above a finding leaves the reviewer
727+
guessing which one wins.
725728

726729
Render the page first, so the reviewer makes any remaining calls off the hoisted flags
727730
rather than off scrollback. Branch delivery and report acceptance are already complete,

‎skills/underwrite/assets/report.css‎

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -192,15 +192,18 @@ h1 .num { color: var(--ink-3); font-variant-numeric: tabular-nums; }
192192
.assessment p { margin: 0 0 8px; }
193193
.assessment .recommendation { font-size: 19px; line-height: 1.5; }
194194
.assessment .decisions-owed { font-weight: 600; }
195+
.assessment .asks { margin: 0 0 8px; padding-left: 18px; }
196+
.assessment .asks li { margin: 0 0 4px; }
197+
.assessment .asks a { color: var(--flag); }
198+
.assessment .asks a:focus-visible { outline: 2px solid var(--flag); outline-offset: 1px; }
199+
.assessment .next-step { color: var(--ink-2); }
195200
.assessment .qualification { color: var(--ink-2); font-size: 14px; }
196201
.assessment h3 { margin: 20px 0 6px; font-size: 15px; line-height: 1.3; }
197202
.assessment .change { color: var(--ink-2); }
198203
.decision { max-width: 70ch; margin-bottom: 20px; }
199204
.decision p { margin: 0 0 6px; }
200205

201-
/* ---- frame ----
202-
The reviewer's two answers stay on the summary line, so the folded live frame still
203-
says what the walk was agreed to be. */
206+
/* ---- frame ---- */
204207
.frame summary {
205208
cursor: pointer;
206209
width: fit-content;
@@ -219,6 +222,13 @@ h1 .num { color: var(--ink-3); font-variant-numeric: tabular-nums; }
219222
color: var(--ink-3);
220223
}
221224
.frame .frame-k { margin: 0 8px 0 4px; }
225+
.frame dt.frame-q {
226+
font-family: var(--sans);
227+
font-size: 14px;
228+
letter-spacing: 0;
229+
text-transform: none;
230+
color: var(--ink-2);
231+
}
222232
.frame .sep { color: var(--rule); margin: 0 10px; }
223233
.frame q { font-style: normal; }
224234
.frame dl { margin: 12px 0 0; max-width: 68ch; }

‎skills/underwrite/scripts/render-report.py‎

Lines changed: 83 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,11 @@
6161

6262
LANDS_TAG = {"landed": "Landed", "ready": "Ready", "open": "Your call"}
6363
FRAME_PROSE = (("claim_check", "Claim check"),)
64-
FRAME_CALLS = (("orient_call", "Your answer on the summary"), ("plan_call", "Your answer on the order"))
64+
# (session key, the question the reviewer answered before the walk)
65+
FRAME_CALLS = (
66+
("orient_call", "Is the summary of this change right?"),
67+
("plan_call", "In what order should the items be read?"),
68+
)
6569

6670
# Only for --standalone. The viewport tag is load-bearing: report.css has a
6771
# 620px breakpoint that never fires without it.
@@ -387,7 +391,7 @@
387391
// ledger both say so; this says it where the click happened.
388392
function owedNote() {
389393
const staged = document.querySelector('#stage .beat.s-flag');
390-
return staged ? `beat ${staged.dataset.n} stays owed` : '';
394+
return staged ? `item ${staged.dataset.n} stays owed` : '';
391395
}
392396
393397
function resumePending() {
@@ -598,6 +602,30 @@ def diff_html(lines):
598602
)
599603

600604

605+
def decision_text(beat, mode, replacement=False, untrusted_pr=False):
606+
"""A flag's (question, recommendation). A session older than the field gets the
607+
choice its buttons offer, and no guessed recommendation."""
608+
decision = beat.get("decision")
609+
if isinstance(decision, dict):
610+
return (
611+
decision.get("question", "Decision not recorded."),
612+
decision.get("recommendation", "Recommended option not recorded."),
613+
)
614+
if beat.get("resolution_kind") != "decision" and (
615+
replacement or (mode == "branch" and untrusted_pr)
616+
):
617+
question = "Replace this blocked session before choosing delivery, or drop this finding?"
618+
elif beat.get("resolution_kind") == "decision":
619+
question = "Choose the policy option in FIX or drop this question?"
620+
else:
621+
question = {
622+
"branch": "Implement the proposed fix or drop this finding?",
623+
"review": "Include in review or drop this finding?",
624+
"report": "Include in report or drop this finding?",
625+
}[mode]
626+
return question, "Recommended option not recorded."
627+
628+
601629
def outcome_html(beat, mode):
602630
"""One plain sentence on an accepted beat: what the reviewer chose and what it did.
603631
Accepting posts or records a finding; only branch mode changes the code."""
@@ -804,26 +832,7 @@ def beat_html(
804832

805833
body = []
806834
if beat.get("state") == "flag":
807-
decision = beat.get("decision")
808-
if isinstance(decision, dict):
809-
question = decision.get("question", "Decision not recorded.")
810-
recommendation = decision.get("recommendation", "Recommended option not recorded.")
811-
else:
812-
if beat.get("resolution_kind") != "decision" and (
813-
replacement or (mode == "branch" and untrusted_pr)
814-
):
815-
question = "Replace this blocked session before choosing delivery, or drop this finding?"
816-
else:
817-
question = (
818-
"Choose the policy option in FIX or drop this question?"
819-
if beat.get("resolution_kind") == "decision"
820-
else {
821-
"branch": "Implement the proposed fix or drop this finding?",
822-
"review": "Include in review or drop this finding?",
823-
"report": "Include in report or drop this finding?",
824-
}[mode]
825-
)
826-
recommendation = "Recommended option not recorded."
835+
question, recommendation = decision_text(beat, mode, replacement, untrusted_pr)
827836
body.append(
828837
'<div class="decision">'
829838
f'<p><strong>Decision:</strong> {md(question)}</p>'
@@ -950,7 +959,8 @@ def beat_html(
950959
)
951960

952961
return (
953-
f'<details class="beat s-{suffix}" data-n="{attr(n)}"{" open" if expanded else ""}>'
962+
f'<details class="beat s-{suffix}" id="item-{attr(n)}" data-n="{attr(n)}"'
963+
f'{" open" if expanded else ""}>'
954964
f"<summary>"
955965
f'<span class="b-num">{md(n)}</span>'
956966
f'<span class="b-tier">{md(beat.get("tier", ""))}</span>'
@@ -1097,7 +1107,7 @@ def stage_html(session, beats, current, card):
10971107
)
10981108

10991109

1100-
def assessment(session, beats, problems_by_n, validation_problems=()):
1110+
def assessment(session, beats, problems_by_n, validation_problems=(), walking=False):
11011111
text = session.get("recommendation")
11021112
if not isinstance(text, str) or not text.strip():
11031113
text = "Recommendation not recorded. Do not infer merge readiness from the counts."
@@ -1114,7 +1124,11 @@ def assessment(session, beats, problems_by_n, validation_problems=()):
11141124
limits.append(f"{unverified} item{'s' if unverified != 1 else ''} unverified.")
11151125
if problems_by_n or validation_problems:
11161126
limits.append("The report has validation problems; resolve them before relying on it.")
1117-
label = "Provisional recommendation" if limits else "Recommendation"
1127+
owed = sum(beat.get("state") == "flag" for beat in beats)
1128+
if limits:
1129+
label = "Provisional recommendation"
1130+
else:
1131+
label = "Recommendation" if owed or walking else "Final recommendation"
11181132
if execution_mode(session) == "no_exec":
11191133
limits.append("Static inspection only; runtime behavior was not verified.")
11201134
mode = session_mode(session)
@@ -1124,17 +1138,46 @@ def assessment(session, beats, problems_by_n, validation_problems=()):
11241138
f"{included} finding{'s' if included != 1 else ''} included in the {mode}. "
11251139
"Including a finding does not fix the issue it describes."
11261140
)
1127-
owed = sum(beat.get("state") == "flag" for beat in beats)
11281141
return label, text, limits, owed
11291142

11301143

1131-
def assessment_html(session, beats, problems_by_n, validation_problems=()):
1132-
label, text, limits, owed = assessment(session, beats, problems_by_n, validation_problems)
1144+
NEXT_STEP = {
1145+
"review": "Nothing reaches GitHub until you post the whole review at the end.",
1146+
"report": "Including a finding records it in this report and posts nothing.",
1147+
"branch": "Implement applies the stated fix and commits it.",
1148+
}
1149+
1150+
1151+
def assessment_html(
1152+
session, beats, problems_by_n, validation_problems=(),
1153+
replacement=False, untrusted_pr=False, walking=False,
1154+
):
1155+
label, text, limits, owed = assessment(
1156+
session, beats, problems_by_n, validation_problems, walking,
1157+
)
11331158
change = session.get("reconstruction")
1159+
mode = session_mode(session)
1160+
asks = "".join(
1161+
f'<li><a href="#item-{attr(beat.get("n", "?"))}">Item {md(beat.get("n", "?"))}</a>: '
1162+
f'{md(decision_text(beat, mode, replacement, untrusted_pr)[0])}</li>'
1163+
for beat in beats if beat.get("state") == "flag"
1164+
)
1165+
if asks:
1166+
blocked = replacement or (mode == "branch" and untrusted_pr)
1167+
step = (
1168+
"Implementing is blocked for this session; a finding can only be dropped or noted."
1169+
if blocked else NEXT_STEP[mode]
1170+
)
1171+
waiting = f'<ul class="asks">{asks}</ul><p class="next-step">{step}</p>'
1172+
elif label == "Final recommendation":
1173+
waiting = '<p class="next-step">The review is complete. Nothing is waiting on you.</p>'
1174+
else:
1175+
waiting = ""
11341176
return (
11351177
'<section class="assessment" aria-label="Review recommendation">'
11361178
f'<h2>{label}</h2><p class="recommendation">{md(text)}</p>'
11371179
f'<p class="decisions-owed">{owed} decision{"s" if owed != 1 else ""} owed</p>'
1180+
+ waiting
11381181
+ "".join(f'<p class="qualification">{md(limit)}</p>' for limit in limits)
11391182
+ (f'<h3>What this change is</h3><p class="change">{md(change)}</p>' if change else "")
11401183
+ '</section>'
@@ -1162,7 +1205,10 @@ def body_html(
11621205
key=lambda b: b["n"],
11631206
default=None,
11641207
)
1165-
parts = [assessment_html(session, beats, problems_by_n, validation_problems)]
1208+
parts = [assessment_html(
1209+
session, beats, problems_by_n, validation_problems, replacement, untrusted_pr,
1210+
walking=live and phase != "done",
1211+
)]
11661212
if live:
11671213
parts.append(bar_html(session, beats, current, phase))
11681214
if phase != "done":
@@ -1257,10 +1303,11 @@ def frame_html(session, live):
12571303
calls = [(label, session.get(key)) for key, label in FRAME_CALLS if session.get(key)]
12581304
if not prose and not calls:
12591305
return ""
1260-
summary = '<span class="sep">·</span>'.join(
1261-
f'<span class="frame-k">{label}</span><q>{md(text)}</q>' for label, text in calls
1262-
) or '<span class="frame-k">How this review was framed</span>'
1263-
body = "".join(f"<dt>{label}</dt><dd>{md(text)}</dd>" for label, text in prose)
1306+
summary = '<span class="frame-k">How this review was framed</span>'
1307+
body = "".join(
1308+
f'<dt class="frame-q">{label}</dt><dd>You answered <q>{md(text)}</q></dd>'
1309+
for label, text in calls
1310+
) + "".join(f"<dt>{label}</dt><dd>{md(text)}</dd>" for label, text in prose)
12641311
if body:
12651312
body = f"<dl>{body}</dl>"
12661313
return (
@@ -1516,7 +1563,9 @@ def main():
15161563
)
15171564
out.write_text(SHELL + page if args.standalone else page, encoding="utf-8")
15181565

1519-
label, recommendation, limits, owed = assessment(session, beats, problems_by_n, all_problems)
1566+
label, recommendation, limits, owed = assessment(
1567+
session, beats, problems_by_n, all_problems, walking=args.live,
1568+
)
15201569
print(f"{label}: {recommendation}", file=sys.stderr)
15211570
print(f"{owed} decision{'s' if owed != 1 else ''} owed", file=sys.stderr)
15221571
for limit in limits:

‎tests/test_scripts.py‎

Lines changed: 53 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -559,11 +559,11 @@ def test_the_status_line_lives_in_the_bar_once(self):
559559

560560
def test_a_resolved_beat_folds_to_its_line_with_the_call_while_walking(self):
561561
html = self.live(self.beats())
562-
self.assertRegex(html, r'<details class="beat s-acc" data-n="2">')
562+
self.assertRegex(html, r'<details class="beat s-acc" id="item-2" data-n="2">')
563563
self.assertIn('<span class="b-call">“leave it”</span>', html)
564564
# and stays open once the walk is done, as the final report has it
565565
self.assertRegex(self.live(self.beats(), phase="done"),
566-
r'<details class="beat s-acc" data-n="2" open>')
566+
r'<details class="beat s-acc" id="item-2" data-n="2" open>')
567567

568568

569569
class WalkCoverage(unittest.TestCase):
@@ -1093,6 +1093,47 @@ def test_live_fragment_counts_only_unresolved_flags(self):
10931093
resolved = rr.body_html(self.session(), states, {}, live=True, phase="done")
10941094
self.assertIn("0 decisions owed", resolved)
10951095

1096+
def test_the_summary_lists_each_open_question_and_the_next_step(self):
1097+
second = self.flag(n=2)
1098+
second["decision"] = dict(second["decision"], question="Keep or revert the cache change?")
1099+
page = rr.render(self.session(audience={"mode": "review"}),
1100+
[self.flag(), second, beat(n=3)], "", {})
1101+
summary = page.split('class="assessment"')[1].split("</section>")[0]
1102+
self.assertIn('<a href="#item-1">Item 1</a>: Include the authorization finding', summary)
1103+
self.assertIn('<a href="#item-2">Item 2</a>: Keep or revert the cache change?', summary)
1104+
self.assertIn("Nothing reaches GitHub until you post the whole review at the end.", summary)
1105+
self.assertLess(summary.index('class="asks"'), summary.index("What this change is")
1106+
if "What this change is" in summary else len(summary))
1107+
self.assertIn('id="item-2"', page)
1108+
legacy = self.flag()
1109+
legacy.pop("decision")
1110+
old = rr.render(self.session(audience={"mode": "review"}), [legacy], "", {})
1111+
self.assertIn('<a href="#item-1">Item 1</a>: Include in review or drop this finding?', old)
1112+
1113+
def test_a_finished_review_is_labelled_final_and_says_nothing_is_waiting(self):
1114+
done = rr.render(self.session(), [beat(n=1), beat(n=2)], "", {})
1115+
self.assertIn("<h2>Final recommendation</h2>", done)
1116+
self.assertIn("The review is complete. Nothing is waiting on you.", done)
1117+
owed = rr.render(self.session(), [self.flag(), beat(n=2)], "", {})
1118+
self.assertIn("<h2>Recommendation</h2>", owed)
1119+
self.assertNotIn("Nothing is waiting on you", owed)
1120+
partial = rr.render(self.session(), [beat(n=1)], "", {})
1121+
self.assertIn("<h2>Provisional recommendation</h2>", partial)
1122+
self.assertNotIn("Nothing is waiting on you", partial)
1123+
# every item read but the walk has not said done: not final yet
1124+
walking = rr.render(self.session(), [beat(n=1), beat(n=2)], "", {}, live=True, phase="parked")
1125+
self.assertIn("<h2>Recommendation</h2>", walking)
1126+
self.assertNotIn("Nothing is waiting on you", walking)
1127+
ended = rr.render(self.session(), [beat(n=1), beat(n=2)], "", {}, live=True, phase="done")
1128+
self.assertIn("<h2>Final recommendation</h2>", ended)
1129+
1130+
def test_a_blocked_session_does_not_promise_the_blocked_action(self):
1131+
page = rr.render(self.session(target={"kind": "github_pr", "repo": "a/b", "number": 1}),
1132+
[self.flag()], "", {})
1133+
summary = page.split('class="assessment"')[1].split("</section>")[0]
1134+
self.assertIn("Implementing is blocked for this session", summary)
1135+
self.assertNotIn("Implement applies the stated fix", summary)
1136+
10961137
def test_every_flag_names_the_decision_and_recommendation(self):
10971138
policy = self.flag(n=2, resolution_kind="decision")
10981139
policy["decision"] = {
@@ -1210,11 +1251,16 @@ def test_a_final_page_keeps_context_after_the_review_assessment(self):
12101251
self.assertIn('<h3>What this change is</h3><p class="change">closes the hole <code>S0</code> left</p>', html)
12111252
self.assertIn("<dd>the body holds up</dd>", html)
12121253

1213-
def test_a_live_walk_folds_it_to_the_reviewers_answers(self):
1254+
def test_a_live_walk_folds_it_and_each_answer_sits_beside_its_question(self):
12141255
html = rr.render(self.SESSION, [], "", {}, live=True, phase="parked")
1215-
summary = html.split('<details class="frame"><summary>')[1].split("</summary>")[0]
1216-
self.assertIn("<q>confirm</q>", summary)
1217-
self.assertIn("<q>Walk it in this order</q>", summary)
1256+
frame = html.split('<details class="frame">')[1].split("</details>")[0]
1257+
self.assertIn('<summary><span class="frame-k">How this review was framed</span></summary>', frame)
1258+
self.assertIn(
1259+
'<dt class="frame-q">Is the summary of this change right?</dt>'
1260+
"<dd>You answered <q>confirm</q></dd>", frame)
1261+
self.assertIn(
1262+
'<dt class="frame-q">In what order should the items be read?</dt>'
1263+
"<dd>You answered <q>Walk it in this order</q></dd>", frame)
12181264

12191265
def test_prose_with_no_recorded_answer_is_still_labelled(self):
12201266
html = rr.render({"repo": "r", "claim_check": "the body holds up"}, [], "", {})
@@ -1225,7 +1271,7 @@ def test_the_reconstruction_leads_as_what_the_change_is_and_leaves_the_frame(sel
12251271
self.assertLess(html.index("What this change is"), html.index('class="frame"'))
12261272
frame = html.split('<details class="frame"')[1]
12271273
self.assertNotIn("closes the hole", frame)
1228-
self.assertIn("Your answer on the summary", frame)
1274+
self.assertIn("Is the summary of this change right?", frame)
12291275
self.assertNotIn("What this change is", rr.render({"repo": "r"}, [], "", {}))
12301276

12311277
def test_the_reviewers_words_cannot_carry_markup(self):

‎tests/test_skill.py‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,7 @@ def test_assessment_and_decisions_are_explicit_in_both_surfaces(self):
7777
"agent's recommendation, not the reviewer's GitHub verdict",
7878
"A static review cannot claim verified runtime behavior",
7979
"Before the walk is complete, call it provisional",
80+
"say in the recommendation whether it blocks the merge",
8081
):
8182
self.assertIn(phrase, text)
8283

0 commit comments

Comments
 (0)