From 2af064eaa8dc2375e35caf64fc2901f141aeca5d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javier=20Miralles=20Ranca=C3=B1o?= <68760931+FlagshipDev@users.noreply.github.com> Date: Mon, 10 Aug 2026 02:10:54 +0200 Subject: [PATCH] Three more checks that were claiming, or not saying, what they knew MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All three are the same defect wearing different clothes, and it is the one this product exists to find in other people's software: a check whose answer is a story it tells itself rather than something it went and looked at. ## A check that was told where to look and did not `hullwork status` on a healthy deployment reported, permanently: deployment: not checked: no environment file at .env, so nothing here can compare what you configured against what arrived. Point HULLWORK_DEPLOYMENT_ENV_FILE and … The instance had done every single thing that message asks for: the variable set, the file mounted read-only at exactly that path, the compose passing it to both services. The message names `.env`, the default, because `status` never read the setting. Three call sites resolved the same pair of paths and only `doctor` honoured the configuration. The setting exists because inside a container the working directory holds neither file, so the check silently never ran on any real deployment — that fix reached one place of three, and one of the two it missed is the command an operator actually looks at. What was dark meanwhile is not an ornament: it is the mechanism that catches *a variable is correct in the file, correctly read, and never arrives*, which is the state where a feature reports itself unconfigured — true of the process and false of the machine. Its second half compares the file against the neighbouring compose, and that is a real past failure where enrichment had never once run in production. There is one resolver now, and a test asserts by construction that no call site builds the path by hand again. ## The manifest error that sent people to delete a line they meant Adding an optional field to the manifest means a build that does not know it refuses the file: autofix.open_upgrades: Extra inputs are not permitted (got: True) Correct, and it says nothing about the possibility that the field is simply newer than the binary reading it — which the schema version exists to say, but only when the file *declares* a higher version, and that field is optional and nobody writes it. The refusal now names the schema this build understands, beside the offending field and never in place of it: most unknown fields are typos, and sending somebody to upgrade over a missing letter is a worse answer than the wall was. The schema version itself stays where it is. Adding an optional field with a safe default is not a schema change, and a number that increments on every added key stops telling anybody whether their existing file still means what it meant. ## The issue that did not carry the error An issue filed from a production error contained a lane, an occurrence count, a first-seen timestamp and a fingerprint. No exception message, no location, no link. A person opening it could not act on it without knowing a tracker existed and searching it by hand. All of it had been fetched and stored: exception type, culprit, stack frames, release. The agent's brief renders those, and has for months, because a brief without them was measured to be worthless. The person was never given the same courtesy, and two items sat untouched for four days as a result. Now the body carries the exception type and its full message — untruncated, because providers cut titles at 100 characters and for a `KeyError` the half they cut is often the input that reproduces the bug — the culprit, release, environment, when it happened, frame locations innermost last, and the link. **Locations only: no variables, and no source lines.** A captured variable can hold a credential and this body is republished into a repository; source lines go stale as the code moves, and the link is one line above them. The body says so, every time, because what a report leaves out should be a decision somebody made rather than a default nobody chose. An issue's body is written once, at creation, and enrichment usually arrives afterwards — so that alone would fix the next issue and none of the existing ones. When the detail lands for an issue already filed, it arrives as a comment. ## Verified Twenty defects reintroduced across the three, and the tests that failed to catch four of them were rewritten rather than counted as passes: - one measured a cache window and called it idempotence; - one passed because an exception was being swallowed, so *nothing was posted* was true while the code was doing the wrong thing badly; - one asserted what a message says and nothing about where it stops, so gluing a hint onto every failure — including ones it has no reason to believe — went unnoticed; - one guard turned out not to be what makes its property true, and is now labelled as defence in the code rather than sold as the guarantee it is not. Signed-off-by: Javier Miralles Rancaño <68760931+FlagshipDev@users.noreply.github.com> --- README.md | 2 +- docs/status.md | 2 +- docs/what-hullwork-is.md | 2 +- hullwork/cli.py | 56 ++++-- hullwork/ingest.py | 131 +++++++++++++- hullwork/manifest.py | 21 ++- tests/test_a_field_from_a_newer_hullwork.py | 93 ++++++++++ ...t_the_check_that_was_told_where_to_look.py | 118 +++++++++++++ tests/test_the_desk_it_cleared.py | 30 +++- ...tation_describes_the_published_artefact.py | 25 +++ ...evidence_reaches_an_issue_already_filed.py | 163 ++++++++++++++++++ tests/test_the_issue_carries_the_error.py | 146 ++++++++++++++++ 12 files changed, 761 insertions(+), 28 deletions(-) create mode 100644 tests/test_a_field_from_a_newer_hullwork.py create mode 100644 tests/test_the_check_that_was_told_where_to_look.py create mode 100644 tests/test_the_evidence_reaches_an_issue_already_filed.py create mode 100644 tests/test_the_issue_carries_the_error.py diff --git a/README.md b/README.md index d771dd0..c06ff5d 100644 --- a/README.md +++ b/README.md @@ -51,7 +51,7 @@ returns a verdict with the run attached. The oracle changes per signal; the mach |---|---|---| | a production error | *something broke* | a test that fails before the change and passes after | | a dependency advisory | *this version is vulnerable* | your own suite, run against the upgrade | -| a static finding | *this could be exploited* | a test naming the hostile input | +| a static finding | *this could be exploited*, or *this code is dead* | a test naming the hostile input — or removing the code and running your suite, where coverage proves the line ran | **The first two rows are in a release** — the second since `0.1.0a8`, and it says no more often than yes. The third does not exist. diff --git a/docs/status.md b/docs/status.md index 72e89f4..7a45bf1 100644 --- a/docs/status.md +++ b/docs/status.md @@ -13,7 +13,7 @@ each claim is part of the claim. > |---|---|---| > | a production error | a test that fails first and passes after | **released**, and everything below describes it | > | a dependency advisory | your own suite, run against the upgrade | **released in `0.1.0a8`** — `hullwork deps`, and it refuses far more than it verifies | -> | a static finding | a test naming the hostile input | does not exist | +> | a static finding | a test naming the hostile input, or removing the code under coverage | **does not exist** — its shape is decided (DR-0020) and its build is not ordered | > > The second row is work items 172–180. Until `0.1.0a8` this page named its state and deliberately > **not its command**, because a command a reader cannot run is an invitation to type it and be told diff --git a/docs/what-hullwork-is.md b/docs/what-hullwork-is.md index f28607c..25e9a95 100644 --- a/docs/what-hullwork-is.md +++ b/docs/what-hullwork-is.md @@ -21,7 +21,7 @@ Every signal Hullwork accepts arrives from a tool that **asserts something and p |---|---|---|---| | a production error | Sentry, GlitchTip | *something broke* | a test that reproduces it | | a dependency advisory | Renovate, Dependabot, OSV | *this version is vulnerable* | the project's own suite | -| a static finding | CodeQL, Opengrep | *this could be exploited* | a test naming the hostile input | +| a static finding | CodeQL, Opengrep | *this could be exploited*, or *this code is dead* | a test naming the hostile input, or removing the code under coverage | Three signals, three oracles, **one mechanism**: take the claim into a sandbox, submit it to an oracle the agent cannot influence, return a verdict with the run attached. diff --git a/hullwork/cli.py b/hullwork/cli.py index 7ec926c..190174d 100644 --- a/hullwork/cli.py +++ b/hullwork/cli.py @@ -2106,10 +2106,8 @@ def _cmd_status( findings = credentials.audit( session, make_permission_reader(settings), probe=_scope_probe(settings) ) - compose = DEFAULT_COMPOSE_FILE if DEFAULT_COMPOSE_FILE.exists() else None - gaps = doctor.environment_gaps( - settings, env_file=DEFAULT_ENV_FILE, compose_file=compose - ) + env_file, compose = where_the_deployment_files_are(settings) + gaps = doctor.environment_gaps(settings, env_file=env_file, compose_file=compose) payload = report.as_dict() payload["dispatcher_loop"] = { "state": loop_state, @@ -2348,6 +2346,36 @@ def _cmd_status( DEFAULT_COMPOSE_FILE = Path("docker-compose.yml") +def where_the_deployment_files_are( + settings: Settings, + *, + env_file: str | None = None, + compose_file: str | None = None, +) -> tuple[Path, Path | None]: + """The env file and the compose file `environment_gaps` should read, for every command. + + **One question, one answer** (item 194, and item 193 the same day for the same reason). Item 144 + added these settings so a containerised instance could point the check at the host's files — + inside a container the working directory holds neither, so it silently never ran on any real + deployment. That fix reached `doctor` and neither of the two call sites `status` uses, so on the + live instance the configured path was set, the file was mounted at it, and `status` printed *not + checked: no environment file at `.env`* — the default it never replaced. + + Precedence is a person, then the machine, then the default: `--env-file` is the only place + somebody names the file by hand, and somebody standing in front of the machine outranks how it + was configured. + + The compose falls back to the default **only when it exists**, because `None` there means *no + compose to compare against*, which is a different fact from *a compose that passes nothing on*. + """ + resolved_env = Path(env_file or settings.deployment_env_file or DEFAULT_ENV_FILE) + + named = compose_file or settings.deployment_compose_file + if named: + return resolved_env, Path(named) + return resolved_env, DEFAULT_COMPOSE_FILE if DEFAULT_COMPOSE_FILE.exists() else None + + def _cmd_doctor( args: argparse.Namespace, session: Session, settings: Settings, out: TextIO ) -> int: @@ -2361,18 +2389,12 @@ def _cmd_doctor( Exit code is the answer, as everywhere else — and an `unknown` never sets it. That is item 073's lesson: a warning wired into an exit code with no action available to clear it is not a signal. """ - # **Configuration before the working directory** (item 144). The flags still win, because - # somebody running this from a host shell knows where the files are. What changed is the - # fallback: it used to be the working directory, which inside a container holds neither file, so - # the check silently never ran on any real deployment. Now the instance can say where they are. - env_file = Path( - args.env_file or settings.deployment_env_file or DEFAULT_ENV_FILE + # **Configuration before the working directory** (item 144), and now in one place for all three + # commands (item 194) — this was the only one that had it, which is why `status` was reporting + # `not checked` on an instance that had configured everything the message asked for. + env_file, compose_file = where_the_deployment_files_are( + settings, env_file=args.env_file, compose_file=args.compose_file ) - named_compose = args.compose_file or settings.deployment_compose_file - if named_compose: - compose_file: Path | None = Path(named_compose) - else: - compose_file = DEFAULT_COMPOSE_FILE if DEFAULT_COMPOSE_FILE.exists() else None code_forge = make_code_forge(settings) # The **ingest** credential for the inventory check: asking whether an issue still exists is a @@ -2612,8 +2634,8 @@ def _report_environment(settings: Settings, out: TextIO) -> list["doctor.Finding `tracker configured: false` is true of this process and can be false of the machine, and that sentence is what sent somebody looking in the wrong place for a day. """ - compose = DEFAULT_COMPOSE_FILE if DEFAULT_COMPOSE_FILE.exists() else None - gaps = doctor.environment_gaps(settings, env_file=DEFAULT_ENV_FILE, compose_file=compose) + env_file, compose = where_the_deployment_files_are(settings) + gaps = doctor.environment_gaps(settings, env_file=env_file, compose_file=compose) if not gaps: return [] print("\n Configuration that did not arrive:", file=out) diff --git a/hullwork/ingest.py b/hullwork/ingest.py index eb9666a..a0f956b 100644 --- a/hullwork/ingest.py +++ b/hullwork/ingest.py @@ -365,6 +365,7 @@ def fetch_context( tracker: Tracker | None = None, limit: int = 20, recheck_after: int = 600, + forge: Forge | None = None, ) -> int: """Ask the tracker for the full error behind each item that has not got one yet (item 036). @@ -403,14 +404,16 @@ def fetch_context( # `finally`, so a tracker that raises mid-pass does not cost the earlier items their # backoff. try: - if _fetch_one(session, tracker, item): + if _fetch_one(session, tracker, item, forge): fetched += 1 finally: session.commit() return fetched -def _fetch_one(session: Session, tracker: Tracker, item: Item) -> bool: +def _fetch_one( + session: Session, tracker: Tracker, item: Item, forge: "Forge | None" = None +) -> bool: """Bring one item's context up to date. Returns whether anything was fetched. Extracted from the loop by item 083 so there is **one** place that commits. Five `continue` @@ -458,12 +461,55 @@ def _fetch_one(session: Session, tracker: Tracker, item: Item) -> bool: if event is None: item.context_error = "the tracker no longer has this issue" return False + first_sample = not has_sample _store_context(session, item, event) + # **The half that reaches an issue already filed** (item 196). A body is written once, at + # creation, and on the live instance every issue predated its own enrichment by days — so + # putting the evidence in the body fixes the next issue and none of the existing ones. + # + # **Once per item, and `first_sample` is not what makes that true.** Measured by mutation: + # removing it changes nothing, because with a sample already stored both branches above return + # before reaching `_store_context`, so this line is reached at most once in an item's life. + # The term stays as a second lock on a door that is already shut — cheap, and the day + # `_fetch_one` learns to re-fetch, the alternative is a comment on every pass until somebody + # mutes the repository. It is defence, and it is labelled as defence rather than sold as the + # guarantee. + if first_sample and forge is not None and item.forge_issue_ref: + _post_the_evidence(session, forge, item) item.context_error = None _relane_now_that_we_know_where(session, item, event) return True +def _post_the_evidence(session: Session, forge: "Forge", item: Item) -> None: + """Comment the evidence onto an issue that was filed before it arrived. + + A failure here is logged and swallowed: enrichment is worth having whether or not the forge is + answering this minute, and the fetch that produced it must not be lost to a comment that could + not be posted. What is lost is the comment, and the body of the next issue still carries it. + """ + project = item.project + stored = ( + session.query(FetchedEvent) + .filter(FetchedEvent.item_id == item.id) + .order_by(FetchedEvent.id.desc()) + .first() + ) + if stored is None: # pragma: no cover - defensive; the caller has just stored one + return + body = "\n".join( + ["Hullwork has the error's detail now, which arrived after this issue was filed.", + *_evidence_lines(stored, item.permalink)] + ) + try: + forge.comment(project.repo, int(str(item.forge_issue_ref).lstrip("#")), body) + except (ForgeError, ValueError) as exc: + log.warning( + "could not post the evidence onto the issue", + extra={"item": item.id, "issue": item.forge_issue_ref, "error": str(exc)}, + ) + + def _relane_from_stored_sample(session: Session, item: Item) -> bool: """Re-decide a lane using a sample fetched before item 070 existed. @@ -843,7 +889,7 @@ def sweep( # Last: it is the only step that is pure enrichment. An item is filed and reconciled # whether or not the tracker answers, and a tracker having a bad minute must not delay # the work that has a human waiting on it. - fetched = fetch_context(session, tracker, recheck_after=recheck_after) + fetched = fetch_context(session, tracker, recheck_after=recheck_after, forge=forge) # After enrichment, and last of all: DR-0011. The inventory is how an issue that never got a # webhook — because the tracker speaks once per issue for its whole life — arrives at all. # It goes here rather than in the dispatcher because it needs the ingest credential and @@ -1004,7 +1050,83 @@ def _file(session: Session, forge: Forge, project: Project, item: Item) -> bool: return True -def _issue_body(item: Item) -> str: +#: How many frame locations an issue shows. `brief.py` shows the same number to the agent, and for +#: the same reason: the innermost are the defect and the rest is the framework that called it. +MAX_FRAMES_IN_AN_ISSUE = 8 + +#: Said in the artefact, every time, rather than in a document somebody would have to go and find. +#: Item 196's second criterion: what an issue leaves out is a decision the instance makes and can +#: state, not a default nobody chose. +WHAT_THE_ISSUE_LEAVES_OUT = ( + "Locations only — **no variables**, and no source lines. A captured variable can hold a " + "credential and this body is republished into a repository; source lines go stale as the code " + "moves. The link above has both, from the tracker, under whatever access it already enforces." +) + + +def _where_it_happened(frames: list[dict[str, Any]]) -> list[str]: + """Frame locations, innermost last, as `module.function:lineno`. + + Deliberately not `brief.py`'s renderer, which is right for the agent and wrong here: that one + includes `context_line`, because a model writing a reproducing test needs the failing source in + front of it. A person has the link. + """ + said = [] + for frame in frames[-MAX_FRAMES_IN_AN_ISSUE:]: + where = frame.get("module") or frame.get("filename") or frame.get("abs_path") or "?" + function = frame.get("function") + line = frame.get("lineno") + said.append(f"{where}.{function}:{line}" if function else f"{where}:{line}") + return said + + +def _evidence_lines(event: "FetchedEvent | None", permalink: str | None) -> list[str]: + """What Hullwork knows about the error, for the person who has to decide about it. + + **This existed and went only to the agent** (item 196). `brief.py` renders frames, the culprit + and whether locals were captured; `_issue_body` read the `Item` row alone and never touched the + enrichment sitting one table away. Measured on the live instance: two items whose issues carried + four rows of table and no link, untouched for four days, about the page crashing in production. + """ + if event is None: + return [ + "", + "The error's detail has **not arrived yet** — the tracker has not been asked, or could " + "not answer. That is different from there being none, and this line is here so it " + "cannot be read as the second.", + *(["", f"The error: {permalink}"] if permalink else []), + ] + + said = ["", "## The error"] + if event.message: + said += ["", f"**{event.exception_type or 'Error'}**: {event.message}"] + elif event.exception_type: + said += ["", f"**{event.exception_type}**"] + if event.culprit: + said.append(f"In `{event.culprit}`.") + + facts = [ + (name, value) + for name, value in ( + ("Release", event.release), + ("Environment", event.environment), + ("When", event.occurred_at.isoformat() if event.occurred_at else None), + ) + if value + ] + if facts: + said += ["", "| | |", "|---|---|", *[f"| {name} | {value} |" for name, value in facts]] + + where = _where_it_happened(event.frames or []) + if where: + said += ["", "Where it happened, innermost last:", "", *[f"- `{one}`" for one in where]] + if permalink: + said += ["", f"The error, in full: {permalink}"] + said += ["", WHAT_THE_ISSUE_LEAVES_OUT] + return said + + +def _issue_body(item: Item, event: "FetchedEvent | None" = None) -> str: lines = [ "Reported by Hullwork from a production error.", "", @@ -1032,6 +1154,7 @@ def _issue_body(item: Item) -> str: "", f"Red lane: an agent will never touch this. Reclassify it in {where} if that is wrong.", ] + lines += _evidence_lines(event, item.permalink) lines += ["", marker_for(item.fingerprint), ""] return "\n".join(lines) diff --git a/hullwork/manifest.py b/hullwork/manifest.py index da6bd37..0983ff4 100644 --- a/hullwork/manifest.py +++ b/hullwork/manifest.py @@ -42,6 +42,22 @@ HUMAN_MERGE = "human-merge" +#: What an unknown field's refusal adds, and it is a **hint rather than a diagnosis** (item 195). +#: `SCHEMA_VERSION`'s docstring promised a manifest from the future would be refused clearly instead +#: of producing this wall — but that path needs the file to *declare* the higher version, and +#: `version:` is optional and nobody writes it. So the wall is what a project adopting a new field +#: actually gets, and it says nothing about the possibility that the field is simply newer than the +#: binary reading it. +#: +#: Added beside the field and never in place of it: **most unknown fields are typos**, and sending +#: somebody to upgrade over a missing letter is a worse answer than the wall was. +_MAY_BE_NEWER = ( + "One of these may be a field from a newer Hullwork rather than a mistake: this build " + "understands schema {version}. If your project needs it, upgrade Hullwork; if it is a typo, " + "the name and value above are what it read." +) + + class ManifestError(Exception): """The manifest is not usable. Carries every problem found, not just the first.""" @@ -49,7 +65,10 @@ def __init__(self, source: str, problems: list[str]) -> None: self.source = source self.problems = problems listed = "\n".join(f" {problem}" for problem in problems) - super().__init__(f"{source} is not a valid Hullwork manifest:\n{listed}") + said = f"{source} is not a valid Hullwork manifest:\n{listed}" + if any("Extra inputs are not permitted" in problem for problem in problems): + said += "\n\n" + _MAY_BE_NEWER.format(version=SCHEMA_VERSION) + super().__init__(said) class _Strict(BaseModel): diff --git a/tests/test_a_field_from_a_newer_hullwork.py b/tests/test_a_field_from_a_newer_hullwork.py new file mode 100644 index 0000000..07d3645 --- /dev/null +++ b/tests/test_a_field_from_a_newer_hullwork.py @@ -0,0 +1,93 @@ +"""What a manifest is told when it carries a field this build does not know. Item 195. + +`SCHEMA_VERSION`'s own docstring promises this: + +> A manifest may say `version: 1`; one that says something higher was written for a newer Hullwork +> and is refused with a message saying so, **rather than producing a wall of `Extra inputs are not +> permitted` about fields that will exist one day.** + +The wall is what a project actually gets, because `version:` is optional and nobody writes it. That +was found asking whether `0.1.0a8` adding `autofix.open_upgrades` justified bumping the schema — it +does not, and the message is the thing that needed fixing instead. + +The bound matters as much as the fix: **most unknown fields are typos**, and a typo sent looking for +a Hullwork release is a worse answer than the wall. So the field and its value stay in the message, +and the version is added beside them rather than in place of them. + +Every test here was verified by reintroducing the defect it covers. +""" + +from __future__ import annotations + +import pytest + +from hullwork.manifest import SCHEMA_VERSION, ManifestError, parse_manifest + +BASE = """ +project: p +git: {provider: github, repo: o/r} +tests: "pytest" +test_path: tests +""" + + +def _refused(text: str) -> str: + with pytest.raises(ManifestError) as raised: + parse_manifest(text) + return str(raised.value) + + +def test_an_unknown_field_says_which_schema_this_build_understands() -> None: + """The sentence a reader needs and did not have: *this build is 1*, so the field may simply be + newer than the binary rather than wrong.""" + said = _refused(BASE + "autofix: {invented_field: 3}\n") + + assert f"schema {SCHEMA_VERSION}" in said or f"version {SCHEMA_VERSION}" in said + assert "newer" in said.lower() + + +def test_it_still_names_the_field_and_the_value() -> None: + """**The bound.** Most of these are typos, and a message that replaced the field name with a + paragraph about releases would send somebody to upgrade over a missing letter.""" + said = _refused(BASE + "autofix: {invented_field: 3}\n") + + assert "invented_field" in said + assert "3" in said + + +def test_a_manifest_from_the_future_keeps_its_own_message() -> None: + """Two different failures that must not be merged. This one is unambiguous — the file *says* it + is newer — and its remedy is exact: upgrade, or pin the manifest down.""" + said = _refused(BASE + f"version: {SCHEMA_VERSION + 1}\n") + + assert "upgrade Hullwork" in said + assert "invented" not in said + + +def test_the_schema_version_is_unchanged() -> None: + """Signed by the operator on 2026-08-09: adding an optional field with a safe default is not a + schema change. A number that increments on every added key stops telling anybody whether their + existing file still means what it meant. + """ + assert SCHEMA_VERSION == 1 + + +def test_an_ordinary_mistake_is_not_told_to_go_and_upgrade() -> None: + """**Found by mutation, and it was the one no test covered.** Gluing the hint onto every failure + passed everything else here. + + A malformed `repo` has nothing to do with the schema version, and a suggestion to upgrade + Hullwork attached to it is worse than silence: it is the product guessing at a cause it has no + reason to believe, in the message somebody reads while already stuck. The hint belongs to + unknown fields and to nothing else. + """ + said = _refused(BASE.replace("repo: o/r", "repo: not-a-repo")) + + assert "newer Hullwork" not in said + assert "upgrade" not in said.lower() + + +def test_a_valid_manifest_is_not_lectured() -> None: + """The message is attached to the failure, never to the parse — a hint printed on success is a + hint printed for ever.""" + assert parse_manifest(BASE).project == "p" diff --git a/tests/test_the_check_that_was_told_where_to_look.py b/tests/test_the_check_that_was_told_where_to_look.py new file mode 100644 index 0000000..77d2ec5 --- /dev/null +++ b/tests/test_the_check_that_was_told_where_to_look.py @@ -0,0 +1,118 @@ +"""Where `status` looks for the deployment's own files. Item 194. + +Item 144 added `deployment_env_file` so a containerised instance could point the environment check +at the host's files — inside a container the working directory holds neither, so the check silently +never ran on any real deployment. It was wired into `doctor` and into nothing else. + +Measured on the live instance, 2026-08-09: `HULLWORK_DEPLOYMENT_ENV_FILE=/deployment/deploy.env`, +the file mounted read-only at that path, the compose setting it for both services — and `status` +printing *not checked: no environment file at `.env`*, which is the default it never replaced. + +That is worse than the two alarms fixed the same day. Those claimed more than they knew; this one +goes quiet with the answer in front of it, and what stays dark is the mechanism that caught the +2026-07-28 tracker failure, where enrichment had never once run in production. + +Every test here was verified by reintroducing the defect it covers. +""" + +from __future__ import annotations + +from pathlib import Path + +from hullwork import cli +from hullwork.config import Settings + +ENV = "HULLWORK_TRACKER_URL=https://tracker.example\n" + + +def _configured(tmp_path: Path, compose: Path | None = None) -> tuple[Settings, Path]: + """A deployment that has done everything the error message asks for.""" + env = tmp_path / "deployment" / "deploy.env" + env.parent.mkdir(parents=True, exist_ok=True) + env.write_text(ENV) + settings = Settings( + deployment_env_file=str(env), + deployment_compose_file=str(compose) if compose else None, + ) + return settings, env + + +# --- the finding --------------------------------------------------------------------------------- + + +def test_the_configured_path_is_the_one_that_is_read(tmp_path: Path) -> None: + """The whole item. A setting that two of three call sites ignore is not a setting.""" + settings, env = _configured(tmp_path) + + resolved, _ = cli.where_the_deployment_files_are(settings) + + assert resolved == env + + +def test_status_reports_the_comparison_rather_than_not_checked(tmp_path: Path) -> None: + """What the operator actually reads. `doctor` was right about this file all along, so an + instance where the two disagree is one where the useful answer is in the command nobody runs on + a bad morning.""" + settings, env = _configured(tmp_path) + + resolved, _ = cli.where_the_deployment_files_are(settings) + from hullwork import doctor + + gaps = doctor.environment_gaps(settings, env_file=resolved) + + assert [f.state for f in gaps] != [doctor.State.UNKNOWN], ( + f"the file at {env} exists and was read; nothing here is unknown" + ) + + +def test_nothing_configured_still_falls_back_to_the_default(tmp_path: Path) -> None: + """**`cannot look` is a finding and has to stay one** (item 144). The failure mode of this fix + is inventing a path that exists, which would turn a true `unknown` into a false `ok`.""" + resolved, _ = cli.where_the_deployment_files_are(Settings()) + + assert resolved == cli.DEFAULT_ENV_FILE + + +def test_the_compose_path_travels_the_same_way(tmp_path: Path) -> None: + """Both halves or neither. Half one is *file → this process*; half two is *file → the + neighbouring compose*, and half two is the one that caught the 2026-07-28 tracker failure — the + host process read the file correctly and it was the container that was not configured. + """ + compose = tmp_path / "deployment" / "docker-compose.yml" + compose.parent.mkdir(parents=True, exist_ok=True) + compose.write_text("services: {}\n") + settings, _ = _configured(tmp_path, compose) + + _, resolved = cli.where_the_deployment_files_are(settings) + + assert resolved == compose + + +def test_a_named_file_beats_the_setting(tmp_path: Path) -> None: + """`doctor --env-file` is the only place a person names the file by hand, and a person standing + in front of the machine outranks what the machine was configured with.""" + settings, _ = _configured(tmp_path) + named = tmp_path / "elsewhere.env" + named.write_text(ENV) + + resolved, _ = cli.where_the_deployment_files_are(settings, env_file=str(named)) + + assert resolved == named + + +def test_every_command_that_runs_the_check_resolves_it_the_same_way() -> None: + """**Asserted by construction**, which is item 193's lesson arriving in a second place. + + Three call sites resolved this pair by hand and two were wrong. A fourth command added later + must not be able to get it wrong by omitting something, so there is one function and the test + is that nobody calls the check with a hand-built path. + """ + source = Path(cli.__file__).read_text(encoding="utf-8") + calls = source.count("environment_gaps(") + resolved_by_hand = source.count("env_file=DEFAULT_ENV_FILE") + + assert calls >= 2, "this test is watching a call site that no longer exists" + assert resolved_by_hand == 0, ( + "a call site resolves the deployment's env file by hand again; " + "use cli.where_the_deployment_files_are so the three commands cannot disagree" + ) diff --git a/tests/test_the_desk_it_cleared.py b/tests/test_the_desk_it_cleared.py index 6244b38..9b6f628 100644 --- a/tests/test_the_desk_it_cleared.py +++ b/tests/test_the_desk_it_cleared.py @@ -194,6 +194,18 @@ def test_a_refusal_is_reported_beside_a_change_and_not_inside_a_total( *"I could not verify this" is a first-class result*, so a total that hides how much of the number it is would be the one place this product rounds its own honesty off. + + **The assertions used to read the whole paragraph** — `"1" in said and "2" in said`, plus + `"refus" in said.lower()` — and deleting the split does fail them, on any fixture: the word is + what catches it, and the digits were redundant rather than load-bearing (item 195 measured that + the other way round first and was wrong). + + Deleting the split is not the only way to lose the honest shape, though, and the other way looks + like tidying: keep both numbers and put them on **separate lines**, so the headline reads `3 + left your desk with evidence attached` and `2 refusals` appears somewhere below it. Every old + assertion passes, and a reader who stops at the headline is not told that two of the three are + refusals — which is exactly what this test's name forbids. So it reads the sentence now, and + asserts the parts are behind the total rather than merely present in the same output. """ _attempt(session, _item(session, project, ItemState.PR_OPEN), AttemptOutcome.PR_OPEN) _attempt( @@ -201,15 +213,27 @@ def test_a_refusal_is_reported_beside_a_change_and_not_inside_a_total( AttemptOutcome.NOT_REPRODUCIBLE, ) _attempt(session, _item(session, project, ItemState.FAILED, n=2), AttemptOutcome.FAILED) + # Queued work, so the paragraph carries other numbers — which is the ordinary case and the one + # the old assertions could not survive. + _item(session, project, ItemState.READY, n=3) + _item(session, project, ItemState.READY, n=4) desk = outcomes.desk(session) assert desk.left_with_evidence == 3 assert desk.with_a_change == 1 assert desk.with_a_refusal == 2 - said = " ".join(outcomes.desk_lines(desk)) - assert "1" in said and "2" in said - assert "refus" in said.lower() + + line = next(one for one in outcomes.desk_lines(desk) if "left your desk" in one) + + assert line.startswith("3 left your desk with evidence attached:"), ( + f"the headline is not the total with its parts behind it: {line!r}" + ) + assert "1 with a change" in line + assert "2 with a reasoned refusal" in line + # And the refusals are **behind** the total rather than instead of it: a reader who stops at the + # first number has not been told something false, only something less. + assert line.index("with a change") < line.index("reasoned refusal") def test_an_instance_that_attempted_nothing_says_so_in_words( diff --git a/tests/test_the_documentation_describes_the_published_artefact.py b/tests/test_the_documentation_describes_the_published_artefact.py index 9482165..f7d38bc 100644 --- a/tests/test_the_documentation_describes_the_published_artefact.py +++ b/tests/test_the_documentation_describes_the_published_artefact.py @@ -259,6 +259,31 @@ def test_every_pin_names_the_version_the_surface_was_recorded_from() -> None: ) +#: A version that is not an image pin and still has to move with a release: what `PRIVACY.md` shows +#: a reader we send about them. `PIN` cannot see it — there is no `ghcr.io/…:` in front of it — so +#: the file appeared in `docs/releasing.md`'s list of pins to move and in no test at all. Item 192 +#: found it by reading that list rather than by anything failing. +SAMPLE_RELEASE = re.compile(r'"release":\s*"(?P[\w.\-]+)"') + + +def test_the_sample_payload_shows_the_version_it_would_really_carry() -> None: + """`PRIVACY.md` prints the exact JSON a crash report contains, and the promise it is making is + that this is what leaves the machine — *"a property of the repository you can check, rather than + a promise you have to accept"*, in that document's own words. + + `release` is filled from `settings.release or __version__`, so a stale one shows somebody a + payload naming a version nobody runs, in the one document written to be audited by a stranger. + Cheap to keep true, and the whole file is worth nothing if a reader catches it being wrong. + """ + shown = {found.group("tag") for found in SAMPLE_RELEASE.finditer(_text("PRIVACY.md"))} + + assert shown, "PRIVACY.md no longer shows the payload, so nothing here is checking anything" + assert shown == {SURFACE["version"]}, ( + f"PRIVACY.md shows a crash report carrying release {sorted(shown)} while the published " + f"image is {SURFACE['version']}. docs/releasing.md lists this file among the pins to move." + ) + + def test_every_document_is_either_published_or_withheld() -> None: """A document added without a decision defaults to invisible, which is the wrong default. diff --git a/tests/test_the_evidence_reaches_an_issue_already_filed.py b/tests/test_the_evidence_reaches_an_issue_already_filed.py new file mode 100644 index 0000000..3b00403 --- /dev/null +++ b/tests/test_the_evidence_reaches_an_issue_already_filed.py @@ -0,0 +1,163 @@ +"""The half that fixes the case that was actually measured. Item 196. + +An issue's body is written **once**, when it is created. On the live instance the two page crashes +were filed on 2026-08-06 and 2026-08-07 and their enrichment arrived on 2026-08-09 — so putting the +evidence in the body fixes every issue filed from now on and **not one that already exists**, which +was every issue on the instance that produced the finding. + +The forge protocol has no `edit_issue` and adding one means three adapters, so the evidence arrives +as a comment. Posted when the **first** sample lands and never again: that is idempotence without a +migration and without a second API call to ask what was already said. + +Every test here was verified by reintroducing the defect it covers. +""" + +from __future__ import annotations + +import logging +from collections.abc import Sequence +from datetime import UTC, datetime + +import pytest +from sqlalchemy.orm import Session +from test_recurrence import FakeForge + +from hullwork import ingest +from hullwork.models import Item, Lane, Project +from hullwork.tracker import FetchedEvent as FetchedEventData +from hullwork.tracker import Frame + + +#: **The real type, not a double.** Three hand-written versions of this drifted from +#: `tracker.FetchedEvent` in a row — missing `grouping_hashes`, missing +#: `is_useful_for_reproduction` — and a double that has to be repaired to match is a double that +#: proves nothing about the code it stands in for. Item 186 measured the same lesson on hand-written +#: manifests. +def _fetched() -> FetchedEventData: + return FetchedEventData( + provider_event_id="e1", + exception_type="OperationalError", + message="database is locked", + culprit="hullwork.page in items", + frames=( + Frame(module="hullwork.page", function="items", lineno=986, abs_path="/a/page.py"), + ), + release="0.1.0a3", + environment="production", + occurred_at=datetime(2026, 8, 6, 10, 17, 58, tzinfo=UTC), + ) + + +class _Tracker: + """Both halves of the protocol, because a partial one is not the thing it stands in for.""" + + def fetch_latest(self, permalink: str) -> FetchedEventData: + return _fetched() + + def fetch_samples( + self, permalink: str, limit: int = 2 + ) -> Sequence[FetchedEventData]: + return [_fetched()] + + +#: The repository's own forge double, extended rather than rewritten — `test_undecidable_fix` sets +#: the precedent. A second partial fake would fail the `Forge` protocol and, worse, would be a +#: second thing to keep in step with it. +class _Forge(FakeForge): + def __init__(self) -> None: + super().__init__() + self.comments: list[tuple[str, int, str]] = [] + + def comment(self, repo: str, number: int, body: str) -> None: + self.comments.append((repo, number, body)) + + +def _item(session: Session, *, issue: str | None) -> Item: + project = Project( + slug="p", forge="forgejo", repo="o/r", active=True, + webhook_secret_hash="x", # noqa: S106 + manifest={}, + ) + session.add(project) + session.flush() + item = Item( + project_id=project.id, fingerprint="fp", title="OperationalError", + lane=Lane.AMBER, occurrences=1, forge_issue_ref=issue, + permalink="http://tracker.example/o/issues/37", + ) + session.add(item) + session.commit() + return item + + +def test_evidence_that_arrives_after_the_issue_is_posted_to_it(session: Session) -> None: + """The measured case: filed 2026-08-07, enriched 2026-08-09, body unchanged for ever.""" + _item(session, issue="#24") + forge = _Forge() + + ingest.fetch_context(session, _Tracker(), forge=forge) + + assert len(forge.comments) == 1 + repo, number, body = forge.comments[0] + assert (repo, number) == ("o/r", 24) + assert "database is locked" in body + assert "hullwork.page.items:986" in body + + +def test_it_is_posted_once_and_not_on_every_pass(session: Session) -> None: + """**Idempotence without a migration.** `_fetch_one` runs again to re-decide a lane from a + stored sample, so a comment guarded only by *the issue exists* would arrive on every pass until + somebody muted the repository. + + `recheck_after=0` is load-bearing and was found by mutation: with the default 600 seconds the + second and third passes do not re-select the item at all, so this test was measuring the recheck + window and calling it idempotence. + + **And the guard it looks like it is testing is not what makes this pass.** Removing + `first_sample` from the condition leaves this green, because once a sample is stored both of + `_fetch_one`'s early branches return before the storing line is reached — so the property is + real, the mechanism is the short-circuit, and the extra term is defence. Said here rather than + left for the next person to discover by deleting it and seeing nothing happen. + """ + _item(session, issue="#24") + forge = _Forge() + + for _ in range(3): + ingest.fetch_context(session, _Tracker(), forge=forge, recheck_after=0) + + assert len(forge.comments) == 1 + + +def test_an_item_with_no_issue_yet_is_not_even_attempted( + session: Session, caplog: pytest.LogCaptureFixture +) -> None: + """Its body will carry the evidence when it is filed, so a comment would say it twice — and + this product's cardinal sin is the duplicate. + + **Asserting the absence of a comment is not enough**, found by mutation: with the guard removed, + `_post_the_evidence` calls `int("None")`, raises, is caught and logged — so no comment appears + and the weaker version of this test passed while the code was doing the wrong thing badly. What + separates the two is whether anything was *tried*. + """ + _item(session, issue=None) + forge = _Forge() + + with caplog.at_level(logging.WARNING, logger="hullwork.ingest"): + ingest.fetch_context(session, _Tracker(), forge=forge) + + assert forge.comments == [] + assert not [r for r in caplog.records if "evidence" in r.message], ( + "it tried to post and failed, rather than correctly not trying" + ) + + +def test_enrichment_still_works_with_no_forge_at_all(session: Session) -> None: + """`fetch_context` is called from places that hold no forge credential, and enrichment is worth + having on its own. A missing forge must cost the fetch nothing.""" + item = _item(session, issue="#24") + + fetched = ingest.fetch_context(session, _Tracker()) + + assert fetched == 1 + session.refresh(item) + assert item.context_checked_at is not None diff --git a/tests/test_the_issue_carries_the_error.py b/tests/test_the_issue_carries_the_error.py new file mode 100644 index 0000000..3a10f3a --- /dev/null +++ b/tests/test_the_issue_carries_the_error.py @@ -0,0 +1,146 @@ +"""What a person opens when Hullwork files an issue. Item 196. + +Found on the live instance: issue `#24` on this repository's own forge, filed from a real production +error, containing a lane, an occurrence count, a first-seen timestamp and a fingerprint marker — and +**no exception message, no location, and no link to the error**. Hullwork had all of it: the frames, +the culprit and the release were sitting in `fetched_events` for that exact item. + +The asymmetry is the point. `brief.py` renders frames for the **agent**, and item 165 made it say +which kind of run you are looking at because a brief with no frames was worthless. The person was +never given the same courtesy, and the two items that crashed the page sat untouched for four days. + +Every test here was verified by reintroducing the defect it covers. +""" + +from __future__ import annotations + +from datetime import UTC, datetime + +from sqlalchemy.orm import Session + +from hullwork import ingest +from hullwork.models import FetchedEvent, Item, Lane, Project + +FRAMES = [ + {"module": "hullwork.main", "function": "page_items", "lineno": 328, "context_line": " x = 1"}, + {"module": "hullwork.page", "function": "items", "lineno": 986, "vars": {"token": "s3cret"}}, +] + + +def _project(session: Session) -> Project: + project = Project( + slug="p", forge="forgejo", repo="o/r", active=True, + webhook_secret_hash="x", # noqa: S106 + manifest={}, + ) + session.add(project) + session.flush() + return project + + +def _item(session: Session, project: Project, *, lane: Lane = Lane.AMBER) -> Item: + item = Item( + project_id=project.id, fingerprint="fp", title="OperationalError: in hullwork.page.items", + lane=lane, occurrences=1, + permalink="http://tracker.example/easybyte-hub/issues/37", + ) + session.add(item) + session.flush() + return item + + +def _evidence(session: Session, item: Item) -> FetchedEvent: + event = FetchedEvent( + item_id=item.id, provider_event_id="80ab0de2", + exception_type="OperationalError", + message="database is locked, while reading the page", + culprit="hullwork.page in items", + frames=FRAMES, release="0.1.0a3", environment="production", + occurred_at=datetime(2026, 8, 6, 10, 17, 58, tzinfo=UTC), + ) + session.add(event) + session.flush() + return event + + +# --- what the issue has to carry ---------------------------------------------------------------- + + +def test_the_body_carries_the_exception_message_and_the_link(session: Session) -> None: + """The two the gate calls a minimum. Without the link a reader cannot reach the error at all; + without the message they are deciding from a title the provider truncated at 100 characters.""" + project = _project(session) + item = _item(session, project) + event = _evidence(session, item) + + body = ingest._issue_body(item, event) + + assert "database is locked, while reading the page" in body + assert "http://tracker.example/easybyte-hub/issues/37" in body + + +def test_the_body_carries_where_it_happened(session: Session) -> None: + """A location is what turns *something broke* into somewhere to look, and it is the whole + difference between the agent's brief and the person's issue.""" + project = _project(session) + item = _item(session, project) + event = _evidence(session, item) + + body = ingest._issue_body(item, event) + + assert "hullwork.page" in body and "items" in body and "986" in body + assert "0.1.0a3" in body, "the release is what says whether this is still true" + + +def test_it_never_carries_a_local_variable(session: Session) -> None: + """**The bound, and the reason frames are rendered rather than dumped.** A frame's captured + variables can hold a token; this body is written into a repository whose readers are not + necessarily the people who may see secrets. Locations are the project's own code and are safe; + variables are not ours to republish. + """ + project = _project(session) + item = _item(session, project) + event = _evidence(session, item) + + body = ingest._issue_body(item, event) + + assert "s3cret" not in body + assert "x = 1" not in body, "source lines go stale and the link above has them" + + +def test_it_states_what_it_leaves_out(session: Session) -> None: + """*A decision the instance makes and can state, not a default nobody chose.* Said in the + artefact every time rather than in a document somebody would have to find.""" + project = _project(session) + item = _item(session, project) + event = _evidence(session, item) + + body = ingest._issue_body(item, event) + + assert "no variables" in body.lower() + + +def test_an_item_with_no_evidence_yet_says_so(session: Session) -> None: + """The commonest case at filing time, and it is why this defect existed: the issue is written + once, at creation, and the enrichment arrives later. Silence there reads as *there was nothing*, + which is a different fact from *it has not arrived*.""" + project = _project(session) + item = _item(session, project) + + body = ingest._issue_body(item, None) + + assert "not arrived" in body or "not yet" in body + assert "| Lane | amber |" in body, "everything it had before is still there" + + +def test_a_red_lane_item_carries_the_evidence_too(session: Session) -> None: + """Red means no agent will touch it, so a person is the **only** one who will — which makes the + evidence more load-bearing there, not less.""" + project = _project(session) + item = _item(session, project, lane=Lane.RED) + event = _evidence(session, item) + + body = ingest._issue_body(item, event) + + assert "database is locked" in body + assert "Red lane" in body, "the existing reclassification sentence survives"