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"