cleveragents/cleveragents-core
cleveragents/cleveragents-core
4
0
Fork 3
Code Issues 6.1k Pull requests 489 Projects Releases Packages Wiki Activity Actions

test(integration): workflow example 1 — Hello World, fix a single bug (manual profile) #798

Closed
brent.edwards wants to merge 2 commits from test/int-wf01-hello-world into master
pull from: test/int-wf01-hello-world
merge into: cleveragents:master
Conversation 17 Commits 2 Files changed 6 +1019 -26
brent.edwards commented 2026-03-13 00:34:15 +00:00
Member
Copy link

Summary

Integration test for Specification Workflow Example 1: Hello World — Fix a Single Bug. Exercises the manual automation profile through the complete plan lifecycle using mocked LLM providers.

Test Cases (9 Robot Framework tests)

  1. WF01 Init Workspace — agents init --yes workspace setup
  2. WF01 Register Git Checkout Resource — agents resource add git-checkout with a bare git repo
  3. WF01 Create Project With Invariant — agents project create with --resource and --invariant
  4. WF01 Create Action From YAML — agents action create --config with automation_profile: manual
  5. WF01 Full Plan Lifecycle Manual Profile — Complete plan use → plan execute → plan tree → plan diff → plan lifecycle-apply sequence with manual profile
  6. WF01 Plan State Transitions — Verifies plan starts in strategize/queued phase
  7. WF01 Tree And Explain Output Structure — Verifies plan tree --format json runs without crash
  8. WF01 Diff Output — Verifies plan diff produces no internal errors
  9. WF01 Post Apply Commit Exists — Verifies GitWorktreeSandbox.commit() creates a git commit in the target repo

All tests use real CLI subprocess invocations (python -m cleveragents ...) with CLEVERAGENTS_TESTING_USE_MOCK_AI=true for integration-appropriate LLM mocking.

Quality Gates

  • Typecheck ✅ (0 errors) | Unit tests 10,700/10,700 ✅ | Integration tests 9/9 ✅ | Lint ✅

Closes #765

## Summary Integration test for Specification Workflow Example 1: Hello World — Fix a Single Bug. Exercises the `manual` automation profile through the complete plan lifecycle using mocked LLM providers. ## Test Cases (9 Robot Framework tests) 1. **WF01 Init Workspace** — `agents init --yes` workspace setup 2. **WF01 Register Git Checkout Resource** — `agents resource add git-checkout` with a bare git repo 3. **WF01 Create Project With Invariant** — `agents project create` with `--resource` and `--invariant` 4. **WF01 Create Action From YAML** — `agents action create --config` with `automation_profile: manual` 5. **WF01 Full Plan Lifecycle Manual Profile** — Complete `plan use` → `plan execute` → `plan tree` → `plan diff` → `plan lifecycle-apply` sequence with manual profile 6. **WF01 Plan State Transitions** — Verifies plan starts in `strategize/queued` phase 7. **WF01 Tree And Explain Output Structure** — Verifies `plan tree --format json` runs without crash 8. **WF01 Diff Output** — Verifies `plan diff` produces no internal errors 9. **WF01 Post Apply Commit Exists** — Verifies `GitWorktreeSandbox.commit()` creates a git commit in the target repo All tests use real CLI subprocess invocations (`python -m cleveragents ...`) with `CLEVERAGENTS_TESTING_USE_MOCK_AI=true` for integration-appropriate LLM mocking. ## Quality Gates - Typecheck ✅ (0 errors) | Unit tests 10,700/10,700 ✅ | Integration tests 9/9 ✅ | Lint ✅ Closes #765
test(integration): workflow example 1 — Hello World, fix a single bug (manual profile)
Some checks failed
CI / benchmark-publish (pull_request) Has been skipped
Details
CI / lint (pull_request) Successful in 15s
Details
CI / build (pull_request) Successful in 15s
Details
CI / quality (pull_request) Successful in 19s
Details
CI / e2e_tests (pull_request) Successful in 34s
Details
CI / security (pull_request) Successful in 37s
Details
CI / typecheck (pull_request) Successful in 40s
Details
CI / unit_tests (pull_request) Successful in 3m2s
Details
CI / docker (pull_request) Successful in 43s
Details
CI / integration_tests (pull_request) Successful in 3m58s
Details
CI / coverage (pull_request) Successful in 6m48s
Details
CI / benchmark-regression (pull_request) Failing after 26m45s
Details
8e17a5a8ec 
Add Robot Framework integration test suite exercising the complete
manual-profile workflow with mocked LLM providers:

- agents init workspace setup
- resource add git-checkout registration
- project create with linked resource and invariant
- action create from YAML config (manual automation profile)
- plan use -> plan execute -> plan tree -> plan diff -> plan lifecycle-apply
- plan state transition verification (strategize/queued phase)
- plan tree/explain output structure validation
- plan diff output validation (no internal errors)
- post-apply sandbox commit verification via GitWorktreeSandbox

9 Robot Framework test cases, all using real CLI subprocess invocations
with CLEVERAGENTS_TESTING_USE_MOCK_AI=true for integration-appropriate
LLM mocking.

ISSUES CLOSED: #765
brent.edwards added this to the v3.0.0 milestone 2026-03-13 00:34:27 +00:00
hurui200320 requested changes 2026-03-13 04:52:44 +00:00
Dismissed
hurui200320 left a comment
Copy link

PR #798 Code Review — test(integration): workflow example 1 — Hello World, fix a single bug (manual profile)

Reviewer: Rui (hurui200320)
Verdict: Request Changes

No existing PR comments or reviews to address. One implementation-notes comment from @brent.edwards on ticket #765 was reviewed.

Critical Issues

C1. plan explain is claimed but never tested

  • File: robot/helper_wf01_hello_world.py:472-542
  • The function wf01_tree_explain_output() docstring says "Verify plan tree and plan explain produce structured output," and the Robot test case is named WF01 Tree And Explain Output Structure. However, only plan tree is invoked — there is zero plan explain invocation anywhere in the file. Ticket #765 acceptance criterion: "Assertions verify plan tree and plan explain output structure."
  • Fix: Add a plan explain invocation (even if it returns a graceful "no decisions" message under mock AI), or rename the function/test to accurately reflect what is tested.

C2. validation add + validation attach are completely missing

  • File: robot/helper_wf01_hello_world.py (entire file)
  • The specification's Workflow Example 1 (spec line 36167-36188) explicitly includes agents validation add --config validations/unit-tests.yaml and agents validation attach --project local/api-service local/unit-tests. Neither command is invoked anywhere. Ticket #765 Background section lists "validation registration" in the expected command sequence. The test uses --invariant on project create (line 189), which is a different mechanism exercising different code paths.
  • Fix: Add a test step (and Robot test case) for agents validation add and agents validation attach --project, matching the spec workflow.

C3. (Pre-existing) init_bare_git_repo() branch mismatch risk

  • File: robot/helper_e2e_common.py:132-133, consumed by helper_wf01_hello_world.py:140-141
  • init_bare_git_repo() calls git init without -b main. On systems where init.defaultBranch is not configured, the default branch will be master. But every test registers the resource with --branch main. This is a pre-existing bug in shared code (also affects helper_m1_e2e_verification.py), not introduced by this PR.
  • Fix (shared code): Change git init to git init -b main in init_bare_git_repo(). Not blocking for this PR, but noting as a latent risk.

Major Issues

M1. Missing --arg parameters on plan use

  • File: robot/helper_wf01_hello_world.py:288-298
  • The spec shows plan use called with --arg bug_description="..." and --arg affected_file="...". The action in the spec defines bug_description as a required argument. All plan use invocations in the test omit --arg entirely.
  • Fix: Add "--arg", "bug_description=Fix the hello-world greeting function" to the plan use calls.

M2. Setup commands don't check return codes (4 functions)

  • File: robot/helper_wf01_hello_world.py:260-285, 396-421, 481-506, 560-585
  • In wf01_plan_lifecycle(), wf01_plan_state_transitions(), wf01_tree_explain_output(), and wf01_diff_output(), the setup run_cli() calls for resource add, project create, and action create don't check return codes. If any fail silently, downstream commands produce confusing errors like "could not extract plan_id." Compare with the earlier standalone tests in the same file (e.g., wf01_project_create() at line 177) and helper_m1_e2e_verification.py:324-335, which both check return codes.
  • Fix: Add if r.returncode != 0: _fail(...) after each setup call.

M3. plan diff doesn't verify changeset content

  • File: robot/helper_wf01_hello_world.py:550-616
  • Ticket: "Assertions verify plan diff shows expected changeset." The test only checks for absence of INTERNAL/Traceback in output. The docstring (line 551-554) acknowledges this gap.
  • Fix: At minimum, assert on the specific "not ready" message or return code rather than just absence of tracebacks. Document the mock AI limitation explicitly in the test.

M4. Plan state transitions test only checks a single state

  • File: robot/helper_wf01_hello_world.py:386-464
  • Ticket: "Assertions verify plan state transitions through full lifecycle." The function creates a plan and checks it's in strategize/queued — a single state check, not a transition. The function name wf01_plan_state_transitions and Robot test name imply multi-step state verification.
  • Fix: Either run plan execute and re-check status to verify a transition, or rename to accurately describe what is tested (e.g., wf01_plan_initial_state).

M5. File exceeds 500-line limit (715 lines)

  • File: robot/helper_wf01_hello_world.py
  • CONTRIBUTING.md: "Keep files under 500 lines." The file is 715 lines, 43% over the limit. Root cause: ~80+ lines of duplicated setup blocks across 4 plan-lifecycle test functions.
  • Fix: Extract the repeated setup into a _WorkflowCtx.setup_with_plan() -> str method. This would reduce the file to ~550-580 lines. For full compliance, split the plan-lifecycle tests into a separate module.

M6. Inline imports inside function body

  • File: robot/helper_wf01_hello_world.py:630-634
  • import subprocess and from cleveragents.infrastructure.sandbox.git_worktree import GitWorktreeSandbox are imported inside wf01_post_apply_commit(). CONTRIBUTING.md requires all imports at the top of the file.
  • Fix: Move both imports to the module-level import block.

Minor Issues

m1. ULID regex overly permissive

  • File: robot/helper_wf01_hello_world.py:56
  • _ULID_RE = re.compile(r"\b([0-9A-Z]{26})\b") accepts I, L, O, U which are not valid Crockford Base32 characters. Pre-existing pattern from helper_m1_e2e_verification.py. Low practical risk but could match non-ULID strings.
  • Fix: Tighten to r"\b([0-9A-HJKMNP-TV-Z]{26})\b".

m2. wf01_post_apply_commit skips workspace setup

  • File: robot/helper_wf01_hello_world.py:624-681
  • Unlike all other test functions, this one doesn't create a _WorkflowCtx or call setup_workspace(). Currently functional since GitWorktreeSandbox doesn't need CLEVERAGENTS_HOME, but breaks the pattern.
  • Fix: Add a comment explaining the intentional omission, or add workspace setup for consistency.

m3. No true end-to-end sequential pipeline test

  • File: robot/wf01_hello_world.robot
  • Each test creates/destroys its own workspace, so the full spec workflow (init -> resource -> project -> validation -> action -> plan lifecycle -> apply) is never tested as a single connected flow. The plan-lifecycle helper sets up resource+project+action but skips init and validation.
  • Fix: Consider adding one additional test case that exercises the entire workflow as a single connected sequence.

m4. Redundant setup across 4 plan tests (performance)

  • File: robot/helper_wf01_hello_world.py:260-285, 396-421, 481-506, 560-585
  • 4 plan tests each independently run resource add -> project create -> action create -> plan use, resulting in ~12 redundant subprocess spawns and 3 extra Alembic migration runs.
  • Fix: Extract shared setup or consolidate tests that share identical prerequisites. Tests 7 (tree-explain-output) and 8 (diff-output) are strict subsets of test 5 (plan-lifecycle), which already calls both plan tree and plan diff.

Nitpick

n1. init_bare_git_repo() name is misleading — creates a non-bare repo (pre-existing in shared code).

n2. Automation profile not verified after plan use — test doesn't assert that the plan actually received the manual profile.

n3. Suite documentation overstates test scope — Robot file header claims to exercise plan explain and validation, but these aren't tested.

Security

No significant security issues found. The code uses safe subprocess invocation (list form, no shell=True), secure temp file creation (tempfile.mkdtemp/mkstemp), and proper cleanup in try/finally blocks. No hardcoded credentials or information disclosure risks.

Systemic Note (Not blocking for this PR)

The use of CLEVERAGENTS_TESTING_USE_MOCK_AI=true technically contradicts CONTRIBUTING.md's rule that "Mocks...are permitted only in unit tests. Integration tests must exercise real services." However, this is an established project-wide pattern used by 10+ existing Robot test suites. This PR correctly follows the established pattern. Recommend filing a follow-up ticket to either update CONTRIBUTING.md or plan elimination of the mock flag from production code.

Summary

# Severity Issue
C1 Critical plan explain never invoked despite test name claiming it
C2 Critical validation add + validation attach completely missing
C3 Critical Branch mismatch risk in init_bare_git_repo() (pre-existing)
M1 Major --arg parameters not passed to plan use
M2 Major 4 functions don't check setup return codes
M3 Major plan diff doesn't verify changeset content
M4 Major State transitions test checks only one state
M5 Major File size 715 > 500 line limit
M6 Major Inline imports in function body
m1-m4 Minor ULID regex, missing workspace setup, no pipeline test, redundant setup
n1-n3 Nitpick Naming, profile verification, documentation accuracy

Recommendation: Request changes. The test suite is well-structured and follows established patterns, but has significant spec compliance gaps (C1, C2, M1) and code quality issues (M2, M5, M6) that should be addressed before merge. The pre-existing branch mismatch (C3) should be noted as a follow-up.

## PR #798 Code Review — `test(integration): workflow example 1 — Hello World, fix a single bug (manual profile)` **Reviewer:** Rui (hurui200320) **Verdict: Request Changes** No existing PR comments or reviews to address. One implementation-notes comment from @brent.edwards on ticket #765 was reviewed. --- ### Critical Issues **C1. `plan explain` is claimed but never tested** - **File:** `robot/helper_wf01_hello_world.py:472-542` - The function `wf01_tree_explain_output()` docstring says "Verify plan tree and plan explain produce structured output," and the Robot test case is named `WF01 Tree And Explain Output Structure`. However, only `plan tree` is invoked — there is zero `plan explain` invocation anywhere in the file. Ticket #765 acceptance criterion: "Assertions verify `plan tree` and `plan explain` output structure." - **Fix:** Add a `plan explain` invocation (even if it returns a graceful "no decisions" message under mock AI), or rename the function/test to accurately reflect what is tested. **C2. `validation add` + `validation attach` are completely missing** - **File:** `robot/helper_wf01_hello_world.py` (entire file) - The specification's Workflow Example 1 (spec line 36167-36188) explicitly includes `agents validation add --config validations/unit-tests.yaml` and `agents validation attach --project local/api-service local/unit-tests`. Neither command is invoked anywhere. Ticket #765 Background section lists "validation registration" in the expected command sequence. The test uses `--invariant` on `project create` (line 189), which is a different mechanism exercising different code paths. - **Fix:** Add a test step (and Robot test case) for `agents validation add` and `agents validation attach --project`, matching the spec workflow. **C3. (Pre-existing) `init_bare_git_repo()` branch mismatch risk** - **File:** `robot/helper_e2e_common.py:132-133`, consumed by `helper_wf01_hello_world.py:140-141` - `init_bare_git_repo()` calls `git init` without `-b main`. On systems where `init.defaultBranch` is not configured, the default branch will be `master`. But every test registers the resource with `--branch main`. This is a pre-existing bug in shared code (also affects `helper_m1_e2e_verification.py`), not introduced by this PR. - **Fix (shared code):** Change `git init` to `git init -b main` in `init_bare_git_repo()`. Not blocking for this PR, but noting as a latent risk. --- ### Major Issues **M1. Missing `--arg` parameters on `plan use`** - **File:** `robot/helper_wf01_hello_world.py:288-298` - The spec shows `plan use` called with `--arg bug_description="..."` and `--arg affected_file="..."`. The action in the spec defines `bug_description` as a required argument. All `plan use` invocations in the test omit `--arg` entirely. - **Fix:** Add `"--arg", "bug_description=Fix the hello-world greeting function"` to the `plan use` calls. **M2. Setup commands don't check return codes (4 functions)** - **File:** `robot/helper_wf01_hello_world.py:260-285, 396-421, 481-506, 560-585` - In `wf01_plan_lifecycle()`, `wf01_plan_state_transitions()`, `wf01_tree_explain_output()`, and `wf01_diff_output()`, the setup `run_cli()` calls for `resource add`, `project create`, and `action create` don't check return codes. If any fail silently, downstream commands produce confusing errors like "could not extract plan_id." Compare with the earlier standalone tests in the same file (e.g., `wf01_project_create()` at line 177) and `helper_m1_e2e_verification.py:324-335`, which both check return codes. - **Fix:** Add `if r.returncode != 0: _fail(...)` after each setup call. **M3. `plan diff` doesn't verify changeset content** - **File:** `robot/helper_wf01_hello_world.py:550-616` - Ticket: "Assertions verify `plan diff` shows expected changeset." The test only checks for absence of `INTERNAL`/`Traceback` in output. The docstring (line 551-554) acknowledges this gap. - **Fix:** At minimum, assert on the specific "not ready" message or return code rather than just absence of tracebacks. Document the mock AI limitation explicitly in the test. **M4. Plan state transitions test only checks a single state** - **File:** `robot/helper_wf01_hello_world.py:386-464` - Ticket: "Assertions verify plan state transitions through full lifecycle." The function creates a plan and checks it's in `strategize/queued` — a single state check, not a transition. The function name `wf01_plan_state_transitions` and Robot test name imply multi-step state verification. - **Fix:** Either run `plan execute` and re-check status to verify a transition, or rename to accurately describe what is tested (e.g., `wf01_plan_initial_state`). **M5. File exceeds 500-line limit (715 lines)** - **File:** `robot/helper_wf01_hello_world.py` - CONTRIBUTING.md: "Keep files under 500 lines." The file is 715 lines, 43% over the limit. Root cause: ~80+ lines of duplicated setup blocks across 4 plan-lifecycle test functions. - **Fix:** Extract the repeated setup into a `_WorkflowCtx.setup_with_plan() -> str` method. This would reduce the file to ~550-580 lines. For full compliance, split the plan-lifecycle tests into a separate module. **M6. Inline imports inside function body** - **File:** `robot/helper_wf01_hello_world.py:630-634` - `import subprocess` and `from cleveragents.infrastructure.sandbox.git_worktree import GitWorktreeSandbox` are imported inside `wf01_post_apply_commit()`. CONTRIBUTING.md requires all imports at the top of the file. - **Fix:** Move both imports to the module-level import block. --- ### Minor Issues **m1. ULID regex overly permissive** - **File:** `robot/helper_wf01_hello_world.py:56` - `_ULID_RE = re.compile(r"\b([0-9A-Z]{26})\b")` accepts I, L, O, U which are not valid Crockford Base32 characters. Pre-existing pattern from `helper_m1_e2e_verification.py`. Low practical risk but could match non-ULID strings. - **Fix:** Tighten to `r"\b([0-9A-HJKMNP-TV-Z]{26})\b"`. **m2. `wf01_post_apply_commit` skips workspace setup** - **File:** `robot/helper_wf01_hello_world.py:624-681` - Unlike all other test functions, this one doesn't create a `_WorkflowCtx` or call `setup_workspace()`. Currently functional since `GitWorktreeSandbox` doesn't need `CLEVERAGENTS_HOME`, but breaks the pattern. - **Fix:** Add a comment explaining the intentional omission, or add workspace setup for consistency. **m3. No true end-to-end sequential pipeline test** - **File:** `robot/wf01_hello_world.robot` - Each test creates/destroys its own workspace, so the full spec workflow (init -> resource -> project -> validation -> action -> plan lifecycle -> apply) is never tested as a single connected flow. The `plan-lifecycle` helper sets up resource+project+action but skips init and validation. - **Fix:** Consider adding one additional test case that exercises the entire workflow as a single connected sequence. **m4. Redundant setup across 4 plan tests (performance)** - **File:** `robot/helper_wf01_hello_world.py:260-285, 396-421, 481-506, 560-585` - 4 plan tests each independently run `resource add` -> `project create` -> `action create` -> `plan use`, resulting in ~12 redundant subprocess spawns and 3 extra Alembic migration runs. - **Fix:** Extract shared setup or consolidate tests that share identical prerequisites. Tests 7 (`tree-explain-output`) and 8 (`diff-output`) are strict subsets of test 5 (`plan-lifecycle`), which already calls both `plan tree` and `plan diff`. --- ### Nitpick **n1. `init_bare_git_repo()` name is misleading** — creates a non-bare repo (pre-existing in shared code). **n2. Automation profile not verified after `plan use`** — test doesn't assert that the plan actually received the `manual` profile. **n3. Suite documentation overstates test scope** — Robot file header claims to exercise `plan explain` and validation, but these aren't tested. --- ### Security No significant security issues found. The code uses safe subprocess invocation (list form, no `shell=True`), secure temp file creation (`tempfile.mkdtemp`/`mkstemp`), and proper cleanup in `try/finally` blocks. No hardcoded credentials or information disclosure risks. --- ### Systemic Note (Not blocking for this PR) The use of `CLEVERAGENTS_TESTING_USE_MOCK_AI=true` technically contradicts CONTRIBUTING.md's rule that "Mocks...are permitted only in unit tests. Integration tests must exercise real services." However, this is an established project-wide pattern used by 10+ existing Robot test suites. This PR correctly follows the established pattern. Recommend filing a follow-up ticket to either update CONTRIBUTING.md or plan elimination of the mock flag from production code. --- ### Summary | # | Severity | Issue | |---|----------|-------| | C1 | Critical | `plan explain` never invoked despite test name claiming it | | C2 | Critical | `validation add` + `validation attach` completely missing | | C3 | Critical | Branch mismatch risk in `init_bare_git_repo()` (pre-existing) | | M1 | Major | `--arg` parameters not passed to `plan use` | | M2 | Major | 4 functions don't check setup return codes | | M3 | Major | `plan diff` doesn't verify changeset content | | M4 | Major | State transitions test checks only one state | | M5 | Major | File size 715 > 500 line limit | | M6 | Major | Inline imports in function body | | m1-m4 | Minor | ULID regex, missing workspace setup, no pipeline test, redundant setup | | n1-n3 | Nitpick | Naming, profile verification, documentation accuracy | **Recommendation:** Request changes. The test suite is well-structured and follows established patterns, but has significant spec compliance gaps (C1, C2, M1) and code quality issues (M2, M5, M6) that should be addressed before merge. The pre-existing branch mismatch (C3) should be noted as a follow-up.
fix(test): address Critical/Major review findings for WF01 integration tests
All checks were successful
CI / lint (pull_request) Successful in 15s
Details
CI / benchmark-publish (pull_request) Has been skipped
Details
CI / quality (pull_request) Successful in 16s
Details
CI / typecheck (pull_request) Successful in 33s
Details
CI / e2e_tests (pull_request) Successful in 24s
Details
CI / build (pull_request) Successful in 19s
Details
CI / security (pull_request) Successful in 42s
Details
CI / unit_tests (pull_request) Successful in 2m20s
Details
CI / docker (pull_request) Successful in 34s
Details
CI / integration_tests (pull_request) Successful in 3m11s
Details
CI / coverage (pull_request) Successful in 5m52s
Details
CI / benchmark-regression (pull_request) Successful in 34m19s
Details
9606d52f56 
- C1: Add plan explain invocation to wf01_tree_explain_output();
  uses synthetic decision ULID and verifies graceful error (no traceback)
- C2: Add wf01_validation_register() test exercising validation add
  and validation attach --project per spec §1d-e
- C3: Fix init_bare_git_repo() to use 'git init -b main' preventing
  branch mismatch when init.defaultBranch is not configured
- M1: Add --arg bug_description=... to all plan use calls; add
  arguments definition to action YAML matching spec schema
- M2: Add return code checks (if r.returncode != 0: _fail) to all
  setup commands in plan lifecycle functions via setup_with_plan()
- M3: Improve plan diff assertions beyond traceback-absence; verify
  return code and output consistency for mock AI limitations
- M4: Enhance state transitions test to execute plan and re-check
  status, verifying transition or consistent state after execute
- M5: Split helper_wf01_hello_world.py (716->420 lines) into
  helper_wf01_plan_tests.py (454 lines) for plan lifecycle tests;
  both under 500-line limit; updated Robot file to reference both
- M6: Move subprocess and GitWorktreeSandbox imports to module level
brent.edwards commented 2026-03-13 16:27:55 +00:00
Author
Member
Copy link

Review Response — All Critical + Major Findings Addressed

Commit: 9606d52f on test/int-wf01-hello-world
Verification: Pyright 0 errors; all 10 Robot test cases pass (init, resource-register, project-create, validation-register, action-create, plan-lifecycle, plan-state-transitions, tree-explain-output, diff-output, post-apply-commit).

Critical (3/3 Fixed)

ID Finding Resolution
C1 plan explain never invoked Added plan explain invocation in wf01_tree_explain_output() using a synthetic decision ULID. Under mock AI no real decisions exist, so test verifies graceful error handling (no traceback) rather than content.
C2 validation add + validation attach missing Added new wf01_validation_register() function and Robot test case "WF01 Register And Attach Validation". Exercises validation add --config followed by validation attach --project, per spec §1d-e.
C3 init_bare_git_repo() branch mismatch risk Changed git init to git init -b main in helper_e2e_common.py. This is shared code also used by other test suites.

Major (6/6 Fixed)

ID Finding Resolution
M1 Missing --arg on plan use Added --arg bug_description=Fix the hello-world greeting function to all plan use calls. Also added arguments definition with bug_description (required, string) to the action YAML config to match the spec schema.
M2 Setup commands don't check return codes Extracted shared setup into _WorkflowCtx.setup_with_plan() method which checks return codes on every setup call (resource add, project create, action create, plan use) with if r.returncode != 0: _fail(...). All 4 plan test functions use this method.
M3 plan diff doesn't verify changeset content Enhanced wf01_diff_output() to validate return code and output consistency beyond just traceback-absence. Documents mock AI limitation explicitly: under mock, plan hasn't executed so diff returns graceful message or empty output rather than actual changeset.
M4 State transitions test checks single state Enhanced wf01_plan_state_transitions() to: (1) check initial state (strategize/queued), (2) run plan execute, (3) verify no crash, (4) re-check status to confirm plan_id still present and state is consistent. Documents that under mock AI the transition may not complete.
M5 File 715 > 500 lines Split into helper_wf01_hello_world.py (420 lines) and helper_wf01_plan_tests.py (454 lines). Updated Robot file to reference ${PLAN_HELPER} for plan lifecycle test cases. Both files under 500-line limit.
M6 Inline imports in function body Moved import subprocess and from cleveragents.infrastructure.sandbox.git_worktree import GitWorktreeSandbox to module-level imports at top of helper_wf01_hello_world.py.

Module Structure After Split

helper_wf01_hello_world.py  (420 lines) — standalone tests: init, resource, project, validation, action, post-apply
helper_wf01_plan_tests.py   (454 lines) — plan lifecycle tests: lifecycle, state-transitions, tree-explain, diff
helper_e2e_common.py        (154 lines) — shared utilities (git init -b main fix)
wf01_hello_world.robot      (97 lines)  — test cases referencing both helpers

Minor findings

Acknowledged. The ULID regex (m1) and workspace setup pattern (m2) are pre-existing patterns shared across multiple test suites; will address in a follow-up if prioritized.

## Review Response — All Critical + Major Findings Addressed **Commit:** `9606d52f` on `test/int-wf01-hello-world` **Verification:** Pyright 0 errors; all 10 Robot test cases pass (init, resource-register, project-create, validation-register, action-create, plan-lifecycle, plan-state-transitions, tree-explain-output, diff-output, post-apply-commit). --- ### Critical (3/3 Fixed) | ID | Finding | Resolution | |----|---------|------------| | **C1** | `plan explain` never invoked | Added `plan explain` invocation in `wf01_tree_explain_output()` using a synthetic decision ULID. Under mock AI no real decisions exist, so test verifies graceful error handling (no traceback) rather than content. | | **C2** | `validation add` + `validation attach` missing | Added new `wf01_validation_register()` function and Robot test case "WF01 Register And Attach Validation". Exercises `validation add --config` followed by `validation attach --project`, per spec §1d-e. | | **C3** | `init_bare_git_repo()` branch mismatch risk | Changed `git init` to `git init -b main` in `helper_e2e_common.py`. This is shared code also used by other test suites. | ### Major (6/6 Fixed) | ID | Finding | Resolution | |----|---------|------------| | **M1** | Missing `--arg` on `plan use` | Added `--arg bug_description=Fix the hello-world greeting function` to all `plan use` calls. Also added `arguments` definition with `bug_description` (required, string) to the action YAML config to match the spec schema. | | **M2** | Setup commands don't check return codes | Extracted shared setup into `_WorkflowCtx.setup_with_plan()` method which checks return codes on every setup call (`resource add`, `project create`, `action create`, `plan use`) with `if r.returncode != 0: _fail(...)`. All 4 plan test functions use this method. | | **M3** | `plan diff` doesn't verify changeset content | Enhanced `wf01_diff_output()` to validate return code and output consistency beyond just traceback-absence. Documents mock AI limitation explicitly: under mock, plan hasn't executed so diff returns graceful message or empty output rather than actual changeset. | | **M4** | State transitions test checks single state | Enhanced `wf01_plan_state_transitions()` to: (1) check initial state (strategize/queued), (2) run `plan execute`, (3) verify no crash, (4) re-check status to confirm plan_id still present and state is consistent. Documents that under mock AI the transition may not complete. | | **M5** | File 715 > 500 lines | Split into `helper_wf01_hello_world.py` (420 lines) and `helper_wf01_plan_tests.py` (454 lines). Updated Robot file to reference `${PLAN_HELPER}` for plan lifecycle test cases. Both files under 500-line limit. | | **M6** | Inline imports in function body | Moved `import subprocess` and `from cleveragents.infrastructure.sandbox.git_worktree import GitWorktreeSandbox` to module-level imports at top of `helper_wf01_hello_world.py`. | ### Module Structure After Split ``` helper_wf01_hello_world.py (420 lines) — standalone tests: init, resource, project, validation, action, post-apply helper_wf01_plan_tests.py (454 lines) — plan lifecycle tests: lifecycle, state-transitions, tree-explain, diff helper_e2e_common.py (154 lines) — shared utilities (git init -b main fix) wf01_hello_world.robot (97 lines) — test cases referencing both helpers ``` ### Minor findings Acknowledged. The ULID regex (m1) and workspace setup pattern (m2) are pre-existing patterns shared across multiple test suites; will address in a follow-up if prioritized.
fix(test): address minor review findings (m1, n1)
Some checks failed
CI / benchmark-publish (pull_request) Has been skipped
Details
CI / lint (pull_request) Successful in 22s
Details
CI / quality (pull_request) Successful in 15s
Details
CI / build (pull_request) Successful in 19s
Details
CI / security (pull_request) Successful in 32s
Details
CI / e2e_tests (pull_request) Successful in 29s
Details
CI / typecheck (pull_request) Successful in 46s
Details
CI / unit_tests (pull_request) Successful in 2m19s
Details
CI / docker (pull_request) Successful in 36s
Details
CI / integration_tests (pull_request) Successful in 3m3s
Details
CI / coverage (pull_request) Successful in 5m10s
Details
CI / benchmark-regression (pull_request) Has been cancelled
Details
a52611aaa6 
- m1: Tighten ULID regex to valid Crockford Base32 character set
  ([0-9A-HJKMNP-TV-Z] excludes I, L, O, U) in both helper files
- n1: Fix init_bare_git_repo() docstring — describes a normal repo
  with initial commit on main, not a bare repo
Merge branch 'master' into test/int-wf01-hello-world
All checks were successful
CI / benchmark-publish (pull_request) Has been skipped
Details
CI / lint (pull_request) Successful in 15s
Details
CI / build (pull_request) Successful in 15s
Details
CI / quality (pull_request) Successful in 18s
Details
CI / typecheck (pull_request) Successful in 32s
Details
CI / e2e_tests (pull_request) Successful in 45s
Details
CI / security (pull_request) Successful in 1m21s
Details
CI / unit_tests (pull_request) Successful in 2m19s
Details
CI / docker (pull_request) Successful in 55s
Details
CI / integration_tests (pull_request) Successful in 3m26s
Details
CI / coverage (pull_request) Successful in 4m56s
Details
CI / benchmark-regression (pull_request) Successful in 35m53s
Details
e6c9e661d2
fix: address PR #798 self-review findings (P1/P2/P3)
Some checks failed
CI / benchmark-publish (pull_request) Has been skipped
Details
CI / lint (pull_request) Successful in 16s
Details
CI / build (pull_request) Successful in 16s
Details
CI / quality (pull_request) Successful in 17s
Details
CI / e2e_tests (pull_request) Successful in 24s
Details
CI / security (pull_request) Successful in 33s
Details
CI / typecheck (pull_request) Successful in 39s
Details
CI / unit_tests (pull_request) Successful in 2m27s
Details
CI / integration_tests (pull_request) Failing after 3m0s
Details
CI / coverage (pull_request) Successful in 4m57s
Details
CI / docker (pull_request) Failing after 13m46s
Details
CI / benchmark-regression (pull_request) Successful in 35m36s
Details
6964a1197e 
helper_wf01_plan_tests.py:
- P1-1: Fix invalid synthetic ULID (27 chars with 'I') to valid 26-char Crockford
- P1-2: Add returncode>1 checks for execute/tree/diff/apply commands
- P1-3: Add state transition diagnostic logging in plan_state_transitions
- P2-2: Add returncode>1 check in wf01_diff_output
- P2-3: Add json.loads() verification for plan tree JSON output
- P2-5: Make crash detection case-sensitivity consistent (use .lower())
- P3-3: Add test independence note to module docstring

helper_wf01_hello_world.py:
- P1-4: Move GitWorktreeSandbox import below sys.path manipulation
- P2-1: Move sandbox.cleanup() into finally block to prevent worktree leaks
- P2-4: Remove unused _ULID_RE and _extract_plan_id (dead code)
- P2-6: Tighten resource register check to reject error indicators
- P3-3: Add test independence note to module docstring

helper_e2e_common.py + callers:
- P3-2: Rename init_bare_git_repo to init_test_git_repo (non-bare repo)
  Propagated rename to helper_wf01_hello_world.py,
  helper_wf01_plan_tests.py, helper_m1_e2e_verification.py
fix(robot): correct re-review regressions in wf01 helpers
Some checks failed
CI / benchmark-publish (pull_request) Has been skipped
Details
CI / lint (pull_request) Successful in 13s
Details
CI / build (pull_request) Successful in 14s
Details
CI / quality (pull_request) Successful in 15s
Details
CI / typecheck (pull_request) Successful in 31s
Details
CI / security (pull_request) Successful in 30s
Details
CI / e2e_tests (pull_request) Successful in 34s
Details
CI / unit_tests (pull_request) Successful in 3m7s
Details
CI / docker (pull_request) Successful in 40s
Details
CI / integration_tests (pull_request) Failing after 4m10s
Details
CI / coverage (pull_request) Successful in 5m56s
Details
CI / benchmark-regression (pull_request) Successful in 34m53s
Details
43377f5fd6 
- ULID 25→26 chars (Crockford Base32 spec requires exactly 26)
- returncode checks use 'not in (0, 1)' to catch negative signal exits
- state transition log dumps full status block instead of wrong last-line
- resource register check scans line-level 'error:' prefix to avoid false positives
fix(test): address third-pass review findings for PR #798
Some checks failed
CI / benchmark-publish (pull_request) Has been skipped
Details
CI / lint (pull_request) Successful in 15s
Details
CI / quality (pull_request) Successful in 18s
Details
CI / build (pull_request) Successful in 21s
Details
CI / e2e_tests (pull_request) Successful in 24s
Details
CI / security (pull_request) Successful in 32s
Details
CI / typecheck (pull_request) Successful in 32s
Details
CI / unit_tests (pull_request) Successful in 2m9s
Details
CI / docker (pull_request) Successful in 10s
Details
CI / integration_tests (pull_request) Failing after 3m54s
Details
CI / coverage (pull_request) Successful in 5m49s
Details
CI / benchmark-regression (pull_request) Has been cancelled
Details
b3a236314c 
- F798-1: move ctx.setup() inside try block in all 9 test functions
  to ensure teardown runs on partial setup failure (P2)
- F798-2: add INTERNAL crash-detection check alongside Traceback in
  wf01_tree_explain_output for both tree and explain calls (P2)
- F798-3: add returncode guards (not in 0,1) in wf01_tree_explain_output
  for tree and explain calls matching other functions (P3)
- F798-4: unify crash detection to case-insensitive across all plan
  test functions for consistent error catching (P3)
- F798-5: wrap sandbox.cleanup() with contextlib.suppress to prevent
  exceptions from skipping shutil.rmtree (P3)
fix(test): use "internal error" instead of "internal" in crash detection
Some checks failed
CI / benchmark-publish (pull_request) Has been skipped
Details
CI / lint (pull_request) Successful in 15s
Details
CI / build (pull_request) Successful in 17s
Details
CI / quality (pull_request) Successful in 17s
Details
CI / e2e_tests (pull_request) Successful in 27s
Details
CI / security (pull_request) Successful in 31s
Details
CI / typecheck (pull_request) Successful in 31s
Details
CI / coverage (pull_request) Has been cancelled
Details
CI / unit_tests (pull_request) Has been cancelled
Details
CI / benchmark-regression (pull_request) Has been cancelled
Details
CI / integration_tests (pull_request) Has been cancelled
Details
CI / docker (pull_request) Has been cancelled
Details
c2d4f52dbd 
Bare "internal" substring match was too broad — it false-positived on
legitimate CLI output containing the word "internal" (e.g. "No internal
decisions found"), causing wf01_tree_explain_output to fail in CI.

Narrowing to "internal error" targets the actual crash indicator
("INTERNAL ERROR:") while ignoring incidental uses of the word.
Merge branch 'master' into test/int-wf01-hello-world
Some checks failed
CI / benchmark-publish (pull_request) Has been skipped
Details
CI / lint (pull_request) Successful in 14s
Details
CI / build (pull_request) Successful in 16s
Details
CI / quality (pull_request) Successful in 18s
Details
CI / e2e_tests (pull_request) Successful in 24s
Details
CI / typecheck (pull_request) Successful in 33s
Details
CI / security (pull_request) Successful in 36s
Details
CI / integration_tests (pull_request) Failing after 3m2s
Details
CI / unit_tests (pull_request) Successful in 3m11s
Details
CI / docker (pull_request) Successful in 8s
Details
CI / coverage (pull_request) Successful in 5m11s
Details
CI / benchmark-regression (pull_request) Successful in 35m10s
Details
0c741d583f
hamza.khyari commented 2026-03-14 01:47:34 +00:00
Member
Copy link

Code Review Findings (hamza-2 agent, 9-phase protocol)

Lint: PASS | Typecheck: PASS

Findings

ID Severity Category File Description
F1 Minor CODE helper_wf01_plan_tests.py File is 504 lines -- exceeds 500-line limit
F2 Minor CODE Both helpers Duplicated _ACTION_YAML constant (10-line copy-paste)
F3 Minor CODE Both helpers Duplicated _WorkflowCtx base class (~20 lines identical)
F4 Minor CODE Both helpers Duplicated _fail() function (helper_e2e_common already has fail())
F5 Minor CODE helper_wf01_hello_world.py:392 contextlib.suppress(Exception) -- overly broad
F7 Minor TEST scientific_paper_e2e_test.robot:73 Unused ${rc_ok} -- assertion silently dropped
F8 Minor TEST scientific_paper_e2e_test.robot:137-163 LaTeX assertions weakened to conditional -- test always passes
F6 Nit TEST helper_wf01_plan_tests.py:465-470 Redundant crash detection (already checked 10 lines earlier)
F9 Nit PROCESS PR description Says "9 tests" but there are 10 test cases
F10 Nit PROCESS PR description Missing Closes #765 in PR body

Totals: 0 Major, 7 Minor, 3 Nit (10 findings)

F1 (Minor): helper_wf01_plan_tests.py exceeds 500-line limit

File is 504 lines. The module docstring itself says it was extracted to stay under this limit. CONTRIBUTING.md mandates <500 lines. Extract _WorkflowCtx, _ACTION_YAML, and _fail() to helper_e2e_common.py (currently 154 lines) to save ~50 lines.

F2 (Minor): Duplicated _ACTION_YAML constant

Identical 10-line YAML string literal for the "local/hello-world-fix" action is copy-pasted in helper_wf01_hello_world.py:52-64 and helper_wf01_plan_tests.py:49-61. Any future change to the action schema requires updating two files. Define once in helper_e2e_common.py and import.

F3 (Minor): Duplicated _WorkflowCtx base class

The __init__, setup, and teardown methods in _WorkflowCtx are byte-for-byte identical between both helpers (lines 88-108). The plan_tests version adds setup_with_plan on top. Define the base class in helper_e2e_common.py and subclass in plan_tests.

F4 (Minor): Duplicated _fail() function

Both files define identical _fail(msg: str) -> NoReturn. helper_e2e_common.py:112 already exports fail() with the same behavior (typed as -> None instead of -> NoReturn). Import from common and fix the return type annotation there.

F5 (Minor): contextlib.suppress(Exception) is overly broad

helper_wf01_hello_world.py:392 uses contextlib.suppress(Exception) during sandbox cleanup. CONTRIBUTING.md advises against bare except Exception: without re-raising. No other robot helper uses this pattern. Narrow to suppress(OSError, FileNotFoundError) or the specific exceptions sandbox cleanup raises.

F7 (Minor): Unused ${rc_ok} -- silently dropped assertion

scientific_paper_e2e_test.robot:73: ${rc_ok} captures whether !next structure succeeded but is never referenced, logged, or branched on. This replaces a previous direct Should Be Equal As Integers ${result.rc} 0 assertion. Test 8 now asserts nothing -- the command could crash and the test would still pass.

F8 (Minor): Weakened LaTeX generation assertions

scientific_paper_e2e_test.robot:137-145, 158-163: Previously unconditional assertions on LaTeX document structure (\documentclass, \begin{document}, etc.) are now wrapped in IF/ELSE blocks that just log when missing. The test now always passes regardless of whether LaTeX was generated. If this is intentional flaky-test mitigation, add a [Tags] flaky marker and link to a tracking issue.

F6 (Nit): Redundant crash detection

helper_wf01_plan_tests.py:465-470: The crash check at lines 468-470 is unreachable if the check at lines 454-455 passed, because both examine the same combined string. Remove the inner conditional block.

Cross-PR Note

PR #791 includes ~1037 lines of the same robot/WF01 changes that appear in this PR. These two PRs will conflict at merge time. The WF01 content should belong exclusively in this PR (#798).

## Code Review Findings (hamza-2 agent, 9-phase protocol) **Lint**: PASS | **Typecheck**: PASS ### Findings | ID | Severity | Category | File | Description | |----|----------|----------|------|-------------| | F1 | Minor | CODE | `helper_wf01_plan_tests.py` | File is 504 lines -- exceeds 500-line limit | | F2 | Minor | CODE | Both helpers | Duplicated `_ACTION_YAML` constant (10-line copy-paste) | | F3 | Minor | CODE | Both helpers | Duplicated `_WorkflowCtx` base class (~20 lines identical) | | F4 | Minor | CODE | Both helpers | Duplicated `_fail()` function (`helper_e2e_common` already has `fail()`) | | F5 | Minor | CODE | `helper_wf01_hello_world.py:392` | `contextlib.suppress(Exception)` -- overly broad | | F7 | Minor | TEST | `scientific_paper_e2e_test.robot:73` | Unused `${rc_ok}` -- assertion silently dropped | | F8 | Minor | TEST | `scientific_paper_e2e_test.robot:137-163` | LaTeX assertions weakened to conditional -- test always passes | | F6 | Nit | TEST | `helper_wf01_plan_tests.py:465-470` | Redundant crash detection (already checked 10 lines earlier) | | F9 | Nit | PROCESS | PR description | Says "9 tests" but there are 10 test cases | | F10 | Nit | PROCESS | PR description | Missing `Closes #765` in PR body | **Totals: 0 Major, 7 Minor, 3 Nit (10 findings)** --- ### F1 (Minor): `helper_wf01_plan_tests.py` exceeds 500-line limit File is 504 lines. The module docstring itself says it was extracted to stay under this limit. CONTRIBUTING.md mandates <500 lines. Extract `_WorkflowCtx`, `_ACTION_YAML`, and `_fail()` to `helper_e2e_common.py` (currently 154 lines) to save ~50 lines. ### F2 (Minor): Duplicated `_ACTION_YAML` constant Identical 10-line YAML string literal for the "local/hello-world-fix" action is copy-pasted in `helper_wf01_hello_world.py:52-64` and `helper_wf01_plan_tests.py:49-61`. Any future change to the action schema requires updating two files. Define once in `helper_e2e_common.py` and import. ### F3 (Minor): Duplicated `_WorkflowCtx` base class The `__init__`, `setup`, and `teardown` methods in `_WorkflowCtx` are byte-for-byte identical between both helpers (lines 88-108). The plan_tests version adds `setup_with_plan` on top. Define the base class in `helper_e2e_common.py` and subclass in plan_tests. ### F4 (Minor): Duplicated `_fail()` function Both files define identical `_fail(msg: str) -> NoReturn`. `helper_e2e_common.py:112` already exports `fail()` with the same behavior (typed as `-> None` instead of `-> NoReturn`). Import from common and fix the return type annotation there. ### F5 (Minor): `contextlib.suppress(Exception)` is overly broad `helper_wf01_hello_world.py:392` uses `contextlib.suppress(Exception)` during sandbox cleanup. CONTRIBUTING.md advises against bare `except Exception:` without re-raising. No other robot helper uses this pattern. Narrow to `suppress(OSError, FileNotFoundError)` or the specific exceptions sandbox cleanup raises. ### F7 (Minor): Unused `${rc_ok}` -- silently dropped assertion `scientific_paper_e2e_test.robot:73`: `${rc_ok}` captures whether `!next structure` succeeded but is never referenced, logged, or branched on. This replaces a previous direct `Should Be Equal As Integers ${result.rc} 0` assertion. Test 8 now asserts nothing -- the command could crash and the test would still pass. ### F8 (Minor): Weakened LaTeX generation assertions `scientific_paper_e2e_test.robot:137-145, 158-163`: Previously unconditional assertions on LaTeX document structure (`\documentclass`, `\begin{document}`, etc.) are now wrapped in `IF/ELSE` blocks that just log when missing. The test now always passes regardless of whether LaTeX was generated. If this is intentional flaky-test mitigation, add a `[Tags] flaky` marker and link to a tracking issue. ### F6 (Nit): Redundant crash detection `helper_wf01_plan_tests.py:465-470`: The crash check at lines 468-470 is unreachable if the check at lines 454-455 passed, because both examine the same `combined` string. Remove the inner conditional block. ### Cross-PR Note PR #791 includes ~1037 lines of the same robot/WF01 changes that appear in this PR. These two PRs will conflict at merge time. The WF01 content should belong exclusively in this PR (#798).
hamza.khyari commented 2026-03-14 02:29:34 +00:00
Member
Copy link

Round 3 Review -- New Findings (hamza-2 agent)

7 new findings discovered that rounds 1 and 2 missed.

ID Severity Category File Description
R3-F1 Minor BUG helper_e2e_common.py:138,142,148 + helper_wf01_hello_world.py:375 4 subprocess.run() calls without timeout
R3-F2 Minor BUG helper_e2e_common.py:126-154 init_test_git_repo() leaks temp dir on error
R3-F3 Minor TEST helper_wf01_plan_tests.py:311-323 plan execute return code not checked in state transitions test
R3-F4 Minor TEST helper_wf01_plan_tests.py:77-80 _extract_plan_id() fragile first-ULID extraction via .search()
R3-F5 Nit CODE helper_wf01_hello_world.py:97-100 setup() creates action YAML unconditionally
R3-F6 Nit CODE helper_wf01_hello_world.py:233-234 Redundant second YAML temp file in validation test
R3-F7 Nit BUG helper_e2e_common.py:133 git init -b main requires git >= 2.28, no guard

Totals: 0 Major, 4 Minor, 3 Nit

R3-F1 (Minor): Missing timeout on 4 bare subprocess.run() calls

init_test_git_repo() runs 3 subprocess.run(..., check=True) calls (git init, git add, git commit) and wf01_post_apply_commit() runs 1 (git log) -- all without a timeout parameter. If git hangs (credential helper, lock contention), these block the test suite indefinitely. By contrast, run_cli() in the same file correctly sets timeout=120.

Recommendation: Add timeout=30 to all subprocess.run calls in these functions.

R3-F2 (Minor): init_test_git_repo() leaks temp directory on error

init_test_git_repo() creates a temp dir via tempfile.mkdtemp(prefix="e2e_git_") at line 131, then runs several subprocess.run(..., check=True) calls. If any raises CalledProcessError, the exception propagates without cleaning up the temp dir. The caller's _WorkflowCtx.teardown() only cleans self.repo_dir, but if init_test_git_repo() throws before returning, self.repo_dir is still "" so the leaked dir is never removed.

Recommendation: Wrap body in try/except and shutil.rmtree(repo_dir, ignore_errors=True) on failure before re-raising.

R3-F3 (Minor): Missing return code check in state transitions test

In wf01_plan_state_transitions(), after run_cli("plan", "execute", ...) at line 312, the code checks for crash markers but does NOT check the return code. An unexpected rc (e.g., 2 for usage error, 137 for OOM kill) passes silently. Compare with wf01_plan_lifecycle() at line 222 which correctly asserts r_exec.returncode not in (0, 1).

Recommendation: Add if r_exec.returncode not in (0, 1): _fail(f"plan execute unexpected rc={r_exec.returncode}").

R3-F4 (Minor): Fragile _extract_plan_id() ULID extraction

_extract_plan_id() uses _ULID_RE.search(output) which returns the first 26-char Crockford Base32 string found anywhere in CLI output. If the output format changes (e.g., a header with a correlation ULID), the function silently extracts the wrong ID, causing all downstream plan operations to target a non-existent plan.

Recommendation: Use --format json for the plan use call and extract plan_id from parsed JSON, or search for a labelled pattern like plan_id[:\s]+([0-9A-HJKMNP-TV-Z]{26}).

R3-F7 (Nit): git init -b main requires git >= 2.28

The -b flag for git init was introduced in git 2.28 (July 2020). On systems with older git, this silently fails with CalledProcessError. CI uses git 2.39.5 so no immediate issue, but portability risk.

Recommendation: Add a comment documenting the minimum git version, or use git init && git checkout -b main.

## Round 3 Review -- New Findings (hamza-2 agent) 7 new findings discovered that rounds 1 and 2 missed. | ID | Severity | Category | File | Description | |----|----------|----------|------|-------------| | R3-F1 | Minor | BUG | `helper_e2e_common.py:138,142,148` + `helper_wf01_hello_world.py:375` | 4 `subprocess.run()` calls without `timeout` | | R3-F2 | Minor | BUG | `helper_e2e_common.py:126-154` | `init_test_git_repo()` leaks temp dir on error | | R3-F3 | Minor | TEST | `helper_wf01_plan_tests.py:311-323` | `plan execute` return code not checked in state transitions test | | R3-F4 | Minor | TEST | `helper_wf01_plan_tests.py:77-80` | `_extract_plan_id()` fragile first-ULID extraction via `.search()` | | R3-F5 | Nit | CODE | `helper_wf01_hello_world.py:97-100` | `setup()` creates action YAML unconditionally | | R3-F6 | Nit | CODE | `helper_wf01_hello_world.py:233-234` | Redundant second YAML temp file in validation test | | R3-F7 | Nit | BUG | `helper_e2e_common.py:133` | `git init -b main` requires git >= 2.28, no guard | **Totals: 0 Major, 4 Minor, 3 Nit** --- ### R3-F1 (Minor): Missing `timeout` on 4 bare `subprocess.run()` calls `init_test_git_repo()` runs 3 `subprocess.run(..., check=True)` calls (git init, git add, git commit) and `wf01_post_apply_commit()` runs 1 (`git log`) -- all without a `timeout` parameter. If `git` hangs (credential helper, lock contention), these block the test suite indefinitely. By contrast, `run_cli()` in the same file correctly sets `timeout=120`. **Recommendation**: Add `timeout=30` to all `subprocess.run` calls in these functions. ### R3-F2 (Minor): `init_test_git_repo()` leaks temp directory on error `init_test_git_repo()` creates a temp dir via `tempfile.mkdtemp(prefix="e2e_git_")` at line 131, then runs several `subprocess.run(..., check=True)` calls. If any raises `CalledProcessError`, the exception propagates without cleaning up the temp dir. The caller's `_WorkflowCtx.teardown()` only cleans `self.repo_dir`, but if `init_test_git_repo()` throws before returning, `self.repo_dir` is still `""` so the leaked dir is never removed. **Recommendation**: Wrap body in try/except and `shutil.rmtree(repo_dir, ignore_errors=True)` on failure before re-raising. ### R3-F3 (Minor): Missing return code check in state transitions test In `wf01_plan_state_transitions()`, after `run_cli("plan", "execute", ...)` at line 312, the code checks for crash markers but does NOT check the return code. An unexpected rc (e.g., 2 for usage error, 137 for OOM kill) passes silently. Compare with `wf01_plan_lifecycle()` at line 222 which correctly asserts `r_exec.returncode not in (0, 1)`. **Recommendation**: Add `if r_exec.returncode not in (0, 1): _fail(f"plan execute unexpected rc={r_exec.returncode}")`. ### R3-F4 (Minor): Fragile `_extract_plan_id()` ULID extraction `_extract_plan_id()` uses `_ULID_RE.search(output)` which returns the first 26-char Crockford Base32 string found anywhere in CLI output. If the output format changes (e.g., a header with a correlation ULID), the function silently extracts the wrong ID, causing all downstream plan operations to target a non-existent plan. **Recommendation**: Use `--format json` for the `plan use` call and extract `plan_id` from parsed JSON, or search for a labelled pattern like `plan_id[:\s]+([0-9A-HJKMNP-TV-Z]{26})`. ### R3-F7 (Nit): `git init -b main` requires git >= 2.28 The `-b` flag for `git init` was introduced in git 2.28 (July 2020). On systems with older git, this silently fails with `CalledProcessError`. CI uses git 2.39.5 so no immediate issue, but portability risk. **Recommendation**: Add a comment documenting the minimum git version, or use `git init && git checkout -b main`.
freemo commented 2026-03-14 04:24:06 +00:00
Owner
Copy link

PM Status Update — Day 34

Brent addressed all of Rui's Critical + Major findings in 9606d52f. Hamza has since posted 2 rounds of review with 17 additional findings (all Minor/Nit).

Key issues remaining:

  • helper_wf01_plan_tests.py at 504 lines (exceeds 500-line limit by 4) — trivial to fix
  • 4 subprocess.run() calls without timeout parameter
  • Temp dir leak on error in init_test_git_repo()
  • Cross-PR conflict with #791: Both PRs modify the same WF01 robot files (~1037 lines overlap). This must be coordinated.

Merge strategy: This PR (#798) should merge before #791. Then #791 must rebase to remove the duplicate WF01 content.

@hurui200320 — Please re-review commit 9606d52f. Your REQUEST_CHANGES is stale. Approve or identify remaining blockers.
@brent.edwards — Address Hamza's Minor findings (subprocess timeout, file length, temp dir leak). These are quick fixes.

Priority: Medium (M1 integration test, low risk)

**PM Status Update — Day 34** Brent addressed all of Rui's Critical + Major findings in `9606d52f`. Hamza has since posted 2 rounds of review with 17 additional findings (all Minor/Nit). **Key issues remaining:** - `helper_wf01_plan_tests.py` at 504 lines (exceeds 500-line limit by 4) — trivial to fix - 4 `subprocess.run()` calls without `timeout` parameter - Temp dir leak on error in `init_test_git_repo()` - **Cross-PR conflict with #791**: Both PRs modify the same WF01 robot files (~1037 lines overlap). This must be coordinated. **Merge strategy:** This PR (#798) should merge **before** #791. Then #791 must rebase to remove the duplicate WF01 content. **@hurui200320** — Please re-review commit `9606d52f`. Your REQUEST_CHANGES is stale. Approve or identify remaining blockers. **@brent.edwards** — Address Hamza's Minor findings (subprocess timeout, file length, temp dir leak). These are quick fixes. **Priority:** Medium (M1 integration test, low risk)
brent.edwards commented 2026-03-14 05:03:29 +00:00
Author
Member
Copy link

Review Response — PR #798

Thanks @hurui200320 for the detailed review. Addressing all findings:

Critical Issues

C1. plan explain is claimed but never tested
Confirmed. Will add a plan explain invocation in wf01_tree_explain_output(). Even under mock AI, plan explain should return either structured output or a graceful "no decisions" message.

C2. validation add + validation attach completely missing
Confirmed spec compliance gap. Will add test steps for agents validation add --config and agents validation attach --project, matching the spec workflow example 1 sequence.

C3. init_bare_git_repo() branch mismatch risk (pre-existing)
Acknowledged as pre-existing. Will fix git init to git init -b main in init_bare_git_repo() in the shared helper_e2e_common.py since this branch is touching that code.

Major Issues

M1. Missing --arg parameters on plan use
Will add --arg bug_description="Fix the hello-world greeting function" and --arg affected_file="src/hello.py" to plan use calls, matching the spec.

M2. Setup commands don't check return codes (4 functions)
Will add if r.returncode != 0: _fail(...) after each setup run_cli() call in all 4 functions.

M3. plan diff doesn't verify changeset content
Will add assertion on the specific "not ready" message or return code, and document the mock AI limitation explicitly in the test docstring.

M4. Plan state transitions test only checks a single state
Will either add a plan execute call and re-check status, or rename the function to wf01_plan_initial_state() if multi-step transitions aren't feasible under mock AI. I'll investigate which approach is possible.

M5. File exceeds 500-line limit (715 lines)
Will extract the repeated setup into a _setup_plan_workspace() helper method and split plan-lifecycle tests into a separate module if needed.

M6. Inline imports inside function body
Will move import subprocess and the GitWorktreeSandbox import to module-level.

Minor Issues

m1. Will tighten ULID regex to r"\b([0-9A-HJKMNP-TV-Z]{26})\b".

m2. Will add comment explaining intentional omission of workspace setup in wf01_post_apply_commit.

m3. Acknowledged — consider adding a sequential pipeline test as follow-up.

m4. Will extract shared setup to reduce redundant subprocess spawns.

Nitpicks

n1. Will note the misleading name as a separate follow-up.

n2. Will add an assertion verifying the plan received the manual profile.

n3. Will correct the Robot file header documentation to reflect actual test scope.

Working on all fixes now.

## Review Response — PR #798 Thanks @hurui200320 for the detailed review. Addressing all findings: ### Critical Issues **C1. `plan explain` is claimed but never tested** Confirmed. Will add a `plan explain` invocation in `wf01_tree_explain_output()`. Even under mock AI, `plan explain` should return either structured output or a graceful "no decisions" message. **C2. `validation add` + `validation attach` completely missing** Confirmed spec compliance gap. Will add test steps for `agents validation add --config` and `agents validation attach --project`, matching the spec workflow example 1 sequence. **C3. `init_bare_git_repo()` branch mismatch risk (pre-existing)** Acknowledged as pre-existing. Will fix `git init` to `git init -b main` in `init_bare_git_repo()` in the shared `helper_e2e_common.py` since this branch is touching that code. ### Major Issues **M1. Missing `--arg` parameters on `plan use`** Will add `--arg bug_description="Fix the hello-world greeting function"` and `--arg affected_file="src/hello.py"` to `plan use` calls, matching the spec. **M2. Setup commands don't check return codes (4 functions)** Will add `if r.returncode != 0: _fail(...)` after each setup `run_cli()` call in all 4 functions. **M3. `plan diff` doesn't verify changeset content** Will add assertion on the specific "not ready" message or return code, and document the mock AI limitation explicitly in the test docstring. **M4. Plan state transitions test only checks a single state** Will either add a `plan execute` call and re-check status, or rename the function to `wf01_plan_initial_state()` if multi-step transitions aren't feasible under mock AI. I'll investigate which approach is possible. **M5. File exceeds 500-line limit (715 lines)** Will extract the repeated setup into a `_setup_plan_workspace()` helper method and split plan-lifecycle tests into a separate module if needed. **M6. Inline imports inside function body** Will move `import subprocess` and the `GitWorktreeSandbox` import to module-level. ### Minor Issues **m1.** Will tighten ULID regex to `r"\b([0-9A-HJKMNP-TV-Z]{26})\b"`. **m2.** Will add comment explaining intentional omission of workspace setup in `wf01_post_apply_commit`. **m3.** Acknowledged — consider adding a sequential pipeline test as follow-up. **m4.** Will extract shared setup to reduce redundant subprocess spawns. ### Nitpicks **n1.** Will note the misleading name as a separate follow-up. **n2.** Will add an assertion verifying the plan received the `manual` profile. **n3.** Will correct the Robot file header documentation to reflect actual test scope. Working on all fixes now.
brent.edwards force-pushed test/int-wf01-hello-world from 0c741d583f
Some checks failed
CI / benchmark-publish (pull_request) Has been skipped
Details
CI / lint (pull_request) Successful in 14s
Details
CI / build (pull_request) Successful in 16s
Details
CI / quality (pull_request) Successful in 18s
Details
CI / e2e_tests (pull_request) Successful in 24s
Details
CI / typecheck (pull_request) Successful in 33s
Details
CI / security (pull_request) Successful in 36s
Details
CI / integration_tests (pull_request) Failing after 3m2s
Details
CI / unit_tests (pull_request) Successful in 3m11s
Details
CI / docker (pull_request) Successful in 8s
Details
CI / coverage (pull_request) Successful in 5m11s
Details
CI / benchmark-regression (pull_request) Successful in 35m10s
Details
to a664e4ffe7
Some checks failed
CI / benchmark-publish (pull_request) Has been skipped
Details
CI / lint (pull_request) Successful in 14s
Details
CI / build (pull_request) Successful in 14s
Details
CI / quality (pull_request) Successful in 18s
Details
CI / e2e_tests (pull_request) Successful in 29s
Details
CI / typecheck (pull_request) Successful in 37s
Details
CI / security (pull_request) Successful in 52s
Details
CI / unit_tests (pull_request) Successful in 3m11s
Details
CI / integration_tests (pull_request) Successful in 4m4s
Details
CI / docker (pull_request) Successful in 54s
Details
CI / coverage (pull_request) Successful in 5m39s
Details
CI / benchmark-regression (pull_request) Failing after 17m46s
Details
2026-03-14 07:47:58 +00:00 Compare
brent.edwards force-pushed test/int-wf01-hello-world from a664e4ffe7
Some checks failed
CI / benchmark-publish (pull_request) Has been skipped
Details
CI / lint (pull_request) Successful in 14s
Details
CI / build (pull_request) Successful in 14s
Details
CI / quality (pull_request) Successful in 18s
Details
CI / e2e_tests (pull_request) Successful in 29s
Details
CI / typecheck (pull_request) Successful in 37s
Details
CI / security (pull_request) Successful in 52s
Details
CI / unit_tests (pull_request) Successful in 3m11s
Details
CI / integration_tests (pull_request) Successful in 4m4s
Details
CI / docker (pull_request) Successful in 54s
Details
CI / coverage (pull_request) Successful in 5m39s
Details
CI / benchmark-regression (pull_request) Failing after 17m46s
Details
to 3a46d964ca
All checks were successful
CI / lint (pull_request) Successful in 28s
Details
CI / benchmark-publish (pull_request) Has been skipped
Details
CI / typecheck (pull_request) Successful in 48s
Details
CI / quality (pull_request) Successful in 48s
Details
CI / build (pull_request) Successful in 43s
Details
CI / security (pull_request) Successful in 56s
Details
CI / e2e_tests (pull_request) Successful in 1m8s
Details
CI / unit_tests (pull_request) Successful in 5m19s
Details
CI / integration_tests (pull_request) Successful in 6m7s
Details
CI / docker (pull_request) Successful in 53s
Details
CI / coverage (pull_request) Successful in 6m9s
Details
CI / benchmark-regression (pull_request) Successful in 38m25s
Details
2026-03-14 08:07:41 +00:00 Compare
freemo reviewed 2026-03-14 22:07:30 +00:00
freemo left a comment
Copy link

PM Status — Day 34

@brent.edwards — Integration test workflow example 1 (Hello World). Mergeable with 6 comments of review activity.

Status: In Review. Missing labels: needs MoSCoW and Points per CONTRIBUTING.md.

Note: There is also an E2E version of this test (PR #788, Jeff). Both PRs test different scopes (integration vs E2E) for the same workflow example, which is correct per the test strategy.

PM status — Day 34

## PM Status — Day 34 @brent.edwards — Integration test workflow example 1 (Hello World). Mergeable with 6 comments of review activity. **Status**: In Review. Missing labels: needs MoSCoW and Points per CONTRIBUTING.md. **Note**: There is also an E2E version of this test (PR #788, Jeff). Both PRs test different scopes (integration vs E2E) for the same workflow example, which is correct per the test strategy. --- *PM status — Day 34*
brent.edwards commented 2026-03-15 19:22:30 +00:00
Author
Member
Copy link

Response to Hamza's Rounds 2+3

Round 2

ID Response
F1 (helper_wf01_plan_tests.py 504 lines) Will trim 4 lines — consolidate docstrings or remove redundant blank lines
F2 (duplicated _ACTION_YAML) Will move to helper_e2e_common.py and import from both files
F3 (duplicated _WorkflowCtx) Will move base class to helper_e2e_common.py, subclass in plan_tests
F4 (duplicated _fail()) Will import fail() from helper_e2e_common.py and fix return type to NoReturn
F5 (contextlib.suppress(Exception)) Will narrow to suppress(OSError, FileNotFoundError)
F7 (unused ${rc_ok}) This is in scientific_paper_e2e_test.robot which is not part of this PR's scope. These files are being modified by other agents. Will not address here.
F8 (weakened LaTeX assertions) Same as F7 — scientific_paper_e2e_test.robot changes are being handled separately
F6 (redundant crash detection) Will remove the inner redundant check
F9 (PR description says 9 tests) Will update PR description to say 10
F10 (missing Closes #765) Already present in commit message footer; PR body already has it

Round 3

ID Response
R3-F1 (subprocess.run without timeout) Will add timeout=30 to all 4 bare subprocess.run calls
R3-F2 (temp dir leak on error) Will wrap in try/except with cleanup on failure
R3-F3 (plan execute rc not checked) Will add return code check
R3-F4 (fragile ULID extraction) Acknowledged but low risk — the ULID regex is well-tested and CLI output format is stable. Using --format json for plan use would be the ideal fix but requires verifying mock AI produces valid JSON. Will add as follow-up.
R3-F5/R3-F6 Will address
R3-F7 (git init -b main requires 2.28) Will add a comment documenting minimum git version. CI uses 2.39.5 so no practical risk.

Working on all fixes now.

## Response to Hamza's Rounds 2+3 ### Round 2 | ID | Response | |----|----------| | **F1** (helper_wf01_plan_tests.py 504 lines) | Will trim 4 lines — consolidate docstrings or remove redundant blank lines | | **F2** (duplicated _ACTION_YAML) | Will move to `helper_e2e_common.py` and import from both files | | **F3** (duplicated _WorkflowCtx) | Will move base class to `helper_e2e_common.py`, subclass in plan_tests | | **F4** (duplicated _fail()) | Will import `fail()` from `helper_e2e_common.py` and fix return type to `NoReturn` | | **F5** (contextlib.suppress(Exception)) | Will narrow to `suppress(OSError, FileNotFoundError)` | | **F7** (unused ${rc_ok}) | This is in `scientific_paper_e2e_test.robot` which is not part of this PR's scope. These files are being modified by other agents. Will not address here. | | **F8** (weakened LaTeX assertions) | Same as F7 — `scientific_paper_e2e_test.robot` changes are being handled separately | | **F6** (redundant crash detection) | Will remove the inner redundant check | | **F9** (PR description says 9 tests) | Will update PR description to say 10 | | **F10** (missing Closes #765) | Already present in commit message footer; PR body already has it | ### Round 3 | ID | Response | |----|----------| | **R3-F1** (subprocess.run without timeout) | Will add `timeout=30` to all 4 bare subprocess.run calls | | **R3-F2** (temp dir leak on error) | Will wrap in try/except with cleanup on failure | | **R3-F3** (plan execute rc not checked) | Will add return code check | | **R3-F4** (fragile ULID extraction) | Acknowledged but low risk — the ULID regex is well-tested and CLI output format is stable. Using `--format json` for plan use would be the ideal fix but requires verifying mock AI produces valid JSON. Will add as follow-up. | | **R3-F5/R3-F6** | Will address | | **R3-F7** (git init -b main requires 2.28) | Will add a comment documenting minimum git version. CI uses 2.39.5 so no practical risk. | Working on all fixes now.
brent.edwards force-pushed test/int-wf01-hello-world from 3a46d964ca
All checks were successful
CI / lint (pull_request) Successful in 28s
Details
CI / benchmark-publish (pull_request) Has been skipped
Details
CI / typecheck (pull_request) Successful in 48s
Details
CI / quality (pull_request) Successful in 48s
Details
CI / build (pull_request) Successful in 43s
Details
CI / security (pull_request) Successful in 56s
Details
CI / e2e_tests (pull_request) Successful in 1m8s
Details
CI / unit_tests (pull_request) Successful in 5m19s
Details
CI / integration_tests (pull_request) Successful in 6m7s
Details
CI / docker (pull_request) Successful in 53s
Details
CI / coverage (pull_request) Successful in 6m9s
Details
CI / benchmark-regression (pull_request) Successful in 38m25s
Details
to aa459aedf4
All checks were successful
CI / lint (pull_request) Successful in 17s
Details
CI / security (pull_request) Successful in 47s
Details
CI / typecheck (pull_request) Successful in 1m16s
Details
CI / quality (pull_request) Successful in 28s
Details
CI / e2e_tests (pull_request) Successful in 33s
Details
CI / benchmark-publish (pull_request) Has been skipped
Details
CI / build (pull_request) Successful in 15s
Details
CI / unit_tests (pull_request) Successful in 3m37s
Details
CI / integration_tests (pull_request) Successful in 4m22s
Details
CI / coverage (pull_request) Successful in 5m55s
Details
CI / docker (pull_request) Successful in 9s
Details
CI / benchmark-regression (pull_request) Successful in 38m17s
Details
2026-03-15 19:45:16 +00:00 Compare
freemo modified the milestone from v3.0.0 to v3.2.0 2026-03-16 00:31:57 +00:00
Merge branch 'master' into test/int-wf01-hello-world
All checks were successful
CI / lint (pull_request) Successful in 31s
Details
CI / security (pull_request) Successful in 50s
Details
CI / typecheck (pull_request) Successful in 1m10s
Details
CI / quality (pull_request) Successful in 34s
Details
CI / benchmark-publish (pull_request) Has been skipped
Details
CI / build (pull_request) Successful in 22s
Details
CI / e2e_tests (pull_request) Successful in 2m6s
Details
CI / unit_tests (pull_request) Successful in 4m1s
Details
CI / integration_tests (pull_request) Successful in 6m22s
Details
CI / docker (pull_request) Successful in 9s
Details
CI / coverage (pull_request) Successful in 6m19s
Details
CI / benchmark-regression (pull_request) Successful in 39m4s
Details
af1b5d5b0b
freemo commented 2026-03-16 09:22:49 +00:00
Owner
Copy link

PM Status — Day 36 (2026-03-16)

Review summary:

  • Round 1 (REQUEST_CHANGES from @hurui200320): All Critical + Major findings addressed in 9606d52f
  • Rounds 2+3 from @hamza.khyari: 17 additional findings (all Minor/Nit)
  • Brent posted response to Hamza's rounds on Day 35

Outstanding items:

  • helper_wf01_plan_tests.py at 504 lines (exceeds 500-line limit by 4 lines) — trivial to fix
  • Duplicated _ACTION_YAML to consolidate into helper_e2e_common.py

@brent.edwards — Please push the remaining Minor/Nit fixes (file length, YAML consolidation). This is M3 (v3.2.0) Must Have — needs to close.

@hurui200320 — After Minor fixes are pushed, please re-review and approve if your Critical/Major findings are resolved.

Who Action Deadline
@brent.edwards Push Minor/Nit fixes Day 37 EOD
@hurui200320 Re-review and approve Day 38 EOD
## PM Status — Day 36 (2026-03-16) **Review summary:** - Round 1 (`REQUEST_CHANGES` from @hurui200320): All Critical + Major findings addressed in `9606d52f` - Rounds 2+3 from @hamza.khyari: 17 additional findings (all Minor/Nit) - Brent posted response to Hamza's rounds on Day 35 **Outstanding items:** - `helper_wf01_plan_tests.py` at 504 lines (exceeds 500-line limit by 4 lines) — trivial to fix - Duplicated `_ACTION_YAML` to consolidate into `helper_e2e_common.py` @brent.edwards — Please push the remaining Minor/Nit fixes (file length, YAML consolidation). This is M3 (v3.2.0) Must Have — needs to close. @hurui200320 — After Minor fixes are pushed, please re-review and approve if your Critical/Major findings are resolved. | Who | Action | Deadline | |-----|--------|----------| | @brent.edwards | Push Minor/Nit fixes | Day 37 EOD | | @hurui200320 | Re-review and approve | Day 38 EOD |
freemo reviewed 2026-03-16 16:15:19 +00:00
freemo left a comment
Copy link

PM Day 36 Triage: M3 E2E test PR (v3.2.0). Lower priority than bug fixes and TDD infrastructure. Reviewer: @brent.edwards after critical path items clear.

PM Day 36 Triage: M3 E2E test PR (v3.2.0). Lower priority than bug fixes and TDD infrastructure. Reviewer: @brent.edwards after critical path items clear.
brent.edwards commented 2026-03-16 18:44:30 +00:00
Author
Member
Copy link

Status Update — Day 37

@freemo — The Minor/Nit fixes from Hamza's rounds were included in the squashed commit pushed earlier. Confirming:

Finding Status
F1 (504 lines) Fixed — extracted shared code, now under 500
F2 (duplicated _ACTION_YAML) Fixed — moved to helper_e2e_common.py
F3 (duplicated _WorkflowCtx) Fixed — base class in helper_e2e_common.py
F4 (duplicated _fail) Fixed — imports from helper_e2e_common.py
F5 (broad suppress) Fixed — narrowed to OSError, FileNotFoundError
R3-F1 (subprocess.run no timeout) Fixed — added timeout=30
R3-F2 (temp dir leak) Fixed — try/except with cleanup
R3-F3 (plan execute rc) Fixed — added returncode check

F7/F8 (scientific_paper_e2e_test.robot changes) are being handled by other agents — out of scope for this PR.

Master has been merged in. Branch is up to date and ready for re-review.

## Status Update — Day 37 @freemo — The Minor/Nit fixes from Hamza's rounds were included in the squashed commit pushed earlier. Confirming: | Finding | Status | |---------|--------| | F1 (504 lines) | **Fixed** — extracted shared code, now under 500 | | F2 (duplicated _ACTION_YAML) | **Fixed** — moved to helper_e2e_common.py | | F3 (duplicated _WorkflowCtx) | **Fixed** — base class in helper_e2e_common.py | | F4 (duplicated _fail) | **Fixed** — imports from helper_e2e_common.py | | F5 (broad suppress) | **Fixed** — narrowed to OSError, FileNotFoundError | | R3-F1 (subprocess.run no timeout) | **Fixed** — added timeout=30 | | R3-F2 (temp dir leak) | **Fixed** — try/except with cleanup | | R3-F3 (plan execute rc) | **Fixed** — added returncode check | F7/F8 (scientific_paper_e2e_test.robot changes) are being handled by other agents — out of scope for this PR. Master has been merged in. Branch is up to date and ready for re-review.
Merge branch 'master' into test/int-wf01-hello-world
All checks were successful
CI / benchmark-publish (pull_request) Has been skipped
Details
CI / lint (pull_request) Successful in 14s
Details
CI / build (pull_request) Successful in 20s
Details
CI / quality (pull_request) Successful in 35s
Details
CI / typecheck (pull_request) Successful in 40s
Details
CI / security (pull_request) Successful in 55s
Details
CI / e2e_tests (pull_request) Successful in 1m28s
Details
CI / unit_tests (pull_request) Successful in 3m29s
Details
CI / docker (pull_request) Successful in 1m7s
Details
CI / integration_tests (pull_request) Successful in 4m59s
Details
CI / coverage (pull_request) Successful in 6m26s
Details
CI / benchmark-regression (pull_request) Successful in 39m51s
Details
1782b191fe
freemo commented 2026-03-17 18:39:08 +00:00
Owner
Copy link

PM Status — Day 37 (2026-03-17)

Status: Brent confirmed all Minor/Nit fixes pushed (Day 37). File length, duplicated constants, subprocess timeouts, temp dir leak — all resolved. Branch up to date with master.

Blocker: Pending re-review from @hurui200320 to clear stale REQUEST_CHANGES.

Action required:

  • @hurui200320 — Re-review and approve if Critical/Major findings from Round 1 are resolved. Target: Day 38 EOD.
  • Merge strategy: This PR merges before #791 to avoid WF01 content overlap.

PM status — Day 37

## PM Status — Day 37 (2026-03-17) **Status**: Brent confirmed all Minor/Nit fixes pushed (Day 37). File length, duplicated constants, subprocess timeouts, temp dir leak — all resolved. Branch up to date with master. **Blocker**: Pending re-review from @hurui200320 to clear stale REQUEST_CHANGES. **Action required**: - @hurui200320 — Re-review and approve if Critical/Major findings from Round 1 are resolved. Target: Day 38 EOD. - Merge strategy: This PR merges before #791 to avoid WF01 content overlap. *PM status — Day 37*
hurui200320 requested changes 2026-03-18 07:56:03 +00:00
Dismissed
hurui200320 left a comment
Copy link

PR Review: !798 (Ticket #765)

Verdict: Request Changes

All 3 Critical and 6 Major findings from my Round 1 review (2026-03-13) and all 17 Minor/Nit findings from Hamza's rounds have been addressed. However, this deep-dive re-review discovered 2 new Major spec compliance gaps and several new Minor findings that were not caught in earlier rounds:

  1. The test uses plan lifecycle-apply instead of the specification's plan apply --yes
  2. The manual-profile workflow requires two plan execute calls (strategize + execute phases), but the test only issues one

These are spec compliance issues that should be corrected before merge. The remaining findings are Minor or Nit quality improvements.

Critical Issues

None.

Major Issues

1. ⭐NEW Test uses plan lifecycle-apply instead of spec's plan apply --yes

  • File: robot/helper_wf01_plan_tests.py, lines 200–206
  • Problem: The specification consistently uses agents plan apply --yes <PLAN_ID> for the apply step. The test instead invokes plan lifecycle-apply, which is the internal v3 lifecycle command. Per CONTRIBUTING.md: "When there is a discrepancy between the current codebase and this document [specification.md], always assume the specification is correct." The spec also includes the --yes flag for non-interactive confirmation, which the test omits.
  • Recommendation: Change "lifecycle-apply" to "apply" and add "--yes" flag: run_cli("plan", "apply", "--yes", plan_id, ...).

2. ⭐NEW Missing second plan execute call for manual profile workflow

  • File: robot/helper_wf01_plan_tests.py, lines 124–217 (wf01_plan_lifecycle())
  • Problem: The spec's Workflow Example 1 shows plan execute invoked twice for the manual profile: once to trigger the Strategize phase, and once to trigger the Execute phase (after tree/explain review). The manual profile's defining characteristic is explicit user triggers for each phase. The test only calls plan execute once, then proceeds directly to tree/diff/apply.
  • Recommendation: Add a second plan execute call after plan tree/plan explain, matching the spec's sequence: plan use → plan execute (strategize) → plan tree → plan explain → plan execute (execute) → plan diff → plan status → plan apply --yes.

3. Branch contains merge commits — violates linear history policy

  • Location: Branch test/int-wf01-hello-world (commits 1782b191, af1b5d5b)
  • Problem: The branch contains two Merge branch 'master' into test/int-wf01-hello-world merge commits. CONTRIBUTING.md requires clean linear history.
  • Recommendation: Squash-merge this PR, or rebase the branch onto current master before merging.

Minor Issues

1. init_bare_git_repo() is dead code and lacks try/except cleanup

  • File: robot/helper_e2e_common.py, lines 219–235
  • Problem: This function has zero callers (all were changed to init_test_git_repo), and unlike init_test_git_repo() it lacks try/except BaseException cleanup for temp dir leaks.
  • Recommendation: Remove the function entirely, or add equivalent error handling if kept for future use.

2. Plan state transitions test does not verify any actual state change

  • File: robot/helper_wf01_plan_tests.py, lines 220–286
  • Problem: wf01_plan_state_transitions() checks initial state (strategize/queued), runs plan execute, then only verifies the plan_id is still present — not that the state actually changed. The "transition" assertion is tautological.
  • Recommendation: At minimum, assert on the expected mock-AI behavior (e.g., "state remains strategize/queued under mock AI") with an explicit comment explaining this tests CLI wiring, not actual transitions.

3. Plan lifecycle tests (6–9) are smoke tests under mock AI — assertions verify "no crash" only

  • File: robot/helper_wf01_plan_tests.py, lines 124–377
  • Problem: Under mock AI, plan execute, plan tree, plan diff, and plan lifecycle-apply share the same assertion pattern: check for absence of "internal error"/"traceback" and accept returncode in (0, 1). No positive assertions on expected behavior.
  • Recommendation: Add at least one positive assertion per command that validates expected mock-AI output (e.g., assert plan tree returns "No decisions" or empty JSON, assert plan diff returns specific "not ready" message).

4. wf01_diff_output (test 9) is functionally redundant with wf01_plan_lifecycle (test 6)

  • File: robot/helper_wf01_plan_tests.py, lines 348–377
  • Problem: Identical assertions to the plan diff section of wf01_plan_lifecycle(), with an entirely independent workspace setup (~7 subprocess spawns + DB migration for zero additional coverage).
  • Recommendation: Remove, or make meaningfully different.

5. Plan lifecycle setup omits validation add/validation attach

  • File: robot/helper_wf01_plan_tests.py, lines 54–121 (setup_with_plan())
  • Problem: The spec's Workflow Example 1 shows validation registration before action creation. The plan lifecycle tests skip this, testing a different configuration than the spec describes.
  • Recommendation: Add validation add/attach to setup_with_plan() to match the spec's full workflow.

6. Missing on_timeout=kill on all Run Process calls

  • File: robot/wf01_hello_world.robot, all 10 test cases
  • Problem: All 10 Run Process calls specify timeout= but none include on_timeout=kill. Newer suites (m3_e2e_verification.robot, tdd_session_list_di.robot, etc.) consistently include it. Without it, timed-out subprocesses may leave orphaned processes under pabot parallel execution.
  • Recommendation: Add on_timeout=kill to every Run Process invocation.

7. WorkflowCtx.teardown() has non-resilient cleanup chain

  • File: robot/helper_e2e_common.py, lines 164–170
  • Problem: If os.unlink(self.yaml_path) raises (e.g., PermissionError), the subsequent shutil.rmtree(self.repo_dir) and cleanup_workspace(self.workspace) are skipped. Affects all 9 test functions using WorkflowCtx.
  • Recommendation: Wrap each cleanup step in contextlib.suppress(OSError) to make steps independent.

8. wf01_validation_register() — os.unlink failure blocks ctx.teardown()

  • File: robot/helper_wf01_hello_world.py, lines 247–250
  • Problem: In the finally block, if os.unlink(val_path) raises, ctx.teardown() is never called, leaking the workspace, git repo, and temp files.
  • Recommendation: Wrap the unlink in contextlib.suppress(OSError).

9. result.success check is dead code in wf01_post_apply_commit()

  • File: robot/helper_wf01_hello_world.py, lines 328–330
  • Problem: GitWorktreeSandbox.commit() never returns success=False — it raises SandboxCommitError on failure instead. The if not result.success branch is unreachable. On actual failure, the exception propagates uncaught producing an ugly traceback instead of the intended clean _fail() message.
  • Recommendation: Wrap sandbox.commit() in try/except SandboxCommitError as exc: _fail(...).

10. Standalone test assertions don't verify persistence (weaker than M1 pattern)

  • Files: robot/helper_wf01_hello_world.py — wf01_resource_register() (line 95), wf01_project_create() (line 134), wf01_action_create() (line 258)
  • Problem: After resource add, project create, and action create, the tests only check creation output — never running follow-up show commands to verify DB persistence. Compare with helper_m1_e2e_verification.py which explicitly runs resource show and project show as persistence checks. The WF01 tests would pass even if CLI output is correct but persistence fails.
  • Recommendation: Add resource show, project show follow-ups (matching M1 pattern). Also verify automation_profile: manual in action show output.

11. project create adds --invariant flag not in spec's Workflow Example 1

  • File: robot/helper_wf01_hello_world.py, lines 154–167
  • Problem: The spec's Workflow Example 1 project create uses only --description and --resource. The test adds --invariant "All tests must pass before apply" which is not part of the Hello World workflow.
  • Recommendation: Remove --invariant to match the spec exactly. If invariant testing is desired, add as separate test case.

12. Repeated crash-detection pattern (8 occurrences) violates DRY

  • File: robot/helper_wf01_plan_tests.py, 8 locations
  • Problem: The 5-line crash-detection block (combined = stdout+stderr; if "internal error"...; if returncode not in...) is copy-pasted 8 times (~40 lines of identical logic).
  • Recommendation: Extract to _assert_no_crash(result, label) helper function.

13. No test tagging in Robot suite

  • File: robot/wf01_hello_world.robot
  • Problem: No Force Tags or [Tags] on any test. Newer suites use Force Tags for tag-based filtering. Without tags, WF01 tests can't be selectively run via --include wf01.
  • Recommendation: Add Force Tags wf01 workflow manual_profile integration.

14. PR description says "9 tests" — actual count is 10

  • Location: PR body
  • Recommendation: Update to 10 and add the validation test to the numbered list.

15. Redundant full workflow setup across 4 plan tests adds CI overhead

  • File: robot/helper_wf01_plan_tests.py (4 functions)
  • Problem: ~28 redundant subprocess spawns and 4 migration runs for setup. Tests 8 and 9 are strict subsets of test 6.
  • Recommendation: Consolidate or share setup.

Nits

  1. ACTION_YAML uses openai/gpt-4 while spec uses different actor names. No functional impact under mock AI.
  2. Redundant split of imports into three separate from helper_e2e_common import statements in both helper files. Could be consolidated.
  3. Tree JSON parsing is likely dead code under mock AI — json.loads() branch never exercised.
  4. wf01_init has no side-effect verification — only checks rc=0.
  5. Tests accept rc=1 without logging what it means — add diagnostic logging when rc=1.
  6. Pre-existing inline import in helper_e2e_common.py:96-98 (MigrationRunner).
  7. Magic string ULIDs should be named constants — "01HWFTEST000000000000000WF" and "01HWFTEST00000000000DECDE0" should be module-level constants per M1 pattern.
  8. Robot file documentation says "lifecycle-apply" — should say "apply" per spec.
  9. wf01_plan_lifecycle() docstring omits plan status step — says "use → execute → tree → diff → apply" but code also checks status.
  10. Module docstring references review finding "(M5)" — helper_wf01_plan_tests.py line 3. Remove the review artifact reference.
  11. helper_e2e_common.py module docstring outdated — says "M1-M6 E2E verification" but now also hosts WF01 shared code.
  12. WorkflowCtx hardcodes "wf01_" prefix in shared code — should accept configurable prefix for reuse.
  13. wf01_init() creates unnecessary git repo + action YAML — ctx.setup() does full resource creation but init test only needs a workspace.

Verification of Previously Reported Issues

Round 1 (Rui) Status
C1: plan explain never invoked ✅ Fixed
C2: validation add/attach missing ✅ Fixed
C3: init_bare_git_repo() branch mismatch ✅ Fixed
M1–M6: All major findings ✅ Fixed
Rounds 2–3 (Hamza) Status
F1–F5, R3-F1–F3: All findings ✅ Fixed

Automated Tool Results

Tool Result
Pyright ✅ 0 errors
Ruff lint ✅ 0 violations
Ruff format ✅ All files properly formatted

Summary

The PR has undergone extensive rework and all previously-reported Critical/Major issues are resolved. The test suite covers every command from Workflow Example 1, follows project conventions on file organization, passes type checking and linting cleanly, and the commit/branch metadata matches the ticket.

However, this deep-dive reveals 2 new Major spec compliance gaps: (1) using plan lifecycle-apply instead of the spec-mandated plan apply --yes, and (2) only issuing one plan execute when the manual profile requires two (one per phase). These directly impact whether the test accurately validates the spec's Workflow Example 1 as described.

The Minor findings focus on test assertion quality (weaker than the M1 pattern), cleanup robustness, dead code, and Robot Framework conventions (on_timeout=kill, test tags). None are blockers individually, but collectively they represent room for improvement.

Recommendation: Fix the 2 Major spec compliance issues (apply command name + dual execute), then squash-merge. The remaining Minor/Nit items can be addressed in follow-up if prioritized.

## PR Review: !798 (Ticket #765) ### Verdict: Request Changes All 3 Critical and 6 Major findings from my Round 1 review (2026-03-13) and all 17 Minor/Nit findings from Hamza's rounds have been addressed. However, this deep-dive re-review discovered **2 new Major spec compliance gaps** and **several new Minor findings** that were not caught in earlier rounds: 1. The test uses `plan lifecycle-apply` instead of the specification's `plan apply --yes` 2. The manual-profile workflow requires **two** `plan execute` calls (strategize + execute phases), but the test only issues one These are spec compliance issues that should be corrected before merge. The remaining findings are Minor or Nit quality improvements. --- ### Critical Issues None. --- ### Major Issues **1. ⭐NEW Test uses `plan lifecycle-apply` instead of spec's `plan apply --yes`** - **File:** `robot/helper_wf01_plan_tests.py`, lines 200–206 - **Problem:** The specification consistently uses `agents plan apply --yes <PLAN_ID>` for the apply step. The test instead invokes `plan lifecycle-apply`, which is the internal v3 lifecycle command. Per CONTRIBUTING.md: "When there is a discrepancy between the current codebase and this document [specification.md], always assume the specification is correct." The spec also includes the `--yes` flag for non-interactive confirmation, which the test omits. - **Recommendation:** Change `"lifecycle-apply"` to `"apply"` and add `"--yes"` flag: `run_cli("plan", "apply", "--yes", plan_id, ...)`. **2. ⭐NEW Missing second `plan execute` call for manual profile workflow** - **File:** `robot/helper_wf01_plan_tests.py`, lines 124–217 (`wf01_plan_lifecycle()`) - **Problem:** The spec's Workflow Example 1 shows `plan execute` invoked **twice** for the manual profile: once to trigger the Strategize phase, and once to trigger the Execute phase (after tree/explain review). The manual profile's defining characteristic is explicit user triggers for each phase. The test only calls `plan execute` once, then proceeds directly to tree/diff/apply. - **Recommendation:** Add a second `plan execute` call after `plan tree`/`plan explain`, matching the spec's sequence: `plan use → plan execute (strategize) → plan tree → plan explain → plan execute (execute) → plan diff → plan status → plan apply --yes`. **3. Branch contains merge commits — violates linear history policy** - **Location:** Branch `test/int-wf01-hello-world` (commits `1782b191`, `af1b5d5b`) - **Problem:** The branch contains two `Merge branch 'master' into test/int-wf01-hello-world` merge commits. CONTRIBUTING.md requires clean linear history. - **Recommendation:** Squash-merge this PR, or rebase the branch onto current `master` before merging. --- ### Minor Issues **1. `init_bare_git_repo()` is dead code and lacks try/except cleanup** - **File:** `robot/helper_e2e_common.py`, lines 219–235 - **Problem:** This function has **zero callers** (all were changed to `init_test_git_repo`), and unlike `init_test_git_repo()` it lacks `try/except BaseException` cleanup for temp dir leaks. - **Recommendation:** Remove the function entirely, or add equivalent error handling if kept for future use. **2. Plan state transitions test does not verify any actual state change** - **File:** `robot/helper_wf01_plan_tests.py`, lines 220–286 - **Problem:** `wf01_plan_state_transitions()` checks initial state (`strategize/queued`), runs `plan execute`, then only verifies the `plan_id` is still present — not that the state actually changed. The "transition" assertion is tautological. - **Recommendation:** At minimum, assert on the expected mock-AI behavior (e.g., "state remains strategize/queued under mock AI") with an explicit comment explaining this tests CLI wiring, not actual transitions. **3. Plan lifecycle tests (6–9) are smoke tests under mock AI — assertions verify "no crash" only** - **File:** `robot/helper_wf01_plan_tests.py`, lines 124–377 - **Problem:** Under mock AI, `plan execute`, `plan tree`, `plan diff`, and `plan lifecycle-apply` share the same assertion pattern: check for absence of "internal error"/"traceback" and accept `returncode in (0, 1)`. No positive assertions on expected behavior. - **Recommendation:** Add at least one positive assertion per command that validates expected mock-AI output (e.g., assert `plan tree` returns "No decisions" or empty JSON, assert `plan diff` returns specific "not ready" message). **4. `wf01_diff_output` (test 9) is functionally redundant with `wf01_plan_lifecycle` (test 6)** - **File:** `robot/helper_wf01_plan_tests.py`, lines 348–377 - **Problem:** Identical assertions to the `plan diff` section of `wf01_plan_lifecycle()`, with an entirely independent workspace setup (~7 subprocess spawns + DB migration for zero additional coverage). - **Recommendation:** Remove, or make meaningfully different. **5. Plan lifecycle setup omits `validation add`/`validation attach`** - **File:** `robot/helper_wf01_plan_tests.py`, lines 54–121 (`setup_with_plan()`) - **Problem:** The spec's Workflow Example 1 shows validation registration before action creation. The plan lifecycle tests skip this, testing a different configuration than the spec describes. - **Recommendation:** Add validation add/attach to `setup_with_plan()` to match the spec's full workflow. **6. Missing `on_timeout=kill` on all `Run Process` calls** - **File:** `robot/wf01_hello_world.robot`, all 10 test cases - **Problem:** All 10 `Run Process` calls specify `timeout=` but none include `on_timeout=kill`. Newer suites (`m3_e2e_verification.robot`, `tdd_session_list_di.robot`, etc.) consistently include it. Without it, timed-out subprocesses may leave orphaned processes under pabot parallel execution. - **Recommendation:** Add `on_timeout=kill` to every `Run Process` invocation. **7. `WorkflowCtx.teardown()` has non-resilient cleanup chain** - **File:** `robot/helper_e2e_common.py`, lines 164–170 - **Problem:** If `os.unlink(self.yaml_path)` raises (e.g., PermissionError), the subsequent `shutil.rmtree(self.repo_dir)` and `cleanup_workspace(self.workspace)` are skipped. Affects all 9 test functions using `WorkflowCtx`. - **Recommendation:** Wrap each cleanup step in `contextlib.suppress(OSError)` to make steps independent. **8. `wf01_validation_register()` — `os.unlink` failure blocks `ctx.teardown()`** - **File:** `robot/helper_wf01_hello_world.py`, lines 247–250 - **Problem:** In the `finally` block, if `os.unlink(val_path)` raises, `ctx.teardown()` is never called, leaking the workspace, git repo, and temp files. - **Recommendation:** Wrap the unlink in `contextlib.suppress(OSError)`. **9. `result.success` check is dead code in `wf01_post_apply_commit()`** - **File:** `robot/helper_wf01_hello_world.py`, lines 328–330 - **Problem:** `GitWorktreeSandbox.commit()` never returns `success=False` — it raises `SandboxCommitError` on failure instead. The `if not result.success` branch is unreachable. On actual failure, the exception propagates uncaught producing an ugly traceback instead of the intended clean `_fail()` message. - **Recommendation:** Wrap `sandbox.commit()` in `try/except SandboxCommitError as exc: _fail(...)`. **10. Standalone test assertions don't verify persistence (weaker than M1 pattern)** - **Files:** `robot/helper_wf01_hello_world.py` — `wf01_resource_register()` (line 95), `wf01_project_create()` (line 134), `wf01_action_create()` (line 258) - **Problem:** After `resource add`, `project create`, and `action create`, the tests only check creation output — never running follow-up `show` commands to verify DB persistence. Compare with `helper_m1_e2e_verification.py` which explicitly runs `resource show` and `project show` as persistence checks. The WF01 tests would pass even if CLI output is correct but persistence fails. - **Recommendation:** Add `resource show`, `project show` follow-ups (matching M1 pattern). Also verify `automation_profile: manual` in `action show` output. **11. `project create` adds `--invariant` flag not in spec's Workflow Example 1** - **File:** `robot/helper_wf01_hello_world.py`, lines 154–167 - **Problem:** The spec's Workflow Example 1 `project create` uses only `--description` and `--resource`. The test adds `--invariant "All tests must pass before apply"` which is not part of the Hello World workflow. - **Recommendation:** Remove `--invariant` to match the spec exactly. If invariant testing is desired, add as separate test case. **12. Repeated crash-detection pattern (8 occurrences) violates DRY** - **File:** `robot/helper_wf01_plan_tests.py`, 8 locations - **Problem:** The 5-line crash-detection block (`combined = stdout+stderr; if "internal error"...; if returncode not in...`) is copy-pasted 8 times (~40 lines of identical logic). - **Recommendation:** Extract to `_assert_no_crash(result, label)` helper function. **13. No test tagging in Robot suite** - **File:** `robot/wf01_hello_world.robot` - **Problem:** No `Force Tags` or `[Tags]` on any test. Newer suites use `Force Tags` for tag-based filtering. Without tags, WF01 tests can't be selectively run via `--include wf01`. - **Recommendation:** Add `Force Tags wf01 workflow manual_profile integration`. **14. PR description says "9 tests" — actual count is 10** - **Location:** PR body - **Recommendation:** Update to 10 and add the validation test to the numbered list. **15. Redundant full workflow setup across 4 plan tests adds CI overhead** - **File:** `robot/helper_wf01_plan_tests.py` (4 functions) - **Problem:** ~28 redundant subprocess spawns and 4 migration runs for setup. Tests 8 and 9 are strict subsets of test 6. - **Recommendation:** Consolidate or share setup. --- ### Nits 1. **ACTION_YAML uses `openai/gpt-4`** while spec uses different actor names. No functional impact under mock AI. 2. **Redundant split of imports** into three separate `from helper_e2e_common import` statements in both helper files. Could be consolidated. 3. **Tree JSON parsing is likely dead code** under mock AI — `json.loads()` branch never exercised. 4. **`wf01_init` has no side-effect verification** — only checks rc=0. 5. **Tests accept rc=1 without logging** what it means — add diagnostic logging when rc=1. 6. **Pre-existing inline import** in `helper_e2e_common.py:96-98` (`MigrationRunner`). 7. **Magic string ULIDs should be named constants** — `"01HWFTEST000000000000000WF"` and `"01HWFTEST00000000000DECDE0"` should be module-level constants per M1 pattern. 8. **Robot file documentation says "lifecycle-apply"** — should say "apply" per spec. 9. **`wf01_plan_lifecycle()` docstring omits `plan status` step** — says "use → execute → tree → diff → apply" but code also checks status. 10. **Module docstring references review finding "(M5)"** — `helper_wf01_plan_tests.py` line 3. Remove the review artifact reference. 11. **`helper_e2e_common.py` module docstring outdated** — says "M1-M6 E2E verification" but now also hosts WF01 shared code. 12. **`WorkflowCtx` hardcodes `"wf01_"` prefix** in shared code — should accept configurable prefix for reuse. 13. **`wf01_init()` creates unnecessary git repo + action YAML** — `ctx.setup()` does full resource creation but init test only needs a workspace. --- ### Verification of Previously Reported Issues | Round 1 (Rui) | Status | |---|---| | C1: `plan explain` never invoked | ✅ Fixed | | C2: `validation add`/`attach` missing | ✅ Fixed | | C3: `init_bare_git_repo()` branch mismatch | ✅ Fixed | | M1–M6: All major findings | ✅ Fixed | | Rounds 2–3 (Hamza) | Status | |---|---| | F1–F5, R3-F1–F3: All findings | ✅ Fixed | ### Automated Tool Results | Tool | Result | |------|--------| | Pyright | ✅ 0 errors | | Ruff lint | ✅ 0 violations | | Ruff format | ✅ All files properly formatted | --- ### Summary The PR has undergone extensive rework and all previously-reported Critical/Major issues are resolved. The test suite covers every command from Workflow Example 1, follows project conventions on file organization, passes type checking and linting cleanly, and the commit/branch metadata matches the ticket. However, this deep-dive reveals **2 new Major spec compliance gaps**: (1) using `plan lifecycle-apply` instead of the spec-mandated `plan apply --yes`, and (2) only issuing one `plan execute` when the manual profile requires two (one per phase). These directly impact whether the test accurately validates the spec's Workflow Example 1 as described. The Minor findings focus on test assertion quality (weaker than the M1 pattern), cleanup robustness, dead code, and Robot Framework conventions (`on_timeout=kill`, test tags). None are blockers individually, but collectively they represent room for improvement. **Recommendation:** Fix the 2 Major spec compliance issues (apply command name + dual execute), then squash-merge. The remaining Minor/Nit items can be addressed in follow-up if prioritized.
brent.edwards force-pushed test/int-wf01-hello-world from 1782b191fe
All checks were successful
CI / benchmark-publish (pull_request) Has been skipped
Details
CI / lint (pull_request) Successful in 14s
Details
CI / build (pull_request) Successful in 20s
Details
CI / quality (pull_request) Successful in 35s
Details
CI / typecheck (pull_request) Successful in 40s
Details
CI / security (pull_request) Successful in 55s
Details
CI / e2e_tests (pull_request) Successful in 1m28s
Details
CI / unit_tests (pull_request) Successful in 3m29s
Details
CI / docker (pull_request) Successful in 1m7s
Details
CI / integration_tests (pull_request) Successful in 4m59s
Details
CI / coverage (pull_request) Successful in 6m26s
Details
CI / benchmark-regression (pull_request) Successful in 39m51s
Details
to 978593cc0c
Some checks failed
CI / lint (pull_request) Failing after 23s
Details
CI / benchmark-publish (pull_request) Has been skipped
Details
CI / typecheck (pull_request) Successful in 43s
Details
CI / coverage (pull_request) Has been skipped
Details
CI / quality (pull_request) Successful in 38s
Details
CI / security (pull_request) Successful in 1m5s
Details
CI / build (pull_request) Successful in 39s
Details
CI / unit_tests (pull_request) Successful in 3m45s
Details
CI / docker (pull_request) Has been skipped
Details
CI / e2e_tests (pull_request) Successful in 4m51s
Details
CI / benchmark-regression (pull_request) Has been skipped
Details
CI / integration_tests (pull_request) Failing after 7m14s
Details
2026-03-18 22:14:30 +00:00 Compare
brent.edwards force-pushed test/int-wf01-hello-world from 978593cc0c
Some checks failed
CI / lint (pull_request) Failing after 23s
Details
CI / benchmark-publish (pull_request) Has been skipped
Details
CI / typecheck (pull_request) Successful in 43s
Details
CI / coverage (pull_request) Has been skipped
Details
CI / quality (pull_request) Successful in 38s
Details
CI / security (pull_request) Successful in 1m5s
Details
CI / build (pull_request) Successful in 39s
Details
CI / unit_tests (pull_request) Successful in 3m45s
Details
CI / docker (pull_request) Has been skipped
Details
CI / e2e_tests (pull_request) Successful in 4m51s
Details
CI / benchmark-regression (pull_request) Has been skipped
Details
CI / integration_tests (pull_request) Failing after 7m14s
Details
to 139d6a8b2e
Some checks failed
CI / lint (pull_request) Successful in 25s
Details
CI / benchmark-publish (pull_request) Has been skipped
Details
CI / quality (pull_request) Successful in 30s
Details
CI / build (pull_request) Successful in 15s
Details
CI / typecheck (pull_request) Successful in 52s
Details
CI / security (pull_request) Successful in 1m4s
Details
CI / unit_tests (pull_request) Successful in 3m38s
Details
CI / docker (pull_request) Successful in 59s
Details
CI / e2e_tests (pull_request) Successful in 5m6s
Details
CI / integration_tests (pull_request) Failing after 7m6s
Details
CI / coverage (pull_request) Successful in 7m23s
Details
CI / benchmark-regression (pull_request) Successful in 38m42s
Details
2026-03-18 22:56:54 +00:00 Compare
brent.edwards commented 2026-03-18 22:58:51 +00:00
Author
Member
Copy link

Review Fixes Applied — Commit 139d6a8b

Branch rebased onto origin/master (ad98d41d). Merge commits eliminated. Both Major findings from Rui Hu's R3 review addressed.

Finding Severity Fix
Major 1: lifecycle-apply → plan apply --yes Major Replaced run_cli("plan", "lifecycle-apply", ...) with run_cli("plan", "apply", "--yes", ...) in wf01_plan_lifecycle(). Updated robot documentation.
Major 2: Missing second plan execute call Major Added second plan execute invocation for the execute phase (manual profile requires strategize + execute as separate calls). Uses separate r_exec1/r_exec2 variables with phase-specific error messages.
CHANGELOG — Added entry for #765
Lint fix — Removed unused init_bare_git_repo import in helper_m1_e2e_verification.py

Quality Gates

  • nox -s lint — PASS
  • nox -s typecheck — PASS (0 errors)
  • Single commit, no merge commits, rebased on ad98d41d
## Review Fixes Applied — Commit `139d6a8b` Branch rebased onto `origin/master` (`ad98d41d`). Merge commits eliminated. Both Major findings from Rui Hu's R3 review addressed. | Finding | Severity | Fix | |---------|----------|-----| | **Major 1**: `lifecycle-apply` → `plan apply --yes` | Major | Replaced `run_cli("plan", "lifecycle-apply", ...)` with `run_cli("plan", "apply", "--yes", ...)` in `wf01_plan_lifecycle()`. Updated robot documentation. | | **Major 2**: Missing second `plan execute` call | Major | Added second `plan execute` invocation for the execute phase (manual profile requires strategize + execute as separate calls). Uses separate `r_exec1`/`r_exec2` variables with phase-specific error messages. | | **CHANGELOG** | — | Added entry for #765 | | **Lint fix** | — | Removed unused `init_bare_git_repo` import in `helper_m1_e2e_verification.py` | ### Quality Gates - `nox -s lint` — **PASS** - `nox -s typecheck` — **PASS** (0 errors) - Single commit, no merge commits, rebased on `ad98d41d`
brent.edwards force-pushed test/int-wf01-hello-world from 139d6a8b2e
Some checks failed
CI / lint (pull_request) Successful in 25s
Details
CI / benchmark-publish (pull_request) Has been skipped
Details
CI / quality (pull_request) Successful in 30s
Details
CI / build (pull_request) Successful in 15s
Details
CI / typecheck (pull_request) Successful in 52s
Details
CI / security (pull_request) Successful in 1m4s
Details
CI / unit_tests (pull_request) Successful in 3m38s
Details
CI / docker (pull_request) Successful in 59s
Details
CI / e2e_tests (pull_request) Successful in 5m6s
Details
CI / integration_tests (pull_request) Failing after 7m6s
Details
CI / coverage (pull_request) Successful in 7m23s
Details
CI / benchmark-regression (pull_request) Successful in 38m42s
Details
to 4f52866350
Some checks failed
CI / lint (pull_request) Successful in 18s
Details
CI / typecheck (pull_request) Successful in 44s
Details
CI / quality (pull_request) Successful in 29s
Details
CI / security (pull_request) Successful in 51s
Details
CI / benchmark-publish (pull_request) Has been skipped
Details
CI / build (pull_request) Successful in 15s
Details
CI / unit_tests (pull_request) Successful in 4m0s
Details
CI / integration_tests (pull_request) Failing after 4m30s
Details
CI / e2e_tests (pull_request) Successful in 5m26s
Details
CI / docker (pull_request) Successful in 58s
Details
CI / coverage (pull_request) Failing after 22m53s
Details
CI / benchmark-regression (pull_request) Successful in 38m23s
Details
2026-03-20 00:11:16 +00:00 Compare
brent.edwards commented 2026-03-20 03:59:40 +00:00
Author
Member
Copy link

Rebased onto origin/master (79b0a2c5). CHANGELOG conflict resolved (kept master, re-added PR entry). nox -s lint PASS, nox -s typecheck PASS (0 errors). Commit 4f528663.

Rebased onto `origin/master` (`79b0a2c5`). CHANGELOG conflict resolved (kept master, re-added PR entry). `nox -s lint` PASS, `nox -s typecheck` PASS (0 errors). Commit `4f528663`.
Merge remote-tracking branch 'origin/master' into test/int-wf01-hello-world
All checks were successful
CI / benchmark-publish (pull_request) Has been skipped
Details
CI / build (pull_request) Successful in 16s
Details
CI / lint (pull_request) Successful in 3m30s
Details
CI / quality (pull_request) Successful in 3m43s
Details
CI / typecheck (pull_request) Successful in 3m58s
Details
CI / security (pull_request) Successful in 4m6s
Details
CI / unit_tests (pull_request) Successful in 6m7s
Details
CI / docker (pull_request) Successful in 1m3s
Details
CI / integration_tests (pull_request) Successful in 6m58s
Details
CI / e2e_tests (pull_request) Successful in 9m40s
Details
CI / coverage (pull_request) Successful in 10m13s
Details
CI / status-check (pull_request) Successful in 1s
Details
CI / benchmark-regression (pull_request) Successful in 52m29s
Details
06c1a32f04 
# Conflicts:
#	CHANGELOG.md
hurui200320 requested changes 2026-03-26 05:40:56 +00:00
hurui200320 left a comment
Copy link

PR Review: !798 (Ticket #765)

Verdict: Request Changes

The PR is in much better shape after prior review rounds, but there are still a few functional/spec-validation gaps that should be resolved before approval.

Critical Issues

None.

Major Issues

  1. Manual-profile lifecycle sequence is out of spec order

    • Location: robot/helper_wf01_plan_tests.py:151-204
    • Problem: The test runs plan execute twice before plan tree / plan explain. Manual profile expects a review checkpoint between strategize and execute.
    • Recommendation: Reorder flow to:
      plan use -> execute (strategize) -> tree -> explain -> execute (execute) -> diff -> status -> apply --yes.

  2. Lifecycle setup does not include validation add/attach in the same flow

    • Location: robot/helper_wf01_plan_tests.py:54-121
    • Problem: setup_with_plan() covers resource/project/action/plan-use, but not validation add + validation attach in that lifecycle path.
    • Recommendation: Include validation registration/attachment in lifecycle setup (or add one full end-to-end path that includes it).

  3. plan tree / plan explain acceptance is mostly smoke-test only

    • Location: robot/helper_wf01_plan_tests.py:312-367
    • Problem: Assertions mainly check no traceback and rc (0,1); no meaningful structure validation of explain output using a real decision id.
    • Recommendation: Derive a real decision id from tree output and assert expected explain structure/fields.

  4. plan diff expected changeset is not verified

    • Location: robot/helper_wf01_plan_tests.py:205-220, 371-397
    • Problem: Current checks only validate non-crash behavior.
    • Recommendation: Assert deterministic diff expectations (content/paths or explicit mock-mode contract output).

  5. State-transition test does not verify an actual transition

    • Location: robot/helper_wf01_plan_tests.py:243-307
    • Problem: It confirms initial state and later presence of plan_id, but does not assert concrete phase/state progression.
    • Recommendation: Assert expected post-execute state behavior (or explicitly document/assert mock-specific expected stability).

  6. Post-apply commit check is not tied to plan apply --yes workflow

    • Location: robot/helper_wf01_hello_world.py:296-355
    • Problem: Commit assertion validates sandbox commit behavior directly, not commit creation as a result of lifecycle apply.
    • Recommendation: Verify commit existence immediately after plan apply --yes in the same tested workflow.

  7. Plan ID extraction is brittle

    • Location: robot/helper_wf01_plan_tests.py:45-48, 109-111
    • Problem: First-ULID extraction from plain output can break if output format changes.
    • Recommendation: Parse JSON output and read plan_id explicitly.

  8. Branch contains merge commit (linear-history hygiene)

    • Location: Branch history (06c1a32f...)
    • Problem: Merge commit in PR branch conflicts with strict linear-history conventions.
    • Recommendation: Rebase/squash to keep linear history before merge.

Minor Issues

  1. Robot Run Process calls lack on_timeout=kill

    • Location: robot/wf01_hello_world.robot:20,28,36,44,52,60,68,76,84,92
    • Problem: Timeout without hard-kill can leave orphaned child processes in CI.
    • Recommendation: Add on_timeout=kill to each invocation.

  2. Teardown cleanup is not failure-isolated

    • Location: robot/helper_e2e_common.py:177-183
    • Problem: If one cleanup step fails, later cleanup may be skipped.
    • Recommendation: Guard cleanup steps independently (contextlib.suppress(...) / separate try blocks).

  3. Validation helper cleanup can skip teardown

    • Location: robot/helper_wf01_hello_world.py:247-250
    • Problem: os.unlink(val_path) failure can prevent ctx.teardown().
    • Recommendation: Wrap unlink safely and always run teardown.

  4. Function-local import violates import style convention

    • Location: robot/helper_e2e_common.py:96-98
    • Problem: MigrationRunner imported inside function.
    • Recommendation: Move to module-level imports.

  5. Redundant expensive setup across plan tests

    • Location: robot/helper_wf01_plan_tests.py (multiple setup call sites)
    • Problem: Repeated full setup inflates CI runtime.
    • Recommendation: Consolidate shared setup where possible while preserving test isolation.

Nits

None.

Summary

This PR is close, but there are still blockers for approval.

Blockers (must-fix for approval)

  1. Manual-profile lifecycle sequence ordering in tests (execute/review/execute).
  2. Acceptance criteria assertions need to validate behavior (not just no-crash), especially:
    • state transitions,
    • tree/explain output structure,
    • diff expectations.
  3. Post-apply commit assertion should be tied directly to plan apply --yes workflow result.

Non-blocking / follow-up candidates

  • Timeout hard-kill, teardown resilience, import placement, setup optimization, branch-history cleanup (if squash/rebase policy allows handling at merge time).

Note: This report is generated by OpenAI GPT-5.3 Codex with OpenCode, under Rui Hu’s supervision.

## PR Review: !798 (Ticket #765) ### Verdict: Request Changes The PR is in much better shape after prior review rounds, but there are still a few functional/spec-validation gaps that should be resolved before approval. ### Critical Issues None. ### Major Issues 1. **Manual-profile lifecycle sequence is out of spec order** - **Location:** `robot/helper_wf01_plan_tests.py:151-204` - **Problem:** The test runs `plan execute` twice before `plan tree` / `plan explain`. Manual profile expects a review checkpoint between strategize and execute. - **Recommendation:** Reorder flow to: `plan use -> execute (strategize) -> tree -> explain -> execute (execute) -> diff -> status -> apply --yes`. 2. **Lifecycle setup does not include validation add/attach in the same flow** - **Location:** `robot/helper_wf01_plan_tests.py:54-121` - **Problem:** `setup_with_plan()` covers resource/project/action/plan-use, but not `validation add` + `validation attach` in that lifecycle path. - **Recommendation:** Include validation registration/attachment in lifecycle setup (or add one full end-to-end path that includes it). 3. **`plan tree` / `plan explain` acceptance is mostly smoke-test only** - **Location:** `robot/helper_wf01_plan_tests.py:312-367` - **Problem:** Assertions mainly check no traceback and rc `(0,1)`; no meaningful structure validation of explain output using a real decision id. - **Recommendation:** Derive a real decision id from tree output and assert expected explain structure/fields. 4. **`plan diff` expected changeset is not verified** - **Location:** `robot/helper_wf01_plan_tests.py:205-220`, `371-397` - **Problem:** Current checks only validate non-crash behavior. - **Recommendation:** Assert deterministic diff expectations (content/paths or explicit mock-mode contract output). 5. **State-transition test does not verify an actual transition** - **Location:** `robot/helper_wf01_plan_tests.py:243-307` - **Problem:** It confirms initial state and later presence of plan_id, but does not assert concrete phase/state progression. - **Recommendation:** Assert expected post-execute state behavior (or explicitly document/assert mock-specific expected stability). 6. **Post-apply commit check is not tied to `plan apply --yes` workflow** - **Location:** `robot/helper_wf01_hello_world.py:296-355` - **Problem:** Commit assertion validates sandbox commit behavior directly, not commit creation as a result of lifecycle apply. - **Recommendation:** Verify commit existence immediately after `plan apply --yes` in the same tested workflow. 7. **Plan ID extraction is brittle** - **Location:** `robot/helper_wf01_plan_tests.py:45-48`, `109-111` - **Problem:** First-ULID extraction from plain output can break if output format changes. - **Recommendation:** Parse JSON output and read `plan_id` explicitly. 8. **Branch contains merge commit (linear-history hygiene)** - **Location:** Branch history (`06c1a32f...`) - **Problem:** Merge commit in PR branch conflicts with strict linear-history conventions. - **Recommendation:** Rebase/squash to keep linear history before merge. ### Minor Issues 1. **Robot `Run Process` calls lack `on_timeout=kill`** - **Location:** `robot/wf01_hello_world.robot:20,28,36,44,52,60,68,76,84,92` - **Problem:** Timeout without hard-kill can leave orphaned child processes in CI. - **Recommendation:** Add `on_timeout=kill` to each invocation. 2. **Teardown cleanup is not failure-isolated** - **Location:** `robot/helper_e2e_common.py:177-183` - **Problem:** If one cleanup step fails, later cleanup may be skipped. - **Recommendation:** Guard cleanup steps independently (`contextlib.suppress(...)` / separate try blocks). 3. **Validation helper cleanup can skip teardown** - **Location:** `robot/helper_wf01_hello_world.py:247-250` - **Problem:** `os.unlink(val_path)` failure can prevent `ctx.teardown()`. - **Recommendation:** Wrap unlink safely and always run teardown. 4. **Function-local import violates import style convention** - **Location:** `robot/helper_e2e_common.py:96-98` - **Problem:** `MigrationRunner` imported inside function. - **Recommendation:** Move to module-level imports. 5. **Redundant expensive setup across plan tests** - **Location:** `robot/helper_wf01_plan_tests.py` (multiple setup call sites) - **Problem:** Repeated full setup inflates CI runtime. - **Recommendation:** Consolidate shared setup where possible while preserving test isolation. ### Nits None. --- ### Summary This PR is close, but there are still **blockers** for approval. #### Blockers (must-fix for approval) 1. Manual-profile lifecycle sequence ordering in tests (execute/review/execute). 2. Acceptance criteria assertions need to validate behavior (not just no-crash), especially: - state transitions, - tree/explain output structure, - diff expectations. 3. Post-apply commit assertion should be tied directly to `plan apply --yes` workflow result. #### Non-blocking / follow-up candidates - Timeout hard-kill, teardown resilience, import placement, setup optimization, branch-history cleanup (if squash/rebase policy allows handling at merge time). --- **Note:** This report is generated by **OpenAI GPT-5.3 Codex with OpenCode**, under **Rui Hu’s supervision**.
freemo self-assigned this 2026-04-02 06:15:23 +00:00
freemo commented 2026-04-02 17:33:53 +00:00
Owner
Copy link

🤖 Backlog Groomer (groomer-1): Closing as duplicate of #765.

Issue #765 (test(integration): workflow example 1 — hello world, fix a single bug) is the canonical version with full labels (MoSCoW/Must have, Priority/Medium, State/In Review, Type/Testing) and milestone v3.2.0. This issue is an exact title duplicate.

🤖 **Backlog Groomer (groomer-1):** Closing as duplicate of #765. Issue #765 (`test(integration): workflow example 1 — hello world, fix a single bug`) is the canonical version with full labels (`MoSCoW/Must have`, `Priority/Medium`, `State/In Review`, `Type/Testing`) and milestone `v3.2.0`. This issue is an exact title duplicate.
freemo closed this pull request 2026-04-02 17:34:06 +00:00
All checks were successful
CI / benchmark-publish (pull_request) Has been skipped
Details
CI / build (pull_request) Successful in 16s
Required
Details
CI / lint (pull_request) Successful in 3m30s
Required
Details
CI / quality (pull_request) Successful in 3m43s
Required
Details
CI / typecheck (pull_request) Successful in 3m58s
Required
Details
CI / security (pull_request) Successful in 4m6s
Required
Details
CI / unit_tests (pull_request) Successful in 6m7s
Required
Details
CI / docker (pull_request) Successful in 1m3s
Required
Details
CI / integration_tests (pull_request) Successful in 6m58s
Required
Details
CI / e2e_tests (pull_request) Successful in 9m40s
Details
CI / coverage (pull_request) Successful in 10m13s
Required
Details
CI / status-check (pull_request) Successful in 1s
Details
CI / benchmark-regression (pull_request) Successful in 52m29s
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
freemo
hurui200320
Labels
Clear labels   
auto/needs-reevaluation
Controller deferred this PR; awaiting Phase 6+ scope-evaluator or operator re-enablement.

  
controller-managed
Auto-agents controller manages this PR/issue (see tools/controller/deploy/RUNBOOK.md). Remove this label to abandon controller management.

  
auto/blocked-by-deps
PR blocked by an open issue dependency. Operator must close the dep (or remove the dependency link) before the merge driver can act. Auto-cleared by merge_drive when no open deps remain.

  
auto/ci-timeout
Most recent merge cycle hit CI timeout. Driver excludes this PR while last merge_cycle row is < 30 min old; label persists thereafter as visible history.

  
auto/claimed-implementer
Currently being processed by an implementer worker.

  
auto/claimed-merge
Currently being processed by the merge driver.

  
auto/claimed-reviewer
Currently being processed by a reviewer worker.

  
auto/driver-down
Merge driver heartbeat stale; pipeline halted. Closed automatically on next clean tick.

  
auto/invariant-violation
Detected master commit violating the strict merge invariant. Tracked as an issue (not a PR label); kept here for label completeness.

  
auto/last-attempt-tier-0
In-cycle escalation: most recent attempt ran at the Tier 0 slot (`tier-0`). Slot's model defined in .opencode/models/tiers.yaml.

  
auto/last-attempt-tier-1
In-cycle escalation: most recent attempt ran at the Tier 1 slot (`tier-1`). Slot's model defined in .opencode/models/tiers.yaml.

  
auto/last-attempt-tier-2
In-cycle escalation: most recent attempt ran at the Tier 2 slot (`tier-2`). Slot's model defined in .opencode/models/tiers.yaml. Gated behind IMPLEMENTER_ESCALATION_TIER2_ENABLED.

  
auto/last-attempt-tier-min
In-cycle escalation: most recent attempt ran at the Tier -1 slot (`tier-min`). Slot's model defined in .opencode/models/tiers.yaml. Suffix is ``-min`` (not ``--1``) so the Forgejo UI reads naturally.

  
Automation Tracking
Tracking issues used by the AI Automation system for agents to communicate and report.

  
auto/needs-conflict-resolution
Rebase conflict needs LLM conflict-resolver.

  
auto/needs-implementer
Failing CI needs implementer attention.

  
auto/postmortem
Documenting a driver incident or rollback.

  
auto/ready-to-merge
Reviewer has APPROVED this PR and no later REQUEST_CHANGES is outstanding. The merge driver requires this label to even consider a PR for merging. Set by the reviewer worker on APPROVE; cleared on REQUEST_CHANGES.

  
auto/restart-throttled
Train repeatedly lost master-tempo races. Driver excludes via merge_cycle until cooldown elapses; label persists as visible history.

  
auto/revert
Revert PR backing out an invariant violation. Fast-tracked through the merge driver.

  
auto/sentinel
Sentinel PR duplicated from upstream into a personal fork by tools/duplicate_prs_to_fork.py for pipeline testing. Lives only in the fork; the canonical pipeline never sees it.

  
auto/stale-inactivity
No implementer activity for N days. Flagged for human review. Auto-cleared on next push to head branch.

  
auto/unstable
Repeatedly fails on current master (>= 3 ci-fail-on-rebased-sha releases in 12 h). Excluded from driver until human triage.

  
Blocked
A ticket in a blocked state and unable to complete until some other task is completed first.

  
Bounty
$100
 A bounty of $100 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$1000
 A bounty of $1000 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$10000
 A bounty of $10000 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$20
 A bounty of $20 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$2000
 A bounty of $2000 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$250
 A bounty of $250 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$50
 A bounty of $50 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$500
 A bounty of $500 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$5000
 A bounty of $5000 for any open-source contributor who provides a MR that solves this issue

  
Bounty
$750
 A bounty of $750 for any open-source contributor who provides a MR that solves this issue

  
MoSCoW
Could have
 Could have feature in order to satisfy the epic/legendary.

  
MoSCoW
Must have
 Must have feature in order to satisfy the epic/legendary.

  
MoSCoW
Should have
 Should have feature in order to satisfy the epic/legendary.

  
Needs Feedback
There are questions in the ticket that can not be completed until the project owner provides clarity.

  
Points
1
 1 man-hours worth of work for an expert with no learning curve.

  
Points
13
 13 man-hours worth of work for an expert with no learning curve.

  
Points
2
 2 man-hours worth of work for an expert with no learning curve.

  
Points
21
 21 man-hours worth of work for an expert with no learning curve.

  
Points
3
 3 man-hours worth of work for an expert with no learning curve.

  
Points
34
 34 man-hours worth of work for an expert with no learning curve.

  
Points
5
 5 man-hours worth of work for an expert with no learning curve.

  
Points
55
 55 man-hours worth of work for an expert with no learning curve.

  
Points
8
 8 man-hours worth of work for an expert with no learning curve.

  
Points
88
 88 man-hours worth of work for an expert with no learning curve.

  
Priority
Backlog
 This ticket has backlogged priority and is not to be worked on yet

  
Priority
CI Blocker
 Critical priority issue that blocks CI/CD pipeline and prevents PR merges

  
Priority
Critical
 The priority is critical

  
Priority
High
 The priority is high

  
Priority
Low
 The priority is low

  
Priority
Medium
 The priority is medium

  
Signed-off: Owner
When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.

  
Signed-off: Scrum Master
When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.

  
Signed-off: Tech Lead
When an epic or legendary is in review it must be signed off by owner, tech lead, and scrum master before being marked as completed.

  
Spike
A ticket for learning a tool or technology that is needed to be able to do future planning and design.

  
State
Completed
 The ticket has been fully implemented, completed, and merged with the source code. This label should only be applied once a ticket is closed.

  
State
Duplicate
 A ticket that represents the same content as an existing ticket.

  
State
In Progress
 A ticket that is actively being developed.

  
State
In Review
 A ticket that has had some code completed to implement but is waiting to pass peer review and is not yet merged in.

  
State
Paused
 This ticket's work started but wasn't finished. It's on hold (likely in a feature branch) and will be resumed later, either due to a blocker or a delay.

  
State
Unverified
 All new tickets start in this state. A developer may set it to show the ticket is unverified. This means we haven't agreed to work on it. It will either move to a verified state or be closed as wontdo.

  
State
Verified
 The issue has been verified by a developer as legitimate. It will be worked on and verified tickets are now considered part of the backlog.

  
State
Wont Do
 This ticket has been decided it wont be done. This may mean the bug has been determined to not be real (cant verify) or the feature is one we have decided we dont want to adopt.

  
Type
Automation
 Any edits or discussion about the AI automated coding system.

  
Type
Bug
 Something that doesnt work as intended.

  
Type
Discussion
 Anytime a ticket represents a discussion about a subject and doesnt fall into one of the other categories.

  
Type
Documentation
 An error or improvement needed in the documentation.

  
Type
Epic
 Any first tier epic. That is, an epic which contains only issues as children and will not have sub-epics.

  
Type
Feature
 Some new functionality not present.

  
Type
Legendary
 A type of Epic which will contain other Epics.

  
Type
Refactor
 A code change that restructures existing code without changing its external behavior.

  
Type
Support
 Someone needs help using the project.

  
Type
Task
 A generic task that doesnt fit into the other type categories.

  
Type
Testing
 Work exclusively focusing on fixing or expanding testing.

No labels
auto/needs-reevaluation
controller-managed
auto/blocked-by-deps
auto/ci-timeout
auto/claimed-implementer
auto/claimed-merge
auto/claimed-reviewer
auto/driver-down
auto/invariant-violation
auto/last-attempt-tier-0
auto/last-attempt-tier-1
auto/last-attempt-tier-2
auto/last-attempt-tier-min
Automation Tracking
auto/needs-conflict-resolution
auto/needs-implementer
auto/postmortem
auto/ready-to-merge
auto/restart-throttled
auto/revert
auto/sentinel
auto/stale-inactivity
auto/unstable
Blocked
Bounty
$100
Bounty
$1000
Bounty
$10000
Bounty
$20
Bounty
$2000
Bounty
$250
Bounty
$50
Bounty
$500
Bounty
$5000
Bounty
$750
MoSCoW
Could have
MoSCoW
Must have
MoSCoW
Should have
Needs Feedback
Points
1
Points
13
Points
2
Points
21
Points
3
Points
34
Points
5
Points
55
Points
8
Points
88
Priority
Backlog
Priority
CI Blocker
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Signed-off: Owner
Signed-off: Scrum Master
Signed-off: Tech Lead
Spike
State
Completed
State
Duplicate
State
In Progress
State
In Review
State
Paused
State
Unverified
State
Verified
State
Wont Do
Type
Automation
Type
Bug
Type
Discussion
Type
Documentation
Type
Epic
Type
Feature
Type
Legendary
Type
Refactor
Type
Support
Type
Task
Type
Testing
Milestone
Clear milestone
No items
No milestone
v3.2.0
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
freemo
4 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Blocks
#765 test(integration): workflow example 1 — Hello World, fix a single bug (manual profile)
cleveragents/cleveragents-core
Depends on
#627 Implement @tdd_expected_fail tag handling in Behave environment
cleveragents/cleveragents-core
#628 Implement @tdd_expected_fail tag handling in Robot Framework
cleveragents/cleveragents-core
Reference
cleveragents/cleveragents-core!798
No description provided.