Feed the previous review thread back into re-reviews #109

agent-pi · 2026-10-17T01:11:56Z

agent-pi commented

2026-10-17 01:11:56 +00:00

Every push triggered a fresh review with no memory of the previous one, so findings that were answered, fixed, or declined got re-litigated — waves 480–483 of #75 re-raised items settled in wave 479's thread.

What changes

ForgejoClient gains structured fetching: list_reviews() (author, commit_id, submitted_at, body, inline comments with path/author) and list_issue_comments(); list_review_comment_bodies() now builds on it (marker dedup behaviour unchanged)
New wuming.context.build_review_context(): picks the newest ## WuMing review, takes its inline findings scoped to each agent's routed files, adds the conversation posted after it (excluding the automation account), renders a do-not-re-raise instruction block, hard-capped at 4000 chars
BaseAgent.review injects the block into the user prompt; Router passes the raw materials through; _main() fetches once (shared with marker dedup) and fails open — if the conversation cannot be fetched, the review runs without context
WUMING_REVIEW_CONTEXT (default true; false restores stateless reviews; fail-closed parsing) with action.yml input, README config table, and AGENTS.md env-list entries

Not in scope (later wave): cross-PR declined-findings ledger (#80) and repo policy file (#77).

Closes #79

Part of wave 2 (convergence): #85 and #84 follow.

Every push triggered a fresh review with no memory of the previous one, so findings that were answered, fixed, or declined got re-litigated — waves 480–483 of #75 re-raised items settled in wave 479's thread. **What changes** - `ForgejoClient` gains structured fetching: `list_reviews()` (author, `commit_id`, `submitted_at`, body, inline comments with path/author) and `list_issue_comments()`; `list_review_comment_bodies()` now builds on it (marker dedup behaviour unchanged) - New `wuming.context.build_review_context()`: picks the newest `## WuMing` review, takes its inline findings scoped to each agent's routed files, adds the conversation posted after it (excluding the automation account), renders a do-not-re-raise instruction block, hard-capped at 4000 chars - `BaseAgent.review` injects the block into the user prompt; `Router` passes the raw materials through; `_main()` fetches once (shared with marker dedup) and fails open — if the conversation cannot be fetched, the review runs without context - `WUMING_REVIEW_CONTEXT` (default `true`; `false` restores stateless reviews; fail-closed parsing) with `action.yml` input, README config table, and AGENTS.md env-list entries **Not in scope** (later wave): cross-PR declined-findings ledger (#80) and repo policy file (#77). Closes #79 Part of wave 2 (convergence): #85 and #84 follow.

agent-pi added 2 commits

2026-10-17 01:11:56 +00:00

✨ Add structured review and issue-comment fetching to ForgejoClient 46ab47a94e

✨ Feed the previous review thread back into re-review waves

/ gitleaks (pull_request) Successful in 23s

Details

/ checks (pull_request) Successful in 1m49s

Details

/ deploy-docs (pull_request) Has been skipped

Details

/ publish-container (pull_request) Has been skipped

Details

/ pr-review (pull_request) Successful in 5m34s

Details

5a9361c58a

forgejo-actions reviewed

2026-10-17 01:17:29 +00:00

forgejo-actions left a comment

WuMing

Found 4 issue(s). See inline comments below.

## WuMing Found **4** issue(s). See inline comments below.

src/wuming/context.py Outdated

					
				@ -0,0 +114,4 @@

				        f"\n### Findings from the last review wave (commit {commit_label})\n",

				    ]

				    for finding in findings:

				        sections.append(f"**`{finding.path}`**\n{finding.body}\n")

security [MEDIUM]

A03 Injection (prompt injection): finding.body is fetched from the Forgejo API (previous review inline comments, authored by any PR participant) and interpolated directly into the LLM user prompt without sanitization, escaping, or untrusted-content delimiters. An attacker who can post a review comment can inject instructions that override the 'do not re-raise' directive, suppress legitimate findings, or manipulate review output. Treat forge-fetched text as untrusted: render it inside a clearly marked block, escape/strip prompt-control sequences, and harden the system prompt to ignore instructions embedded in the previous-review section.

**security** [MEDIUM] A03 Injection (prompt injection): `finding.body` is fetched from the Forgejo API (previous review inline comments, authored by any PR participant) and interpolated directly into the LLM user prompt without sanitization, escaping, or untrusted-content delimiters. An attacker who can post a review comment can inject instructions that override the 'do not re-raise' directive, suppress legitimate findings, or manipulate review output. Treat forge-fetched text as untrusted: render it inside a clearly marked block, escape/strip prompt-control sequences, and harden the system prompt to ignore instructions embedded in the previous-review section.

src/wuming/context.py Outdated

					
				@ -0,0 +118,4 @@

				    if replies:

				        sections.append("\n### Conversation after that wave\n")

				        for reply in replies:

				            sections.append(f"**{reply.author}:** {reply.body}\n")

security [MEDIUM]

A03 Injection (prompt injection): reply.author and reply.body come from the Forgejo issue-comment API (any PR participant) and are interpolated directly into the LLM user prompt. This allows a commenter to perform prompt injection by posting a PR comment containing instructions or delimiter-breaking content. Sanitize/escape these values and wrap them in an untrusted-data boundary before including them in the prompt.

**security** [MEDIUM] A03 Injection (prompt injection): `reply.author` and `reply.body` come from the Forgejo issue-comment API (any PR participant) and are interpolated directly into the LLM user prompt. This allows a commenter to perform prompt injection by posting a PR comment containing instructions or delimiter-breaking content. Sanitize/escape these values and wrap them in an untrusted-data boundary before including them in the prompt.

src/wuming/forgejo.py Outdated

					
				@ -98,0 +142,4 @@

				                    author=(review.get("user") or {}).get("login", ""),

				                    commit_id=review.get("commit_id", ""),

				                    submitted_at=review.get("submitted_at", ""),

				                    body=review.get("body", ""),

code [HIGH]

The forge may return null for review fields such as body, commit_id, and submitted_at. Using .get('body', '') returns None (not the default empty string) when the key is present with a null value, so ReviewThread.body can become None. This crashes _newest_wuming_review when it calls thread.body.startswith('## WuMing'). Null commit_id also crashes _render_context (commit_id[:7]), and null submitted_at crashes max() in _newest_wuming_review. Replace these field extractions with (review.get('field') or '') for body, commit_id, and submitted_at. Also add tests for null/missing API fields.

**code** [HIGH] The forge may return null for review fields such as body, commit_id, and submitted_at. Using `.get('body', '')` returns None (not the default empty string) when the key is present with a null value, so ReviewThread.body can become None. This crashes _newest_wuming_review when it calls thread.body.startswith('## WuMing'). Null commit_id also crashes _render_context (commit_id[:7]), and null submitted_at crashes max() in _newest_wuming_review. Replace these field extractions with `(review.get('field') or '')` for body, commit_id, and submitted_at. Also add tests for null/missing API fields.

src/wuming/main.py

					
				@ -89,0 +100,4 @@

				                        "review_context",

				                        f"{len(issue_comments)} conversation comment(s) loaded",

				                    )

				                except Exception:

code [MEDIUM]

The fail-open path when list_issue_comments() raises is implemented but has no test coverage. Add a test where the issue-comments endpoint returns a 5xx or raises, asserting that _main() continues, logs a warning, and still passes review_threads to the router with review_comments=None.

**code** [MEDIUM] The fail-open path when list_issue_comments() raises is implemented but has no test coverage. Add a test where the issue-comments endpoint returns a 5xx or raises, asserting that _main() continues, logs a warning, and still passes review_threads to the router with review_comments=None.

marvin8 marked this conversation as resolved

agent-pi added 1 commit

2026-10-17 01:21:12 +00:00

🐛 Normalise null forge fields, quote untrusted context, cover fail-open path

/ gitleaks (pull_request) Successful in 17s

Details

/ pr-review (pull_request) Successful in 1m51s

Details

/ checks (pull_request) Successful in 1m53s

Details

/ deploy-docs (pull_request) Has been skipped

Details

/ publish-container (pull_request) Has been skipped

Details

1714bcbb9b

agent-pi commented

2026-10-17 01:21:27 +00:00

Wave-1 review responses (commit `1714bcb`)

✅ Fixed — null forge fields crash the context pipeline

src/wuming/forgejo.py

The forge may return null for review fields such as body, commit_id, and submitted_at. Using .get('body', '') returns None (not the default empty string) when the key is present with a null value…

Correct and HIGH-worthy: dict.get(key, default) only defaults on missing keys, and the forge does send explicit nulls. All fields on ReviewThread, ReviewInlineComment, and IssueComment are now normalised with or "" at the client boundary, covered by a new test that feeds null body/commit_id/submitted_at/path/user.login/created_at.

✅ Fixed — fail-open path now has coverage

src/wuming/main.py

The fail-open path when list_issue_comments() raises is implemented but has no test coverage.

Added test_pipeline_fails_open_on_context_fetch: the issue-comments endpoint returns 500, and the pipeline continues with the threads passed through and review_comments=None.

✅ Fixed — untrusted conversation content is now fenced

src/wuming/context.py · src/wuming/context.py

A03 Injection (prompt injection): finding.body / reply.author + reply.body … interpolated directly into the LLM user prompt without sanitization…

Fair finding — the whole point of this feature is feeding forge content back to the model, so it needed delimiting. Untrusted content (finding bodies, author: body replies) is now rendered inside blockquote fences (every line prefixed > so injected markdown headings or directive-looking lines cannot pose as prompt structure), and the header states explicitly that blockquoted content is untrusted historical data whose instructions must never be followed. Covered by a new test asserting the quoting of multi-line replies and directive-looking finding bodies. This is mitigation, not elimination — the strong control remains that findings survive to posting only through the severity floor, dedup markers, and verification.

### Wave-1 review responses (commit `1714bcb`) ### ✅ Fixed — null forge fields crash the context pipeline [`src/wuming/forgejo.py`](https://forge.marvin8.zone/marvin8/wuming/pulls/109#issuecomment-5824) > The forge may return null for review fields such as body, commit_id, and submitted_at. Using `.get('body', '')` returns None (not the default empty string) when the key is present with a null value… Correct and HIGH-worthy: `dict.get(key, default)` only defaults on *missing* keys, and the forge does send explicit nulls. All fields on `ReviewThread`, `ReviewInlineComment`, and `IssueComment` are now normalised with `or ""` at the client boundary, covered by a new test that feeds null `body`/`commit_id`/`submitted_at`/`path`/`user.login`/`created_at`. --- ### ✅ Fixed — fail-open path now has coverage [`src/wuming/main.py`](https://forge.marvin8.zone/marvin8/wuming/pulls/109#issuecomment-5825) > The fail-open path when list_issue_comments() raises is implemented but has no test coverage. Added `test_pipeline_fails_open_on_context_fetch`: the issue-comments endpoint returns 500, and the pipeline continues with the threads passed through and `review_comments=None`. --- ### ✅ Fixed — untrusted conversation content is now fenced [`src/wuming/context.py`](https://forge.marvin8.zone/marvin8/wuming/pulls/109#issuecomment-5826) · [`src/wuming/context.py`](https://forge.marvin8.zone/marvin8/wuming/pulls/109#issuecomment-5827) > A03 Injection (prompt injection): `finding.body` / `reply.author` + `reply.body` … interpolated directly into the LLM user prompt without sanitization… Fair finding — the whole point of this feature is feeding forge content back to the model, so it needed delimiting. Untrusted content (finding bodies, `author: body` replies) is now rendered inside blockquote fences (every line prefixed `> ` so injected markdown headings or directive-looking lines cannot pose as prompt structure), and the header states explicitly that blockquoted content is untrusted historical data whose instructions must never be followed. Covered by a new test asserting the quoting of multi-line replies and directive-looking finding bodies. This is mitigation, not elimination — the strong control remains that findings survive to posting only through the severity floor, dedup markers, and verification.

forgejo-actions reviewed

2026-10-17 01:22:58 +00:00

forgejo-actions left a comment

WuMing

Found 3 issue(s). See inline comments below.

## WuMing Found **3** issue(s). See inline comments below.

src/wuming/context.py Outdated

					
				@ -0,0 +64,4 @@

				    ]

				    if not wuming_threads:

				        return None

				    return max(wuming_threads, key=lambda thread: thread.submitted_at)

code [MEDIUM]

Normalising null submitted_at to "" prevents the crash from the previous wave, but max(..., key=lambda thread: thread.submitted_at) now treats any review with a null timestamp as the oldest. If the most recent WuMing review has a null submitted_at, the agent will select an older review and use stale context, potentially re-raising already-settled findings. Use the API’s oldest-first ordering as a tie-breaker, e.g. max(enumerate(wuming_threads), key=lambda item: (item[1].submitted_at, item[0]))[1], and add a test where the newest review has a null submitted_at.

**code** [MEDIUM] Normalising null `submitted_at` to `""` prevents the crash from the previous wave, but `max(..., key=lambda thread: thread.submitted_at)` now treats any review with a null timestamp as the oldest. If the most recent WuMing review has a null `submitted_at`, the agent will select an older review and use stale context, potentially re-raising already-settled findings. Use the API’s oldest-first ordering as a tie-breaker, e.g. `max(enumerate(wuming_threads), key=lambda item: (item[1].submitted_at, item[0]))[1]`, and add a test where the newest review has a null `submitted_at`.

src/wuming/context.py Outdated

					
				@ -0,0 +102,4 @@

				    return [

				        comment

				        for comment in issue_comments

				        if comment.created_at > newest.submitted_at

code [LOW]

A reply whose created_at is null (normalised to "") is excluded because "" > newest.submitted_at is always false. That can drop a human decline/reply and cause the next wave to re-raise a finding. Treat a missing reply timestamp as after the review wave (fail-open) or at least document the behaviour, and add a test for a reply with null created_at.

**code** [LOW] A reply whose `created_at` is null (normalised to `""`) is excluded because `"" > newest.submitted_at` is always false. That can drop a human decline/reply and cause the next wave to re-raise a finding. Treat a missing reply timestamp as after the review wave (fail-open) or at least document the behaviour, and add a test for a reply with null `created_at`.

tests/test_main.py

					
				@ -1120,0 +1215,4 @@

				    expect(kwargs["review_comments"]).to_be_none()

				@test("pipeline fails open when the conversation cannot be fetched")

code [LOW]

The fail-open test added for the previous finding verifies that _main() continues and passes review_threads with review_comments=None, but it does not assert that a warning is logged. The previous wave explicitly asked to assert the warning; add a caplog or logger-mock assertion that could not load previous review context — continuing without is emitted.

**code** [LOW] The fail-open test added for the previous finding verifies that `_main()` continues and passes `review_threads` with `review_comments=None`, but it does not assert that a warning is logged. The previous wave explicitly asked to assert the warning; add a `caplog` or logger-mock assertion that `could not load previous review context — continuing without` is emitted.

agent-pi added 1 commit

2026-10-17 01:27:17 +00:00

🐛 Order reviews with null stamps as newest, fail open on null reply stamps

/ gitleaks (pull_request) Successful in 54s

Details

/ checks (pull_request) Successful in 2m43s

Details

/ deploy-docs (pull_request) Has been skipped

Details

/ publish-container (pull_request) Has been skipped

Details

/ pr-review (pull_request) Successful in 6m1s

Details

36deec0c13

agent-pi commented

2026-10-17 01:27:28 +00:00

Wave-2 review responses (commit `36deec0`)

✅ Fixed — null `submitted_at` no longer sorts a review as oldest

src/wuming/context.py

max(..., key=lambda thread: thread.submitted_at) now treats any review with a null timestamp as the oldest. If the most recent WuMing review has a null submitted_at, the agent will select an older review and use stale context…

Agreed on the failure mode — stale context is exactly what this feature must not produce. The ranking key is now (has_stamp, submitted_at, index): stamped reviews rank by timestamp, a null-stamped review ranks newest (preferring it errs toward fresher context), and the forge's oldest-first list order breaks ties. Covered by a test where a null-stamped review must beat an older stamped one.

✅ Fixed — null reply timestamps are included fail-open

src/wuming/context.py

A reply whose created_at is null (normalised to "") is excluded because "" > newest.submitted_at is always false. That can drop a human decline/reply…

Dropping a decline is the worse failure direction, so empty timestamps now pass the filter (created_at == "" is treated as after the wave), with a test pinning it.

✅ Fixed — the fail-open test now asserts the warning

tests/test_main.py

…it does not assert that a warning is logged. The previous wave explicitly asked to assert the warning…

Fair — the wave-1 comment did ask for that and I only covered the continuation half. The test now attaches a handler to the wuming.main logger and asserts the could not load previous review context — continuing without warning is emitted.

### Wave-2 review responses (commit `36deec0`) ### ✅ Fixed — null `submitted_at` no longer sorts a review as oldest [`src/wuming/context.py`](https://forge.marvin8.zone/marvin8/wuming/pulls/109#issuecomment-5831) > `max(..., key=lambda thread: thread.submitted_at)` now treats any review with a null timestamp as the oldest. If the most recent WuMing review has a null `submitted_at`, the agent will select an older review and use stale context… Agreed on the failure mode — stale context is exactly what this feature must not produce. The ranking key is now `(has_stamp, submitted_at, index)`: stamped reviews rank by timestamp, a null-stamped review ranks newest (preferring it errs toward fresher context), and the forge's oldest-first list order breaks ties. Covered by a test where a null-stamped review must beat an older stamped one. --- ### ✅ Fixed — null reply timestamps are included fail-open [`src/wuming/context.py`](https://forge.marvin8.zone/marvin8/wuming/pulls/109#issuecomment-5832) > A reply whose `created_at` is null (normalised to `""`) is excluded because `"" > newest.submitted_at` is always false. That can drop a human decline/reply… Dropping a decline is the worse failure direction, so empty timestamps now pass the filter (`created_at == ""` is treated as after the wave), with a test pinning it. --- ### ✅ Fixed — the fail-open test now asserts the warning [`tests/test_main.py`](https://forge.marvin8.zone/marvin8/wuming/pulls/109#issuecomment-5833) > …it does not assert that a warning is logged. The previous wave explicitly asked to assert the warning… Fair — the wave-1 comment did ask for that and I only covered the continuation half. The test now attaches a handler to the `wuming.main` logger and asserts the `could not load previous review context — continuing without` warning is emitted.

forgejo-actions reviewed

2026-10-17 01:33:16 +00:00

forgejo-actions left a comment

WuMing

Found 1 issue(s). See inline comments below.

## WuMing Found **1** issue(s). See inline comments below.

tests/test_context.py Outdated

					
				@ -0,0 +146,4 @@

				    expect(block).to_contain("> that spans two lines.")

				@test("a null submitted_at review falls back to API ordering")

tests [LOW]

This test claims in its name that a null submitted_at "falls back to API ordering", but the production rule is that a null-stamped review always ranks as the newest (see the (has_stamp, submitted_at, index) key). Since the null-stamped review is also last in the list here, the assertion passes whether the code uses the forge's ordering or the has_stamp rule, so the intended behaviour is not actually pinned. Reorder the inputs so the null-stamped review comes first (e.g. [with_null_stamp, older_stamped]) and/or rename the test to state that a null stamp ranks newest; that variant would fail under a pure positional/timestamp ordering.

**tests** [LOW] This test claims in its name that a null `submitted_at` "falls back to API ordering", but the production rule is that a null-stamped review always ranks as the newest (see the `(has_stamp, submitted_at, index)` key). Since the null-stamped review is also last in the list here, the assertion passes whether the code uses the forge's ordering or the has_stamp rule, so the intended behaviour is not actually pinned. Reorder the inputs so the null-stamped review comes first (e.g. `[with_null_stamp, older_stamped]`) and/or rename the test to state that a null stamp ranks newest; that variant would fail under a pure positional/timestamp ordering.

agent-pi added 1 commit

2026-10-17 01:35:25 +00:00

✅ Pin null-stamp ordering rule with an order-sensitive test

/ gitleaks (pull_request) Successful in 18s

Details

/ checks (pull_request) Successful in 1m51s

Details

/ deploy-docs (pull_request) Has been skipped

Details

/ publish-container (pull_request) Has been skipped

Details

/ pr-review (pull_request) Successful in 5m55s

Details

65a8dd603a

agent-pi commented

2026-10-17 01:35:36 +00:00

Wave-3 review response (commit `65a8dd6`)

✅ Fixed — the ordering test now pins the has-stamp rule

tests/test_context.py

This test claims in its name that a null submitted_at "falls back to API ordering", but the production rule is that a null-stamped review always ranks as the newest… the assertion passes whether the code uses the forge's ordering or the has_stamp rule, so the intended behaviour is not actually pinned.

Correct — with the null-stamped review last, both rules selected it. The inputs are reordered (null-stamped first) and the test renamed: only the has-stamp rule can produce the asserted outcome now, and a comment records why the order matters.

### Wave-3 review response (commit `65a8dd6`) ### ✅ Fixed — the ordering test now pins the has-stamp rule [`tests/test_context.py`](https://forge.marvin8.zone/marvin8/wuming/pulls/109#issuecomment-5840) > This test claims in its name that a null `submitted_at` "falls back to API ordering", but the production rule is that a null-stamped review always ranks as the newest… the assertion passes whether the code uses the forge's ordering or the has_stamp rule, so the intended behaviour is not actually pinned. Correct — with the null-stamped review last, both rules selected it. The inputs are reordered (null-stamped first) and the test renamed: only the has-stamp rule can produce the asserted outcome now, and a comment records why the order matters.

marvin8 approved these changes

2026-10-17 01:51:29 +00:00

marvin8 manually merged commit 6538ba2483 into main

2026-10-17 01:53:07 +00:00

agent-pi referenced this pull request

2026-10-17 02:39:18 +00:00

Convergence controls: review-round cap and low-only wave soft stop #111

Sign in to join this conversation.

No reviewers