Merge pull request 'Revise pull request context before requesting review' (#1333) from timmy/1332-revise-pull-context into main

2026-08-24 03:21:15 +00:00 · 2026-08-24 03:21:15 +00:00 · f8ab6977ed
commit f8ab6977ed
parent 8c4969cc44 8285b39e06
7 changed files with 389 additions and 3 deletions
--- a/frontend/dashboard.js
+++ b/frontend/dashboard.js
@ -1287,7 +1287,7 @@
          if (continuing) return await completeOwnershipExitToday(item);
          paintMyWork(lastContextSnapshot);
          return false;
-        });
+        }, ()=>selectedPullDetail, ()=>confirmedOwnerLogin);
      }
      attachReleaseReceipt();
      if (!reviewController) reviewController = createReviewController({ fetchJson: fetchReviewJson, storage: localStorage });
@ -4632,6 +4632,7 @@
    qs('#pull-sheet-title').textContent = item.title || 'Assigned pull request';
    qs('#pull-sheet-status').textContent = 'Loading pull request…';
    qs('#pull-sheet-body').textContent = '';
+
    qs('#pull-files').textContent = '';
    qs('#pull-review').open = false;
    qs('#pull-review-status').textContent = 'Expand to load changed files and merge readiness.';
@ -4663,6 +4664,7 @@
      pullConversation = pullController.conversation(item, detail.conversation);
      qs('#pull-sheet-title').textContent = detail.title || 'Assigned pull request';
      qs('#pull-sheet-body').innerHTML = renderMarkdown(detail.body || 'No description provided.');
+
      renderPullConversation(pullConversation.snapshot());
      qs('#open-pull-gitea').href = detail.url || item.url || '#';
      qs('#pull-sheet-status').textContent = 'Pull request ready · by ' + (detail.author || 'unknown author');
--- a/frontend/index.html
+++ b/frontend/index.html
@ -1651,6 +1651,16 @@
      <div id="pull-sheet-status" class="small" aria-live="polite">Choose a pull request.</div>
      <button class="pull-retry" id="retry-pull-load" type="button" hidden>Retry loading pull request</button>
      <div class="pull-sheet-content markdown-content" id="pull-sheet-body"></div>
+      <button id="edit-pull-content" type="button" hidden>Edit review context</button>
+      <form class="issue-edit-form" id="pull-edit-form" hidden>
+        <label>Title<input id="pull-edit-title" maxlength="255" required /></label>
+        <label>Description<textarea id="pull-edit-body" maxlength="10000"></textarea></label>
+        <div class="issue-edit-actions">
+          <button id="save-pull-content" type="submit">Save context</button>
+          <button id="cancel-pull-edit" type="button">Cancel</button>
+        </div>
+        <div id="pull-edit-status" class="small" role="status" aria-live="assertive"></div>
+      </form>
    </div>
    <section id="pull-conversation" tabindex="-1">
      <h2>Full conversation</h2><div id="pull-comments"></div>
--- a/frontend/pull-sheet.js
+++ b/frontend/pull-sheet.js
@ -99,6 +99,9 @@ function resetOwnershipControls(doc, item, checkpointed) {
  qs('#release-pull').disabled = false;
  qs('#release-pull').textContent = checkpointed(item) ? 'Release & next' : 'Release assignment';
  qs('#pull-handoff-status').textContent = 'Load teammates to transfer ownership.';
+  qs('#edit-pull-content').hidden = true;
+  qs('#pull-edit-form').hidden = true;
+  qs('#pull-edit-status').textContent = '';
 }

 function resetReviewRequestControls(doc, detail) {
@ -118,7 +121,10 @@ function bindReviewRequestControls(doc, controller, getSelected) {
  const load = qs('#load-pull-reviewers');
  if (load.dataset.reviewRequestBound === 'true') return;
  load.dataset.reviewRequestBound = 'true';
-  controller.setReviewDetail = detail => resetReviewRequestControls(doc, detail);
+  controller.setReviewDetail = detail => {
+    resetReviewRequestControls(doc, detail);
+    controller.edit?.setDetail(detail, Boolean(detail?.saved_at));
+  };
  resetReviewRequestControls(doc, null);
  load.addEventListener('click', async () => {
    const selected = getSelected();
@ -163,8 +169,81 @@ function bindReviewRequestControls(doc, controller, getSelected) {
  });
 }

-function bindOwnershipControls(doc, controller, getSelected, finish) {
+function bindContextEditor(doc, controller, getSelected, getDetail, getLogin) {
+  const qs = selector => doc.querySelector(selector);
+  const form = qs('#pull-edit-form');
+  const controls = () => form.querySelectorAll('input,textarea,button');
+  const draft = () => ({
+    title:qs('#pull-edit-title').value.trim(), body:qs('#pull-edit-body').value,
+    expectedHeadSha:getDetail()?.head_sha || '',
+  });
+  qs('#edit-pull-content').addEventListener('click', () => {
+    const item = getSelected(), detail = getDetail();
+    if (!item || !detail) return;
+    const saved = controller.loadEditDraft(item) || {
+      title:detail.title || '', body:detail.body || '', expectedHeadSha:detail.head_sha || '',
+    };
+    qs('#pull-edit-title').value = saved.title;
+    qs('#pull-edit-body').value = saved.body;
+    form.hidden = false;
+    qs('#pull-edit-status').textContent = '';
+    qs('#pull-edit-title').focus();
+  });
+  qs('#cancel-pull-edit').addEventListener('click', () => {
+    form.hidden = true;
+    qs('#edit-pull-content').focus();
+  });
+  ['#pull-edit-title', '#pull-edit-body'].forEach(selector => qs(selector).addEventListener('input', () => {
+    const item = getSelected();
+    if (item && getDetail()) controller.saveEditDraft(item, draft());
+  }));
+  form.addEventListener('submit', async event => {
+    event.preventDefault();
+    const item = getSelected(), update = draft();
+    if (!item || !getDetail()) return;
+    if (!update.title) {
+      qs('#pull-edit-status').textContent = 'Title is required.';
+      qs('#pull-edit-title').focus();
+      return;
+    }
+    controls().forEach(control => { control.disabled = true; });
+    qs('#pull-edit-status').textContent = 'Saving review context…';
+    try {
+      const result = await controller.updateContent(item, update);
+      if (getSelected() !== item) return;
+      const detail = getDetail();
+      Object.assign(detail, result);
+      item.title = result.title;
+      qs('#pull-sheet-title').textContent = result.title;
+      qs('#pull-sheet-body').innerHTML = renderMarkdown(result.body || 'No description provided.');
+      controller.setReviewDetail(detail);
+      qs('#pull-sheet-status').textContent = 'Review context updated · ready to request review';
+      form.hidden = true;
+      qs('#pull-edit-status').textContent = '';
+      qs('#edit-pull-content').focus();
+    } catch (error) {
+      qs('#pull-edit-status').textContent = error.message + ' Your change is still here; retry.';
+      qs('#pull-edit-title').focus();
+    } finally {
+      controls().forEach(control => { control.disabled = false; });
+    }
+  });
+  return {
+    reset() {
+      qs('#edit-pull-content').hidden = true;
+      form.hidden = true;
+      qs('#pull-edit-status').textContent = '';
+    },
+    setDetail(detail, offline = false) {
+      const editable = !offline && detail?.state === 'open' && detail.author === getLogin();
+      qs('#edit-pull-content').hidden = !editable;
+    },
+  };
+}
+
+function bindOwnershipControls(doc, controller, getSelected, finish, getDetail, getLogin) {
  bindReviewRequestControls(doc, controller, getSelected);
+  controller.edit = bindContextEditor(doc, controller, getSelected, getDetail, getLogin);
  const qs = selector => doc.querySelector(selector);
  const load = qs('#load-pull-handoff');
  if (load.dataset.ownershipBound === 'true') return;
@ -237,12 +316,14 @@ function createPullSheet({ fetchJson, storage, createConversationPager = globalT
  let candidateRequest = null;
  let reviewCandidateRequest = null;
  let reviewRequestMutation = null;
+  let editRequest = null;
  const reviewRequests = new Map();
  const reviewCache = new Map();
  const pathFor = item => 'api/v1/repos/' + String(item.repository || '').split('/')
    .map(encodeURIComponent).join('/') + '/pulls/' + encodeURIComponent(item.number);
  const draftKey = item => 'stackchain.pull-comment.v1:' + item.repository + '#' + item.number;
  const operationKey = item => draftKey(item) + ':operation';
+  const editDraftKey = item => 'stackchain.pull-edit.v1:' + item.repository + '#' + item.number;
  const reviewKey = (item, detail) => 'stackchain.pull-review.v1:' + item.repository + '#' + item.number + ':' + detail.head_sha;
  const fileNames = detail => (detail?.files || []).map(file => file.filename).filter(Boolean);

@ -305,6 +386,39 @@ function createPullSheet({ fetchJson, storage, createConversationPager = globalT
      }).finally(() => { reviewRequestMutation = null; });
      return reviewRequestMutation;
    },
+    loadEditDraft(item) {
+      try {
+        const value = JSON.parse(storage?.getItem(editDraftKey(item)) || 'null');
+        return value && typeof value.title === 'string' && typeof value.body === 'string' &&
+          typeof value.expectedHeadSha === 'string' ? value : null;
+      } catch (_error) { return null; }
+    },
+    saveEditDraft(item, draft) {
+      try { storage?.setItem(editDraftKey(item), JSON.stringify(draft)); }
+      catch (_error) { /* The edit fields remain the fallback. */ }
+    },
+    updateContent(item, draft) {
+      if (editRequest) return editRequest;
+      this.saveEditDraft(item, draft);
+      editRequest = fetchJson(pathFor(item) + '/content', {
+        method: 'PATCH',
+        headers: { Accept: 'application/json', 'Content-Type': 'application/json' },
+        body: JSON.stringify({
+          title: draft.title,
+          body: draft.body,
+          expected_head_sha: draft.expectedHeadSha,
+        }),
+      }).then(result => {
+        if (result?.number !== item.number || result?.title !== draft.title ||
+            result?.body !== draft.body || result?.head_sha !== draft.expectedHeadSha) {
+          throw new Error('Pull request update was not confirmed.');
+        }
+        try { storage?.removeItem(editDraftKey(item)); }
+        catch (_error) { /* Confirmed upstream content is authoritative. */ }
+        return result;
+      }).finally(() => { editRequest = null; });
+      return editRequest;
+    },
    handoff(item, recipient) {
      if (ownershipRequest) return ownershipRequest;
      ownershipRequest = fetchJson(pathFor(item) + '/handoff', {
@ -404,6 +518,7 @@ createPullSheet.renderHandoffCandidates = renderHandoffCandidates;
 createPullSheet.ownershipExitMessage = ownershipExitMessage;
 createPullSheet.resetOwnershipControls = resetOwnershipControls;
 createPullSheet.bindOwnershipControls = bindOwnershipControls;
+createPullSheet.bindContextEditor = bindContextEditor;
 createPullSheet.resetReviewRequestControls = resetReviewRequestControls;
 createPullSheet.bindReviewRequestControls = bindReviewRequestControls;
 if (typeof module !== 'undefined' && module.exports) module.exports = createPullSheet;
--- a/src/gitea_proxy.py
+++ b/src/gitea_proxy.py
@ -2703,6 +2703,52 @@ async def is_assigned_pull(repository: str, number: int) -> bool:
    )


+async def update_authored_assigned_pull(
+    repository: str,
+    number: int,
+    title: str,
+    body: str,
+    expected_head_sha: str,
+) -> dict:
+    path = f"repos/{repository}/pulls/{number}"
+    login, pull = await _current_login_and_target(path)
+    author = pull.get("user") if isinstance(pull.get("user"), dict) else {}
+    head = pull.get("head") if isinstance(pull.get("head"), dict) else {}
+    if (
+        pull.get("state") != "open"
+        or pull.get("merged") is True
+        or author.get("login") != login
+        or not _login_in_users(login, pull.get("assignees"))
+    ):
+        raise IssueNotAvailableError("pull request not found")
+    if head.get("sha") != expected_head_sha:
+        raise IssueEditConflictError("pull request head changed upstream")
+    response = await _get_client().patch(
+        f"/api/v1/{path}",
+        headers=_auth(),
+        json={"title": title, "body": body},
+    )
+    response.raise_for_status()
+    confirmed = response.json()
+    confirmed_head = confirmed.get("head") if isinstance(confirmed.get("head"), dict) else {}
+    if (
+        confirmed.get("number") != number
+        or confirmed.get("title") != title
+        or confirmed.get("body", "") != body
+        or confirmed_head.get("sha") != expected_head_sha
+    ):
+        raise ValueError("Gitea did not confirm the pull request update")
+    return {
+        "repository": repository,
+        "number": number,
+        "title": title,
+        "body": body,
+        "head_sha": expected_head_sha,
+        "state": confirmed.get("state", "open"),
+        "url": _safe_gitea_web_url(confirmed.get("html_url")),
+    }
+
+
 async def pull_completion_detail(repository: str, number: int) -> dict:
    base = f"repos/{repository}/pulls/{number}"
    pull = await fetch(base)
--- a/src/main.py
+++ b/src/main.py
@ -1117,6 +1117,22 @@ class PullReviewRequest(BaseModel):
    )


+class PullContentUpdate(BaseModel):
+    title: str = Field(min_length=1, max_length=255)
+    body: str = Field(default="", max_length=10_000)
+    expected_head_sha: str = Field(
+        min_length=7, max_length=64, pattern=r"^[A-Fa-f0-9]+$"
+    )
+
+    @field_validator("title")
+    @classmethod
+    def strip_title(cls, value: str) -> str:
+        value = value.strip()
+        if not value:
+            raise ValueError("title cannot be blank")
+        return value
+
+
 class IssueReassignment(IssueHandoff):
    expected_assignees: list[str] = Field(min_length=1, max_length=10)

@ -6558,6 +6574,41 @@ async def assigned_pull_detail(owner: str, repo: str, number: int = PathParam(gt
        )


+@app.patch("/api/v1/repos/{owner}/{repo}/pulls/{number}/content")
+async def update_authored_assigned_pull_content(
+    update: PullContentUpdate,
+    owner: str,
+    repo: str,
+    number: int = PathParam(gt=0),
+):
+    repository = f"{owner}/{repo}"
+    try:
+        result = await asyncio.wait_for(
+            gitea_proxy.update_authored_assigned_pull(
+                repository,
+                number,
+                update.title,
+                update.body,
+                update.expected_head_sha,
+            ),
+            timeout=ISSUE_ACTION_TIMEOUT_SECONDS,
+        )
+    except gitea_proxy.IssueEditConflictError:
+        return JSONResponse(
+            {"error": "This pull request changed in Gitea. Your draft is safe; reload the latest pull request before saving."},
+            status_code=409,
+        )
+    except gitea_proxy.IssueNotAvailableError:
+        raise HTTPException(status_code=404, detail="Editable pull request not found")
+    except Exception:
+        return JSONResponse(
+            {"error": "The pull request could not be updated. Your draft is safe; please retry."},
+            status_code=503,
+            headers={"Retry-After": "1"},
+        )
+    return JSONResponse(result)
+
+
@app.get("/api/v1/repos/{owner}/{repo}/pulls/{number}/review-data")
 async def assigned_pull_review_data(
    owner: str, repo: str, number: int = PathParam(gt=0)
--- a/tests/test_my_work.py
+++ b/tests/test_my_work.py
@ -7424,6 +7424,80 @@ Promise.all([first, duplicate]).then(async comments => {{
    assert len(output["comments"]) == 2 and len(output["merges"]) == 2


+def test_pull_sheet_preserves_context_draft_until_confirmed_update():
+    script = f"""
+const createPullSheet = require({json.dumps(str(PULL_SHEET))});
+const values = new Map();
+const storage = {{getItem:k => values.get(k) || null, setItem:(k,v) => values.set(k,v), removeItem:k => values.delete(k)}};
+const calls = [];
+let fail = true;
+const controller = createPullSheet({{
+  storage,
+  fetchJson: async (url, options={{}}) => {{
+    const body = JSON.parse(options.body);
+    calls.push({{url, method:options.method, body}});
+    if (fail) {{ fail = false; throw new Error('network down'); }}
+    return {{number:7,title:body.title,body:body.body,head_sha:body.expected_head_sha,state:'open'}};
+  }},
+}});
+const item = {{repository:'stackchain/api',number:7}};
+const draft = {{title:'Clear review brief',body:'Review mobile handoff.',expectedHeadSha:'abc1234'}};
+(async () => {{
+  await controller.updateContent(item, draft).catch(() => null);
+  const kept = controller.loadEditDraft(item);
+  const result = await controller.updateContent(item, draft);
+  process.stdout.write(JSON.stringify({{kept,result,after:controller.loadEditDraft(item),calls}}));
+}})().catch(error => {{ console.error(error); process.exit(1); }});
+"""
+    result = subprocess.run(["node", "-e", script], check=True, capture_output=True, text=True)
+    output = json.loads(result.stdout)
+    assert output["kept"] == {
+        "title": "Clear review brief",
+        "body": "Review mobile handoff.",
+        "expectedHeadSha": "abc1234",
+    }
+    assert output["after"] is None
+    assert output["result"]["head_sha"] == "abc1234"
+    assert output["calls"] == [
+        {
+            "url": "api/v1/repos/stackchain/api/pulls/7/content",
+            "method": "PATCH",
+            "body": {
+                "title": "Clear review brief",
+                "body": "Review mobile handoff.",
+                "expected_head_sha": "abc1234",
+            },
+        },
+        {
+            "url": "api/v1/repos/stackchain/api/pulls/7/content",
+            "method": "PATCH",
+            "body": {
+                "title": "Clear review brief",
+                "body": "Review mobile handoff.",
+                "expected_head_sha": "abc1234",
+            },
+        },
+    ]
+
+
+@pytest.mark.anyio
+async def test_pull_overview_exposes_author_only_context_editor_with_retry_safe_status():
+    html = (Path(__file__).parents[1] / "frontend" / "index.html").read_text()
+    source = await dashboard()
+    pull_source = PULL_SHEET.read_text()
+
+    assert 'id="edit-pull-content"' in html
+    assert 'id="pull-edit-form"' in html
+    assert 'id="pull-edit-title"' in html
+    assert 'id="pull-edit-body"' in html
+    assert 'id="save-pull-content"' in html
+    assert "detail.author === getLogin()" in pull_source
+    assert "controller.loadEditDraft(item)" in pull_source
+    assert "controller.updateContent(item" in pull_source
+    assert "Your change is still here; retry." in pull_source
+    assert "controller.setReviewDetail(detail)" in pull_source
+
+
 def test_pull_sheet_removes_one_pull_from_snapshot_without_touching_other_work():
    script = f"""
 const createPullSheet = require({json.dumps(str(PULL_SHEET))});
@ -7463,6 +7537,9 @@ const elements = new Map([
  ['#load-pull-handoff', {{disabled:true}}],
  ['#release-pull', {{disabled:true,textContent:''}}],
  ['#pull-handoff-status', {{textContent:''}}],
+  ['#edit-pull-content', {{hidden:false}}],
+  ['#pull-edit-form', {{hidden:false}}],
+  ['#pull-edit-status', {{textContent:'stale'}}],
 ]);
 const select = {{textContent:'stale', appendChild:node => children.push(node)}};
 const doc = {{
--- a/tests/test_pull_api.py
+++ b/tests/test_pull_api.py
@ -1,4 +1,5 @@
 import asyncio
+import json

 import httpx
 import pytest
@ -7,6 +8,90 @@ from src import gitea_proxy, main
 from src.security_event_store import SecurityEventStoreError


+@pytest.mark.anyio
+async def test_pull_author_can_update_assigned_open_pull_context(monkeypatch):
+    calls = []
+
+    async def update(repository, number, title, body, expected_head_sha):
+        calls.append((repository, number, title, body, expected_head_sha))
+        return {
+            "repository": repository,
+            "number": number,
+            "title": title,
+            "body": body,
+            "head_sha": expected_head_sha,
+            "state": "open",
+        }
+
+    monkeypatch.setattr(
+        main.gitea_proxy, "update_authored_assigned_pull", update, raising=False
+    )
+    transport = httpx.ASGITransport(app=main.app)
+    async with httpx.AsyncClient(transport=transport, base_url="http://test") as client:
+        response = await client.patch(
+            "/api/v1/repos/stackchain/api/pulls/7/content",
+            json={
+                "title": "Clarify mobile handoff",
+                "body": "Explain the reviewer path.",
+                "expected_head_sha": "abc1234",
+            },
+        )
+
+    assert response.status_code == 200
+    assert response.json()["title"] == "Clarify mobile handoff"
+    assert calls == [
+        (
+            "stackchain/api",
+            7,
+            "Clarify mobile handoff",
+            "Explain the reviewer path.",
+            "abc1234",
+        )
+    ]
+
+
+@pytest.mark.anyio
+async def test_gitea_pull_context_update_requires_author_assignment_and_matching_head():
+    requests = []
+
+    async def handler(request):
+        requests.append((request.method, request.url.path, request.content))
+        if request.url.path == "/api/v1/user":
+            return httpx.Response(200, json={"login": "alex"})
+        if request.url.path == "/api/v1/repos/stackchain/api/pulls/7" and request.method == "GET":
+            return httpx.Response(200, json={
+                "number": 7, "title": "Old context", "body": "Old body", "state": "open",
+                "merged": False, "user": {"login": "alex"}, "assignees": [{"login": "alex"}],
+                "head": {"sha": "abc1234"},
+                "html_url": "https://forge.example/stackchain/api/pulls/7",
+            })
+        if request.url.path == "/api/v1/repos/stackchain/api/pulls/7" and request.method == "PATCH":
+            assert json.loads(request.content) == {
+                "title": "Clear review brief", "body": "Review the mobile handoff."
+            }
+            return httpx.Response(200, json={
+                "number": 7, "title": "Clear review brief", "body": "Review the mobile handoff.",
+                "state": "open", "head": {"sha": "abc1234"},
+                "html_url": "https://forge.example/stackchain/api/pulls/7",
+            })
+        raise AssertionError(f"unexpected request: {request.method} {request.url.path}")
+
+    gitea_proxy.start_client(transport=httpx.MockTransport(handler))
+    try:
+        result = await gitea_proxy.update_authored_assigned_pull(
+            "stackchain/api", 7, "Clear review brief", "Review the mobile handoff.", "abc1234"
+        )
+    finally:
+        await gitea_proxy.stop_client()
+
+    assert result["title"] == "Clear review brief"
+    assert [request[:2] for request in requests] == [
+        ("GET", "/api/v1/user"),
+        ("GET", "/api/v1/repos/stackchain/api/pulls/7"),
+        ("PATCH", "/api/v1/repos/stackchain/api/pulls/7"),
+    ]
+
+
@pytest.mark.anyio
 async def test_assigned_pull_detail_reports_completion_state(monkeypatch):
    async def assigned(repository, number):