fix(ci): complete T0-5 ownership mapping and coverage guard - #217
Conversation
… (T0-5) - Add scripts/ci/test_ownership_map.py with canonical test-to-module mapping - Update tests/conftest.py to inject module/owner pytest markers from map - Add scripts/ci/check_test_failure_ownership.py for failure attribution reports - All 112 active test files mapped to responsible dare_framework modules Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ge guard - Set default owner 'lang' for all 112 test file entries - Add unmapped file warning in conftest.py pytest_collection_modifyitems - Add test_ownership_map_coverage.py with bidirectional sync guard tests - Guard ensures new test files must be added to OWNERSHIP_MAP Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 60e5ad84d0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| report = _build_report(failed) | ||
| print(report) | ||
|
|
||
| return 1 if failed else 0 |
There was a problem hiding this comment.
When the script runs pytest itself (default mode), it exits based only on parsed FAILED/ERROR <nodeid> lines and ignores the subprocess return code, so pytest invocation/config errors (for example lines like ERROR: usage: ... that do not match FAILED_TEST_RE) are reported as "No test failures detected" with exit code 0. This can let CI pass even though pytest itself failed before/without producing node-level failure lines.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9c75c93. Default-mode execution now preserves the pytest subprocess return code and emits the raw subprocess error when pytest exits nonzero without any node-level FAILED/ERROR <nodeid> lines, instead of misreporting a clean run.
Added regression coverage in tests/unit/test_check_test_failure_ownership.py and registered that file in OWNERSHIP_MAP so the coverage guard stays consistent.
Verification:
/Users/lang/workspace/github/Deterministic-Agent-Runtime-Engine/.venv/bin/pytest -q tests/unit/test_check_test_failure_ownership.py->1 passed/Users/lang/workspace/github/Deterministic-Agent-Runtime-Engine/.venv/bin/pytest -q tests/unit/test_ownership_map_coverage.py tests/unit/test_tool_manager.py->6 passed
Handle pytest invocation/configuration failures in check_test_failure_ownership without misreporting them as a clean run. The script now preserves the subprocess return code for default-mode pytest execution and only emits the ownership summary when node-level failures were actually parsed. A dedicated regression test covers the no-nodeid process-error path, and the new test file is registered in OWNERSHIP_MAP so the coverage guard stays consistent.
|
Handled the new ownership-report review comment in Changes:
Verification:
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c75c931f6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if _REPO_ROOT not in sys.path: | ||
| sys.path.insert(0, _REPO_ROOT) | ||
|
|
||
| FAILED_TEST_RE = re.compile(r"^(?:FAILED|ERROR)\s+([^\s]+)") |
There was a problem hiding this comment.
The current regex treats any line starting with ERROR as a failed node ID, so pytest collection output like ERROR collecting tests/unit/test_x.py is parsed as node collecting. In default mode this prevents the pytest_returncode and not failed fallback from running, so the script emits a misleading ownership report (FAILED collecting) and exits 1 instead of propagating pytest’s real process error code/output (e.g., collection/internal errors). This can hide the true failure mode in CI and route it to the wrong owner bucket.
Useful? React with 👍 / 👎.
Prevent ERROR collecting output from being misparsed as a failed test nodeid in the ownership attribution check.\n\nAdd a regression test for collection failures, tighten failed-line parsing so only real pytest nodeids are attributed, and preserve the subprocess-returncode fallback for non-nodeid process errors.
|
Addressed the latest review note about Fixed in
Verification:
Reference discussion: #217 (comment) |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2e4b283db
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| def _looks_like_test_nodeid(token: str) -> bool: | ||
| """Return true when a pytest summary token looks like a real test node id.""" | ||
| return ".py" in token |
There was a problem hiding this comment.
The ERROR parser currently accepts any second token containing .py, so pytest collection/import failures like ERROR tests/unit/test_tmp_collection_error.py are misclassified as failed test nodeids. In main(), that prevents the pytest_returncode fallback from running, so a real collection failure (pytest exit code 2) is converted into a generic ownership report with exit code 1 and without the original diagnostics. This can mask broken test environments in CI and make debugging much harder.
Useful? React with 👍 / 👎.
Tighten ownership parsing so only ERROR summary tokens with real pytest nodeid shape are attributed as failed tests.\n\nThis keeps file-level collection or import failures on the subprocess-returncode path, preserves the original pytest diagnostics, and adds regression coverage for that pseudo-nodeid case.
|
Addressed the latest ownership-parser review note from #217 (comment). Fixed in
Verification:
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d0f75ad88
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if pytest_returncode and not failed: | ||
| if raw: | ||
| print(raw, file=sys.stderr) | ||
| return pytest_returncode |
There was a problem hiding this comment.
In main, stdin/report mode sets pytest_returncode to None, so the non-zero propagation branch is skipped and the function falls through to return 0 whenever no FAILED ... node IDs are parsed. This causes false-success results for real pytest failures like collection/import/usage errors (for example ERROR collecting ...) when users follow the documented pipe/report flows, which can silently greenlight broken test runs.
Useful? React with 👍 / 👎.
| if _REPO_ROOT not in sys.path: | ||
| sys.path.insert(0, _REPO_ROOT) | ||
|
|
||
| FAILED_TEST_RE = re.compile(r"^FAILED\s+([^\s]+)") |
There was a problem hiding this comment.
The FAILED_TEST_RE pattern captures only non-whitespace tokens, but pytest node IDs can legitimately include spaces in parametrized IDs (for example test_x[hello world]). In that case _parse_failed_lines truncates the node ID at the first space, producing ambiguous/incorrect failure entries and potentially collapsing distinct failures into indistinguishable report lines.
Useful? React with 👍 / 👎.
Handle stdin and report inputs as failing runs when pytest emits collection or process errors without attributable node IDs.\n\nPreserve full FAILED and ERROR summary node IDs, including parametrized IDs with spaces, and add regression coverage for stdin/report non-zero behavior plus whitespace-containing node IDs in the ownership parser tests.
|
Addressed the latest ownership-parser review notes from:
Fixed in
Verification:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d34bce2ab1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if fallback_exit_code is None and not failed and _has_unattributed_pytest_error(raw): | ||
| fallback_exit_code = 1 |
There was a problem hiding this comment.
In main(), stdin/report inputs synthesize a non-zero exit only when _has_unattributed_pytest_error(raw) is true, so pytest outputs that have no FAILED ... lines and no ERROR... prefix (for example interrupted runs showing KeyboardInterrupt, or NO_TESTS_COLLECTED output) fall through to return 0 with “No test failures detected.” This can silently greenlight broken runs when using the documented pipe/report workflows, even though pytest itself would exit non-zero (ExitCode.INTERRUPTED=2, ExitCode.NO_TESTS_COLLECTED=5).
Useful? React with 👍 / 👎.
Treat stdin and report inputs as failing runs when pytest output indicates an interrupted execution or that no tests were collected, even when no FAILED or ERROR node IDs are present.\n\nAdd regression coverage for KeyboardInterrupt and no-tests-collected report flows while preserving the earlier ownership parsing fixes and whitespace-safe nodeid handling.
|
Addressed the latest stdin/report fallback review note from #217 (comment). Fixed in
Verification:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f6e34825f1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if args.stdin: | ||
| raw = sys.stdin.read() | ||
| pytest_returncode: int | None = None |
There was a problem hiding this comment.
In --stdin mode the script always sets pytest_returncode to None and only synthesizes a failure if _has_unattributed_pytest_error(raw) matches the stdin text, so an empty stdin stream falls through to success. With the documented pipeline (pytest ... | ... --stdin), pytest usage/config errors are emitted on stderr (not piped), which leaves raw empty and makes this script return 0 with "No test failures detected" even though pytest failed; this can silently greenlight broken CI unless every caller also adds 2>&1/pipefail.
Useful? React with 👍 / 👎.
| FAILED_TEST_RE = re.compile(r"^FAILED\s+(.+?)(?=\s+-\s+|$)") | ||
| ERROR_TEST_RE = re.compile(r"^ERROR\s+(.+?)(?=\s+-\s+|$)") |
There was a problem hiding this comment.
The summary regex stops at the first " - ", but pytest node IDs can legally contain that substring in parametrized IDs (for example test_case[hello - world] in -q --tb=line output). In that case _parse_failed_lines truncates the node ID before the real summary delimiter, which can collapse distinct parametrized failures into ambiguous report entries and misstate which test case failed.
Useful? React with 👍 / 👎.
Treat empty stdin in report mode as an upstream pytest failure and preserve full node IDs when parametrized summaries contain the literal " - " delimiter.\n\nAdd regression coverage for empty stdin pipelines and dash-containing parametrized IDs while keeping the prior stdin/report non-zero handling and ownership parsing fixes intact.
|
Addressed the latest stdin/report parser review notes from:
Fixed in
Verification:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cdaf675a4f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| raw = args.report.read_text(encoding="utf-8") | ||
| pytest_returncode = None |
There was a problem hiding this comment.
--report input is empty
The --report branch accepts an empty file and falls through to a success path (No test failures detected with exit 0), because no node IDs are parsed and _has_unattributed_pytest_error has no lines to match. This masks real pytest failures when callers capture stdout only (for example, pytest --badflag > report.txt writes diagnostics to stderr and leaves the report empty), so CI wrappers relying on this script’s exit code can incorrectly pass a broken run.
Useful? React with 👍 / 👎.
| rel = str(Path(item.fspath).relative_to(ROOT)) | ||
| entry = OWNERSHIP_MAP.get(rel) |
There was a problem hiding this comment.
This lookup uses str(Path(...).relative_to(ROOT)), which is OS-dependent; on Windows it yields backslash paths like tests\\unit\\... while OWNERSHIP_MAP keys are slash-delimited (tests/unit/...). In that environment every lookup misses, so module/owner markers are never injected and the unmapped-file warning reports nearly all tests as missing even though entries exist.
Useful? React with 👍 / 👎.
Treat empty --report inputs as upstream pytest failures so report-mode wrappers do not silently pass on stdout-only capture mistakes.\n\nNormalize ownership-map lookup keys to forward-slash repo-relative paths in tests/conftest.py and the coverage checks so Windows collection uses the same key shape as OWNERSHIP_MAP. Add targeted regression coverage for empty report files and path normalization, and register the new conftest ownership test in the canonical map.
|
Addressed the latest review notes from:
Fixed in
Verification:
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 87d378b6c7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if fallback_exit_code is None and not failed and _has_unattributed_pytest_error(raw): | ||
| fallback_exit_code = 1 | ||
| if fallback_exit_code and not failed: |
There was a problem hiding this comment.
When a run contains both normal test failures and a collection/usage error (for example FAILED ...::test_x plus ERROR collecting ...), this branch only treats the pytest error path when not failed, so the script emits a clean ownership report and drops the unattributed error text even though pytest returned an error code (often 2). That misattributes the run as ordinary test failures and hides the real collection/runtime problem from CI triage; the error-handling path should also trigger when unattributed pytest errors are present together with failed node IDs.
Useful? React with 👍 / 👎.
Preserve unattributed pytest process and collection errors even when a run also contains ordinary failed test node IDs.\n\nWhen mixed failure modes occur, stderr now retains the raw pytest diagnostics and the script returns the synthesized or upstream error exit code instead of downgrading the run to a generic failed-test report. Add regression coverage for the mixed failed-nodeid plus collection-error scenario.
|
Addressed the latest mixed-failure review note from #217 (comment). Fixed in
Verification:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6aeacaa654
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if failed_nodeid is not None: | ||
| results.append(failed_nodeid) |
There was a problem hiding this comment.
_parse_failed_lines appends every stripped line that starts with FAILED without validating that the token is an actual pytest nodeid. In pytest -q --tb=line output, captured stdout/stderr from a failing test is printed verbatim, so a test that logs a line like FAILED not-a-nodeid is incorrectly parsed as another failed test, which inflates failure counts and can misroute attribution into UNMAPPED; this branch should apply a nodeid-shape check (similar to the ERROR path) before appending.
Useful? React with 👍 / 👎.
Filter FAILED summary lines through the same nodeid-shape validation used for ERROR entries so captured test stdout or stderr cannot inflate ownership parsing with fake failures.\n\nAdd a regression case covering a logged line alongside a real pytest failure and verify the ownership-report parser still preserves all previously fixed stdin/report and mixed-failure behaviors.
|
Addressed the latest parser review note from #217 (comment). Fixed in
Verification:
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 35af5f34ca
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if args.stdin: | ||
| raw = sys.stdin.read() | ||
| pytest_returncode: int | None = None | ||
| if not raw.strip(): |
There was a problem hiding this comment.
In the --stdin branch, pytest_returncode is always set to None, so this command can only fail when it recognizes an error token in the piped text. Because the documented pipeline pipes only stdout, pytest failures that are emitted on stderr (for example ERROR: not found ... / usage failures) are interpreted as "No test failures detected" and this script exits 0, which can mask real test-run failures unless every caller also enforces pipefail and separately checks pytest’s exit code.
Useful? React with 👍 / 👎.
| for p in REPO_ROOT.joinpath("tests").rglob("test_*.py") | ||
| ) |
There was a problem hiding this comment.
The coverage guard only scans test_*.py, but pytest’s default discovery also includes *_test.py when python_files is not overridden. That means a collected *_test.py file can bypass the ownership-map missing-entry check, and if someone adds it to OWNERSHIP_MAP the stale-entry check will still report it as stale because the same narrow glob is reused. This creates a blind spot in the ownership enforcement this commit introduces.
Useful? React with 👍 / 👎.
Match ownership-map coverage and collection warnings to pytest's default filename patterns by recognizing both test_*.py and *_test.py.\n\nAdd a shared discovery helper in scripts/ci/test_ownership_map.py and reuse it from tests/conftest.py plus the ownership coverage tests so the enforcement paths stay consistent across platforms and future test additions.
|
Addressed the latest review items. For discussion_r2922685411, I pushed For discussion_r2922685407, the false-success case is already covered by the existing stdin/report guards: empty stdin now exits non-zero instead of falling through to Verification:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed2c784eb1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for line in text.splitlines(): | ||
| stripped = line.strip() | ||
| failed_nodeid = _summary_nodeid(stripped, FAILED_PREFIX) | ||
| if failed_nodeid is not None and _looks_like_test_nodeid(failed_nodeid): | ||
| results.append(failed_nodeid) |
There was a problem hiding this comment.
_parse_failed_lines scans every output line and treats any FAILED ... token that contains .py:: as a real test failure, but pytest prints captured stdout/stderr verbatim for failing tests, so user log lines like FAILED tests/unit/test_other.py::test_noise - ... are currently counted as extra failures; this inflates failure counts and can misroute ownership to the wrong module even when the summary has a different set of failed tests.
Useful? React with 👍 / 👎.
Limit failure-node parsing to pytest's short test summary section when a full report is available so captured stdout or stderr lines that happen to start with FAILED do not get counted as extra test failures.\n\nKeep the existing fallback for summary-only inputs, and add a regression test covering a fake FAILED tests/...::... log line that appears before the real summary output.
|
Addressed the latest parser review note from #217 (comment). Fixed in
Verification:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3672055e44
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| failed_nodeid = _summary_nodeid(stripped, FAILED_PREFIX) | ||
| if failed_nodeid is not None and _looks_like_test_nodeid(failed_nodeid): | ||
| results.append(failed_nodeid) | ||
| continue | ||
| error_nodeid = _summary_nodeid(stripped, ERROR_PREFIX) |
There was a problem hiding this comment.
_parse_failed_lines only extracts FAILED/ERROR summary rows, but pytest reports strict-xfail regressions as XPASS <nodeid> in the short summary. In --stdin/--report flows main() has no subprocess return code to fall back on, so an output containing only XPASS ... is treated as “No test failures detected” and exits 0, which can incorrectly pass CI when strict xfail expectations are violated.
Useful? React with 👍 / 👎.
Extend the pytest summary parser to recognize XPASS node IDs alongside FAILED and ERROR entries so stdin/report flows do not silently succeed when strict xfail expectations unexpectedly pass.\n\nAdd a regression test covering a short-summary-only report with XPASS output and verify that the ownership report stays non-zero instead of falling through to the clean-run message.
|
Addressed the latest parser review note from #217 (comment). Fixed in
Verification:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 287cdb7e57
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for prefix in (FAILED_PREFIX, ERROR_PREFIX, XPASS_PREFIX): | ||
| nodeid = _summary_nodeid(stripped, prefix) | ||
| if nodeid is not None and _looks_like_test_nodeid(nodeid): | ||
| results.append(nodeid) |
There was a problem hiding this comment.
Treating XPASS summary rows as failed nodeids causes false failures when tests use non-strict xfail: pytest --help documents that -r can include (X)passed, and this repo enables -ra in pytest.ini, so successful runs can legitimately emit XPASS ... while exiting 0. Because _parse_failed_lines currently appends XPASS entries to failed, main() reports them as FAILED ... and returns non-zero in default/--stdin/--report flows, which can incorrectly fail CI despite pytest success.
Useful? React with 👍 / 👎.
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
failed test -> module -> ownerattribution pipeline for T0-5OWNERSHIP_MAPand route failure reports throughcheck_test_failure_ownership.pyTest Plan
.venv/bin/pytest -q tests/unit/test_ownership_map_coverage.py tests/unit/test_tool_manager.py.venv/bin/pytest -q --collect-only testsprintf 'FAILED tests/unit/test_tool_manager.py::test_tool_manager_register_update_unregister - AssertionError\n' | .venv/bin/python scripts/ci/check_test_failure_ownership.py --stdinNotes