google / google/artemis

CI is red on main: the formatting check fails, and the gated test step hides two real test failures

Open
#18 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Python
Stars
5.9k
Forks
516
Avg merge
22m
Merged PRs (30d)
5

Description

`main` has been failing CI since e983180, and because the test step only runs on `workflow_dispatch`, two genuine test failures have gone unreported alongside it.

I opened #17 for the first two items. The third needs a maintainer decision, so I deliberately left it out of that pull request.

---

## 1. `ruff format --check .` fails on a clean checkout of `main`

`docs/assets/generate_banner.py` was rewritten in e983180 without being formatted, so the **Check Python formatting** step fails:

```console
$ uv sync --dev --frozen
$ uv run ruff format --check .
1 file would be reformatted, 583 files already formatted
$ echo $?
1
```

a45daf2, the commit immediately before it, exits 0. Every push since has been red on both `ubuntu-latest` and `windows-latest`:

| Run | Commit | Failing step |
| --- | --- | --- |
| [34452724171](https://github.com/google/artemis/actions/runs/34452724171) | 38a2de6 | Check Python formatting |
| [34437793526](https://github.com/google/artemis/actions/runs/34437793526) | 5d92939 | Check Python formatting |
| [34307275379](https://github.com/google/artemis/actions/runs/34307275379) | e983180 | Check Python formatting |

The practical side effect is that any new pull request opens with a red check that has nothing to do with its own contents.

## 2. `ReadinessEngine.run_all(force_refresh=True)` silently returns the cached report on Windows

`artemis/core/diagnostics/engine.py` decides whether a refresh completed while the caller waited on the lock by comparing two `time.monotonic()` readings:

```python
request_started = time.monotonic()
...
async with self._report_lock:
if cacheable and self._report_cache_time >= request_started:
```

On Windows, `time.monotonic()` is `GetTickCount64()`, whose resolution is 15.625 ms:

```pycon
>>> time.get_clock_info("monotonic")
namespace(implementation='GetTickCount64()', monotonic=True, adjustable=False, resolution=0.015625)
>>> len({time.monotonic() for _ in range(200_000)})
1
```

A report published in the same tick that a forced request begins carries an identical timestamp, so `>=` treats an earlier report as one that landed while the caller waited. The forced request is answered from cache and no probe runs, which means `artemis doctor`, the console readiness wizard and `mobile_diagnose` can all reuse a stale snapshot.

`tests/unit/core/test_diagnostics.py::test_readiness_engine_reuses_cache_until_forced` already covers this and fails on Windows today:

```
tests/unit/core/test_diagnostics.py:222: in test_readiness_engine_reuses_cache_until_forced
assert probe.probe.await_count == 2
E AssertionError: assert 1 == 2
```

It passes on Linux only because the clock there is fine-grained, so the test as written cannot catch a regression on a Linux-only run. #17 replaces the timestamp comparison with a publication counter and adds a clock-frozen regression test that is deterministic on every runner.

## 3. The pinned `adb_server` manifest has never matched the generated one

`tests/unit/mcp/test_adb_server_contract.py::test_adb_server_manifest_matches_fixture` fails. Names and input schemas agree for all 13 tools, but 6 descriptions differ, and every one of the 6 is explained exactly by `inspect.cleandoc`: the fixture stores the dedented docstring while FastMCP passes `fn.__doc__` through verbatim.

```
E AssertionError: adb_server tool 'tap': The generated schema differs from the pinned fixture.
E {'description': "Taps on the screen at coordinates.\n\n 'coordinates' is a list [x, y].\n..."}
E != {'description': "Taps on the screen at coordinates.\n\n'coordinates' is a list [x, y].\n..."}
```

This is not recent drift. The docstrings were already indented in a4c2886, the commit that introduced the fixture, and no `mcp` release in the supported range dedents them:

```console
$ for v in 1.26.0 1.27.0 1.28.0 1.29.0; do
uv run --no-project --with "mcp==$v" python -c \
"import inspect;from mcp.server.fastmcp.tools.base import Tool;\
print([l.strip() for l in inspect.getsource(Tool.from_function).splitlines() if 'func_doc' in l][0])"
done
func_doc = description or fn.__doc__ or "" # 1.26.0
func_doc = description or fn.__doc__ or "" # 1.27.0
func_doc = description or fn.__doc__ or "" # 1.28.0
func_doc = description or fn.__doc__ or "" # 1.29.0
```

So the test has never passed since it was added. There are two opposite ways to resolve it, and the choice belongs to you, since external MCP clients read these descriptions directly:

1. Regenerate the fixture and accept the raw indentation on the wire.
2. Clean the descriptions at the source so the shipped surface matches the intent the fixture already encodes.

I have a patch ready for either one and would rather not guess which you want. Happy to send it as a follow-up.

## Why none of this surfaces automatically

The test step in `.github/workflows/ci.yml` is guarded:

```yaml
- name: Run deterministic Python tests
if: github.event_name == 'workflow_dispatch'
```

Pushes and pull requests therefore never run the suite; only a manual dispatch does. Items 2 and 3 both predate #17 and neither has ever been reported by an automatic run. If the guard exists to keep runner cost down, running the suite on `pull_request` alone would still have caught both.

## Environment

Windows 11, Python 3.12, `uv sync --dev --frozen` against the committed `uv.lock`. Item 1 and item 3 reproduce on Linux as well; item 2 is specific to platforms whose monotonic clock is coarse.

Contributor guide

Open the contributing guide

Research direction

Start by reviewing #17, then run `ruff format --check .` and the named diagnostics and adb-server contract tests. Read `docs/assets/generate_banner.py`, `artemis/core/diagnostics/engine.py`, `tests/unit/core/test_diagnostics.py`, `tests/unit/mcp/test_adb_server_contract.py`, and `.github/workflows/ci.yml`. Confirm the maintainer’s choice for the manifest mismatch; done means the reported failures are resolved and the intended CI test coverage is clear.

Written by the indexing model from the issue text.

Assessment

Tech stack
github-actions, python
Domain
ci-cd, testing-qa
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.