From d9efb8be1e86e6d371aa1892fdf6bb30fa2ca91b Mon Sep 17 00:00:00 2001 From: huangruiteng <14976749+huangruiteng@users.noreply.github.com> Date: Thu, 17 Sep 2026 00:35:59 +0800 Subject: [PATCH 1/2] feat(pr-review): require an evidenced goal delivery judgment Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com> --- examples/pr-review-command-smoke.py | 19 ++-- loopx/capabilities/pr_review_queue/README.md | 46 ++++++-- .../pr_review_queue/review_contract.py | 66 +++++++++-- skills/loopx-pr-review/SKILL.md | 6 +- tests/capabilities/test_pr_review_behavior.py | 41 +++++++ tests/capabilities/test_pr_review_contract.py | 1 + .../test_pr_review_result_check.py | 105 +++++++++++++++++- 7 files changed, 251 insertions(+), 33 deletions(-) diff --git a/examples/pr-review-command-smoke.py b/examples/pr-review-command-smoke.py index 6e0bbd1129..258832bf60 100644 --- a/examples/pr-review-command-smoke.py +++ b/examples/pr-review-command-smoke.py @@ -411,13 +411,7 @@ def fake_run_gh_json(args: list[str], *, cwd: Path | None = None) -> object: assert section["word_hint"], section assert section["agent_instruction"], section assert "quota.py" not in section["agent_instruction"], section - assert [section["word_hint"] for section in template["sections"]] == [ - "200-350字", - "300-500字", - "450-800字", - "250-500字", - "150-300字", - ], template + assert all("无最低字数" in section["word_hint"] for section in template["sections"]) concrete_change = next( section for section in template["sections"] if section["label"] == "具体改动" ) @@ -962,6 +956,7 @@ def fake_run_gh_json(args: list[str], *, cwd: Path | None = None) -> object: assert execution["completion_gate"]["metadata_only_verdict_allowed"] is False assert execution["completion_gate"]["stale_head_verdict_allowed"] is False assert execution["completion_gate"]["blocking_evidence_verdicts"] == { + "problem_context": ["off_goal", "fragmented", "not_yet_proven"], "repository_reuse": ["unjustified_duplication", "not_yet_proven"], "observable_semantics": ["unintended_drift", "not_yet_proven"], "change_proportionality": ["disproportionate", "not_yet_proven"], @@ -1100,11 +1095,11 @@ def fake_run_gh_json(args: list[str], *, cwd: Path | None = None) -> object: assert "template below is intentionally blank" in markdown, markdown assert "- 推荐阅读顺序:" in markdown, markdown assert "- 五块模板（留空给 agentloop 填写）:" in markdown, markdown - assert "动机（200-350字）" in markdown, markdown - assert "改动思路（300-500字）" in markdown, markdown - assert "具体改动（450-800字）" in markdown, markdown - assert "对主干的风险（250-500字）" in markdown, markdown - assert "我的整体评价（150-300字）" in markdown, markdown + assert "动机（按证据需要；无最低字数）" in markdown, markdown + assert "改动思路（按证据需要；无最低字数）" in markdown, markdown + assert "具体改动（按证据需要；无最低字数）" in markdown, markdown + assert "对主干的风险（按证据需要；无最低字数）" in markdown, markdown + assert "我的整体评价（按证据需要；无最低字数）" in markdown, markdown assert "main regression risk:" not in markdown, markdown assert "## Combined Review Sequence" in markdown, markdown assert "PR #771" in markdown, markdown diff --git a/loopx/capabilities/pr_review_queue/README.md b/loopx/capabilities/pr_review_queue/README.md index 91d593af03..27dee8fe23 100644 --- a/loopx/capabilities/pr_review_queue/README.md +++ b/loopx/capabilities/pr_review_queue/README.md @@ -309,10 +309,42 @@ progress toward approval by themselves; the reviewer should request the smallest viable fix, deletion, split, or hold when the benefit does not justify the accumulated mechanism. +### Goal-oriented delivery judgment (policy revision 6) + +Every actionable review now extends the existing `problem_context` evidence +with `goal_basis` and a typed `verdict`. This applies to code, docs, maintenance +and test-only changes. Resolve the current user request, issue/task, accepted +contract or demonstrated regression before accepting the author's narrowed +scope. A public roadmap id is optional; do not copy private goal content or +require another repository to use LoopX's S/G/R identifiers. + +| Delivery verdict | Meaning and additional evidence | Approval effect | +| --- | --- | --- | +| `goal_achieved` | Existing before/after, observable outcome and non-goals prove the named task's acceptance; no invented successor required | May approve within that scope, without claiming the parent program complete | +| `justified_increment` | Useful delivered delta, plus `remaining_gap`, `next_step` (owner/dependency) and `boundary_reason` for independent review, verification and rollback | May approve a prerequisite, research, docs or maintenance increment without shipping the entire feature | +| `off_goal` / `fragmented` / `not_yet_proven` | `reason` and `minimum_repair` explain the mismatch, avoidable premature stop or missing evidence | Blocks APPROVE even when other evidence and checks pass | + +Reuse `problem_context.observable_outcome`, `walkthroughs` and +`validation_matrix` instead of writing another evidence report. Small diffs are +not fragmentation; a new field or sent message is not proof of a promised +end-to-end user journey. For collaboration, follow the required dependency, +receiver adoption, artifact acceptance and return through real callers. +Independent prerequisites and characterization remain valid when their boundary +and real successor are justified. Do not reward larger diffs or fabricated +follow-ups. The checker validates declared evidence consistency, not whether a +reviewer's semantic judgment is true, and it never settles a Goal. + +Revision 6 changes review requirements, not queue selection, scheduler, runtime +permissions or the wire schema. Revision-5 results must be regenerated and +reviewed under the new policy before current approval. The five public review +sections remain, but `word_hint` no longer suggests fixed lengths: scale prose +to the change, reuse evidence, and do not pad simple reviews. This capability +is not behind a new feature flag; invoking review uses the installed policy. + ### Semantic alignment and CI constraint recovery -Policy revision 5 replaces the universal detailed semantic review with bounded -triage for code and behavior-bearing policy changes. The required row starts +The semantic triage introduced in policy revision 5 replaces universal detailed +semantic review with bounded triage for code and behavior-bearing policy changes. The required row starts with `checked_scope`, `impact_reason`, and `verdict` (plus the standard evidence `status`). Review the full diff and relevant definitions/callers, then stop at `not_applicable` if no shared contract is affected. No candidate value, separate @@ -575,31 +607,31 @@ absolute paths, private source bodies, or hidden CI artifacts. "sections": [ { "label": "动机", - "word_hint": "200-350字", + "word_hint": "按证据需要；无最低字数", "content": "", "agent_instruction": "解释旧行为、具体痛点、受影响的用户或调用方、目标结果与必要性；说明不合并会继续付出什么代价，以及需求来自活跃调用方还是未来设想。" }, { "label": "改动思路", - "word_hint": "250-450字", + "word_hint": "按证据需要；无最低字数", "content": "", "agent_instruction": "解释所选架构、改动前后的控制流或数据流、所有权边界、关键不变量和替代方案取舍；为不熟悉子系统的读者给出一条正向运行链路。" }, { "label": "具体改动", - "word_hint": "300-600字", + "word_hint": "按证据需要；无最低字数", "content": "", "agent_instruction": "把关键文件和符号映射到行为，覆盖接口、配置或状态、兼容路径、测试与文档；说明各部分如何协作，并给出一个具体输入到输出的例子。" }, { "label": "对主干的风险", - "word_hint": "250-500字", + "word_hint": "按证据需要；无最低字数", "content": "", "agent_instruction": "按严重度列出有文件或符号证据的发现，评估爆炸半径、兼容性、权限、默认副作用、失败与回滚、可观测性和缺失覆盖；策略或生命周期改动必须解释一条负向链路。" }, { "label": "我的整体评价", - "word_hint": "150-300字", + "word_hint": "按证据需要；无最低字数", "content": "", "agent_instruction": "权衡价值与复杂度，列出实际检查或运行的验证，注明审阅的 head SHA，并给出精确结论；若阻塞，说明最小修复和复审所需证据。" } diff --git a/loopx/capabilities/pr_review_queue/review_contract.py b/loopx/capabilities/pr_review_queue/review_contract.py index ac107176ae..7a4170f54c 100644 --- a/loopx/capabilities/pr_review_queue/review_contract.py +++ b/loopx/capabilities/pr_review_queue/review_contract.py @@ -4,7 +4,7 @@ from typing import Any # Increment when review requirements change without changing the packet shape. -REVIEW_POLICY_REVISION = 5 +REVIEW_POLICY_REVISION = 6 REQUIRED_FINAL_SECTIONS = [ "动机", @@ -83,34 +83,34 @@ def build_review_template(item: Mapping[str, Any]) -> dict[str, Any]: "sections": [ _section( "动机", - "200-350字", - "Use evidence `problem_context`: old behavior, affected caller, concrete cost, before/after outcome, and why the nearest smaller fix is or is not enough.", + "按证据需要；无最低字数", + "Use `problem_context`: verified goal basis, old behavior, before/after outcome and delivery verdict. Distinguish completing the scoped goal from a justified increment; explain why this is a complete useful slice, not just why the code works.", ), _section( "改动思路", - "300-500字", + "按证据需要；无最低字数", "Use `architecture_flow`, `repository_reuse`, and `walkthroughs`: entry point, authoritative state, decision boundary, positive path, existing implementation comparison, and ownership trade-off. For introduced or newly enforced state, explain derivation versus irreducible intent and the real producer/trigger, not just its serializer.", ), _section( "具体改动", - "450-800字", + "按证据需要；无最低字数", "Use `changed_line_classification` and `symbol_map`. Code changes require `### 关键代码讲解` for 2-5 behavior-bearing exact-head symbols; docs-only changes use `### 关键内容讲解`.", ), _section( "对主干的风险", - "250-500字", + "按证据需要；无最低字数", "Use `failure_analysis`, `walkthroughs.negative`, and `validation_matrix`; trace each finding from triggering state to observed outcome and minimum repair. When `scope_fit` applies, name the active production caller or explicitly record a coverage-only boundary. When `change_proportionality` applies, compare verified problem impact with mechanism and maintenance cost; a resolved implementation blocker does not justify approval when the full exact-head scope remains disproportionate. For opt-in changes, prove disabled-path parity through `default_off_isolation`; do not infer isolation from an absent feature object. Use `authority_semantics` to verify that public protocol names do not claim a broader actor lifecycle or authority model than the implementation provides. For a `semantic_alignment` contract impact or finding, include a concise `### 语义与 CI 对齐` subsection; ordinary `not_applicable` triage needs no separate subsection. For a blocker, name the current obligation, triggering change, observed evidence, minimum repair and rerun command. Surface typed-state-rule, domain-neutrality, behavior-change-disclosure, and guidance-vs-obligation findings when their evidence applies.", ), _section( "我的整体评价", - "150-300字", + "按证据需要；无最低字数", "Use `observable_semantics` to report baseline/head comparisons and remaining compatibility gaps; equal decision codes are insufficient. Use `code_volume`, `change_proportionality`, `default_off_isolation`, `authority_semantics`, validation results, residual risk, and exact-head freshness to state the verdict and the evidence needed for re-review. For semantic or constraint-related changes, state whether the PR reuses an existing vocabulary, extends one, creates one, stays local, or remains unknown, and link any required registry/RFC/CI repair.", ), ], "review_order": _review_order(key_files), "output_hint": ( "Render the verified structured result using the five sections. " - "The capability-owned review_execution_contract is the evidence and completeness authority." + "The capability-owned review_execution_contract is the evidence and completeness authority. Scale prose to evidence and complexity; simple changes can use one or two sentences per section. Do not repeat evidence or pad to a word count." ), } @@ -126,11 +126,21 @@ def build_review_execution_contract(*, wait_for_ci: bool = True) -> dict[str, An "evidence_status_values": ["verified", "unverified", "not_applicable"], "decision_procedure": { "order": [ + "establish_goal", "challenge_design", "falsify_claims", "inspect_implementation", "reconcile_verdict", ], + "establish_goal": ( + "Resolve the current requested outcome from the user request, issue/task, " + "accepted contract or demonstrated regression. Check changed direction " + "and existing related work before accepting the author's narrowed frame. " + "Use problem_context for one delivery judgment, referencing existing " + "walkthrough/validation evidence rather than another report. A roadmap " + "id is optional; never impose this repository's roadmap on another repo " + "or copy private goals into public review." + ), "challenge_design": ( "Before explaining how the patch works, make the strongest evidence-backed " "case for not shipping it. Compare doing nothing, a smaller fix in the existing " @@ -174,7 +184,15 @@ def build_review_execution_contract(*, wait_for_ci: bool = True) -> dict[str, An { "evidence_id": "problem_context", "required_when": "always", + "verdict_values": [ + "goal_achieved", + "justified_increment", + "off_goal", + "fragmented", + "not_yet_proven", + ], "fields": [ + "goal_basis", "author_claim", "old_behavior", "affected_caller_or_operator", @@ -184,6 +202,32 @@ def build_review_execution_contract(*, wait_for_ci: bool = True) -> dict[str, An "observable_outcome", "non_goals", ], + "fields_by_verdict": { + "justified_increment": [ + "remaining_gap", "next_step", "boundary_reason", + ], + "off_goal": ["reason", "minimum_repair"], + "fragmented": ["reason", "minimum_repair"], + "not_yet_proven": ["reason", "minimum_repair"], + }, + "rule": ( + "Judge the delta against the verified requested outcome, not file/PR/test " + "counts or the author's completion label. goal_achieved closes the named " + "task's acceptance, not an unimplemented parent roadmap. justified_increment " + "requires a real useful delta, the remaining gap, an existing or concrete " + "scoped successor with its owner/dependency, and why this boundary is " + "independently reviewable, testable and reversible. Reuse observable_outcome, " + "walkthroughs and validation_matrix; do not duplicate their evidence. " + "Valid prerequisites, characterization, research findings, documentation " + "and maintenance can qualify without shipping an entire feature or " + "inventing follow-up work for a completed task. Fragmentation means an " + "avoidable stop before the accepted slice's useful outcome, not a small " + "diff. Where applicable, follow dependencies, peer handoff, artifact " + "acceptance and result return through the actual user entrypoints. " + "Missing evidence is not_yet_proven; name the minimum repair. " + "These are reviewer judgments, not automatic semantic detection, Goal " + "settlement, a mandatory roadmap schema or a minimum batch-size policy." + ), }, { "evidence_id": "architecture_flow", @@ -892,6 +936,7 @@ def build_review_execution_contract(*, wait_for_ci: bool = True) -> dict[str, An "exact_head_recheck_required": True, "stale_head_verdict_allowed": False, "blocking_evidence_verdicts": { + "problem_context": ["off_goal", "fragmented", "not_yet_proven"], "repository_reuse": ["unjustified_duplication", "not_yet_proven"], "observable_semantics": ["unintended_drift", "not_yet_proven"], "change_proportionality": [ @@ -912,6 +957,11 @@ def build_review_execution_contract(*, wait_for_ci: bool = True) -> dict[str, An }, "verdict_policy": { "open_pr_blocking_finding": "REQUEST_CHANGES", + "open_pr_unjustified_delivery": ( + "REQUEST_CHANGES when problem_context is off_goal, fragmented or " + "not_yet_proven. Green checks cannot replace an evidenced goal delta; " + "a justified bounded increment need not complete its parent goal." + ), "open_pr_unresolved_semantics": ( "REQUEST_CHANGES when observable_semantics is unintended_drift or " "not_yet_proven; equal decision codes, green suites, stricter checks " diff --git a/skills/loopx-pr-review/SKILL.md b/skills/loopx-pr-review/SKILL.md index b453831a96..7e80800e43 100644 --- a/skills/loopx-pr-review/SKILL.md +++ b/skills/loopx-pr-review/SKILL.md @@ -47,7 +47,7 @@ Do not pipe the only copy through `jq`. When an exhaustive request has `result_completeness.complete=false`, rerun with its `recommended_limit` before reviewing. -Require execution `policy_revision == 5`; a schema name alone is insufficient. +Require execution `policy_revision == 6`; a schema name alone is insufficient. If missing or unequal, do not publish APPROVE; a conservative REQUEST_CHANGES is allowed only when it names the incompatible-policy evidence gap. Do not retain expired temporary worktree overrides. Honor explicit runtime pins, but report @@ -67,8 +67,8 @@ names the contract the change is judged against. Follow `scheduling_policy` and its ranked actionable `review_sequence`; explicit current-request PR selection may override ordering only, never `pull_requests[].review_action_kind` or exact-head idempotency. Generic `re-review`, `重新review`, and `复审` wording selects the named PR; it is not a force-refresh token. Todo/monitor prose may not select work. When `review_action_kind` is null, the row stays in `pull_requests` inventory but must not appear in `review_sequence`; its `review_plan` and `review_template` are null and `evidence_commands` is empty. Do one compact exact-head conclusion readback and report the existing verdict or bounded invalid/missing reason. Run a fresh audit only when the user explicitly requests fresh evidence despite that no-action result, or supplies a concrete new concern/evidence invalidation; regenerate with `--fresh-audit-exact-head NUMBER@HEAD_OID`, then execute the complete current plan and never inherit the earlier approval. For every actionable PR: -1. Record the packet's exact head. Start with the capability's - `review_execution_contract.decision_procedure`, including on re-review; +1. Record the packet's exact head. Follow `review_execution_contract.decision_procedure`, starting with the current goal + and its delivery judgment in `problem_context`, including on re-review; then run `evidence_commands` and relevant repository-native validation. 2. Fill `review_plan.result_template` from the shared execution contract; preserve missing evidence as `unverified`. Execute its repository-reuse, diff --git a/tests/capabilities/test_pr_review_behavior.py b/tests/capabilities/test_pr_review_behavior.py index 73761cbd2c..82ece836d0 100644 --- a/tests/capabilities/test_pr_review_behavior.py +++ b/tests/capabilities/test_pr_review_behavior.py @@ -101,12 +101,53 @@ "APPROVE", "none", ), + ( + { + "request": "Review a team-work delivery slice against its accepted outcome.", + "problem": "The owner needs worker B to consume worker A's accepted artifact after restart.", + "proposal": "Add a handoff status field, serializer and test. The producer and consumer are left to later unspecified PRs; title says peer handoff delivered.", + "evidence": "Serialization tests pass. No runtime path consumes the field; B still cannot see A's result. The same owner could complete the existing bounded exchange path in this slice without new authority.", + }, + "REQUEST_CHANGES", + "architecture", + ), + ( + { + "request": "Review a prerequisite for durable peer handoff, not the whole team feature.", + "problem": "Receiver B loses A's accepted artifact reference on restart.", + "proposal": "Repair the existing persisted reference and independent readback. Automatic wake remains in existing scheduler task #43; the owning scheduler team consumes this contract next.", + "evidence": "Real A→store→B restart and stale-reference negative tests pass; ownership and default behavior are preserved. Separate wake integration has a different retry owner and rollback boundary. Remaining gap is explicitly disclosed; all applicable review evidence verified.", + }, + "APPROVE", + "none", + ), + ( + { + "request": "Review a maintenance change with no product-roadmap id.", + "problem": "A supported release's documented install command is broken.", + "proposal": "Correct the existing command and delete the stale alternative. No new capability or runtime behavior.", + "evidence": "The exact command succeeds from the released package in a clean environment. Documentation links and public-boundary checks pass. The requested repair is complete; no further task is needed.", + }, + "APPROVE", + "none", + ), + ( + { + "request": "Review a correct patch for the current user request.", + "problem": "The user changed priority to restoring lost result delivery, and withdrew the earlier dashboard redesign request.", + "proposal": "Deliver the old dashboard redesign with passing rendering tests and a polished completion report. No result-delivery path is changed.", + "evidence": "The current request and owner correction are available. The author cites only the superseded task. The redesign has no demonstrated prerequisite relationship to restoring delivery.", + }, + "REQUEST_CHANGES", + "architecture", + ), ] def test_decision_procedure_is_in_the_real_packet_before_prose(): response = build_agent_response_contract() assert response["review_execution_contract"]["decision_procedure"]["order"] == [ + "establish_goal", "challenge_design", "falsify_claims", "inspect_implementation", diff --git a/tests/capabilities/test_pr_review_contract.py b/tests/capabilities/test_pr_review_contract.py index 5b4873150d..c907933d02 100644 --- a/tests/capabilities/test_pr_review_contract.py +++ b/tests/capabilities/test_pr_review_contract.py @@ -160,6 +160,7 @@ def test_execution_contract_owns_deep_review_requirements() -> None: assert contract["completion_gate"]["metadata_only_verdict_allowed"] is False assert contract["completion_gate"]["stale_head_verdict_allowed"] is False assert contract["completion_gate"]["blocking_evidence_verdicts"] == { + "problem_context": ["off_goal", "fragmented", "not_yet_proven"], "repository_reuse": ["unjustified_duplication", "not_yet_proven"], "observable_semantics": ["unintended_drift", "not_yet_proven"], "change_proportionality": ["disproportionate", "not_yet_proven"], diff --git a/tests/capabilities/test_pr_review_result_check.py b/tests/capabilities/test_pr_review_result_check.py index b889f4765e..15d07798ea 100644 --- a/tests/capabilities/test_pr_review_result_check.py +++ b/tests/capabilities/test_pr_review_result_check.py @@ -14,11 +14,11 @@ from loopx.cli import main -def _review(): +def _review(*, area="product_runtime"): item = { "number": 42, "head_oid": "a" * 40, - "areas": {"product_runtime": 1}, + "areas": {area: 1}, "review_action_kind": "review_pull_request_exact_head", } result = build_review_plan(item)["result_template"] @@ -228,7 +228,7 @@ def test_nonblocking_suggestion_does_not_force_rejection(): assert check_review_result(packet, result)["approval_consistent"] -@pytest.mark.parametrize("revision", [None, 0, True, "1", 999]) +@pytest.mark.parametrize("revision", [None, 0, True, "1", 5, 999]) def test_old_or_invalid_policy_cannot_certify_current_approval(revision): packet, result = _review() result["review_policy_revision"] = revision @@ -410,3 +410,102 @@ def test_unreadable_check_input_does_not_expose_local_path(tmp_path, capsys): output = capsys.readouterr().out assert str(tmp_path) not in output assert "unreadable" in json.loads(output)["error"] + + +def _delivery_review(*, area="product_runtime", verdict="goal_achieved"): + packet, result = _review(area=area) + result["evidence"]["problem_context"].update( + goal_basis="Public issue #42: interrupted exports must resume without duplicate rows.", + author_claim="Repair export retry on the existing command.", + before_after_scenario="After a lost response, retry returns the original export receipt.", + observable_outcome="The real command reuses the committed receipt; regression fails on base.", + verdict=verdict, + ) + return packet, result + + +@pytest.mark.parametrize("area", ["product_runtime", "public_docs", "test_or_example"]) +@pytest.mark.parametrize("verdict", ["off_goal", "fragmented", "not_yet_proven"]) +def test_green_review_cannot_approve_unjustified_delivery(area, verdict): + packet, result = _delivery_review(area=area, verdict=verdict) + result["evidence"]["problem_context"].update( + reason="The change adds a receipt field but does not repair export retry.", + minimum_repair="Wire and validate the existing retry path in this slice.", + ) + checked = check_review_result(packet, result) + assert not checked["approval_consistent"] + assert "problem_context:blocking_verdict" in checked["approval_blockers"] + result["verdict"] = "REQUEST_CHANGES" + assert check_review_result(packet, result)["ok"] + + +@pytest.mark.parametrize("verdict", [None, "looks_useful", "not_applicable"]) +def test_delivery_judgment_cannot_be_omitted_or_invented(verdict): + packet, result = _delivery_review(verdict=verdict) + checked = check_review_result(packet, result) + assert not checked["approval_consistent"] + assert "problem_context:missing_or_invalid_verdict" in checked["approval_blockers"] + + +def test_goal_basis_is_required_even_for_small_docs_changes(): + packet, result = _delivery_review(area="public_docs") + del result["evidence"]["problem_context"]["goal_basis"] + checked = check_review_result(packet, result) + assert "problem_context:missing_field:goal_basis" in checked["approval_blockers"] + + +@pytest.mark.parametrize("area", ["product_runtime", "public_docs", "test_or_example"]) +def test_qualified_increment_does_not_have_to_finish_the_parent_goal(area): + packet, result = _delivery_review(area=area, verdict="justified_increment") + result["evidence"]["problem_context"].update( + goal_basis="Accepted export-recovery contract; no LoopX roadmap id required.", + observable_outcome="The real writer now preserves the operation id across restart.", + remaining_gap="Automatic retries still need the scheduler integration.", + next_step="Existing recovery task #43 consumes this writer; its owner retains scheduling.", + boundary_reason="Durability is independently testable and revertible; coupling scheduler behavior would obscure this contract.", + ) + assert check_review_result(packet, result)["approval_consistent"] + for field in ("remaining_gap", "next_step", "boundary_reason"): + incomplete = copy.deepcopy(result) + del incomplete["evidence"]["problem_context"][field] + checked = check_review_result(packet, incomplete) + assert f"problem_context:missing_field:{field}" in checked["approval_blockers"] + + +def test_completed_scoped_task_does_not_require_invented_followup(): + packet, result = _delivery_review(area="public_docs") + result["evidence"]["problem_context"].update( + goal_basis="Maintainer request to correct a broken installation command.", + observable_outcome="The documented command succeeds on the supported release.", + non_goals="No claim of implementing the broader product roadmap.", + ) + assert check_review_result(packet, result)["approval_consistent"] + assert not check_review_result(packet, result)["evidence_truth_verified"] + + +def test_cli_rejects_goal_incomplete_approval_without_mutating_packet(tmp_path, capsys): + packet, result = _delivery_review(verdict="fragmented") + result["evidence"]["problem_context"].update( + reason="The new serializer has no production consumer and leaves retry broken.", + minimum_repair="Complete the owning retry transaction or remove the unused serializer.", + ) + packet_path, result_path = tmp_path / "packet.json", tmp_path / "review.json" + packet_path.write_text(json.dumps(packet)) + result_path.write_text(json.dumps(result)) + before = result_path.read_bytes() + main( + [ + "--format", + "json", + "pr-review", + "--check-result", + str(result_path), + "--packet", + str(packet_path), + ] + ) + checked = json.loads(capsys.readouterr().out) + assert not checked["approval_consistent"] + assert "problem_context:blocking_verdict" in checked["approval_blockers"] + assert not checked["external_writes_performed"] + assert result_path.read_bytes() == before From bdc09a546cd786ccd73501d67f1047aac66352cb Mon Sep 17 00:00:00 2001 From: huangruiteng <14976749+huangruiteng@users.noreply.github.com> Date: Thu, 17 Sep 2026 00:35:59 +0800 Subject: [PATCH 2/2] docs(contributing): connect task selection and PR delivery to accepted outcomes Signed-off-by: huangruiteng <14976749+huangruiteng@users.noreply.github.com> --- .github/ISSUE_TEMPLATE/contributor-task.yml | 25 ++++++----- .github/PULL_REQUEST_TEMPLATE.md | 43 +++++++++++++------ AGENTS.md | 40 +++++++++++++++++ CONTRIBUTING.md | 18 +++++++- docs/development/contributor-tasks.md | 36 ++++++++-------- examples/docs-governance-smoke.py | 8 ++-- .../references/repair-patterns.md | 1 + 7 files changed, 122 insertions(+), 49 deletions(-) diff --git a/.github/ISSUE_TEMPLATE/contributor-task.yml b/.github/ISSUE_TEMPLATE/contributor-task.yml index bebe5d957a..fedbdefa7b 100644 --- a/.github/ISSUE_TEMPLATE/contributor-task.yml +++ b/.github/ISSUE_TEMPLATE/contributor-task.yml @@ -41,21 +41,24 @@ body: - type: textarea id: summary attributes: - label: Summary - description: What should change, and why is it useful? + label: Goal and acceptance gap + description: Name the current request or contract, affected user/caller and observable result. Link an existing goal/direction when relevant; a roadmap id is not required. + placeholder: | + Goal/source: + Current gap: + Accepted outcome (before → after): validations: required: true - type: textarea id: scope attributes: label: Proposed scope - description: List the smallest useful slice and any known non-goals. + description: Choose a complete useful slice. State ownership, dependencies, non-goals and any staged remainder; do not split only to minimize file or PR size. placeholder: | - In scope: - - ... - + In scope / owner: + Existing related work / dependencies: Out of scope: - - ... + If staged: useful delta, remaining gap, next owner/task, and why this boundary: validations: required: true - type: input @@ -79,10 +82,12 @@ body: id: validation attributes: label: Validation plan - description: What command or review will prove the task is done? + description: What independently observable result proves completion? Include the actual entrypoint/readback and relevant failure case; passing a test count is insufficient. placeholder: | - - python3 -m py_compile loopx/*.py - - loopx check --scan-root . + Accepted result and independent oracle: + Actual entrypoint / safe command: + Negative or recovery case: + Frontend / Lark / CLI impact or verified N/A: validations: required: true - type: checkboxes diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index d73612d7f0..001182f421 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -1,11 +1,28 @@ -## Summary +## Goal And Delivered Outcome -- + + +- Goal/source and gap: +- Observable before → after, with the validation row that proves it: +- Issue/task and intended base: + +## Scope And Continuation -## Issue Or Task + -- Closes # -- Contributor task ID: +- Completed scope and remaining work: +- Slice boundary / successor: ## Validation @@ -88,16 +105,14 @@ even when the underlying access was authorized. ## Technical Direction - - -- [ ] Core control-plane hardening -- [ ] Long-horizon benchmark evidence -- [ ] Operator surface and IM integration -- [ ] Shared Goal Authority and cross-host coordination -- [ ] Architecture and research incubator + -- Target base branch: -- Direction tracker or promotion unit: +- Direction / acceptance reference, when applicable: ## Shared-authority RFC fixture impact diff --git a/AGENTS.md b/AGENTS.md index e2bb06b182..cb6df8df3f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -1,5 +1,45 @@ # Agent Instructions +## Goal-Oriented Development + +Before selecting non-trivial work, resolve the current requested outcome from +user direction, the linked issue/task, accepted contract or demonstrated bug. +For cross-cutting LoopX work, consult the [overall roadmap](docs/architecture/rfcs/loopx-overall-roadmap-v0.md) +and relevant domain acceptance; do not make every fix wait for every RFC or +invent a roadmap id. Check latest `main`, related PRs and canonical Todos so an +older task description cannot override corrected direction or duplicate work. + +Carry one compact delivery brief from task to PR: goal/source, current gap, +observable result, owning boundary and decisive acceptance evidence. Reuse the +existing task/PR fields; keep private Goal state out of public artifacts. +Choose a complete, independently reviewable and reversible outcome slice. +Small diffs, fields, receipts, test counts and merged PR counts do not establish +progress. Characterization, prerequisites, research, docs and maintenance are +valid when they remove an evidenced gap or enable a named real next step. + +Continue through the selected slice's implementation, integration, negative +cases and readback while authorized work remains feasible. Do not stop after +setup, a serializer, a mock or an isolated smoke when the useful outcome is +still missing. Do not expand scope merely to make a PR larger. When a staged +boundary is necessary, name the delivered delta, remaining gap, next owner/ +dependency and why the boundary improves verification or rollback. Reuse or +update an existing successor; do not create ceremonial follow-up tasks for a +completed request. Real authorization, cost and operational stop gates remain. + +For multi-Agent changes, qualify the relationship the user needs: dependency +artifacts, receiver adoption, claim/lease handling, independent acceptance and +result return as applicable. Sending a message or registering workers does not +prove collaboration. Missing frontend/Lark/CLI companion work makes a product +journey partial even when a backend slice is ready to merge. + +Before delivery, reconcile the result with the original/current goal and +update its task and RFC checkpoint when the boundary changes. Preserve passed, +failed and untested distinctions. If user feedback exposes the same missing +outcome, repair the owning rule, active task or projection through self-repair; +do not merely append stronger instructions. PR review must execute the current +capability-owned `problem_context` delivery judgment; author declarations and +this prose do not certify it or settle a Goal. + ## Commit And PR Hygiene ### Worktree And PR Gate diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 5e2c09ce79..b4adcbee65 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -230,12 +230,26 @@ npm run smoke:demo-readiness Before opening a pull request: - link the issue or task ID when one exists; -- describe the behavior change and the validation you ran; +- state the requested outcome, current gap and observable before/after result; +- distinguish completion of the scoped task from a justified increment; for an + increment, name the remaining gap, next owner/dependency and why the boundary + is independently testable and reversible; +- link decisive validation to that outcome, including relevant user-entrypoint + readback and failure/recovery cases; - keep unrelated formatting or refactors out of the PR; - include docs or tests when changing user-visible behavior; - confirm that no private/local runtime state was committed. -Maintainers may ask for a smaller PR if the change mixes unrelated concerns. +Use the [overall roadmap](docs/architecture/rfcs/loopx-overall-roadmap-v0.md) for +cross-cutting work, without inventing roadmap ids for ordinary fixes. Existing +issues and canonical Todos own execution; update them instead of duplicating +follow-up work. A completed task needs no invented successor. Prerequisites, +research, docs and maintenance can be useful delivered outcomes. A schema, +message, mock or passing suite alone does not complete a promised user journey. + +Maintainers may request consolidation when a useful outcome was unnecessarily +split, or a smaller PR when unrelated concerns were mixed. Review evaluates the +verified goal delta and evidence, not minimum size, model identity or PR count. ### Validation disclosure diff --git a/docs/development/contributor-tasks.md b/docs/development/contributor-tasks.md index 45f38cfcef..79f05a2118 100644 --- a/docs/development/contributor-tasks.md +++ b/docs/development/contributor-tasks.md @@ -17,7 +17,7 @@ into a mirror of maintainer scratch state. | Status | Meaning | | --- | --- | -| Available | Ready for someone to comment on the linked issue or open a small PR. | +| Available | Ready for a contributor to claim the linked outcome and deliver a cohesive PR. | | Claimed | Someone has said they are working on it, or a maintainer assigned it. | | Maintainer-owned | Active work is happening in maintainer/local automation; ask before touching. | | Needs design | Discussion is welcome, but implementation needs agreement first. | @@ -34,7 +34,7 @@ preferred review contact is not an exclusive task claim or new merge authority. 1. Prefer a linked GitHub issue. If there is no issue yet, open one with the contributor task template. 2. Comment that you would like to work on the task. Maintainers will mark it - `claimed` or suggest a smaller slice. + `claimed` or agree a complete, independently verifiable slice. 3. For docs-only typo fixes or obviously tiny cleanups, opening a direct PR is fine. 4. If a claimed task has no update for 14 days, maintainers may release it back @@ -44,21 +44,19 @@ preferred review contact is not an exclusive task claim or new merge authority. ## Current Technical Directions -The canonical [Technical Directions map](../project/technical-directions.md) -explains outcomes, maturity, ownership boundaries, and promotion gates. This -board lists bounded work; it does not redefine those directions. +The [overall roadmap](../architecture/rfcs/loopx-overall-roadmap-v0.md) and +[tracking issue #4574](https://github.com/huangruiteng/loopx/issues/4574) own +cross-domain priorities and G0–G5 acceptance. The [Technical Directions map](../project/technical-directions.md) +owns contributor routing and current maturity; this board does not keep a second +copy of those stages. Before claiming a row, reconcile its linked task with +latest main, related PRs and the roadmap. Historical rows are not proof that a +missing feature remains unimplemented or a proposed slice is still useful. -| Direction | Current stage | Contributor entry | Boundary | -| --- | --- | --- | --- | -| Long-Horizon Benchmarks and Evidence | Active research | [#3243](https://github.com/huangruiteng/loopx/issues/3243) | Work on public-safe fixtures, treatment integrity, reducers, and docs; live cases and scoring remain maintainer-owned. | -| Operator Surface and IM Integration | Incubating on `frontend-control-plane-im-prototype-rfc` | [#3244](https://github.com/huangruiteng/loopx/issues/3244) | State the target base branch; UI remains a projection and promotion to `main` is staged. | -| Shared Goal Authority and Cross-host Coordination | Stage 2 slice shipped (aggregate head, file provider, `claim_work` executor); NoKV stays an unpromoted candidate | [#3245](https://github.com/huangruiteng/loopx/issues/3245) | Keep slices provider-neutral and file-backed; no second scheduler or write authority. | -| Architecture and Research Incubator | Mixed by RFC | [#3246](https://github.com/huangruiteng/loopx/issues/3246) | Read the per-exploration stage; an RFC alone does not make implementation claimable. | - -Core control-plane reliability remains the shared shipped foundation. Effect -Program hardening, verified transitions, recovery, observability, -maintainability, and contributor experience continue through the focused rows -below and the existing `control-plane` label. +A claimable task names the current gap, independently useful outcome, existing +owner/caller, dependencies and decisive validation. For a staged increment, +record the remaining gap and next owner/task; do not make a field, fixture or +PR count the completion target. Preserve existing authoritative Todo/issue +identity rather than copying the whole plan here. ## Priority Queue @@ -66,9 +64,9 @@ below and the existing `control-plane` label. | --- | --- | --- | --- | --- | | P0 | Core hardening | Exact-head review of remote execution and terminal writeback fencing: fenced journal recovery absorbed into TypeScript | #3074 | Done | | P0 | Core hardening | Wire caller-approved `validation_command` into the remaining self-report entry points | #3082 / #3142 #3291 #3343 | Done | -| P1 | Benchmark evidence | Split one deterministic adapter-fidelity or treatment-integrity fixture | #3243 | Needs design | -| P1 | Operator surface / IM | Split one projection or session-contract characterization unit from the incubation branch | #3244 | Needs design | -| P1 | Shared coordination | Characterize the shipped file-backed `claim_work` executor with a provider-neutral parity fixture | #3700 / #3245 | Needs design | +| P1 | Benchmark evidence | Qualify a reproducible adapter-fidelity or treatment-integrity gap with existing focused fixtures | #3243 | Needs design | +| P0 | Operator surface / IM | Close the R1 confirmed-team commitment/readback gap, then qualify R2 real peer dependency handoff | #4574 / #4339 | Needs design | +| P1 | Shared coordination | Qualify the selected local authority durability and crash/replay boundary against existing D2 acceptance | #4224 / #3245 | Needs design | | P1 | Core hardening | One budget-aware CLI output ergonomics slice | #2881 | Needs design | | P2 | Project docs | Release docs install, activation, and recovery guidance through v0.5.4 | GH-C04 | Landed via #3982 | | P2 | Maintainability | CLI ownership and hot-module extraction | GH-C06 | Available | diff --git a/examples/docs-governance-smoke.py b/examples/docs-governance-smoke.py index e8e437a874..b79788be8d 100644 --- a/examples/docs-governance-smoke.py +++ b/examples/docs-governance-smoke.py @@ -513,11 +513,11 @@ def assert_technical_direction_governance_is_current() -> None: assert required in rfc_index, required assert "## Status matrix" not in rfc_index + # The task board routes to canonical direction/roadmap owners instead of + # duplicating their mutable maturity table. for required in ( - "Long-Horizon Benchmarks and Evidence", - "Operator Surface and IM Integration", - "Shared Goal Authority and Cross-host Coordination", - "Architecture and Research Incubator", + "../project/technical-directions.md", + "../architecture/rfcs/loopx-overall-roadmap-v0.md", ): assert required in tasks, required diff --git a/skills/loopx-self-repair/references/repair-patterns.md b/skills/loopx-self-repair/references/repair-patterns.md index 7fb1aa78ca..3617d663ba 100644 --- a/skills/loopx-self-repair/references/repair-patterns.md +++ b/skills/loopx-self-repair/references/repair-patterns.md @@ -205,6 +205,7 @@ teaches a reusable control-plane lesson. | `dashboard_verified_mutation_projection_gap` | A preview-locked dashboard mutation reports a successful shared-state readback, but the initiating control still shows its old value; a second click then says the requested setting already exists. | Exact apply receipt, no-change canonical preview, shared-state readback verification, status projection generation/revision, rendered control state, and refresh outcome. | The data adapter verified the canonical write or no-change state, then the UI discarded that receipt and rebound immediately to a separate stale status projection. | Return the verified configuration through apply and no-change preview callbacks and use it as a drawer-scoped read model for the same Goal; refresh the normal projection independently, surface refresh failure without undoing the verified result, and clear the override when the drawer selection changes. Keep preview state visibly pending rather than presenting it as applied. Cover a deliberately stale status response in the browser smoke. | | `dashboard_open_token_picker_gap` | A bounded dashboard setting asks users to type protocol tokens such as `task_domain`; the placeholder looks like a current value, users cannot discover legal choices, or Goals without tagged work cannot enable a capability whose runtime treats the token filter as optional. | Current Goal Todo index, configured token allowlist, option-to-Todo match counts, canonical empty-filter semantics, preview payload, empty state, and packaged browser behavior. | An intentionally open optional backend vocabulary was exposed as a required product authority boundary; reading only a compact Goal-card Todo slice can also hide valid choices. | Keep the open typed token contract in the owning control-plane boundary, but present it as an optional per-Goal multi-select derived from the authoritative Todo index plus already configured values, using compact Goal Todo rows only as a compatibility fallback. Empty means no token filter while every independent admission boundary remains enforced; a non-empty selection remains a strict allowlist. Show match counts, preserve configured zero-match values, and cover unrestricted, restricted, invalid-token, preview, readback, empty-state, and packaged parity behavior. | | `app_host_heartbeat_identity_gap` | An App host can create recurring automations, but LoopX classifies it as a generic visible CLI loop; the agent may complete one phase and then stop because no host-owned successor wake is activated. Another form builds the right full scheduler packet but drops it at a compact Turn-envelope boundary that still reads a sibling App's legacy field. | Exact App versus CLI host surface, ambient thread id, runtime profile, activation packet, full and compact scheduler projections, scheduler ownership, settled-turn liveness, and terminal no-follow-up evidence. | Host capability existed, but LoopX modeled only the sibling CLI surface or left a downstream transport coupled to that sibling's packet name, so the App automation contract or successor wake did not survive the real execution path. | Add a distinct App host/runtime identity while reusing the provider-neutral `app_automation` cadence and ACK rules end to end. Bind the host's ambient thread id, preserve the packet through Turn compaction without a sibling-host alias, create/update the host automation after Todo writeback, keep settled non-terminal turns active for a fresh successor turn, and stop only on validated terminal no-follow-up. Preserve the CLI host and its native visible Goal path. Never reuse another App's local-store fallback. | +| `artifact_without_goal_delta` | Repeated individually valid changes leave the requested user outcome or peer handoff unqualified; completion reports count fields, receipts or PRs. | Current user/task acceptance, latest main and related work, actual caller/readback, `problem_context` delivery judgment and remaining dependency. | Work was selected and settled around implementation artifacts rather than an independently useful outcome slice. | Reconcile the accepted goal, consolidate the missing integration/negative/readback work, or justify a prerequisite with its real successor and owner. Update the existing task/vision through its owner when direction changed. Do not repair by minimum LOC/PR quotas, a second task ledger, fabricated follow-ups or stronger prose alone. | ### Runtime diagnostic drift in parity fixtures