diff --git a/.gitignore b/.gitignore index ccc6a0b..52b628b 100644 --- a/.gitignore +++ b/.gitignore @@ -35,6 +35,8 @@ HISTORY_NOTES.md # every added test. It stays next to the notes it argues with, not in the # release. Its structure is guarded by .claude/hooks/check_notes.py. CHANGELOG-INTERNAL.md +# Handoff briefs for a next session: ours, and they name our own mistakes. +HANDOFF-*.md # Internal scripts kept between sessions: measurement rigs, probes, diagnostics. # Never shipped, never a CI dependency, backed up by the owner separately. diff --git a/CHANGELOG.md b/CHANGELOG.md index 85ec976..9cd299b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,30 @@ The format follows [Keep a Changelog](https://keepachangelog.com/); versions fol ### Added +- **A run that would break everything now says so first.** Start with impairment on, nothing to + aim it at and no time limit, and both the command line and the window say: this affects every + connection on this machine, set a target or a time limit to narrow it. A warning, not a + refusal. Silent in `--simulate`, and once the run is aimed or timed. The same line appears when + a running session becomes unbounded, and in `--dry-run`. "LAN mode" counts as impairment here: + on its own it cuts every connection that leaves the local network. +- **Ctrl+F, and the row menu from the keyboard.** Ctrl+F brings Connections forward and puts the + caret in its search box, and does the same inside the event-log window. Shift+F10, or the menu + key, opens the row menu on the selected connection. Nothing in the table needed a mouse before. +- **An empty table says why it is empty.** "0 of N" under the table was the only sign that a + search had simply matched nothing, which reads the same as something being broken. It tells the + two cases apart: nothing captured yet, or nothing matching what you typed. +- **`--help` starts with worked examples.** It used to open with 24 lines listing every flag, + before a single readable sentence. Four examples now come first - a safe trial run, one aimed + at an application, one aimed at a destination, and one for a pipeline - and the flag list + follows. A mistyped flag shows the error instead of burying it under the same wall. +- **Numbers in the tables line up on the right.** Packets, bytes, ports, PIDs and times were + anchored left, so 9 and 1000000 began at the same pixel and a column of numbers could not be + scanned down. Addresses stay on the left where they read properly, and the short columns beside + a number - protocol, "impaired?", the timestamp - sit centred so the two never touch. Applies to + Connections and to both views of the event log. +- **An impaired row no longer relies on colour alone.** It is shown in bold as well as in orange, + so it stays recognisable whichever columns you have chosen to show - including with the + "impaired?" column hidden. - **Connections: the search box can search one column at a time.** Plain text works as before, and now a term can name its column: `port:443`, `ip:10.0.0.0/8`, `pid:>4000`, `scoped:yes`, `dropped:>0`. Values use the same notation as the Control page fields, and several terms narrow @@ -24,6 +48,9 @@ The format follows [Keep a Changelog](https://keepachangelog.com/); versions fol ### Docs +- **`--help` reads like ordinary writing.** Six flags described themselves with a semicolon + joining two halves of a sentence: `--preset`, `--filter`, `--buffer`, `--dst-ip`, `--block-ip` + and `--block-port`. They now use a full stop or a comma, like the rest of the program's text. - Both READMEs now say in the licence section that **what you make with the tool is yours**. Scenarios you write, saved profiles and config files, reproduction reports, CSV exports, logs and screenshots are your own work: the GPL covers the program, not its output, and using the @@ -32,6 +59,18 @@ The format follows [Keep a Changelog](https://keepachangelog.com/); versions fol ### Fixed +- **Error messages say what to do instead of who is at fault.** A config file with an unusable + number answered "Invalid value for 'loss'". It now says the setting needs a number between 0 + and 100, and quotes back what it got. A misspelled setting in a scenario file gets the same + "did you mean" suggestion config files have always had, an unknown scenario action lists the + ones that exist, and saving a profile names the field instead of "Values must be numbers". +- **A skipped expression no longer names the wrong feature.** The line about an expression that + could not be read claimed targeting was switched off, even when the expression was a blocking + rule. +- **A broken scenario file no longer starts the session first.** `--scenario` with a file the tool + cannot read used to open the capture, impair traffic and only then report the problem. The file + is now read before anything starts, exactly as `--dry-run` already checked it. Same exit code as + before. - In the Settings window, "Capture only the targeted traffic" stayed clickable after you pressed START, if the window was already open at the time. Ticking it did nothing until the next session. It now greys out for as long as the session runs, with the same "locked while running" diff --git a/README.md b/README.md index ce1c5f3..41b42b5 100644 --- a/README.md +++ b/README.md @@ -152,7 +152,7 @@ current screen. row per address with the port cells left empty. - **"down"/"up"/"total" are what the application actually got**, the same quantity the session panel calls "Downloaded (MB)". **"down seen"/"up seen" are what the tool captured** before - impairing anything. With nothing set they are equal; the moment you add loss or a speed limit + impairing anything. With nothing set they are equal. The moment you add loss or a speed limit they part, and **the gap between them is the damage on that connection**. Hover any of them for the full sentence. (Before this split there was one pair, holding the captured bytes under headings that meant delivered: a row could read 5 MB received while its application got 0.4 MB.) Plus a search box (debounced, so it does not churn the table on every keystroke), diff --git a/beantester/cli.py b/beantester/cli.py index 3930112..c8b9f27 100644 --- a/beantester/cli.py +++ b/beantester/cli.py @@ -30,7 +30,7 @@ from .scenario import load_scenario_file from .settings import (DEFAULT_SETTINGS, apply_settings, build_matchers, load_config_file, parse_schedule, save_config_file, - validate_ranges) + validate_ranges, warn_if_unbounded) from .synthetic import SyntheticDivert from .utils import bytes_to_mb @@ -59,12 +59,49 @@ def _fail(code, message): raise CliError(code, message) +# Examples BEFORE the flag list, because that is the order a reader needs them in +# (clig.dev). The usage block alone runs to 24 lines of about fifty flags, and the +# first thing anyone wants from a tool that size is one line they can copy. Four, +# chosen to cover the four shapes a run comes in: harmless, aimed, timed and +# machine-readable - the last two being what a pipeline needs. +# +# Kept in the description rather than the epilog: the epilog holds the exit-code +# table, which is the reference half, and argparse prints the description ABOVE +# the flags and the epilog below them. +_DESCRIPTION = """Bean Network Tester - poor network conditions simulator. +Without arguments it launches the GUI. + +Examples: + %(prog)s --simulate --loss 20 --duration 10 + try it out. No driver, no real traffic, nothing to break. + + %(prog)s --target chrome.exe --latency 200 --duration 30 + impair one application for half a minute, and nothing else. + + %(prog)s --dst-ip 10.0.0.5 --dst-port 443 --loss 5 --duration 60 + impair one destination. Narrow beats broad: the rest of the + machine, including this shell, keeps working. + + %(prog)s --preset 3g --duration 60 --format json --repro-out run.json + a named link profile, machine-readable output, and a report that + carries the command needed to repeat the run. + +A run with impairment turned on, no target and no --duration affects every +connection on this machine until you stop it, and says so before it starts. +""" + + def build_arg_parser(): p = argparse.ArgumentParser( prog=program_name(), + # One line, not the 24 argparse generates from about fifty flags. Those + # 24 lines sat above every readable thing in --help, and above the message + # on a typo too - so the one sentence saying what was wrong arrived at the + # bottom of a wall nobody reads. The flags are listed in full immediately + # below, which is where a list belongs (clig.dev). + usage="%(prog)s [options]", formatter_class=argparse.RawDescriptionHelpFormatter, - description="Bean Network Tester - poor network conditions simulator. " - "Without arguments it launches the GUI.", + description=_DESCRIPTION, epilog=exitcodes.HELP_TABLE) p.add_argument("--version", action="version", version=f"{APP_NAME} {__version__}") @@ -76,10 +113,10 @@ def build_arg_parser(): p.add_argument("--config", help="load settings from a JSON file") p.add_argument("--save-config", help="save effective settings to a JSON file and exit") p.add_argument("--preset", metavar="PRESET", - help="load a preset (canonical id or its name in any UI language; " - "see README for the list)") + help="load a preset by canonical id or by its name in any UI " + "language (the README lists them)") p.add_argument("--filter", choices=list(CLI_FILTERS), default=None, - help="which traffic to capture at all (IPv4 and IPv6); ports are " + help="which traffic to capture at all (IPv4 and IPv6). Ports are " "filtered with --dst-port, not here") p.add_argument("--loss", type=float, help="packet loss [%%]") p.add_argument("--corrupt", type=float, help="corruption [%%]") @@ -89,7 +126,7 @@ def build_arg_parser(): p.add_argument("--down", type=float, help="download limit [KB/s]") p.add_argument("--up", type=float, help="upload limit [KB/s]") p.add_argument("--buffer", type=float, - help="link buffer for the speed limit [ms], 0 = unlimited; " + help="link buffer for the speed limit [ms], 0 = unlimited. It " "bounds the queueing delay a rate-limited link builds up " "before it drops (bufferbloat)") p.add_argument("--target", @@ -97,9 +134,9 @@ def build_arg_parser(): "wildcard, re: pattern, ! to exclude " "(e.g. 'chrome.exe,!chromedriver' or 're:^fire')") p.add_argument("--dst-ip", - help="affect only traffic to/from these remote IPs: address, list, " - "range a-b, CIDR, wildcard, comparison, re: pattern, ! to exclude " - "(e.g. '10.0.0.1-10.0.0.50,!10.0.0.7'); IPv4 and IPv6") + help="affect only traffic to/from these remote IPs, IPv4 and IPv6: " + "address, list, range a-b, CIDR, wildcard, comparison, re: " + "pattern, ! to exclude (e.g. '10.0.0.1-10.0.0.50,!10.0.0.7')") p.add_argument("--dst-port", help="affect only these remote ports: number, list, range a-b, " "comparison (>1024), wildcard, re: pattern, ! to exclude " @@ -113,12 +150,13 @@ def build_arg_parser(): "and then statistics and connections cover the narrowed " "traffic only - the summary says when it took effect") p.add_argument("--block-ip", - help="block (drop) all traffic to these remote IPs: address, list, " - "range a-b, CIDR, wildcard, re: pattern, ! to exclude; IPv4 and IPv6") + help="block (drop) all traffic to these remote IPs, IPv4 and IPv6: " + "address, list, range a-b, CIDR, wildcard, re: pattern, " + "! to exclude") p.add_argument("--block-port", help="block (drop) all traffic to these remote ports: number, list, " "range a-b, comparison (>1024), wildcard, re: pattern, ! to exclude " - "(blocks on IP OR port; e.g. '--block-port 443')") + "(blocks on IP OR port, for example '--block-port 443')") p.add_argument("--syn-drop", type=float, help="dropped TCP SYN rate [%%]") p.add_argument("--max-size", type=int, help="MTU black hole: drop packets > N B") p.add_argument("--spike-prob", type=float, help="latency spike probability [%%]") @@ -204,7 +242,11 @@ def config_from_args(args): try: s.update(load_config_file(args.config)) except ValueError as e: - _fail(exitcodes.CONFIG, f"invalid config file {args.config!r}: {e}") + # The translated message already says "in the config file" and which + # setting - so this prefix carries the PATH and nothing else. It used + # to say "invalid config file", which put the word "invalid" and the + # words "config file" on the line twice each. + _fail(exitcodes.CONFIG, f"{args.config!r}: {e}") except OSError as e: _fail(exitcodes.CONFIG, f"cannot read config file {args.config!r}: {e}") if args.preset: @@ -532,6 +574,27 @@ def _run_session(args, cfg, log, sleep, clock, engine): if cfg["simulate"]: log.info("SIMULATION mode (synthetic traffic, no WinDivert).") + # Said BEFORE the divert opens, while stopping still costs nothing. This is + # the mode our own documentation calls the most dangerous, and until now the + # tool started it in silence: measured, `--lat 5` alone impaired 11 844 + # packets of a live machine for 202 s before anyone noticed. A + # warning, not a refusal - refusing would break every pipeline that already + # runs this way. Nothing to warn about in --simulate: there is no real traffic. + if not cfg["simulate"]: + warn_if_unbounded(cfg["settings"], log.warn) + + # Loaded BEFORE the capture starts, exactly like --dry-run does it. A scenario + # file that cannot be read is knowable without touching the driver, and the + # old order proved it: the run opened the divert, printed "Start.", impaired + # traffic and only then said the file was broken. Failures from RUNNING the + # scenario still land below, where the session can report them properly. + scen = None + if cfg["scenario"]: + try: + scen = load_scenario_file(cfg["scenario"]) + except Exception as e: + _fail(exitcodes.SCENARIO, f"scenario error in {cfg['scenario']!r}: {e}") + try: log.debug("opening the divert...") engine.start(cfg["filter"], divert=divert, duration=cfg["duration"], @@ -560,9 +623,8 @@ def _run_session(args, cfg, log, sleep, clock, engine): scenario_failed = None cfg["stop_on_scenario"] = False - if cfg["scenario"]: + if scen is not None: try: - scen = load_scenario_file(cfg["scenario"]) scen.loop = scen.loop or cfg["loop"] engine.start_scenario(scen, cfg["settings"], log=log.info) log.debug(f"scenario: {len(scen.steps)} steps, " @@ -751,6 +813,13 @@ def run_cli(argv=None, sleep=time.sleep, clock=time.monotonic, engine=None, log.debug(f"scenario: {len(scen.steps)} steps, " f"{scen.duration:.0f}s, loop={scen.loop or cfg['loop']}") _log_effective_settings(log, cfg) + # A preview that stays quiet about the dangerous shape is a preview + # that misleads: "Configuration is valid" is about each value, and the + # warning is about the SHAPE - impairment armed, nothing aimed at, + # nothing to end it. This is the cheapest place a user can find that + # out, since --dry-run touches neither the driver nor the traffic. + if not cfg["simulate"]: + warn_if_unbounded(cfg["settings"], log.warn) log.info("Configuration is valid (--dry-run: nothing was started). " "This checks the settings, not the machine - run --doctor " "for Administrator rights and the WinDivert driver.") diff --git a/beantester/fields.py b/beantester/fields.py index 7f3ca85..2c0c3f2 100644 --- a/beantester/fields.py +++ b/beantester/fields.py @@ -34,6 +34,22 @@ RATE = (0.0, 10000000.0) # KB/s, 0 = unlimited SECONDS = (0.0, 86400.0) +# -- blast radius ---------------------------------------------------------- # +# What a field does to traffic, so that "is this run about to damage everything +# on this machine?" is answered from the registry instead of from a list some +# later session has to remember to update. +# +# Every value below is taken from the GATE in BeanCore.decide() that reads the +# field, not from its name. Three states, because two cannot describe blocking: +IMPAIRS_ALL = "all" # armed alone, this damages every captured packet +IMPAIRS_MATCHED = "matched" # armed alone, this damages only what it names itself +# +# The trap this encodes: a field that merely PARAMETRISES an impairment arms +# nothing. `spike_ms` without `spike_prob`, `flap_down` without `flap_period` +# and `rst_cooldown` without `rst_prob` all sit behind their trigger's gate +# (decide() steps 4, 5 and 10), so treating them as impairments would warn about +# a run that damages nothing - and a warning that cries wolf is worse than none. + class Field(NamedTuple): """One setting: how it is typed, validated, labelled and rendered.""" @@ -58,19 +74,25 @@ class Field(NamedTuple): ui_only: bool = False # the ENGINE never sees it: a view setting, applied live help_title: str = "" # i18n key of the "?" help-sheet title (optional) help_body: str = "" # i18n key of the "?" help-sheet body (optional) + impairs: str = "" # "" | IMPAIRS_ALL | IMPAIRS_MATCHED (see above) + narrows: bool = False # bounds what EVERY later impairment can reach FIELD_DEFS = ( # -- traffic ----------------------------------------------------------- # Field("filter", CHOICE, "frames.traffic", "traffic", tip="tips.filter", span=True, cli="filter", start_only=True), + # IMPAIRS_ALL, and it is the least obvious entry in this table: LAN mode reads + # like a scope ("only the local network") and is the exact opposite. decide() + # step 2b DROPS every packet whose remote end is public, so this flag alone, + # with the whole rest of the form at zero, cuts the machine's internet. Field("lan_mode", BOOL, "fields.lan_mode", "traffic", - tip="tips.lan_mode", span=True, cli="lan-mode"), + tip="tips.lan_mode", span=True, cli="lan-mode", impairs=IMPAIRS_ALL), # -- target process ---------------------------------------------------- # Field("target", EXPR, TARGET_FIELD, "target_process", expr_kind=KIND_PROCESS, width=34, tip="tips.target_process", hint="fields.target_example", - span=True, cli="target"), + span=True, cli="target", narrows=True), # -- speed limit ------------------------------------------------------- # # A throughput schedule REPLACES the constant limits (BeanCore._current_rates @@ -78,10 +100,12 @@ class Field(NamedTuple): # the two fields sat there looking live while the engine ignored them. Field("down", NUMBER, "fields.download", "speed_limit", unit="KB/s", bounds=RATE, tip="tips.down_limit", in_profile=True, cli="down", - overridden_by="rate_schedule", override_note="fields.schedule_overrides"), + overridden_by="rate_schedule", override_note="fields.schedule_overrides", + impairs=IMPAIRS_ALL), Field("up", NUMBER, "fields.upload", "speed_limit", unit="KB/s", bounds=RATE, tip="tips.up_limit", in_profile=True, cli="up", - overridden_by="rate_schedule", override_note="fields.schedule_overrides"), + overridden_by="rate_schedule", override_note="fields.schedule_overrides", + impairs=IMPAIRS_ALL), # The link buffer for the speed limit: how much queueing delay a rate-limited # link may build up before it drops (bufferbloat), in ms. 0 = unbounded. It # applies to the constant limits AND to a schedule, so it is NOT @@ -105,10 +129,12 @@ class Field(NamedTuple): # registry declares them instead of a second table doing the translation. Field("latency", NUMBER, "fields.latency", "latency", unit="ms", bounds=MS, tip="tips.latency", in_profile=True, preset_key="lat", - cli="latency"), + cli="latency", impairs=IMPAIRS_ALL), + # jitter arms on its own: decide() step 10 spreads the delay around a latency + # of zero and clamps the negative half back to 0, so the packet still waits. Field("jitter", NUMBER, "fields.jitter", "latency", unit="ms", bounds=MS, tip="tips.jitter", in_profile=True, preset_key="jit", - cli="jitter"), + cli="jitter", impairs=IMPAIRS_ALL), # A spike IS latency - an occasional large one - so it belongs next to the # steady value and the jitter around it, not among the NAT/connection knobs # it used to sit with. Nothing outside this registry knew where they lived: @@ -116,18 +142,21 @@ class Field(NamedTuple): # variable wiring) goes by the field KEY. Field("spike_prob", NUMBER, "fields.spike_prob", "latency", unit="%", bounds=PCT, width=6, tip="tips.spike", in_profile=True, - cli="spike-prob"), + cli="spike-prob", impairs=IMPAIRS_ALL), Field("spike_ms", NUMBER, "fields.spike_ms", "latency", unit="ms", bounds=MS, width=8, tip="tips.spike", in_profile=True, cli="spike-ms"), # -- impairments ------------------------------------------------------- # Field("loss", NUMBER, "fields.loss", "impairments", unit="%", - bounds=PCT, width=6, tip="tips.loss", in_profile=True, cli="loss"), + bounds=PCT, width=6, tip="tips.loss", in_profile=True, cli="loss", + impairs=IMPAIRS_ALL), Field("corrupt", NUMBER, "fields.corruption", "impairments", unit="%", - bounds=PCT, width=6, tip="tips.corrupt", in_profile=True, cli="corrupt"), + bounds=PCT, width=6, tip="tips.corrupt", in_profile=True, cli="corrupt", + impairs=IMPAIRS_ALL), Field("dup", NUMBER, "fields.duplication", "impairments", unit="%", - bounds=PCT, width=6, tip="tips.dup", in_profile=True, cli="dup"), + bounds=PCT, width=6, tip="tips.dup", in_profile=True, cli="dup", + impairs=IMPAIRS_ALL), # -- flapping ---------------------------------------------------------- # # in_profile: the outage is PERIODIC and phase-locked to the session start @@ -136,44 +165,53 @@ class Field(NamedTuple): # flaky uplink actually looks like, and what no other profile field can say. Field("flap_period", NUMBER, "fields.period", "flapping", unit="s", bounds=SECONDS, width=6, tip="tips.flap", in_profile=True, - cli="flap-period"), + cli="flap-period", impairs=IMPAIRS_ALL), Field("flap_down", NUMBER, "fields.flap_down_pct", "flapping", unit="%", bounds=PCT, width=6, tip="tips.flap", in_profile=True, cli="flap-down"), # -- destination ------------------------------------------------------- # Field("dst_ip", EXPR, "fields.ip", "destination", expr_kind=KIND_IP, - width=26, tip="tips.dest", span=True, cli="dst-ip"), + width=26, tip="tips.dest", span=True, cli="dst-ip", narrows=True), Field("dst_port", EXPR, "fields.port", "destination", expr_kind=KIND_INT, - bounds=PORT_BOUNDS, width=18, tip="tips.dest", span=True, cli="dst-port"), + bounds=PORT_BOUNDS, width=18, tip="tips.dest", span=True, cli="dst-port", + narrows=True), # -- blocking (firewall) ---------------------------------------------- # # Drop traffic to matching destinations outright. IP OR port (each takes part # only when non-empty), applied after the targeting gate - see BeanCore.decide # step 2c. Same expression fields as destination targeting; distinct because # this DROPS rather than merely scoping what gets impaired. + # + # IMPAIRS_MATCHED, not IMPAIRS_ALL, and not `narrows` either: blocking damages + # only the destinations it names, so a run whose only impairment is a block is + # already bounded - but that bound is its own. It does NOT scope loss, latency + # or anything else, which is why these two need a state of their own instead + # of counting as targeting. Field("block_ip", EXPR, "fields.ip", "block", expr_kind=KIND_IP, - width=26, tip="tips.block", span=True, cli="block-ip"), + width=26, tip="tips.block", span=True, cli="block-ip", + impairs=IMPAIRS_MATCHED), Field("block_port", EXPR, "fields.port", "block", expr_kind=KIND_INT, - bounds=PORT_BOUNDS, width=18, tip="tips.block", span=True, cli="block-port"), + bounds=PORT_BOUNDS, width=18, tip="tips.block", span=True, cli="block-port", + impairs=IMPAIRS_MATCHED), # -- advanced ---------------------------------------------------------- # Field("syn_drop", NUMBER, "fields.syn_drop", "advanced", unit="%", - bounds=PCT, width=6, tip="tips.syn", cli="syn-drop"), + bounds=PCT, width=6, tip="tips.syn", cli="syn-drop", impairs=IMPAIRS_ALL), Field("max_size", NUMBER, "fields.max_size", "advanced", unit_key="fields.unit_b_off", bounds=(0.0, 65535.0), width=8, - tip="tips.mtu", cli="max-size"), + tip="tips.mtu", cli="max-size", impairs=IMPAIRS_ALL), Field("nat_timeout", NUMBER, "fields.nat_timeout", "advanced", unit_key="fields.unit_s_off", bounds=SECONDS, width=6, - tip="tips.nat", cli="nat-timeout"), + tip="tips.nat", cli="nat-timeout", impairs=IMPAIRS_ALL), Field("rst_prob", NUMBER, "fields.rst", "advanced", unit="%", - bounds=PCT, width=6, tip="tips.rst", cli="rst-prob"), + bounds=PCT, width=6, tip="tips.rst", cli="rst-prob", impairs=IMPAIRS_ALL), Field("rst_cooldown", NUMBER, "fields.rst_cooldown", "advanced", unit="s", bounds=(0.0, 3600.0), width=6, tip="tips.rst_cooldown", cli="rst-cooldown"), # -- schedule ---------------------------------------------------------- # Field("rate_schedule", SCHEDULE, "fields.schedule", "schedule", width=34, - tip="tips.schedule", span=True, cli="rate-schedule"), + tip="tips.schedule", span=True, cli="rate-schedule", impairs=IMPAIRS_ALL), # -- session ----------------------------------------------------------- # # Applied at START only, exactly like "filter" - so, exactly like "filter", @@ -306,6 +344,14 @@ def expression_fields(): UI_ONLY_KEYS = frozenset(f.key for f in FIELD_DEFS if f.ui_only) NON_PROFILE_FIELDS = tuple((f.key, f.label) for f in FIELD_DEFS if not f.in_profile) +# Blast-radius views over the registry (see IMPAIRS_ALL above). These exist so that +# "does this run damage everything?" has ONE answer: settings.unbounded_impairment +# reads them, and tests/test_passthrough.py derives its pass-through invariant from +# them, so a new impairment is declared once instead of remembered twice. +IMPAIRING_KEYS = tuple(f.key for f in FIELD_DEFS if f.impairs) +GLOBALLY_IMPAIRING_KEYS = tuple(f.key for f in FIELD_DEFS if f.impairs == IMPAIRS_ALL) +NARROWING_KEYS = tuple(f.key for f in FIELD_DEFS if f.narrows) + def overriding_field(field): """The field that makes ``field`` inert, or None.""" diff --git a/beantester/gui/app.py b/beantester/gui/app.py index 439e8c4..5440a77 100644 --- a/beantester/gui/app.py +++ b/beantester/gui/app.py @@ -44,7 +44,7 @@ from ..scenario import load_scenario_file from ..settings import (DEFAULT_SETTINGS, apply_settings, apply_targeting, load_config_file, non_profile_active, save_config_file, - settings_from_raw) + settings_from_raw, warn_if_unbounded) from ..summary import settings_summary from ..utils import number_string from ..views import avg_packet_bytes, connection_proc, filter_sort_connections @@ -995,8 +995,15 @@ def save_profile(self): try: values = settings_to_preset(self._settings_from_widgets()) self.profiles.set(name, {k: float(v) for k, v in values.items()}) - except ValueError: - dialogs.show_error(self.root, T("log.error"), T("dialogs.values_numbers")) + except ValueError as e: + # The message _settings_from_widgets raises names the field and its + # range ("Field 'Loss' must be between 0 and 100"); this used to + # throw it away and show "Values must be numbers", which named + # neither. _start, four hundred lines down, has always shown str(e) + # for the SAME exception from the SAME call. float() below cannot be + # the one raising: settings_to_preset reads an already validated + # dict, so every value it hands over is a number. + dialogs.show_error(self.root, T("log.error"), str(e)) return self._persist_profiles() self._set_profile_key(name) # saving one also SELECTS it - remember that @@ -1310,6 +1317,11 @@ def apply_if_running(self, *_, announce=False): self.log(f"{T('log.error')}: {e}") return apply_settings(self.engine, s, self.log) + # A session can BECOME unbounded: clear the target, press "Apply changes", + # and from that moment everything on the machine is in scope. Warning only + # at START would mean the one path that reaches this state in silence is + # the one the user takes deliberately. + warn_if_unbounded(s, self.log) self.engine.log_event("CHANGE", settings_summary(s, "en")) self.log(f"{T('log.applied_changes')}: {settings_summary(s, self._lang)}") self._applied_sig = self._signature(self._raw_settings()) @@ -1463,6 +1475,11 @@ def _start(self): self._pending_start_settings = s filt = windivert_for(s["filter"]) duration = s.get("duration", 0) + # Same sentence the CLI prints, from the same condition and the same + # function: nothing narrows this run and nothing ends it, so every + # connection on the machine is in scope. Said before the driver opens, on + # the UI thread, while STOP is still one click away. + warn_if_unbounded(s, self.log) # Immediate feedback: the psutil target resolution and the WinDivert driver # load (~0.5-1 s) run on the worker thread below, so without this the click # feels dead until the driver is up. Log now, on the UI thread, before work. @@ -1505,33 +1522,14 @@ def _finish_start(self, err): self._scenario.loop = self.loop_var.get() self.engine.start_scenario(self._scenario, s, log=self.log) self._snapshot_target() - self._log_capture_scope(s) + note = scope.capture_scope_note(s, self.engine.capture_narrowed()) + if note: + self.log(T(note)) # No refresher thread any more: _tick applies a changed expression and the # engine's resolver keeps the port set fresh (see _refresh_target). self._applied_target = None # re-apply once, now that the engine is up self._sync_running_ui() - def _log_capture_scope(self, s): - """Say whether "Capture only the targeted traffic" actually took effect. - - Asked for and got it, or asked for and did NOT: both have to be said, and - only the second is easy to miss. The option silently does nothing when the - destination cannot be expressed as a driver filter - a wildcard, an ``re:`` - pattern, only a process target, no destination at all, or a port list too - long for the driver's grammar - and the fallback is the safe direction, so - nothing else about the session looks unusual. The CLI has warned about - this since the option shipped (``cli._run_session``); the window said - nothing at all, which left the one interface where the checkbox is - actually visible as the one that never mentioned the outcome. - - Not a dialog: this is information about a session that started fine, and a - modal here would interrupt the run the user just asked for. - """ - if not s.get("narrow_filter"): - return - self.log(T("log.narrow_applied" if self.engine.capture_narrowed() - else "log.narrow_no_effect")) - def _stop(self): if self._transition is not None: return diff --git a/beantester/gui/pages/conns.py b/beantester/gui/pages/conns.py index 996aaf0..9346c0a 100644 --- a/beantester/gui/pages/conns.py +++ b/beantester/gui/pages/conns.py @@ -21,6 +21,7 @@ driver's own filter has been narrowed to the destination. """ +import sys import time import tkinter as tk from tkinter import ttk @@ -62,6 +63,25 @@ "down_seen": "conns.down_seen", "up_seen": "conns.up_seen", "avg": "conns.avg", "dur": "conns.time", "idle": "conns.idle"} +# Columns whose cells are numbers, right-aligned so their orders of magnitude +# line up (Carbon Design System, and Microsoft's typographic guidance). Declared +# beside the registry rather than derived in the widget, so a numeric column +# added here is aligned without anybody remembering that alignment exists. +# +# `remote_ip` is deliberately absent: an address is read left to right, group by +# group, and right-aligning it would ragged the leading octets. `scoped` is a +# word ("yes"/"no"), not a number. +NUMERIC = frozenset({"pid", "remote_port", "local_port", "packets", "dropped", + "down", "up", "kb", "down_seen", "up_seen", "avg", + "dur", "idle"}) + +# Centred, because each of them sits immediately after a number and holds values +# of one width. `proto` follows `pid` and `scoped` follows `packets`, and with the +# numbers moved right the first screenshot showed "67400 TCP" and "550 tak" - +# two columns reading as one value. Centring restores the gap that ttk gives no +# padding for, and on a column of equal-width values it costs nothing. +CENTERED = frozenset({"proto", "scoped"}) + MIN_CHARS = {"proc": 16, "pid": 7, "proto": 5, "remote_ip": 18, "remote_port": 6, "local_port": 6, "packets": 7, "scoped": 7, "dropped": 8, "down": 8, "up": 8, "kb": 8, "down_seen": 11, "up_seen": 11, @@ -173,7 +193,19 @@ def __init__(self, app, parent): entry.pack(side="left", padx=(scaled(4), scaled(8))) entry.bind("", lambda e: self._schedule_search()) entry.bind("", lambda e: self._clear_search()) - add_tooltip(entry, "tips.conn_search") + add_tooltip(entry, "tips.conn_search", shortcut="Ctrl+F") + self._search_entry = entry + # Ctrl+F is what every table in every tool does, and this one had no way + # in from the keyboard at all. Bound on the ROOT, not on the entry: a + # shortcut that only works once the caret is already in the search box is + # not a shortcut. It brings the page forward first, so the answer to + # "find" is the same wherever the user was (WCAG 2.1.1 asks for the + # keyboard path to exist; this is also just how people work). + # Bound here rather than in App._bind_shortcuts because the search box + # belongs to this page - and because app.py sits on the size ratchet. + with crashlog.quiet("gui.pages.conns"): + app.root.bind("", self._focus_search) + app.root.bind("", self._focus_search) # The same "?" affordance the expression fields use (gui/form.py): the search # box understands `port:443` and `ip:10.0.0.0/8`, and a cheat sheet you can # read is the only way anyone finds that out. A tooltip cannot be it - it @@ -214,7 +246,9 @@ def __init__(self, app, parent): self.table = SortableTree(holder, COLUMNS, sort=app.conn_sort, on_sort=self._on_sort, height=18, horizontal=True, tags=CONN_COLORS, - min_chars=MIN_CHARS, tips=COLUMN_TIPS) + min_chars=MIN_CHARS, tips=COLUMN_TIPS, + numeric=NUMERIC, centered=CENTERED, + empty_text="tables.no_conns_yet") self.table.sort.setdefault("default_reverse", True) # A layout saved by an earlier run. Unknown ids are dropped by the table # (a column may have been removed since), and an empty or missing entry @@ -249,6 +283,25 @@ def _build_menu(self): command=self.table.reset_widths) self.table.tree.bind("", self._popup) self.table.tree.bind("", self._popup) # macOS + # The same menu, reachable without a mouse. WCAG 2.1.1: anything doable + # with the pointer has to be doable from the keyboard - and this is a tool + # for testers and admins, where services.msc and the console have had + # Shift+F10 forever. + # + # The dedicated menu key is spelled DIFFERENTLY per platform - "App" on + # Windows, "Menu" on X11 - and Tk RAISES on a keysym the platform does + # not know rather than ignoring it. Binding "App" unconditionally passed + # every Windows test and killed the Linux render check, so the spelling + # is chosen here rather than tried blindly. Shift+F10 exists everywhere, + # so the keyboard route survives even if the menu key does not. + menu_key = "" if sys.platform == "win32" else "" + for sequence in ("", menu_key): + try: + self.table.tree.bind(sequence, self._popup_from_keyboard) + except tk.TclError as _exc: + # insurance, not the expected path: the spelling above is the one + # this platform should know, so a failure here is worth recording + crashlog.note(_exc, "gui.pages.conns") def _popup(self, event): """Show the menu only when it has a row to act on. @@ -263,6 +316,23 @@ def _popup(self, event): # select by MODEL key: the widget's item ids are recycled viewport slots, # so they say nothing about which connection was clicked self.table.select_keys([key]) + return self._show_menu(event.x_root, event.y_root) + + def _popup_from_keyboard(self, _event=None): + """Shift+F10 / the menu key, on whatever row is already selected. + + Nothing to position against here - there is no pointer - so the menu + opens at the table's own corner. It refuses on an empty selection for the + same reason ``_popup`` refuses on an empty table: a menu offering "Copy + row" with no row is a menu that lies. + """ + if not self.table.selected_keys(): + return "break" + tree = self.table.tree + return self._show_menu(tree.winfo_rootx() + scaled(40), + tree.winfo_rooty() + scaled(40)) + + def _show_menu(self, x_root, y_root): # a row whose process could not be resolved (no admin rights) cannot be # targeted - grey the entry out instead of failing after the click selected = self._selected() or {} @@ -274,7 +344,7 @@ def _popup(self, event): except Exception as _exc: crashlog.note(_exc, "gui.pages.conns") try: - self.menu.tk_popup(event.x_root, event.y_root) + self.menu.tk_popup(x_root, y_root) finally: try: self.menu.grab_release() @@ -323,6 +393,14 @@ def _choose_columns(self): self.table.set_visible_columns(chosen) self.app.ui.set("conn_columns", list(self.table.visible_columns())) + def _focus_search(self, _event=None): + """Ctrl+F: show this page and put the caret in its search box.""" + with crashlog.quiet("gui.pages.conns"): + self.app.select_page(self.ID) + self._search_entry.focus_set() + self._search_entry.select_range(0, "end") + return "break" + def _show_search_help(self): """The search cheat sheet, opened by the "?" next to the box.""" dialogs.show_help(self.app.root, T("dialogs.conn_search_help_title"), @@ -569,6 +647,13 @@ def _apply(self, result): """Main thread: swap the finished model in whole.""" rows, total, limit = result["rows"], result["total"], result["limit"] self._scope_active = result.get("scope_active", False) + # WHY it would be empty, before handing the rows over: an empty table + # with nothing typed means no traffic yet, and telling that user that + # "nothing matches what you are looking for" is a lie about a search + # they never made. + self.table.set_empty_text("tables.no_conns_match" + if self.search_var.get().strip() + else "tables.no_conns_yet") # LAZY: hand over the raw rows; _render runs for the visible ones only self.table.set_model(rows, render=self._render, key_of=self._key_of, tag_of=self._tag_of) diff --git a/beantester/gui/pages/stats.py b/beantester/gui/pages/stats.py index fba8017..50dd448 100644 --- a/beantester/gui/pages/stats.py +++ b/beantester/gui/pages/stats.py @@ -249,7 +249,13 @@ def _build_events(self, parent): on_sort=self._on_event_sort, height=14, stretch=("desc",), tips=EVENT_TIPS, tags=EVENT_COLORS, - min_chars={"t": 6, "time": 18, "type": 10, "desc": 40}) + min_chars={"t": 6, "time": 18, "type": 10, "desc": 40}, + # elapsed seconds, the one quantity here + numeric={"t"}, + # the timestamp follows "t" and is always the + # same width: centred, so a right-aligned + # number cannot touch it + centered={"time"}) self._event_sig = None # -- responsive counter grid --------------------------------------------- # diff --git a/beantester/gui/panels/event_log.py b/beantester/gui/panels/event_log.py index 61abee1..10bfccb 100644 --- a/beantester/gui/panels/event_log.py +++ b/beantester/gui/panels/event_log.py @@ -72,7 +72,14 @@ def build(self, body): # every keystroke rebuilds the model several times per word typed entry.bind("", lambda e: self._debounce()) entry.bind("", lambda e: self._clear()) - add_tooltip(entry, "tips.event_search") + add_tooltip(entry, "tips.event_search", shortcut="Ctrl+F") + self._search_entry = entry + # (4b) The window owns its own toplevel, so it binds its own Ctrl+F - the + # main window's binding cannot reach here, and a search box with no + # keyboard way in is the gap this closes on both tables at once. + with crashlog.quiet("gui.windows.event_log"): + self.win.bind("", self._focus_search) + self.win.bind("", self._focus_search) clear = ttk.Button(top, text=T("buttons.clear"), command=self._clear) clear.pack(side="left", padx=(scaled(6), 0)) @@ -92,6 +99,13 @@ def build(self, body): on_sort=self._on_sort, height=20, horizontal=True, tips=TIPS, tags=EVENT_COLORS, min_chars={"t": 6, "time": 18, "type": 10, "desc": 40}, + # "t" is elapsed seconds - a quantity, so it lines up on the right. + # "time" is a timestamp and "type"/"desc" are words: all read left. + numeric={"t"}, + # the timestamp follows "t" and is always the same width: + # centred so a right-aligned number cannot touch it + centered={"time"}, + empty_text="tables.no_events_yet", ) actions = ttk.Frame(body) @@ -123,6 +137,10 @@ def refresh(self, force=False): if limit and len(events) > limit: events = events[:limit] + # Same as the Connections page: say WHY it is empty. With no filter typed + # an empty log means the session has not logged anything yet. + self.table.set_empty_text("tables.no_events_match" if self._query + else "tables.no_events_yet") # (3) LAZY: the raw events go in; _render is called only for what is shown self.table.set_model(events, render=self._render, key_of=self._key, tag_of=lambda e: str(e[2])) @@ -162,6 +180,13 @@ def _run_search(self): self._query = self.search_var.get().strip() self.refresh(force=True) + def _focus_search(self, _event=None): + """Ctrl+F: put the caret in this window's search box.""" + with crashlog.quiet("gui.windows.event_log"): + self._search_entry.focus_set() + self._search_entry.select_range(0, "end") + return "break" + def _clear(self): self.search_var.set("") self._run_search() diff --git a/beantester/gui/scope.py b/beantester/gui/scope.py index 2679198..ecfa005 100644 --- a/beantester/gui/scope.py +++ b/beantester/gui/scope.py @@ -102,3 +102,25 @@ def coverage(capture_narrowed, view_scoped, process_target): else: state = ALL return Coverage(state, capture_narrowed, view_scoped, process_target) + + +def capture_scope_note(settings, capture_narrowed): + """i18n key of the line to log about the capture's scope, or ``None``. + + Asked for and got it, or asked for and did NOT: both have to be said, and + only the second is easy to miss. "Capture only the targeted traffic" + silently does nothing when the destination cannot be expressed as a driver + filter - a wildcard, an ``re:`` pattern, only a process target, no + destination at all, or a port list too long for the driver's grammar - and + the fallback is the safe direction, so nothing else about the session looks + unusual. The CLI has warned about this since the option shipped; the window + said nothing at all, which left the one interface where the checkbox is + actually visible as the one that never mentioned the outcome. + + A key rather than a logged line, and here rather than in the App: this + module is where "what do the numbers cover?" is decided for every surface, + and the answer stops depending on which one is asking. + """ + if not settings.get("narrow_filter"): + return None + return "log.narrow_applied" if capture_narrowed else "log.narrow_no_effect" diff --git a/beantester/gui/theme.py b/beantester/gui/theme.py index 5ccc242..f981900 100644 --- a/beantester/gui/theme.py +++ b/beantester/gui/theme.py @@ -59,8 +59,22 @@ # actually narrowing (see ConnsPage._tag_of), so it never floods. A warm amber # FOREGROUND reads far cleaner than the old brown background, which went muddy over # the blue-grey table and looked like a defect rather than a highlight. +FONT = "Segoe UI" +MONO_FONT = "Consolas" + +# The impaired row carries TWO signals, and the second one is not decoration. +# WCAG 1.4.1 (level A): colour must not be the only visual means of conveying +# information. Until 2026-08-02 the text column "impaired?" was the other +# carrier - then the column chooser shipped and the user could hide it, which +# put the row back on colour alone. +# +# Weight, not a marker inside a cell: EVERY cell belongs to a column, and every +# column can be hidden, so a dot in the process cell would have exactly the +# problem it was meant to fix. A tag's font applies to the row whatever is on +# screen. Same family and size as the Treeview style above it, so the row height +# (`rowheight`) still fits and nothing is clipped at 125% scaling. CONN_COLORS = { - "impaired": {"foreground": "#ffb454"}, + "impaired": {"foreground": "#ffb454", "font": (FONT, 9, "bold")}, } GRID_C = "#333845" # chart grid lines @@ -74,10 +88,6 @@ SCROLL_BG = "#3a4150" # scrollbar thumb SCROLL_TROUGH = "#20232b" -FONT = "Segoe UI" -MONO_FONT = "Consolas" - - def init_style(root=None): """Configure the shared ttk styles for the dark theme.""" s = ttk.Style() diff --git a/beantester/gui/widgets/sortable_tree.py b/beantester/gui/widgets/sortable_tree.py index ded3f41..8ac3f1d 100644 --- a/beantester/gui/widgets/sortable_tree.py +++ b/beantester/gui/widgets/sortable_tree.py @@ -70,8 +70,25 @@ class SortableTree: def __init__(self, parent, columns, sort=None, on_sort=None, height=10, stretch=(), horizontal=False, min_chars=None, - tips=None, tags=None, selectmode="extended"): + tips=None, tags=None, selectmode="extended", numeric=(), + centered=(), empty_text=""): self.columns = dict(columns) # column id -> i18n key of its header + # Alignment is a property of the COLUMN, so it is declared by the page + # that owns the registry and never guessed from a value here. Numbers go + # right, because that is what lines up their orders of magnitude - the + # whole reason a column of numbers is worth reading down. Text stays left. + # Guessing per cell would align a column differently on the row where a + # port is empty, which looks like a rendering fault. + self._numeric = frozenset(numeric) + # Centred columns exist because of what right-alignment DOES to its + # neighbour. A right-aligned cell sits on its column's right edge and a + # left-aligned one on its own left edge, so the two touch: the first + # screenshot after numbers moved right read "67400 TCP" and "550 tak" as + # single values. ttk gives no per-cell padding, so alignment is the only + # lever. For a column whose values are all the SAME WIDTH - a protocol, a + # yes/no, a timestamp - centring looks identical to any other choice and + # leaves a margin on both sides, which is exactly what was missing. + self._centered = frozenset(centered) self._visible = set(self.columns) # see set_visible_columns self.sort = dict(sort or {"col": next(iter(self.columns)), "reverse": False}) self.on_sort = on_sort @@ -122,13 +139,20 @@ def __init__(self, parent, columns, sort=None, on_sort=None, for col, key in self.columns.items(): width = self._width_for(col, key) self._natural[col] = width - self.tree.column(col, anchor="w", stretch=(col in stretch), + self.tree.column(col, anchor=self.anchor_for(col), + stretch=(col in stretch), width=width, minwidth=scaled(40)) for tag, options in (tags or {}).items(): try: self.tree.tag_configure(tag, **options) except Exception as _exc: crashlog.note(_exc, "gui.widgets.sortable_tree") + # i18n KEY, translated at display: the window survives a language change + # (convention 25), so a text resolved once at build time would stay in the + # old language on a table that happened to be empty at the time. + self._empty_text = empty_text + self._empty_note = ttk.Label(self.frame, text="", style="Hint.TLabel", + anchor="center") if empty_text else None self.refresh_headers() self._ensure_slots(self._height + BUFFER_ROWS) @@ -289,6 +313,51 @@ def set_model(self, items, render, key_of, tag_of=None): self._index = None # invalidated; rebuilt on demand self.offset = min(self.offset, self.max_offset()) self.repaint() + self._show_empty_note(not self.items) + + def anchor_for(self, col): + """Where a column's cells sit: numbers right, fixed-width text centred.""" + if col in self._numeric: + return "e" + return "center" if col in self._centered else "w" + + def set_empty_text(self, key): + """Say WHY the table would be empty - the page knows, this widget cannot. + + A table with no rows because nothing has been captured yet and one with + no rows because a search matched nothing are the same picture and + opposite messages - and getting it wrong is worse than saying nothing, + because "nothing matches what you are looking for" is a lie to someone + who has typed nothing. + """ + self._empty_text = key + self._show_empty_note(not self.items) + + def _show_empty_note(self, empty): + """A table with no rows says why, instead of being a blank rectangle. + + The count underneath has always read "0 of N", but the reader is looking + at the table, and a blank one is equally consistent with "nothing + matched" and "something is broken". Since the search box learned column + qualifiers, the empty view is common rather than rare: a half-typed + ``port:44`` on the way to ``port:443`` correctly matches nothing. + + Placed rather than gridded, so the tree keeps its geometry and the note + cannot change the row area's size. Removed again the moment rows return, + because a label left over an empty spot still swallows clicks. + """ + note = getattr(self, "_empty_note", None) + if note is None: + return + from ...i18n import T + try: + if empty and self._empty_text: + note.config(text=T(self._empty_text)) + note.place(relx=0.5, rely=0.4, anchor="center") + else: + note.place_forget() + except Exception as _exc: + crashlog.note(_exc, "gui.widgets.sortable_tree") @property def rows(self): diff --git a/beantester/matchers.py b/beantester/matchers.py index d6bc730..8220130 100644 --- a/beantester/matchers.py +++ b/beantester/matchers.py @@ -160,6 +160,18 @@ def is_empty(self): """True for an empty field - "match everything".""" return not self.terms + @property + def selects_nothing_in_particular(self): + """True when the expression names no thing to hit - only things to spare. + + ``!chrome.exe`` is not empty, so every "is a target set?" check written as + a truth test reads it as narrow. It is the opposite: ``matches()`` skips + the positive branch entirely, so the expression covers everything except + the exclusions. Callers that care about BLAST RADIUS (see + ``settings.unbounded_impairment``) must treat it as unscoped. + """ + return not self._positives + def __bool__(self): """A matcher is falsy when empty, so callers can write ``if matcher:``.""" return not self.is_empty diff --git a/beantester/scenario.py b/beantester/scenario.py index 4dd7751..045b5ee 100644 --- a/beantester/scenario.py +++ b/beantester/scenario.py @@ -6,6 +6,7 @@ scenario with **zero steps**, which then ran a session that did nothing while the UI happily reported "scenario loaded". """ +import difflib import json from .i18n import translate @@ -50,12 +51,22 @@ def _validate_step(index, step): raise _err("errors.scenario_step_settings", step=where) unknown = [k for k in settings if k not in DEFAULT_SETTINGS] if unknown: + # The same help the config loader has given since it learned to + # (settings.load_config_file): one misspelling gets the correction it + # was probably reaching for. It is the same class of mistake made in + # the same kind of file, and answering it two different ways was an + # accident of which loader was written first. + close = difflib.get_close_matches(unknown[0], DEFAULT_SETTINGS, n=1) + if len(unknown) == 1 and close: + raise _err("errors.scenario_unknown_setting_hint", step=where, + field=unknown[0], suggestion=close[0]) raise _err("errors.scenario_unknown_setting", step=where, field=", ".join(sorted(unknown))) action = step.get("action") if action is not None and str(action) not in ACTIONS: - raise _err("errors.scenario_unknown_action", step=where, action=action) + raise _err("errors.scenario_unknown_action", step=where, action=action, + allowed=", ".join(ACTIONS)) if settings is None and action is None: raise _err("errors.scenario_step_empty", step=where) diff --git a/beantester/settings.py b/beantester/settings.py index 6b1b6b6..3d8ee24 100644 --- a/beantester/settings.py +++ b/beantester/settings.py @@ -17,6 +17,7 @@ from .matchers import KIND_PROCESS, parse_matcher, port_expression from .processes import TARGET_FIELD from .targeting import ports_shared_with_others +from .utils import number_string from .validators import parse_number, parse_seed DEFAULT_SETTINGS = dict( @@ -79,6 +80,84 @@ def build_matchers(s): return out +def armed_global_impairments(s): + """Keys of impairments that are ON and damage every captured packet. + + Registry-derived (``fields.GLOBALLY_IMPAIRING_KEYS``), so an impairment added + later is covered by declaring it once, in the table where it is defined. + """ + return tuple(key for key in F.GLOBALLY_IMPAIRING_KEYS + if F.is_active(FIELDS[key], s.get(key, DEFAULT_SETTINGS[key]))) + + +def targeting_is_set(s): + """True when a targeting field names at least one thing to hit. + + Not a truth test on the fields: an expression made of nothing but exclusions + (``!chrome.exe``) is non-empty and still covers the whole machine minus one + application, so it does not bound anything (see + ``Matcher.selects_nothing_in_particular``). A malformed expression is treated + as no scope at all - it is about to be rejected by validation anyway, and the + safe reading of "I cannot tell what this narrows to" is "it narrows nothing". + """ + expressions = {key: (kind, field, bounds) + for key, kind, field, bounds in MATCH_FIELDS} + for key in F.NARROWING_KEYS: + value = s.get(key, DEFAULT_SETTINGS[key]) + if key not in expressions: + # a narrowing field that is not an expression (none today): its plain + # on/off reading is the best answer available, and it is still an answer + if F.is_active(FIELDS[key], value): + return True + continue + text = setting_expression(key, value) + if not text: + continue + try: + matcher = parse_matcher(text, *expressions[key]) + except ValueError: + continue + if not matcher.selects_nothing_in_particular: + return True + return False + + +def unbounded_impairment(s): + """True when a run would damage all traffic, with nothing to bound it. + + The condition behind the start-time warning: something is armed that hits + every packet, no targeting field says WHERE, and no duration says WHEN it + ends. Blocking is deliberately not enough to clear it - a block bounds its + own damage and nothing else, so ``--loss 50 --block-ip 10.0.0.1`` is still a + machine-wide loss run. + + Total by construction: it reads through ``DEFAULT_SETTINGS`` for missing keys + (an older config file), and ``fields.is_active`` never raises on a value that + has not been validated yet. A warning that could abort a start would be worse + than the mode it warns about. + """ + if not armed_global_impairments(s): + return False + if targeting_is_set(s): + return False + try: + duration = float(s.get("duration", 0) or 0) + except (TypeError, ValueError): + duration = 0.0 + return duration <= 0 + + +def warn_if_unbounded(s, log): + """Say the start-time warning through ``log``, if the run has earned it. + + Both interfaces call this rather than each testing the condition and picking + their own words: one sentence, one condition, so the window and the command + line cannot drift into telling the user different things about the same run. + """ + if unbounded_impairment(s): + log(T("warn.global_impairment")) + + def validate_ranges(s, lang=None): """Check every numeric setting against the bounds declared in the registry. @@ -372,6 +451,25 @@ def apply_settings(engine, s, log=lambda *_: None): apply_targeting(engine, str(g("target")).strip(), log) +def _expected_shape(key, lang=None): + """Plain-language description of what a numeric setting accepts. + + Read from the registry, so a field whose bounds change says the new ones + without anybody remembering this message exists. + + Under ``fields.`` and not ``errors.`` on purpose: these are sentence + FRAGMENTS, pasted into a message that supplies the capital letter and the + full stop. Keeping them out of the ``errors.`` namespace is what lets + ``test_every_error_reads_like_a_sentence`` run without an exception list. + """ + field = FIELDS.get(key) + bounds = field.bounds if field is not None else None + if not bounds: + return translate("fields.expects_number", lang) + return translate("fields.expects_number_range", lang, + min=number_string(bounds[0]), max=number_string(bounds[1])) + + def _coerce_setting(key, value): """Coerce a config-file value to the type of its default. @@ -393,8 +491,13 @@ def _coerce_setting(key, value): raise ValueError return float(value) except (TypeError, ValueError): + # Say what the setting DOES take, not just that this is not it. The + # registry already knows - the form has been telling people "must be + # between 0 and 100" for as long as it has existed, while the config + # loader said only "invalid" for the very same value. raise ValueError(translate("errors.bad_config_value", None, - field=key, value=repr(value))) + field=key, value=repr(value), + expected=_expected_shape(key))) return str(value) diff --git a/lang/en.json b/lang/en.json index 09fb6f9..a270cfe 100644 --- a/lang/en.json +++ b/lang/en.json @@ -112,8 +112,7 @@ "dialogs.save_repro": "Save repro report", "dialogs.scenario_not_loaded": "Scenario not loaded", "dialogs.start_failed": "Failed to start", - "dialogs.values_numbers": "Values must be numbers.", - "errors.bad_config_value": "Invalid value for '{field}' in the config file: {value}", + "errors.bad_config_value": "'{field}' in the config file needs {expected}, not {value}.", "errors.bad_filter_bounds": "Field '{field}': '{term}' is out of the allowed range ({min}-{max}).", "errors.bad_filter_compare": "Field '{field}': comparison '{term}' needs a number after the operator.", "errors.bad_filter_compare_name": "Field '{field}': comparison operators (>, <, >=, <=) work only with a PID (a number), not a process name - '{term}'.", @@ -123,12 +122,12 @@ "errors.bad_filter_range": "Field '{field}': range '{term}' is reversed - the start must not be greater than the end.", "errors.bad_filter_regex": "Field '{field}': '{term}' is not a valid regular expression (a comma inside a pattern must be escaped: \\,).", "errors.bad_filter_term": "Empty entry in field '{field}': '{term}'.", - "errors.bad_schedule_step": "bad schedule step: '{part}' (use dur:down:up)", + "errors.bad_schedule_step": "Schedule step '{part}' is not in the form dur:down:up.", "errors.config_unknown_setting": "Unknown setting in the config file: {field}. Remove it or correct the spelling.", "errors.config_unknown_setting_hint": "Unknown setting in the config file: {field}. Did you mean '{suggestion}'?", "errors.field_number": "The '{name}' field must be a number.", "errors.field_range": "Field '{name}' must be between {min} and {max}.", - "errors.scenario_bad_json": "Not a valid JSON file: {error}", + "errors.scenario_bad_json": "Not a valid JSON file: {error}.", "errors.scenario_duration_without_action": "Scenario, step {step}: \"duration\" only applies to an \"action\" - this step has none.", "errors.scenario_empty": "Scenario: the step list is empty.", "errors.scenario_no_steps": "Scenario: the \"steps\" list is missing.", @@ -139,10 +138,11 @@ "errors.scenario_step_settings": "Scenario, step {step}: \"settings\" must be an object.", "errors.scenario_step_type": "Scenario, step {step}: expected an object.", "errors.scenario_too_many": "Scenario: too many steps (limit {limit}).", - "errors.scenario_unknown_action": "Scenario, step {step}: unknown action: {action}.", + "errors.scenario_unknown_action": "Scenario, step {step}: unknown action: {action}. Allowed: {allowed}.", "errors.scenario_unknown_file_key": "Scenario: unknown key: {field}. Only \"steps\" and \"loop\" are allowed.", "errors.scenario_unknown_key": "Scenario, step {step}: unknown key: {field}. A step may hold \"at\", \"settings\", \"action\" and \"duration\".", "errors.scenario_unknown_setting": "Scenario, step {step}: unknown setting: {field}.", + "errors.scenario_unknown_setting_hint": "Scenario, step {step}: unknown setting: {field}. Did you mean '{suggestion}'?", "errors.seed_integer": "The 'Seed' field must be an integer.", "events.bug_marker": "tester marker: bug occurred here", "events.col_desc": "description", @@ -170,6 +170,8 @@ "fields.duplication": "Duplication", "fields.duration": "Run time:", "fields.duration_hint": "0 = until stopped", + "fields.expects_number": "a number", + "fields.expects_number_range": "a number between {min} and {max}", "fields.filter": "Filter", "fields.flap_down_pct": "Downtime percent", "fields.ip": "IP", @@ -255,7 +257,7 @@ "log.duration_reached": "Time limit reached ({v} s) - session stopped.", "log.engine_fault": "Engine fault: {e} - the session was stopped, your network is back to normal.", "log.error": "Error", - "log.filter_skipped": "Invalid filter expression - targeting disabled", + "log.filter_skipped": "This expression could not be read, so it was switched off for this session", "log.layout_reset": "Window layout reset.", "log.loaded_profile": "Loaded profile", "log.loop": "loop", @@ -415,6 +417,10 @@ "summary.syn": "{v}% SYN dropped", "summary.target": "process only '{v}'", "summary.up": "upload <= {v} KB/s", + "tables.no_conns_match": "No connection matches what you are looking for. The search works, nothing here fits it.", + "tables.no_conns_yet": "No connections captured yet. They appear here as soon as traffic goes through.", + "tables.no_events_match": "No event matches what you are looking for. The search works, nothing here fits it.", + "tables.no_events_yet": "No events yet. They appear here as the session runs.", "tips.about": "Version, author, licence and the third-party components this program ships with.", "tips.apply": "Applies setting changes WITHOUT stopping (live). The traffic filter can only be changed by restarting.", "tips.avg_rate": "Average throughput since start = total MB / duration.", @@ -526,6 +532,7 @@ "tips.syn": "Percent of dropped TCP SYN packets, i.e. those starting a connection. Simulates a connection that won't establish - tests retry logic.", "tips.target_process": "Limit the effect to chosen apps: process names, PIDs, ranges, wildcards or re: patterns (comma-separated). Prefix ! to exclude, empty = all traffic. See '?' for the full syntax. Requires psutil.", "tips.up_limit": "Max throughput of OUTGOING traffic (upload) in KB/s. 0 = unlimited.", + "warn.global_impairment": "This run has no target and no time limit, so it affects every connection on this machine. Set a target or a time limit to narrow it.", "warn.not_admin": "Not running as administrator - the WinDivert driver will not load and START will fail. Restart the app as administrator.", "warn.queue_overflow": "The latency queue is full: the tool is dropping packets you did not ask to lose. The loss you are measuring is partly ours. Lower the latency or the traffic rate.", "warn.send_failed": "The tool cannot put packets back on the wire: they are being lost here, not by the network. Check that the connection is up and that the driver is still loaded.", diff --git a/lang/pl.json b/lang/pl.json index 369cbd8..cd49083 100644 --- a/lang/pl.json +++ b/lang/pl.json @@ -112,37 +112,37 @@ "dialogs.save_repro": "Zapisz raport reprodukcji", "dialogs.scenario_not_loaded": "Nie wczytano scenariusza", "dialogs.start_failed": "Nie udało się uruchomić", - "dialogs.values_numbers": "Wartości muszą być liczbami.", - "errors.bad_config_value": "Nieprawidłowa wartość pola '{field}' w pliku konfiguracji: {value}", + "errors.bad_config_value": "Pole '{field}' w pliku konfiguracji wymaga {expected}, a nie {value}.", "errors.bad_filter_bounds": "Pole '{field}': '{term}' jest poza dozwolonym zakresem ({min}-{max}).", "errors.bad_filter_compare": "Pole '{field}': porównanie '{term}' wymaga liczby po operatorze.", "errors.bad_filter_compare_name": "Pole '{field}': operatory porównania (>, <, >=, <=) działają tylko z PID-em (liczbą), nie z nazwą procesu - '{term}'.", - "errors.bad_filter_ip": "Pole '{field}': '{term}' to nie poprawny adres IP, zakres, CIDR, wildcard ani wzorzec re:.", + "errors.bad_filter_ip": "Pole '{field}': '{term}' to nie jest poprawny adres IP, zakres, CIDR, wildcard ani wzorzec re:.", "errors.bad_filter_ip_family": "Pole '{field}': zakres '{term}' miesza IPv4 i IPv6 - oba końce muszą być z tej samej rodziny adresów.", - "errors.bad_filter_number": "Pole '{field}': '{term}' to nie liczba, zakres, wildcard ani wzorzec re:.", + "errors.bad_filter_number": "Pole '{field}': '{term}' to nie jest liczba, zakres, wildcard ani wzorzec re:.", "errors.bad_filter_range": "Pole '{field}': zakres '{term}' jest odwrócony - początek nie może być większy niż koniec.", - "errors.bad_filter_regex": "Pole '{field}': '{term}' to niepoprawne wyrażenie regularne (przecinek we wzorcu trzeba poprzedzić ukośnikiem: \\,).", + "errors.bad_filter_regex": "Pole '{field}': '{term}' to nie jest poprawne wyrażenie regularne (przecinek we wzorcu trzeba poprzedzić ukośnikiem: \\,).", "errors.bad_filter_term": "Puste wyrażenie w polu '{field}': '{term}'.", - "errors.bad_schedule_step": "zły krok harmonogramu: '{part}' (użyj dur:down:up)", + "errors.bad_schedule_step": "Krok harmonogramu '{part}' nie ma postaci dur:down:up.", "errors.config_unknown_setting": "Nieznane ustawienie w pliku konfiguracji: {field}. Usuń je albo popraw pisownię.", "errors.config_unknown_setting_hint": "Nieznane ustawienie w pliku konfiguracji: {field}. Czy chodziło o '{suggestion}'?", "errors.field_number": "Pole '{name}' musi być liczbą.", "errors.field_range": "Pole '{name}' musi mieścić się w zakresie od {min} do {max}.", - "errors.scenario_bad_json": "To nie jest poprawny plik JSON: {error}", - "errors.scenario_duration_without_action": "Scenariusz, krok {step}: „duration” dotyczy tylko „action”, a ten krok jej nie ma.", + "errors.scenario_bad_json": "To nie jest poprawny plik JSON: {error}.", + "errors.scenario_duration_without_action": "Scenariusz, krok {step}: \"duration\" dotyczy tylko \"action\", a ten krok jej nie ma.", "errors.scenario_empty": "Scenariusz: lista kroków jest pusta.", "errors.scenario_no_steps": "Scenariusz: brakuje listy \"steps\".", "errors.scenario_not_a_scenario": "Ten plik nie jest scenariuszem (oczekiwano listy kroków albo obiektu z polem \"steps\").", "errors.scenario_step_at": "Scenariusz, krok {step}: \"at\" musi być liczbą sekund (>= 0).", - "errors.scenario_step_duration": "Scenariusz, krok {step}: „duration” musi być liczbą sekund (>= 0).", + "errors.scenario_step_duration": "Scenariusz, krok {step}: \"duration\" musi być liczbą sekund (>= 0).", "errors.scenario_step_empty": "Scenariusz, krok {step}: nic nie robi (brak \"settings\" i \"action\").", "errors.scenario_step_settings": "Scenariusz, krok {step}: \"settings\" musi być obiektem.", "errors.scenario_step_type": "Scenariusz, krok {step}: oczekiwano obiektu.", "errors.scenario_too_many": "Scenariusz: za dużo kroków (limit {limit}).", - "errors.scenario_unknown_action": "Scenariusz, krok {step}: nieznana akcja: {action}.", - "errors.scenario_unknown_file_key": "Scenariusz: nieznany klucz: {field}. Dozwolone są tylko „steps” i „loop”.", - "errors.scenario_unknown_key": "Scenariusz, krok {step}: nieznany klucz: {field}. Krok może zawierać „at”, „settings”, „action” i „duration”.", + "errors.scenario_unknown_action": "Scenariusz, krok {step}: nieznana akcja: {action}. Dozwolone: {allowed}.", + "errors.scenario_unknown_file_key": "Scenariusz: nieznany klucz: {field}. Dozwolone są tylko \"steps\" i \"loop\".", + "errors.scenario_unknown_key": "Scenariusz, krok {step}: nieznany klucz: {field}. Krok może zawierać \"at\", \"settings\", \"action\" i \"duration\".", "errors.scenario_unknown_setting": "Scenariusz, krok {step}: nieznane ustawienie: {field}.", + "errors.scenario_unknown_setting_hint": "Scenariusz, krok {step}: nieznane ustawienie: {field}. Czy chodziło o '{suggestion}'?", "errors.seed_integer": "Pole 'Seed' musi być liczbą całkowitą.", "events.bug_marker": "znacznik testera: tu wystąpił błąd", "events.col_desc": "opis", @@ -170,6 +170,8 @@ "fields.duplication": "Duplikacja", "fields.duration": "Czas trwania:", "fields.duration_hint": "0 = do zatrzymania", + "fields.expects_number": "liczby", + "fields.expects_number_range": "liczby od {min} do {max}", "fields.filter": "Filtr", "fields.flap_down_pct": "Procent przerwy w łączu", "fields.ip": "IP", @@ -255,7 +257,7 @@ "log.duration_reached": "Osiągnięto limit czasu ({v} s) - sesja zatrzymana.", "log.engine_fault": "Awaria silnika: {e} - sesja została zatrzymana, sieć działa normalnie.", "log.error": "Błąd", - "log.filter_skipped": "Niepoprawne wyrażenie filtra - celowanie wyłączone", + "log.filter_skipped": "Nie udało się odczytać tego wyrażenia, więc zostało wyłączone na tę sesję", "log.layout_reset": "Układ okna zresetowany.", "log.loaded_profile": "Wczytano profil", "log.loop": "pętla", @@ -415,6 +417,10 @@ "summary.syn": "{v}% gubionych SYN", "summary.target": "tylko proces '{v}'", "summary.up": "wysyłanie <= {v} KB/s", + "tables.no_conns_match": "Żadne połączenie nie pasuje do tego, czego szukasz. Wyszukiwarka działa, po prostu nic tu nie pasuje.", + "tables.no_conns_yet": "Nie przechwycono jeszcze żadnego połączenia. Pojawią się tutaj, gdy tylko poleci ruch.", + "tables.no_events_match": "Żadne zdarzenie nie pasuje do tego, czego szukasz. Wyszukiwarka działa, po prostu nic tu nie pasuje.", + "tables.no_events_yet": "Nie ma jeszcze żadnych zdarzeń. Pojawią się w trakcie sesji.", "tips.about": "Wersja, autor, licencja i składniki firm trzecich, które program dostarcza.", "tips.apply": "Nanosi zmiany ustawień BEZ zatrzymywania (działa w trakcie). Filtr ruchu zmienisz tylko po restarcie.", "tips.avg_rate": "Średnia przepustowość od startu = razem MB / czas trwania.", @@ -526,6 +532,7 @@ "tips.syn": "Procent gubionych pakietów TCP SYN, czyli tych rozpoczynających połączenie. Symuluje sytuację, gdy połączenie nie chce się nawiązać - test ponawiania prób.", "tips.target_process": "Ogranicz działanie do wybranych aplikacji: nazwy procesów, PID-y, zakresy, wildcardy lub wzorce re: (po przecinku). Poprzedź !, aby wykluczyć, puste = cały ruch. Pełna składnia pod „?”. Wymaga psutil.", "tips.up_limit": "Maks. przepustowość ruchu WYCHODZĄCEGO (wysyłanie) w KB/s. 0 = bez limitu.", + "warn.global_impairment": "Ten przebieg nie ma celu ani limitu czasu, więc dotyczy każdego połączenia na tym komputerze. Zawęzisz go, ustawiając cel albo limit czasu.", "warn.not_admin": "Uruchomiono bez uprawnień administratora - sterownik WinDivert się nie załaduje i START się nie powiedzie. Uruchom aplikację ponownie jako administrator.", "warn.queue_overflow": "Kolejka opóźnienia jest pełna: narzędzie gubi pakiety, o które nie prosiłeś. Strata, którą mierzysz, jest częściowo nasza. Zmniejsz opóźnienie lub tempo ruchu.", "warn.send_failed": "Narzędzie nie potrafi odesłać pakietów do sieci: giną tutaj, a nie w sieci. Sprawdź, czy połączenie działa i czy sterownik jest nadal załadowany.", diff --git a/tests/fake_tk.py b/tests/fake_tk.py index 23d46d2..b99db7c 100644 --- a/tests/fake_tk.py +++ b/tests/fake_tk.py @@ -124,6 +124,9 @@ def grid_forget(self): def place(self, **kw): self.kw["place"] = dict(kw) + def place_forget(self): + self.kw.pop("place", None) + def columnconfigure(self, *a, **kw): pass diff --git a/tests/test_cli_docs.py b/tests/test_cli_docs.py index 03ca1d3..a039c1e 100644 --- a/tests/test_cli_docs.py +++ b/tests/test_cli_docs.py @@ -56,3 +56,105 @@ def test_no_stale_app_flags_in_readmes(): stale = sorted(_documented_flags(readme) - real - IGNORE) check(f"{readme} lists no flag the parser lacks", not stale, f"(stale: {stale})") + + +def test_no_semicolons_in_the_help_a_user_reads(): + """Conventions 1b and 33: people do not write semicolons in ordinary prose. + + ``lang/*.json`` has been guarded since the day 21 tooltips were found to have + drifted (``test_i18n.py::test_no_semicolons_in_ui_text``). The CLI's help is + the same kind of prose read by the same kind of person, and nothing looked at + it - six had accumulated by the time anybody did. + + Read off the PARSER, not off the source: a help string added anywhere, by any + future refactor, is covered without this test knowing where it was written. + ``description`` and ``epilog`` come along because they are prose too, and the + epilog carries the exit-code table users actually script against. + + No exception list. A semicolon is syntax inside a filter expression, a shell + line or JSON - none of which live in ``help=`` today, and the convention says + the exception is for code, not for prose ABOUT code. If a help string ever + genuinely needs one, this test is where that argument gets made. + """ + import beantester.cli as cli_module + parser = cli_module.build_arg_parser() + helps = [a for a in parser._actions if a.help] + # The canary from test_repo_conventions: a scan that reads nothing satisfies + # every rule ever written and looks like a guard that works. + check("CLI help: the scan actually read the parser", len(helps) >= 30, + f"({len(helps)} help strings)") + offenders = sorted("/".join(a.option_strings) or a.dest + for a in helps if ";" in a.help) + check(f"CLI help: no semicolons in help= ({len(helps)} read)", + not offenders, f"({offenders})") + for name in ("description", "epilog"): + text = getattr(parser, name, None) or "" + check(f"CLI help: no semicolons in the parser {name}", + ";" not in text) + + +def test_help_opens_with_examples_and_not_with_a_wall_of_usage(): + """clig.dev: show the common cases first, then the reference. + + MEASURED before this: 24 lines of generated usage listing about fifty flags, + then one sentence, then the flags again in full. The first thing anyone wants + from a tool that size is a line to copy, and it was below the fold - as was + the error message on a typo, which argparse prints under the same block. + + Asserts ORDER, not wording: the examples have to arrive before the flag list, + or this is decoration. + """ + import beantester.cli as cli_module + text = cli_module.build_arg_parser().format_help() + check("--help: has an examples section", "Examples:" in text) + first_block = text.split("\n\n")[0].splitlines() + check("--help: the usage block is one line, not fifty flags", + first_block[0].startswith("usage:") and len(first_block) == 1, + f"({len(first_block)} lines: {first_block[0]!r})") + check("--help: examples come before the flag list", + text.index("Examples:") < text.index("--simulate "), + "(the reference is above the worked cases)") + examples = text.split("Examples:")[1].split("options:")[0] + for flag in ("--simulate", "--target", "--duration", "--format json"): + check(f"--help: an example actually uses {flag}", flag in examples) + + +def test_the_examples_name_whatever_this_build_is_called(): + """Most people get an .exe; some run the .py. The examples must follow. + + ``%(prog)s`` rather than a literal, so a frozen build prints + ``BeanNetworkTester.exe ...`` and a source checkout prints + ``bean_network_tester.py ...``. A hardcoded name would be a copy-paste line + that does not work for the 90% who have the exe, which is the one thing an + example exists to be. + + The prog is swapped by patching ``cli.program_name``, NOT by reloading the + module. Reloading builds a fresh ``_Terminated`` class, so the ``except`` + inside the already-imported ``run_cli`` stopped matching the exception other + tests raise, and ``test_exit_code_interrupted_and_terminated`` started + returning 1 instead of 143 - measured, a test corrupting the suite around it. + """ + from beantester import appinfo + import beantester.cli as cli_module + + original = cli_module.program_name + try: + for name in (appinfo.LAUNCHER, appinfo.EXE_NAME): + other = (appinfo.EXE_NAME if name == appinfo.LAUNCHER + else appinfo.LAUNCHER) + cli_module.program_name = lambda _n=name: _n + text = cli_module.build_arg_parser().format_help() + examples = text.split("Examples:")[1].split("options:")[0] + lines = [l.strip() for l in examples.splitlines() + if l.strip().startswith((name, other))] + check(f"--help: the examples are runnable lines as {name}", + len(lines) >= 4, f"({len(lines)} found)") + # EVERY line, not just one of them. The first version asked whether the + # right name appeared ANYWHERE, so hardcoding one example still passed - + # measured, that mutant survived. + wrong = [l for l in lines if not l.startswith(name)] + check(f"--help: no example names {other} when run as {name}", + not wrong, f"({wrong})") + check("--help: no unexpanded prog token", "%(prog)s" not in text) + finally: + cli_module.program_name = original diff --git a/tests/test_cli_runtime.py b/tests/test_cli_runtime.py index c34f3cd..b78dbe2 100644 --- a/tests/test_cli_runtime.py +++ b/tests/test_cli_runtime.py @@ -875,3 +875,158 @@ def test_the_saved_config_round_trips_through_the_cli(tmp_path): check("--save-config: stores the settings", saved["loss"] == 3 and saved["duration"] == 7, f"({saved})") check("--save-config: the file exists", os.path.exists(path)) + + +# --- warning: a run that damages everything, with nothing to end it -------- # + + +class _OneTickEngine(_TargetedEngine): + """Enough engine to finish a run that has no ``--duration``. + + Without a deadline the report loop ends only when the engine stops itself + (``is_running()`` going false), which is exactly the shape of run this + warning is about - so a fake that never stops would hang the suite instead + of testing it. ``started`` records whether the capture was ever opened. + """ + + stop_reason = "user" + + def __init__(self, **kw): + super().__init__(**kw) + self.ticks = 0 + self.started = False + + def start(self, *_a, **_k): + self.started = True + + def is_running(self): + self.ticks += 1 + return self.ticks < 2 + + +def _real_run(monkeypatch, argv, engine=None): + """One NON-simulate CLI run on a fake engine, admin gate forced open. + + The warning is deliberately silent in ``--simulate`` (there is no real + traffic to damage), so proving it needs a real-mode run - which on a plain + Windows shell has neither an elevated token nor WinDivert. Same seam and the + same reason as ``_targeted_run`` above. + """ + monkeypatch.setattr(cli_module.winenv, "is_admin", lambda: True) + engine = engine or _OneTickEngine(seen=10) + clock = FakeClock() + out, err = io.StringIO(), io.StringIO() + code = run_cli(argv, sleep=clock.sleep, clock=clock, engine=engine, + out=out, err=err) + return code, out.getvalue(), err.getvalue(), engine + + +def _warned(err): + from beantester.i18n import translate + return translate("warn.global_impairment", "en") in err + + +def test_a_run_that_impairs_everything_forever_says_so_before_it_starts(monkeypatch): + """The accident this whole audit came from: ``--lat 5`` and nothing else. + + A mistyped flag opened a real capture with no target and no deadline and + impaired 11 844 packets of a live machine over 202 s before anybody noticed. + Nothing warned, because nothing looked at the SHAPE of the run - only at + whether each value was in range. The message goes to stderr, so a pipeline + reading stdout is untouched (convention 18). + """ + _, out, err, engine = _real_run(monkeypatch, ["--loss", "50"]) + check("warning: an unscoped, endless impairment is announced", _warned(err)) + check("warning: it goes to stderr, not to the data channel", not _warned(out)) + check("warning: the run still happens (a warning, not a refusal)", + engine.started) + + +def test_the_warning_names_lan_mode_which_reads_like_a_scope(monkeypatch): + """MEASURED in core.decide() step 2b: LAN mode DROPS every public address. + + It is the one flag whose name argues the other way ("only the local + network"), and on its own, with the whole rest of the form at zero, it cuts + the machine's internet. It was found by walking the gates in decide() rather + than by reading the field names, which is the only reason it is here. + """ + _, _, err, _ = _real_run(monkeypatch, ["--lan-mode"]) + check("warning: LAN mode alone is a machine-wide impairment", _warned(err)) + + +def test_a_bounded_run_is_not_warned_about(monkeypatch): + """Three ways to bound a run, and each one has to buy silence. + + A warning that also fires on careful runs is a warning people learn to skip, + which would cost exactly the case above. + """ + for argv, why in ( + (["--loss", "50", "--duration", "5"], "a deadline"), + (["--loss", "50", "--target", "probe.exe"], "a process target"), + (["--loss", "50", "--dst-ip", "10.0.0.1"], "a destination"), + (["--block-ip", "10.0.0.1"], "blocking, which bounds its own damage"), + (["--simulate", "--loss", "50"], "--simulate, where nothing is real"), + ): + _, _, err, _ = _real_run(monkeypatch, argv) + check(f"warning: silent when the run has {why}", not _warned(err), + f"({argv})") + + +def test_dry_run_previews_the_shape_and_not_only_the_values(): + """"Configuration is valid" is about each value. This is about the SHAPE. + + --dry-run is the cheapest place to learn that a config would impair the whole + machine with nothing to end it: it opens no driver and passes no traffic, and + it needs no elevated token, so a pipeline can ask the question for free. + """ + code, _, err = cli(["--dry-run", "--loss", "50"]) + check("--dry-run: still exits OK", code == exitcodes.OK, f"(code={code})") + check("--dry-run: previews an unbounded config", _warned(err)) + _, _, err = cli(["--dry-run", "--loss", "50", "--duration", "5"]) + check("--dry-run: silent on a bounded config", not _warned(err)) + + +def test_blocking_bounds_only_its_own_damage(monkeypatch): + """A block is not a target, and this is the pair that proves the difference. + + ``--block-ip`` alone is bounded: it drops traffic to the address it names and + nothing else, so warning about it would be the false alarm that teaches + people to ignore the real one. Add ``--loss 50`` and the run is machine-wide + again - the block scopes the block, not the loss. + + Written because the mutation "blocking counts as a bound for every other + impairment" SURVIVED the first version of these guards: the silent case alone + reads identically whether blocking is IMPAIRS_MATCHED or a narrowing field. + """ + _, _, err, _ = _real_run(monkeypatch, ["--block-ip", "10.0.0.1"]) + check("warning: a block on its own is already bounded", not _warned(err)) + _, _, err, _ = _real_run(monkeypatch, ["--loss", "50", "--block-ip", "10.0.0.1"]) + check("warning: a block does not bound the loss beside it", _warned(err)) + + +def test_an_exclusion_only_target_is_not_a_bound(monkeypatch): + """``!chrome.exe`` is non-empty and narrows nothing. + + Every "is a target set?" test written as a truth check reads it as scoped; + it means "the whole machine except Chrome". See + ``Matcher.selects_nothing_in_particular``. + """ + _, _, err, _ = _real_run(monkeypatch, ["--loss", "50", "--target", "!chrome.exe"]) + check("warning: an expression of pure exclusions bounds nothing", _warned(err)) + + +def test_a_broken_scenario_never_opens_the_capture(monkeypatch, tmp_path): + """MEASURED before the fix: the run printed "Start.", impaired traffic and + only THEN said the file was broken. + + The file is readable without touching the driver, and ``--dry-run`` already + validated it up front - the real path did not. Same exit code as before, so + no pipeline changes meaning. + """ + path = tmp_path / "broken.json" + path.write_text('{"steps": [{"at": 0, "whatever": 1}]}', encoding="utf-8") + code, _, err, engine = _real_run(monkeypatch, ["--loss", "5", "--scenario", str(path)]) + check("scenario: a broken file still exits SCENARIO", + code == exitcodes.SCENARIO, f"(code={code})") + check("scenario: a broken file never opens the capture", not engine.started) + check("scenario: it says which file", "broken.json" in err, f"({err!r})") diff --git a/tests/test_conns_columns.py b/tests/test_conns_columns.py index 10fadc8..d05d5e7 100644 --- a/tests/test_conns_columns.py +++ b/tests/test_conns_columns.py @@ -113,3 +113,150 @@ def test_two_portless_rows_to_one_address_keep_separate_identities(): index = page.table._ensure_index() assert len(index) == 2, ("the key index collapsed a row", index) ''') + + +def test_numeric_columns_are_right_aligned_and_the_registry_is_honest(): + """Numbers line up by order of magnitude, which is why a column of them is + worth reading down at all. Everything used to be anchored west, in one loop + over the columns, so 9 and 1000000 started at the same pixel. + + Read from the page's NUMERIC registry rather than from a list retyped here: + a test that repeats the answer cannot catch the registry drifting from the + columns it describes, which is the failure this alignment had in the first + place. + """ + run_gui(''' + from beantester.gui.pages import conns + + page = app.pages["connections"] + tree = page.table.tree + + assert conns.NUMERIC <= set(conns.COLUMNS), \ + "NUMERIC names columns that do not exist: %r" % ( + conns.NUMERIC - set(conns.COLUMNS)) + + for col in conns.COLUMNS: + want = ("e" if col in conns.NUMERIC + else "center" if col in conns.CENTERED else "w") + got = tree.column(col, "anchor") + assert got == want, "%s anchored %r, wanted %r" % (col, got, want) + + # A right-aligned cell sits on its column's right edge and a left-aligned + # neighbour on its own left edge, so the two TOUCH - that is how the first + # build after this change rendered "67400 TCP" and "550 tak" as one value. + # ttk offers no per-cell padding, so nothing but alignment can fix it, and + # nothing but this check can stop it coming back with the next column. + cols = list(conns.COLUMNS) + touching = [(a, b) for a, b in zip(cols, cols[1:]) + if a in conns.NUMERIC and b not in conns.NUMERIC + and b not in conns.CENTERED] + assert not touching, "a number is left touching the text beside it: %r" % touching + + # the ones the audit named, spelled out so a shrinking registry is caught + for col in ("pid", "packets", "dropped", "down", "up", "kb", "avg"): + assert col in conns.NUMERIC, "%s stopped counting as a number" % col + for col in ("proc", "proto", "remote_ip", "scoped"): + assert col not in conns.NUMERIC, "%s is not a quantity" % col + ''') + + +def test_an_impaired_row_is_not_marked_by_colour_alone(): + """WCAG 1.4.1 level A: colour may not be the only visual carrier. + + The text column "impaired?" used to be the other one - then the column + chooser shipped (2026-08-02) and the user could hide it, which put the row + back on colour alone. The second signal therefore has to be something no + column setting can remove, which rules out a marker inside a cell: every + cell belongs to a column and every column can be hidden. + """ + run_gui(''' + from beantester.gui import theme + + style = theme.CONN_COLORS["impaired"] + assert "foreground" in style, "the colour itself went missing" + non_colour = set(style) - {"foreground", "background"} + assert non_colour, \ + "impaired rows carry colour and nothing else: %r" % style + + page = app.pages["connections"] + applied = page.table.tree.tag_styles.get("impaired", {}) + assert set(applied) - {"foreground", "background"}, \ + "the table did not apply the non-colour signal: %r" % applied + ''') + + +def test_the_table_is_reachable_and_readable_without_a_mouse(): + """WCAG 2.1.1, and the way testers actually work. + + Two tables carry a search box and neither had Ctrl+F; the context menu hung + on alone, so there was no keyboard path to it at all - while + services.msc and the console have had Shift+F10 since forever. + + Ctrl+F is bound on the ROOT rather than on the entry: a shortcut that only + works once the caret is already in the box is not a shortcut. It brings the + page forward first, so "find" answers the same wherever the user was. + """ + run_gui(''' + page = app.pages["connections"] + app.select_page("control") + + root_binds = set(root.bindings) + assert "" in root_binds, \ + "no Ctrl+F on the main window: %r" % sorted(root_binds) + + page._focus_search() + assert app.current_page() is page, \ + "Ctrl+F did not bring the Connections page forward" + assert root.focus_get() is page._search_entry, \ + "the caret did not land in the search box" + + # The menu key is spelled per platform and Tk RAISES on a spelling it + # does not know, so the test has to ask for the same one the code binds - + # asserting "" everywhere is the Windows-only assumption that broke + # the Linux render check in the first place. + import sys as _sys + tree_binds = set(page.table.tree.bindings) + menu_key = "" if _sys.platform == "win32" else "" + for seq in ("", menu_key): + assert seq in tree_binds, "no keyboard route to the menu: %s" % seq + + # ...and it refuses when there is no row to act on, exactly as the mouse + # route refuses on an empty table + page.table.select_keys([]) + assert page._popup_from_keyboard() == "break" + ''') + + +def test_an_empty_table_says_so_instead_of_showing_a_blank_rectangle(): + """The count underneath read "0 of N"; the table itself said nothing. + + A blank table is equally consistent with "nothing matched" and "something + broke". Since the search box learned column qualifiers, the empty view is + common rather than rare - a half-typed `port:44` on the way to `port:443` + correctly matches nothing. + """ + run_gui(''' + page = app.pages["connections"] + table = page.table + note = table._empty_note + assert note is not None, "the Connections table declares no empty note" + + # nothing typed: an empty table means no traffic YET, and saying + # "nothing matches your search" to someone who never searched is a lie + page.search_var.set("") + page._apply({"rows": [], "total": 0, "limit": 0, + "totals": {"down": 0, "up": 0, "total": 0}, "scope_active": False}) + assert "place" in note.kw, "nothing was shown on an empty table" + assert note.kw.get("text") == bnt.T("tables.no_conns_yet"), "wrong reason with no search typed: %r" % note.kw.get("text") + + # ...and with a query typed, the other reason + page.search_var.set("port:44") + page._apply({"rows": [], "total": 9, "limit": 0, + "totals": {"down": 0, "up": 0, "total": 0}, "scope_active": False}) + assert note.kw.get("text") == bnt.T("tables.no_conns_match"), "wrong reason with a search typed: %r" % note.kw.get("text") + + table.set_model([("k", ("a",) * 17)], render=lambda i: i[1], + key_of=lambda i: i[0]) + assert "place" not in note.kw, \ + "the note stayed over a table that has rows again" + ''') diff --git a/tests/test_gui_file_actions.py b/tests/test_gui_file_actions.py index bad72e1..968db2d 100644 --- a/tests/test_gui_file_actions.py +++ b/tests/test_gui_file_actions.py @@ -219,3 +219,26 @@ def test_saving_a_repro_writes_the_report_and_logs_the_command_to_replay_it(): assert any(report["cli_command"] in line for line in app._log_lines), ( "the replay command must reach the log: " + str(app._log_lines[-3:])) """) + + +def test_saving_a_profile_with_a_bad_value_names_the_field(): + """The precise message existed and was thrown away. + + ``_settings_from_widgets`` raises a translated ValueError naming the field + and its range; this handler caught it and showed "Values must be numbers", + which named neither - while ``_start``, in the same file, has always shown + ``str(e)`` for the same exception from the same call. + """ + run_gui(""" + import beantester as bnt + seen = [] + bnt.gui.dialogs.show_error = lambda root, title, body: seen.append(body) + bnt.gui.dialogs.ask_string = lambda *a, **k: "my profile" + + app.vars["loss"].set("not a number") + app.save_profile() + + assert seen, "no error was shown at all" + assert any("Utrata" in body or "Loss" in body for body in seen), \ + "the message does not name the field: %r" % seen + """) diff --git a/tests/test_gui_release_fixes.py b/tests/test_gui_release_fixes.py index 673de0e..5f79a80 100644 --- a/tests/test_gui_release_fixes.py +++ b/tests/test_gui_release_fixes.py @@ -164,6 +164,11 @@ def test_shortcut_buttons_advertise_their_key(): It used to assert on btn_start and btn_apply only, so dropping `shortcut=` from "Save file" or "Load file" left the suite green (verified by mutation, 2026-07-21). Every button that has a binding in `_bind_shortcuts` is listed here. + + And every control that binds its OWN shortcut, which is the half this test did + not have when Ctrl+F arrived (2026-08-03): the Connections search box binds it + on the root itself, so a control outside `_bind_shortcuts` can advertise + nothing and nobody would notice. """ run_gui(""" for attr, key in (("btn_start", "F5"), ("btn_apply", "Ctrl+Enter"), @@ -172,6 +177,11 @@ def test_shortcut_buttons_advertise_their_key(): tip = getattr(widget, "_bnt_tooltip", None) assert tip is not None, attr + " lost its tooltip" assert key in tip.text, attr + " does not advertise " + key + ": " + tip.text + + entry = app.pages["connections"]._search_entry + tip = getattr(entry, "_bnt_tooltip", None) + assert tip is not None, "the search box lost its tooltip" + assert "Ctrl+F" in tip.text, "the search box does not advertise Ctrl+F: " + tip.text """) diff --git a/tests/test_gui_state.py b/tests/test_gui_state.py index 08f2d91..591ebe2 100644 --- a/tests/test_gui_state.py +++ b/tests/test_gui_state.py @@ -597,3 +597,50 @@ def test_the_chosen_columns_are_remembered_and_restored(): assert app.pages["connections"].table.visible_columns() == [ "proc", "remote_ip", "packets"] """) + + +def test_the_gui_says_the_same_thing_before_an_unbounded_start(): + """The warning is not a CLI feature: in the GUI the same run is one click. + + Same sentence and the same condition as the CLI + (``settings.unbounded_impairment``), logged on the UI thread BEFORE the + driver load starts on the worker - so it is on screen while STOP is still + the next click rather than a minute of impaired traffic later. + """ + run_gui(""" + app.engine.start = lambda *a, **k: None + + def start_with(**values): + app.clear_log() + for key, value in values.items(): + app.vars[key].set(value) + app._start(); app._settle_transition() + return list(app._log_lines) + + warning = bnt.T("warn.global_impairment") + + def warned(log): + return any(warning in line for line in log) + + log = start_with(loss=50, target="", duration=0) + assert warned(log), "no warning for an unscoped, endless run: %r" % log + + log = start_with(loss=50, target="probe.exe") + assert not warned(log), "warned about a targeted run: %r" % log + + log = start_with(loss=50, target="", duration=30) + assert not warned(log), "warned about a run with a deadline: %r" % log + + log = start_with(loss=0, duration=0, lan_mode=True) + assert warned(log), "LAN mode alone cuts the internet: %r" % log + + # a session can BECOME unbounded: start aimed, then clear the target + log = start_with(loss=50, lan_mode=False, target="probe.exe") + assert not warned(log), "warned about a targeted start: %r" % log + app.running = True + app.clear_log() + app.vars["target"].set("") + app.apply_if_running() + assert warned(list(app._log_lines)), \ + "clearing the target mid-session went unannounced: %r" % app._log_lines + """) diff --git a/tests/test_i18n.py b/tests/test_i18n.py index 312b539..27cf087 100644 --- a/tests/test_i18n.py +++ b/tests/test_i18n.py @@ -240,7 +240,7 @@ def test_translated_exceptions(): except ValueError as e: msg_en = str(e) check("exceptions: English in EN mode (so the CLI stays English)", - "bad schedule step" in msg_en, f"({msg_en})") + "Schedule step" in msg_en, f"({msg_en})") n.set_language("pl") check("exceptions: GUI field error translated with the field name", "Pole 'Utrata' musi" in n.T("errors.field_number", name=n.T("fields.loss"))) @@ -248,3 +248,77 @@ def test_translated_exceptions(): check("exceptions: English field error", "must be a number" in n.T("errors.field_number", name="Loss")) n.set_language("pl") + + +# blame words: the message describes the INPUT, never the person who typed it +# (Nielsen Norman Group's error-message guidance). +# Deliberately whole words, and deliberately not "not valid" - "X is not a valid +# IP address" describes the value and tells the reader what shape was wanted, +# which is the opposite of blame. +BLAME = { + "en": (r"\binvalid\b", r"\billegal\b", r"\bbad\b", r"\bwrong\b"), + "pl": (r"nieprawid", r"niepoprawn", r"\bz[lł]y\b", r"\bz[lł]e\b", + r"\bz[lł]a\b", r"b[lł][eę]dn"), +} + + +def _texts_of(code, prefix=""): + import json as _json + with open(os.path.join(LANG_DIR, f"{code}.json"), encoding="utf-8") as f: + data = _json.load(f) + return {k: v for k, v in data.items() + if k.startswith(prefix) and isinstance(v, str) and k != "_meta"} + + +def test_no_message_blames_the_person_reading_it(): + """"Invalid value for 'loss'" told the user they were wrong and nothing else. + + The config loader said that about the very same value the form describes as + "must be between 0 and 100" - so the tool already knew the useful sentence + and used the useless one in the file path. Four texts across the two + languages carried a blame word, and one of them ("bad schedule step") was + also the only error starting in lower case. + + EVERY text, not just ``errors.*``. The first version of this guard scanned + that namespace alone, which would have missed the very offender that started + this: ``log.filter_skipped`` said "Invalid filter expression" and lives under + ``log.``. Measured when the scope was widened - zero offenders in any + namespace - so the wider rule costs nothing today and is the one that + actually holds. + + Deliberately not banned: "is not a valid IP address". It describes the value + and names the shape that was wanted, which is the opposite of blame. If a + text ever genuinely needs one of these words, this test is where that + argument gets made. + """ + import re as _re + for code, patterns in BLAME.items(): + texts = _texts_of(code) + check(f"i18n {code}: the scan actually read the language file", + len(texts) >= 200, f"({len(texts)} keys)") + offenders = sorted(k for k, v in texts.items() + if any(_re.search(p, v, _re.I) for p in patterns)) + check(f"i18n {code}: no message blames the reader", not offenders, + f"({offenders})") + + +def test_every_error_reads_like_a_sentence(): + """Capital letter at the front, terminal punctuation at the end. + + Not pedantry: these strings are shown on their own - under a field in the + form, in a dialog, after ``[bean] error:`` - so a fragment reads as a + truncation. Three were missing their full stop and one began lower case, + which is what a namespace with no guardian looks like after a few years. + """ + for code in ("en", "pl"): + texts = _texts_of(code, "errors.") + check(f"i18n {code}: the error namespace is not empty", len(texts) >= 20, + f"({len(texts)} keys)") + no_stop = sorted(k for k, v in texts.items() + if not v.rstrip().endswith((".", "?", "!"))) + check(f"i18n {code}: every error ends a sentence", not no_stop, + f"({no_stop})") + lower = sorted(k for k, v in texts.items() + if v[:1].islower()) + check(f"i18n {code}: every error starts a sentence", not lower, + f"({lower})") diff --git a/tests/test_mutation_registry.py b/tests/test_mutation_registry.py index eee29f1..3ac316c 100644 --- a/tests/test_mutation_registry.py +++ b/tests/test_mutation_registry.py @@ -102,6 +102,224 @@ "new": "for dirpath, dirnames, filenames in os.walk(ROOT + '_nope'):", "test": "test_the_repository_scanners_actually_read_files", }, + { + "label": "warning: an unbounded run is judged bounded", + "file": "beantester/settings.py", + "old": " if not armed_global_impairments(s):\n return False", + "new": " if armed_global_impairments(s) is not None:\n return False", + "test": "test_a_run_that_impairs_everything_forever_says_so_before_it_starts", + }, + { + "label": "warning: LAN mode is demoted from impairment to scenery", + "file": "beantester/fields.py", + "old": " tip=\"tips.lan_mode\", span=True, cli=\"lan-mode\", impairs=IMPAIRS_ALL),", + "new": " tip=\"tips.lan_mode\", span=True, cli=\"lan-mode\"),", + "test": "test_the_warning_names_lan_mode_which_reads_like_a_scope", + }, + { + "label": "warning: blocking counts as a bound for every other impairment", + "file": "beantester/fields.py", + "old": " width=26, tip=\"tips.block\", span=True, cli=\"block-ip\",\n" + " impairs=IMPAIRS_MATCHED),", + "new": " width=26, tip=\"tips.block\", span=True, cli=\"block-ip\",\n" + " narrows=True),", + "test": "test_blocking_bounds_only_its_own_damage", + }, + { + "label": "warning: an expression of pure exclusions passes as a target", + "file": "beantester/matchers.py", + "old": " return not self._positives", + "new": " return False", + "test": "test_an_exclusion_only_target_is_not_a_bound", + }, + { + "label": "scenario: the file is read only after the capture is open again", + "file": "beantester/cli.py", + "old": " scen = None\n if cfg[\"scenario\"]:\n try:\n" + " scen = load_scenario_file(cfg[\"scenario\"])", + "new": " scen = None\n if False:\n try:\n" + " scen = load_scenario_file(cfg[\"scenario\"])", + "test": "test_a_broken_scenario_never_opens_the_capture", + }, + { + "label": "warning: the GUI starts an unbounded run in silence", + "file": "beantester/gui/app.py", + "old": " warn_if_unbounded(s, self.log)\n" + " # Immediate feedback", + "new": " pass\n" + " # Immediate feedback", + "test": "test_the_gui_says_the_same_thing_before_an_unbounded_start", + }, + { + "label": "warning: a session that BECOMES unbounded says nothing", + "file": "beantester/gui/app.py", + "old": " warn_if_unbounded(s, self.log)\n" + " self.engine.log_event(\"CHANGE\"", + "new": " pass\n" + " self.engine.log_event(\"CHANGE\"", + "test": "test_the_gui_says_the_same_thing_before_an_unbounded_start", + }, + { + "label": "warning: --dry-run previews the values but not the shape", + "file": "beantester/cli.py", + "old": " if not cfg[\"simulate\"]:\n" + " warn_if_unbounded(cfg[\"settings\"], log.warn)", + "new": " if False:\n" + " warn_if_unbounded(cfg[\"settings\"], log.warn)", + "test": "test_dry_run_previews_the_shape_and_not_only_the_values", + }, + { + "label": "help: a semicolon creeps back into a flag's help text", + "file": "beantester/cli.py", + "old": "help=\"which traffic to capture at all (IPv4 and IPv6). Ports are \"", + "new": "help=\"which traffic to capture at all (IPv4 and IPv6); ports are \"", + "test": "test_no_semicolons_in_the_help_a_user_reads", + }, + { + "label": "errors: a config value is called invalid and left at that", + "file": "beantester/settings.py", + "old": " field=key, value=repr(value),\n" + " expected=_expected_shape(key)))", + "new": " field=key, value=repr(value),\n" + " expected=\"\"))", + "test": "test_a_config_value_says_what_the_setting_takes", + }, + { + "label": "errors: the scenario stops suggesting a correction", + "file": "beantester/scenario.py", + "old": " if len(unknown) == 1 and close:", + "new": " if False:", + "test": "test_a_misspelled_scenario_setting_gets_the_same_help_as_a_config_one", + }, + { + "label": "errors: a blame word creeps back into a message", + "file": "lang/en.json", + "old": "\"errors.bad_schedule_step\": \"Schedule step '{part}' is not in " + "the form dur:down:up.\"", + "new": "\"errors.bad_schedule_step\": \"bad schedule step: '{part}'.\"", + "test": "test_no_message_blames_the_person_reading_it", + }, + { + "label": "errors: saving a profile throws the precise message away again", + "file": "beantester/gui/app.py", + "old": " dialogs.show_error(self.root, T(\"log.error\"), str(e))\n" + " return\n" + " self._persist_profiles()", + "new": " dialogs.show_error(self.root, T(\"log.error\"), \"nope\")\n" + " return\n" + " self._persist_profiles()", + "test": "test_saving_a_profile_with_a_bad_value_names_the_field", + }, + { + "label": "errors: a message loses its full stop", + "file": "lang/en.json", + "old": "\"errors.scenario_bad_json\": \"Not a valid JSON file: {error}.\"", + "new": "\"errors.scenario_bad_json\": \"Not a valid JSON file: {error}\"", + "test": "test_every_error_reads_like_a_sentence", + }, + { + "label": "keyboard: Ctrl+F is bound on the entry, not on the window", + "file": "beantester/gui/pages/conns.py", + "old": " app.root.bind(\"\", self._focus_search)", + "new": " entry.bind(\"\", self._focus_search)", + "test": "test_the_table_is_reachable_and_readable_without_a_mouse", + }, + { + "label": "keyboard: the search box stops advertising its shortcut", + "file": "beantester/gui/pages/conns.py", + "old": " add_tooltip(entry, \"tips.conn_search\", shortcut=\"Ctrl+F\")", + "new": " add_tooltip(entry, \"tips.conn_search\")", + "test": "test_shortcut_buttons_advertise_their_key", + }, + { + "label": "keyboard: the context menu goes back to mouse-only", + "file": "beantester/gui/pages/conns.py", + "old": " for sequence in (\"\", menu_key):", + "new": " for sequence in ():", + "test": "test_the_table_is_reachable_and_readable_without_a_mouse", + }, + { + "label": "tables: an empty table goes back to a blank rectangle", + "file": "beantester/gui/widgets/sortable_tree.py", + "old": (" self.repaint()\n" + " self._show_empty_note(not self.items)"), + "new": " self.repaint()", + "test": "test_an_empty_table_says_so_instead_of_showing_a_blank_rectangle", + }, + { + "label": "tables: an unsearched empty table blames a search nobody made", + "file": "beantester/gui/pages/conns.py", + "old": (" self.table.set_empty_text(\"tables.no_conns_match\"\n" + " if self.search_var.get().strip()\n" + " else \"tables.no_conns_yet\")"), + "new": " self.table.set_empty_text(\"tables.no_conns_match\")", + "test": "test_an_empty_table_says_so_instead_of_showing_a_blank_rectangle", + }, + { + "label": "help: an example hardcodes the .py name the exe user lacks", + "file": "beantester/cli.py", + "old": " %(prog)s --simulate --loss 20 --duration 10", + "new": " bean_network_tester.py --simulate --loss 20 --duration 10", + "test": "test_the_examples_name_whatever_this_build_is_called", + }, + { + "label": "help: the usage wall comes back over the examples", + "file": "beantester/cli.py", + "old": " usage=\"%(prog)s [options]\",", + "new": "", + "test": "test_help_opens_with_examples_and_not_with_a_wall_of_usage", + }, + { + "label": "tables: every column goes back to being left-aligned", + "file": "beantester/gui/widgets/sortable_tree.py", + "old": (" if col in self._numeric:\n" + " return \"e\""), + "new": (" if False:\n" + " return \"e\""), + "test": "test_numeric_columns_are_right_aligned_and_the_registry_is_honest", + }, + { + "label": "tables: a number is left touching the text beside it", + "file": "beantester/gui/pages/conns.py", + "old": "CENTERED = frozenset({\"proto\", \"scoped\"})", + "new": "CENTERED = frozenset({\"proto\"})", + "test": "test_numeric_columns_are_right_aligned_and_the_registry_is_honest", + }, + { + "label": "tables: the numeric registry quietly loses a column", + "file": "beantester/gui/pages/conns.py", + "old": "NUMERIC = frozenset({\"pid\", \"remote_port\", \"local_port\", \"packets\", \"dropped\",", + "new": "NUMERIC = frozenset({\"remote_port\", \"local_port\", \"packets\", \"dropped\",", + "test": "test_numeric_columns_are_right_aligned_and_the_registry_is_honest", + }, + { + "label": "tables: an impaired row is marked by colour alone again", + "file": "beantester/gui/theme.py", + "old": " \"impaired\": {\"foreground\": \"#ffb454\", \"font\": (FONT, 9, \"bold\")},", + "new": " \"impaired\": {\"foreground\": \"#ffb454\"},", + "test": "test_an_impaired_row_is_not_marked_by_colour_alone", + }, + { + "label": "readme: a semicolon hides inside a nested list again", + "file": "README.md", + "old": "With nothing set they are equal. The moment", + "new": "With nothing set they are equal; the moment", + "test": "test_no_semicolons_in_readme_prose", + }, + { + "label": "help: the semicolon scan reads an empty parser", + "file": "tests/test_cli_docs.py", + "old": " helps = [a for a in parser._actions if a.help]", + "new": " helps = []", + "test": "test_no_semicolons_in_the_help_a_user_reads", + }, + { + "label": "guards: a HANDOFF brief falls back into the scanned set", + "file": "tests/test_repo_conventions.py", + "old": "SKIP_PREFIXES = (\"HANDOFF-\",)", + "new": "SKIP_PREFIXES = ()", + "test": "test_the_repository_scanners_stay_out_of_what_is_not_in_the_repository", + }, { "label": "guards: internal_tools falls back into the scanned set", "file": "tests/test_repo_conventions.py", @@ -156,7 +374,8 @@ # machine can repeat it. This is weaker than MUTATIONS and stronger than nothing - # and it is the honest state of most "verified by mutation" lines in the notes. PROVEN_BY_HAND = { - "test_shortcut_buttons_advertise_their_key": "2026-07-21, dropping shortcut= from Save/Load", + # test_shortcut_buttons_advertise_their_key moved to MUTATIONS on 2026-08-03, + # when Ctrl+F gave it a patch worth writing down. This list is meant to shrink. "test_an_overridden_field_is_visibly_disabled": "2026-07-21, removing the disabled style maps", "test_no_stale_pending_markers": "2026-07-25, both directions", "test_every_remote_endpoint_gate_fires_in_both_directions": "2026-07, the inbound branch", diff --git a/tests/test_passthrough.py b/tests/test_passthrough.py index 82bb6ad..9b20971 100644 --- a/tests/test_passthrough.py +++ b/tests/test_passthrough.py @@ -31,23 +31,30 @@ from beantester.core import BeanCore, Decision from beantester.engine import BeanEngine +from beantester.fields import (FIELDS, IMPAIRING_KEYS, NARROWING_KEYS, off_value) from beantester.presets import PRESETS, SETTING_TO_PRESET, preset_to_settings from beantester.settings import DEFAULT_SETTINGS, apply_settings from beantester.synthetic import SyntheticDivert from fakes import FakeDivert, FakePacket, check # Every knob that can make the core do something other than "pass the packet -# straight through", paired with the value that means "off". If a new impairment -# is added to the model, add it here too - an omission is exactly how a default -# could start damaging traffic without a test noticing. -IMPAIRMENT_OFF = { - "loss": 0, "corrupt": 0, "dup": 0, "latency": 0, "jitter": 0, - "down": 0, "up": 0, "syn_drop": 0, "max_size": 0, "spike_prob": 0, - "spike_ms": 0, "nat_timeout": 0, "rst_prob": 0, "flap_period": 0, - "flap_down": 0, "target": "", "dst_ip": "", "dst_port": "", - "block_ip": "", "block_port": "", - "rate_schedule": "", "lan_mode": False, -} +# straight through", paired with the value that means "off". +# +# DERIVED from the field registry, not typed out again: a field declares what it +# does to traffic (``impairs`` / ``narrows``, fields.py), and this invariant reads +# that declaration. It used to be a hand-written list, which meant a new impairment +# had to be remembered in two places - and the one that gets forgotten is always +# the test, so the damage ships looking harmless. +# +# The two additions are PARAMETERS, not triggers: `spike_ms` and `flap_down` arm +# nothing on their own (they sit behind `spike_prob` and `flap_period`), so the +# registry rightly does not call them impairments - but a default that shipped +# with either one hot would still be a default nobody chose, and pass-through is +# the one place that should insist on the whole form being cold. +IMPAIRMENT_OFF = dict( + {key: off_value(FIELDS[key]) for key in IMPAIRING_KEYS + NARROWING_KEYS}, + spike_ms=0, flap_down=0, +) # The profile fields that can impair traffic, and their "no impairment" value. # Derived from IMPAIRMENT_OFF (settings keys) so an impairment joining the diff --git a/tests/test_readme_guards.py b/tests/test_readme_guards.py index 690a1e4..3c0bf5f 100644 --- a/tests/test_readme_guards.py +++ b/tests/test_readme_guards.py @@ -170,9 +170,19 @@ def test_no_semicolons_in_readme_prose(): comma, because that is what people write. Code is exempt and has to be, since a semicolon is syntax there - bash, - JSON, PowerShell, filter expressions. So fenced blocks, indented blocks and - inline `code spans` are all cut out before looking, which is what keeps this - check free of false positives. + JSON, PowerShell, filter expressions. So fenced blocks and inline `code + spans` are cut out before looking, which is what keeps this check free of + false positives. + + It used to skip any line INDENTED by four spaces as well, on the theory that + those are indented code blocks. In markdown they are also how a nested list + continues, and that is what the exemption actually hid: a semicolon lived in + a nested bullet of README.md while this guard reported the file clean. + MEASURED before removing it - of the indented lines outside fences, 16 in + README.md and 8 in README.pl.md, EVERY one is list text and none is code. + Both files fence all their code, so the exemption protected nothing and + blinded the check. Should an indented code block ever arrive, this test will + say so and the answer is to fence it, which is better markdown regardless. """ for readme in READMES: offenders, fenced = [], False @@ -180,7 +190,7 @@ def test_no_semicolons_in_readme_prose(): if line.lstrip().startswith("```"): fenced = not fenced continue - if fenced or line.startswith(" "): + if fenced: continue prose = re.sub(r"`[^`]*`", "", line) if ";" in prose: diff --git a/tests/test_repo_conventions.py b/tests/test_repo_conventions.py index 7ff4a11..f4b8d9d 100644 --- a/tests/test_repo_conventions.py +++ b/tests/test_repo_conventions.py @@ -24,6 +24,8 @@ ".hypothesis", "internal_tools", ".claude", "crashes"} SKIP_FILES = {"PROJECT_NOTES.md", "HISTORY_NOTES.md", "CLAUDE.md", "CHANGELOG-INTERNAL.md"} +# Same reason, by prefix: HANDOFF-*.md are maintainer briefs kept out of git. +SKIP_PREFIXES = ("HANDOFF-",) def repo_text_files(exts): @@ -37,7 +39,8 @@ def repo_text_files(exts): for dirpath, dirnames, filenames in os.walk(ROOT): dirnames[:] = [d for d in dirnames if d not in SKIP_DIRS] for name in filenames: - if name.endswith(exts) and name not in SKIP_FILES: + if (name.endswith(exts) and name not in SKIP_FILES + and not name.startswith(SKIP_PREFIXES)): out.append(os.path.join(dirpath, name)) return out @@ -291,7 +294,7 @@ def test_the_repository_scanners_stay_out_of_what_is_not_in_the_repository(): scanned = {os.path.relpath(p, ROOT).replace(os.sep, "/") for p in repo_text_files((".py", ".md", ".json", ".txt"))} for stray in ("PROJECT_NOTES.md", "CLAUDE.md", "HISTORY_NOTES.md", - "CHANGELOG-INTERNAL.md"): + "CHANGELOG-INTERNAL.md", "HANDOFF-UI-CLI.md"): check(f"{stray} is not scanned (it is not in the repository)", stray not in scanned, f"({stray})") for prefix in ("internal_tools/", ".claude/", "crashes/"): diff --git a/tests/test_settings_config_scenario.py b/tests/test_settings_config_scenario.py index 59fe719..19fedf0 100644 --- a/tests/test_settings_config_scenario.py +++ b/tests/test_settings_config_scenario.py @@ -387,3 +387,58 @@ def test_build_matchers_covers_every_filter_field(): check("build_matchers compiles one matcher per field", set(matchers) == keys) check("empty defaults compile to empty matchers", all(m.is_empty for m in matchers.values())) + + +def test_a_misspelled_scenario_setting_gets_the_same_help_as_a_config_one(): + """One class of mistake, one quality of answer. + + ``load_config_file`` has suggested a correction for a near-miss setting name + since it learned to (``difflib``, settings.py); the scenario loader, reading + the same names out of the same kind of JSON file, said only "unknown + setting" - and which loader helped you depended on which one was written + first. The unknown ACTION message had the matching gap: it named what you + typed without naming what exists, and there is exactly one action to name. + + Both halves are asserted, because a suggestion that fires for EVERY typo is + its own bug: a name close to nothing must still fail plainly. + """ + import pytest + from beantester.scenario import parse_scenario + with pytest.raises(ValueError) as near: + parse_scenario([{"at": 0, "settings": {"losss": 10}}]) + check("scenario: a near-miss setting is offered the correction", + "loss" in str(near.value) and "?" in str(near.value), f"({near.value})") + + with pytest.raises(ValueError) as far: + parse_scenario([{"at": 0, "settings": {"zzzzzzzz": 10}}]) + check("scenario: a name close to nothing still fails plainly", + "zzzzzzzz" in str(far.value) and "?" not in str(far.value), + f"({far.value})") + + with pytest.raises(ValueError) as action: + parse_scenario([{"at": 0, "action": "reset_tpc"}]) + check("scenario: an unknown action names the ones that exist", + "reset_tcp" in str(action.value), f"({action.value})") + + +def test_a_config_value_says_what_the_setting_takes(tmp_path): + """"Invalid value for 'loss'" said the value was wrong and stopped there. + + The bounds are in the registry and the form has always used them ("must be + between 0 and 100"). The config loader described the same rejected value + with the word "invalid" and nothing else, so the interface that could not + show you the field was also the one that would not tell you the range. + """ + import json + import pytest + from beantester.settings import load_config_file + path = tmp_path / "config.json" + path.write_text(json.dumps({"loss": "abc"}), encoding="utf-8") + with pytest.raises(ValueError) as e: + load_config_file(str(path)) + message = str(e.value) + check("config: the message names the setting", "loss" in message, f"({message})") + check("config: it names what the setting takes", + "0" in message and "100" in message, f"({message})") + check("config: it quotes back what was actually given", + "abc" in message, f"({message})")