You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
fix: read only the project .env, and close the CI lint-scope gaps from the #2102 review (#2105)
* chore(internal): close lint-config gaps and harden subprocess assertions from the #2102 review
- pyproject.toml: extend the temporary PLC0207/PLR5501 per-file-ignore to the
webhook-proxy dev tree so a reset-dev-from-stable run (which restores the
still-unfixed stable code) cannot open a draft PR that fails Ruff Lint
- pr.yml: add packaging/ to the ruff check scope (was linted by lefthook on
commit but not by CI); packaging/ is already clean
- generate_manifest.py: add maxsplit=1 to the first-line docstring split,
matching the #2102 sweep pattern
- run_story.py: check=True on the two get_git_info lookups so a git that runs
but fails keeps the "unknown" default instead of recording empty strings
- test_config.py: liveness anchor in test_no_env_file_warning_removed so a
startup crash cannot pass the warning-absence assertion vacuously
- test_docker_build.py: pin `which uv` to exit 1 so docker-level failures
(125/126/127) cannot pass as "uv absent"
- test_openai_agent.py: assert argparse exit 2 and the missing-argument
message instead of any non-zero exit
- test_stories.py: a BAT runner crash now fails the test with captured output
instead of skipping as "agent not available"; the genuine no-agent-CLI
signal keeps the skip path
- ha_query.py: report the agent CLI exit code and exit non-zero on a failed
query so an empty answer cannot be scored as a verification result
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* chore(internal): terminal return after pytest.fail for CodeQL py/mixed-returns
The new pytest.fail tail in _run_bat_scenario reads to CodeQL as an
implicit None fall-through mixing with the explicit returns above it.
Use the repo trailer convention (read_only.py, policy/middleware.py).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* chore(internal): address review — narrow lint ignores, harden config anchors, cover the ha_query contract
Review round on this PR (Codex + review agents):
- pyproject.toml: scope the temporary PLC0207/PLR5501 ignores to the four
exact files instead of two whole trees. The -dev tree is where all new
webhook-proxy code is authored, so a tree-wide ignore would have turned
both rules off on the active development path until the next promote.
- test_config.py: the credential subprocess tests could not create the
missing-credentials scenario on a machine with the documented project
.env — config.py loads it by absolute path, so popping the vars from the
child env is not enough (an existing but empty HAMCP_ENV_FILE is, since a
missing one falls back to .env). Add a child_env fixture and use it across
the class. Anchor the assertions on the dynamic " - VAR" missing-list
entries: both reachable startup banners name both variables, so the bare
name matches proved nothing, and the two single-variable tests were green
for the wrong reason.
- test_ha_query_exit_contract.py: new, pins the (text, exit_code) contract
and the exit status of main(). Lives in the CI-gated unit suite because
nothing under tests/uat/ runs in any workflow.
- test_stories.py: drop the "No agents available" skip branch — the caller
resolves the agent through the same shutil.which check the runner uses, so
it cannot fire, and keeping it left a live-looking dependency on a log line
reaching stderr.
- run_story.py: log the git failure instead of a bare pass, so a run that
records sha=unknown says why.
- bat-story-eval SKILL.md + evaluation-protocol.md: the only consumer of
ha_query.py had three outcomes and no failure state, which would have put
the new non-zero exit straight back into a scored "unclear".
- Comment accuracy: an import error exits before parsing, not past it; exit
127 usually means which is absent from the image; a failed query risks a
partial answer being scored, not just an empty one.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* test(internal): pin the CI ruff directory list to the tree
packaging/ went unlinted in CI because pr.yml enumerates directories by hand
while lefthook lints **/*.py — the gap is invisible until someone writes a
violation in a directory nobody added. Assert that every top-level directory
holding tracked Python appears in the ruff check step, and that the step lists
no path that has since been deleted.
Verified the guard is not vacuous: removing packaging/ from pr.yml again makes
it fail by name.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* test(internal): match the whole missing-arguments list, not each flag
The per-flag substring assertions could not detect the regression they exist
to catch. argparse prints the usage block above its error line, and that block
names every flag whether required or optional — so dropping required=True from
--base-url still leaves "--base-url" in stderr, as "[--base-url BASE_URL]".
Reproduced against a parser with the same three flags: all four assertions pass
while the required set is wrong.
Match the full "the following arguments are required: ..." list instead, which
argparse emits unwrapped and which changes the moment the required set does.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(test): walk the tree instead of shelling out to git ls-files
The drift guard failed in CI with git exit 128: the unit-test container runs
as a different UID than the checkout owner, so git refuses the repository as
dubious ownership. That is the same condition pr.yml already works around by
re-adding safe.directory for its own git calls.
Walk the directories directly instead — no git, no subprocess, so the test
behaves the same in a container, a bare checkout and a worktree. Pruning
covers build/tooling dirs plus the repo's own gitignored worktree root, whose
nested checkouts would otherwise report the same directories twice.
Verified: the walk returns exactly the eight directories git ls-files reported,
removing packaging/ from pr.yml still fails by name, and root-level .py files
now get their own assertion since no directory argument would cover them.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(test): close the holes review round 2 found in this PR's own hardening
- run_story.py: log git's stderr, not just str(CalledProcessError). The latter
renders "returned non-zero exit status N" and nothing else, so the warning
named the failure without the reason it was added to carry — "detected
dubious ownership" would have sat in the discarded e.stderr.
- ha_query.py: handle subprocess.TimeoutExpired in both query functions. A hung
CLI escaped as a traceback, so the answer was never printed: the partial
output was lost along with the [exit N] marker the skill docs promise. Now
annotated as [exit 124] (GNU timeout convention) with the partial text kept.
The duplicated annotation block folds into one _annotate_failure helper.
- test_ha_query_exit_contract.py: cover the timeout path, and parametrize the
main() tests over both agents — both hard-coded gemini, so the dispatch arm
that unpacks run_claude_query's tuple was never executed.
- test_stories.py: drop the now-dead "agent unavailable" skip in
_evaluate_result. Removing the synthetic {"available": False} fallback left
its consumer standing, which is the same vacuous skip 25 lines above.
- bat-story-eval: SKILL.md never linked its own references/, so the protocol
file this PR edited could go unread — add both to Key Files. Give
eval_score the "unverified" value the new instruction tells evaluators to
record, and document the two non-zero exits that carry no [exit N] marker.
- ha_query.py module docstring: state the exit contract in the file that
implements it.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(test): isolate the config subprocess tests from BOTH .env readers
The child_env fixture closed only one of two independent readers of the
project .env, so the isolation it documented was not delivered.
Reader one is ha_mcp.config's module-level load_dotenv, which resolves
<project_root>/.env by absolute path; an existing-but-empty HAMCP_ENV_FILE
beats it, which the fixture already did. Reader two is Settings.model_config's
env_file=".env", which pydantic-settings resolves against the CHILD's working
directory and which HAMCP_ENV_FILE cannot reach at all: DotEnvSettingsSource
does Path(env_file).expanduser() with no anchoring, and the dotenv source
outranks the field defaults that carry the OAuth sentinels the credential
check compares against.
So on a machine with the documented `cp .env.example .env`, running the same
command CI runs from the repo root repopulates the credentials the tests pop,
no banner is printed, and all four credential tests fail. CI stays green only
because .env is gitignored and never created there.
Pass cwd=tmp_path alongside the existing fixture — both are required, neither
alone isolates the child. Nothing in the child depends on cwd: the editable
finder mapping is absolute, project_root derives from __file__, get_data_dir
resolves through env vars and absolute fallbacks, and there are no
cwd-relative path literals under src/ha_mcp/.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* test(internal): pin check=False, the kwarg the contract depends on
Mutation-tested: flipping check=False to check=True at both CLI call sites
left the whole contract file green. A return_value mock returns regardless of
kwargs, so the double cannot express check semantics — yet check=False is what
lets execution reach the [exit N] annotation and the tuple return at all. With
check=True a non-zero exit raises CalledProcessError, which neither query
function catches, and the partial answer never reaches the caller.
Assert the kwarg directly, matching test_embedded_server.py, which pins the
same kwarg for the same reason. The assertion sits in the already-parametrized
non-zero test, so one line covers both call sites.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix: read only the project .env, never one in the working directory
Settings.model_config declared env_file=".env" relative, which
pydantic-settings resolves against the process's working directory at
instantiation. That made it a second, independent dotenv read alongside the
load_dotenv at import — one that HAMCP_ENV_FILE could not govern, since
pydantic never looks at that variable.
So launching the server from a directory that happens to hold an unrelated
.env silently supplied values for any key matching a field name, and
HAMCP_ENV_FILE pointing at a custom file did not stop it. The collision
surface is the un-prefixed fields — timeout, debug, log_level, environment,
max_retries, verify_ssl — which are exactly the names other projects' .env
files carry. Real environment variables still won, so this only filled in
values the operator had not set another way.
Resolve the fallback once at import and hand model_config that absolute path,
so both readers agree on one file. No documented workflow relies on the cwd
read: the install guides do not mention .env at all, the Dockerfile's
WORKDIR /app holds none, and the add-on passes configuration as env vars.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---------
Co-authored-by: kingpanther13 <kingpanther13@users.noreply.github.com>
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
0 commit comments