Self review functionality - #9
Conversation
…trol and faster installations
| "taskReferenceName": "verify", | ||
| "type": "SIMPLE", | ||
| "inputParameters": { | ||
| "repoPath": "/Users/patrickropp/Code/conductor-oss/conductor-agents", |
There was a problem hiding this comment.
Hardcoded developer-local absolute path leaked into the workflow. Per the design (workflow-wiring.md §3.3), this must be "${workflow.input.repoPath}" — the integrated change branch. As written, the integration gate runs against /Users/patrickropp/... which won't exist on any other host/CI and is the wrong repo even if it does.
| "type": "SIMPLE", | ||
| "inputParameters": { | ||
| "repoPath": "/Users/patrickropp/Code/conductor-oss/conductor-agents", | ||
| "cmd": "null", |
There was a problem hiding this comment.
"cmd": "null" is the literal string null, not the intended "${workflow.input.verifyCmd}". run_checks treats a non-empty cmd as a real command, so it will execute bash -c null → "command not found" (exit 127) → passed=false on every run, and detection never kicks in. Should be "${workflow.input.verifyCmd}".
| "mergeCost": "${merge.output.costUsd}", | ||
| "queryExpression": "def num: if type == \"number\" then . else 0 end; (.joined // {}) as $j | [$j | to_entries[] | (.value.output // .value // {}) | {status:(.status // \"unknown\"), filesChanged:(.filesChanged // []), costUsd:(.costUsd | num), tokenUsed:(.tokenUsed | num)}] as $g | (.planTokens | num) as $pt | (.planCost | num) as $pc | (.designTokens | num) as $dt | (.designCost | num) as $dc | (.mergeTokens | num) as $mt | (.mergeCost | num) as $mc | ([$g[].tokenUsed] | add | num) as $st | ([$g[].costUsd] | add | num) as $sc | {perSubtask:$g, subtaskCount:($g | length), tokens:{plan:$pt, design:$dt, subtasks:$st, merge:$mt}, cost:{plan:$pc, design:$dc, subtasks:$sc, merge:$mc}, totalTokens:($pt + $dt + $st + $mt), totalCostUsd:($pc + $dc + $sc + $mc)}" | ||
| "verifyPassed": "null", | ||
| "verifyRan": "null", |
There was a problem hiding this comment.
verifyPassed/verifyRan are the literal string "null" instead of "${verify.output.passed}" / "${verify.output.ran}" (workflow-wiring.md §3.4). The aggregate's verified:.verifyPassed therefore always resolves to the string "null" rather than the real result.
| "verified": "null", | ||
| "verifiedRan": "null", | ||
| "verifyExitCode": "null", | ||
| "verifyCmd": "null", |
There was a problem hiding this comment.
The four integration-gate output params (verified, verifiedRan, verifyExitCode, verifyCmd) are all literal "null" strings instead of ${verify.output.passed} / .ran / .exitCode / .cmd (workflow-wiring.md §3.5). This directly breaks the PR's acceptance criterion: code_parallel never surfaces a real verification result.
| assert order.index("merge") < order.index("verify") < order.index("aggregate") | ||
| verify = next(t for t in wf["tasks"] if t["taskReferenceName"] == "verify") | ||
| assert verify["name"] == "run_checks" | ||
| assert wf["outputParameters"]["verified"] == "null" |
There was a problem hiding this comment.
This assertion locks in the bug: it checks outputParameters["verified"] == "null", whereas the design (and the docstring above, "its result is surfaced on the workflow output") requires "${verify.output.passed}". The test should assert the real interpolation so it would catch the broken wiring in code_parallel.json rather than ratify it.
|
|
||
| # --- 7. run_checks gate wiring (see docs/design/testing.md §2) --------------- | ||
|
|
||
| def test_code_subtask_gates_commit_behind_run_checks(): |
There was a problem hiding this comment.
test_code_subtask_gates_commit_behind_run_checks, test_code_parallel_verifies_after_merge_before_aggregate, and test_run_checks_only_in_code_workflows are each defined twice (also at lines 338/353/364). Python keeps only the last definition, so the first block (336–368) is dead code and silently never runs — a merge artifact from combining branches. Remove the duplicated block.
1 participant
This allows the user to kick of a PR review on their own PRs and either save the results locally or as in-line comments instead of a formal PR review.