From 580c92fc66cab433f1e3e32b5dd85745d10f31c7 Mon Sep 17 00:00:00 2001 From: Jaret Arnold <96366172+musharna@users.noreply.github.com> Date: Wed, 2 Sep 2026 12:37:41 -0400 Subject: [PATCH 01/10] fix(broker): fold the project filter like submit; roots without priority is a load error audit 2026-09-02 C-1, C-2, L-1, L-2, L-5. - routes/jobs.py, routes/events.py: the read-side project filter compared the raw string while submit stored the folded, registered spelling, so a project split into two views and no spelling returned all its jobs. Both filters now match {canonical, typed} on project plus typed on project_label. - config.py: the 'priority not in cfg -> continue' skip ran BEFORE _parse_roots, so a project declared with roots and no priority was dropped with no log line, contradicting docs/projects-yaml.md. Raise. _default may not declare roots (skipped by project_from_cwd by design). Leading '//' collapses onto '/' in roots and cwd; normpath preserves it. - db.py: index project_label (list ORs on it); migrate() creates the same index on in-place upgrades. The pre-existing migration test dropped the column directly, which SQLite refuses once an index references it. - routes/config.py: /reload swaps state under projects_mutation_lock so a concurrent set/nudge cannot persist from the swapped-in table. 9 new tests, each watched failing on the pre-fix code. --- changelog.d/audit-2026-09-02-broker.fixed.md | 3 ++ src/jobd/broker/routes/config.py | 21 ++++++---- src/jobd/broker/routes/events.py | 12 +++++- src/jobd/broker/routes/jobs.py | 9 ++++- src/jobd/config.py | 39 +++++++++++++++++-- src/jobd/db.py | 12 +++++- tests/test_projects_yaml.py | 29 ++++++++++++++ tests/unit/test_events_endpoint.py | 14 +++++++ .../test_list_filter_matches_either_name.py | 29 ++++++++++++++ tests/unit/test_project_from_cwd.py | 12 ++++++ tests/unit/test_project_label_column.py | 34 +++++++++++++++- .../test_reload_takes_the_mutation_lock.py | 31 +++++++++++++++ 12 files changed, 228 insertions(+), 17 deletions(-) create mode 100644 changelog.d/audit-2026-09-02-broker.fixed.md create mode 100644 tests/unit/test_reload_takes_the_mutation_lock.py diff --git a/changelog.d/audit-2026-09-02-broker.fixed.md b/changelog.d/audit-2026-09-02-broker.fixed.md new file mode 100644 index 0000000..cf31844 --- /dev/null +++ b/changelog.d/audit-2026-09-02-broker.fixed.md @@ -0,0 +1,3 @@ +- **`job list --project` and `/events?project=` now fold the name the way submit does.** Submit stores a job under its registered spelling (`JEPAGAME` prices as, and is recorded as, `jepagame`), but both read filters compared the raw string, so no single spelling returned all of a project's jobs and a query for `Project_A` found no event written after the fold landed. The filter now matches the folded identity, the typed spelling, and the typed run label. `project_label` gains an index (created by `migrate()` on in-place upgrades too) since the list filter ORs on it. +- **A project declared with `roots:` but no `priority:` is a load error, not a silent drop.** `load_projects` skipped any entry lacking `priority` before the roots validation ran, so `name: {roots: [...]}` vanished with no log line — the invisible removal the roots validation promises to prevent. `_default` may no longer declare roots either (they were validated, accepted, and never consulted). A leading `//` in a root or cwd is collapsed onto `/`; previously `//home/x` was a valid root that could never match. +- **`/reload` takes the projects mutation lock.** A `set`/`nudge` that landed between reload's re-read and its swap of the projects table was persisted from the new table and lost. diff --git a/src/jobd/broker/routes/config.py b/src/jobd/broker/routes/config.py index f3db7b7..9b6ac1f 100644 --- a/src/jobd/broker/routes/config.py +++ b/src/jobd/broker/routes/config.py @@ -109,13 +109,20 @@ def reload_config(): # Re-read the git baseline AND re-apply the runtime overrides overlay, so # a `git pull` of projects.yaml takes effect without discarding priorities # set at runtime via `job projects set/nudge` (audit 2026-07-12). - projects, base_priorities = load_effective_projects( - state["paths"]["projects"], state["paths"]["project_overrides"] - ) - state["projects"] = projects - state["base_priorities"] = base_priorities - state["profiles"] = load_profiles(state["paths"]["profiles"]) - state["classifier"] = load_classifier_rules(state["paths"]["classifier"]) + # + # Under the writers' lock: set/nudge mutate `state["projects"]` and then + # persist FROM `state["projects"]`; a swap between those two reads made + # the overlay be written from the new dict and the mutation vanish + # (audit 2026-09-02 L-1). The loads run inside the lock too, so a + # failed load leaves state untouched and the lock released. + with projects_mutation_lock: + projects, base_priorities = load_effective_projects( + state["paths"]["projects"], state["paths"]["project_overrides"] + ) + state["projects"] = projects + state["base_priorities"] = base_priorities + state["profiles"] = load_profiles(state["paths"]["profiles"]) + state["classifier"] = load_classifier_rules(state["paths"]["classifier"]) return {"reloaded": True} @router.post("/resolve", response_model=ResolvedConfig) diff --git a/src/jobd/broker/routes/events.py b/src/jobd/broker/routes/events.py index 46426eb..f164648 100644 --- a/src/jobd/broker/routes/events.py +++ b/src/jobd/broker/routes/events.py @@ -13,12 +13,14 @@ from jobd import events as _events from jobd.broker.context import BrokerDeps from jobd.broker.events import _emit_event, _parse_since +from jobd.config import canonical_project_name from jobd.models import EventIngest def build_router(deps: BrokerDeps) -> APIRouter: router = APIRouter() logs_dir = deps.logs_dir + state = deps.state @router.post("/events", status_code=204) def ingest_event(body: EventIngest): @@ -58,13 +60,21 @@ def get_events( """ limit = max(1, min(10000, int(limit))) cutoff = _parse_since(since) if since else None + # Broker-emitted rows carry the CANONICAL project (submit folds the + # typed spelling onto the registered one), so the filter folds too, or + # a query for `Project_A` finds nothing written after the fold landed + # (audit 2026-09-02 C-1). The typed spelling is kept as well: rows + # ingested by hooks/workers may carry whatever the caller sent. + project_names: set[str] | None = None + if project is not None: + project_names = {project, canonical_project_name(state["projects"], project)} def _match(row: dict) -> bool: # Rows missing `source` are legacy (pre-schema-v2) — excluded, same # as before. The reverse-reader handles the ts/cutoff early-stop. if "source" not in row: return False - if project is not None and row.get("project") != project: + if project_names is not None and row.get("project") not in project_names: return False if event is not None and row.get("event") != event: return False diff --git a/src/jobd/broker/routes/jobs.py b/src/jobd/broker/routes/jobs.py index df8d6c5..f79450c 100644 --- a/src/jobd/broker/routes/jobs.py +++ b/src/jobd/broker/routes/jobs.py @@ -38,6 +38,7 @@ _reject_stale_worker, ) from jobd.broker.submit import submit_job +from jobd.config import canonical_project_name from jobd.db import Job, Worker from jobd.matcher import eligible_workers from jobd.models import ( @@ -108,8 +109,12 @@ def list_jobs( # Either name: the identity that priced the job, or the label # the submitter typed. A human filtering by the label they used # must still find their run after cwd supplied a different - # identity for it. - conds.append(or_(Job.project == project, Job.project_label == project)) + # identity for it. The identity is matched FOLDED, the same way + # submit folded it: rows are stored under the registered + # spelling, so a filter on `JEPAGAME` that compared raw strings + # split one project into two views (audit 2026-09-02 C-1). + canon = canonical_project_name(state["projects"], project) + conds.append(or_(Job.project.in_({canon, project}), Job.project_label == project)) if warnings_only: conds.append(Job.warning.is_not(None)) if array_id is not None: diff --git a/src/jobd/config.py b/src/jobd/config.py index 81fb0c5..73d6c39 100644 --- a/src/jobd/config.py +++ b/src/jobd/config.py @@ -162,7 +162,11 @@ def _parse_roots(raw: object, project_name: str) -> list[str]: raise ValueError( f"projects.yaml {project_name!r}: roots entry {r!r} is not an absolute path" ) - norm = str(PurePosixPath(r)) + # POSIX lets `//x` differ from `/x` and both PurePosixPath and + # normpath preserve exactly two leading slashes, so `//home/x` was a + # valid root that could never match a cwd of `/home/x/...`. On Linux + # they are the same directory; spell the root that way. + norm = "/" + str(PurePosixPath(r)).lstrip("/") if ".." in PurePosixPath(norm).parts: # A root is a stable identity boundary, so a `..` in it is a config # error, not a path to interpret. Collapsing it silently would @@ -204,10 +208,29 @@ def load_projects(path: Path | str) -> dict[str, ProjectEntry]: projects = data.get("projects", {}) out: dict[str, ProjectEntry] = {} for name, cfg in projects.items(): - if not isinstance(cfg, dict) or "priority" not in cfg: + if not isinstance(cfg, dict): + continue + if "priority" not in cfg: + if "roots" in cfg: + # An entry without `priority` is skipped as not-a-project. That + # skip used to run BEFORE the roots validation, so a project + # declared as `name: {roots: [...]}` vanished with no log line + # -- the invisible removal `_parse_roots` exists to prevent + # (audit 2026-09-02 C-2). + raise ValueError( + f"projects.yaml {name!r}: declares roots but no priority; " + f"a project with roots must be a full entry" + ) continue defaults = _parse_defaults(cfg.get("defaults")) roots = _parse_roots(cfg.get("roots"), name) + if name == "_default" and roots: + # `project_from_cwd` skips `_default` on purpose; accepting roots + # here would validate them and then never consult them. + raise ValueError( + "projects.yaml '_default': may not declare roots; it is the fallback " + "priority row, not a project" + ) out[name] = ProjectEntry(priority=int(cfg["priority"]), defaults=defaults, roots=roots) if "_default" not in out: out["_default"] = ProjectEntry(priority=40) @@ -466,11 +489,19 @@ def _path_is_within(cwd: str, root: str) -> bool: `..` needs no filesystem and is correct on any. Symlinks and bind mounts remain unresolved by design — see docs/projects-yaml.md. """ - c = PurePosixPath(os.path.normpath(cwd)).parts - r = PurePosixPath(os.path.normpath(root)).parts + c = PurePosixPath(_normalize_abs(cwd)).parts + r = PurePosixPath(_normalize_abs(root)).parts return len(c) >= len(r) and c[: len(r)] == r +def _normalize_abs(path: str) -> str: + """`os.path.normpath`, plus collapsing a leading `//` (which POSIX and + normpath both preserve) onto `/`. A relative path is returned as normpath + leaves it; it will match no absolute root, which is the right answer.""" + norm = os.path.normpath(path) + return "/" + norm.lstrip("/") if norm.startswith("/") else norm + + def project_from_cwd(projects: dict[str, ProjectEntry], cwd: str) -> tuple[str, str] | None: """Identify a project by the directory a job runs in. diff --git a/src/jobd/db.py b/src/jobd/db.py index e6d153e..aaa39f6 100644 --- a/src/jobd/db.py +++ b/src/jobd/db.py @@ -40,7 +40,7 @@ class Job(Base): # job); this is the free-text run label a human searches for. NULL means the # two are the same, which is why this needed no backfill: every row that # predates the column is already correct under that reading. - project_label: Mapped[str | None] = mapped_column(String(100), nullable=True) + project_label: Mapped[str | None] = mapped_column(String(100), nullable=True, index=True) profile: Mapped[str | None] = mapped_column(String(100), nullable=True) host_pin: Mapped[str] = mapped_column(String(50), default="any") priority: Mapped[int] = mapped_column(Integer, index=True) @@ -226,6 +226,14 @@ def init_db(engine) -> None: ] +# Indexes that `create_all` builds on a fresh database but an in-place upgrade +# (which only ALTERs columns on) would otherwise never get. Names match +# SQLAlchemy's `index=True` convention so the two paths converge on one index. +_JOB_INDEXES = [ + ("ix_jobs_project_label", "project_label"), +] + + def migrate(engine) -> None: """Additive SQLite migration for Phase 2 capability columns. Idempotent.""" insp = _inspect(engine) @@ -235,6 +243,8 @@ def migrate(engine) -> None: for col, ddl in _JOB_ADDS: if col not in existing: conn.execute(_text(f"ALTER TABLE jobs ADD COLUMN {col} {ddl}")) + for idx, col in _JOB_INDEXES: + conn.execute(_text(f"CREATE INDEX IF NOT EXISTS {idx} ON jobs ({col})")) if "workers" in insp.get_table_names(): existing = {c["name"] for c in insp.get_columns("workers")} with engine.begin() as conn: diff --git a/tests/test_projects_yaml.py b/tests/test_projects_yaml.py index 7a29989..d3d1eb4 100644 --- a/tests/test_projects_yaml.py +++ b/tests/test_projects_yaml.py @@ -463,3 +463,32 @@ def test_no_local_overlay_is_a_noop(tmp_path): tracked = load_projects(tmp_path / "projects.yaml") assert set(eff) == set(tracked) assert [str(r) for r in eff["alpha"].roots] == ["/srv/example/alpha"] + + +def test_roots_without_a_priority_is_a_load_error_not_a_silent_drop(tmp_path): + """audit 2026-09-02 C-2: `load_projects` skipped any entry lacking + `priority` BEFORE `_parse_roots` ran, so a project declared as + `beta: {roots: [...]}` vanished with no log line -- exactly the + invisible removal the roots validation promises to prevent.""" + p = tmp_path / "projects.yaml" + p.write_text("projects:\n beta:\n roots: ['/home/user/beta']\n") + with pytest.raises(ValueError, match="priority"): + load_projects(p) + + +def test_default_may_not_declare_roots(tmp_path): + """`_default` is skipped by `project_from_cwd`, so a root on it was + validated, accepted, and then silently never consulted.""" + p = tmp_path / "projects.yaml" + p.write_text("projects:\n _default:\n priority: 40\n roots: ['/home']\n") + with pytest.raises(ValueError, match="_default"): + load_projects(p) + + +def test_a_leading_double_slash_in_a_root_is_collapsed(tmp_path): + """POSIX lets `//x` mean something other than `/x`, and `os.path.normpath` + preserves it, so `//home/x` was accepted as a root that could never match + a cwd of `/home/x/...` -- a silently dead root.""" + p = tmp_path / "projects.yaml" + p.write_text("projects:\n beta:\n priority: 78\n roots: ['//home/user/x']\n") + assert load_projects(p)["beta"].roots == ["/home/user/x"] diff --git a/tests/unit/test_events_endpoint.py b/tests/unit/test_events_endpoint.py index 12c10e1..695e2c6 100644 --- a/tests/unit/test_events_endpoint.py +++ b/tests/unit/test_events_endpoint.py @@ -189,3 +189,17 @@ def test_events_endpoint_skips_unparseable_ts_when_filtering(client, logs_dir): rows = resp.json() assert len(rows) == 1 assert rows[0]["event"] == "recent" + + +def test_events_endpoint_project_filter_folds_like_submit(client, logs_dir): + """audit 2026-09-02 C-1: broker-emitted rows carry the CANONICAL project + (submit folds `Project_A` onto `project-a`), so an exact-match filter on + the typed spelling returned nothing for every post-fold event.""" + _write_event(logs_dir, _row(event="e", project="project-a", job_id=1)) + _write_event(logs_dir, _row(event="e", project="project-c", job_id=2)) + resp = client.get("/events", params={"project": "Project_A"}) + assert resp.status_code == 200 + rows = resp.json() + assert [r["job_id"] for r in rows] == [1] + # Positive control: an unregistered name still matches only itself. + assert client.get("/events", params={"project": "project-c"}).json()[0]["job_id"] == 2 diff --git a/tests/unit/test_list_filter_matches_either_name.py b/tests/unit/test_list_filter_matches_either_name.py index 23484ea..d3ae400 100644 --- a/tests/unit/test_list_filter_matches_either_name.py +++ b/tests/unit/test_list_filter_matches_either_name.py @@ -45,3 +45,32 @@ def test_an_unrelated_project_filter_still_matches_nothing(rooted_client): }, ) assert rooted_client.get("/jobs", params={"project": "gamma"}).json() == [] + assert rooted_client.get("/jobs", params={"project": "gamma"}).json() == [] + + +def test_filtering_by_a_different_spelling_of_the_identity_finds_the_job(rooted_client): + """audit 2026-09-02 C-1: the filter compared the raw string, so a project + stored under its registered spelling was unfindable under any other. The + submit path folds case and -/_ (`BETA` prices as `beta`); the read + path must fold the same way or one project splits into two views.""" + rooted_client.post( + "/submit", + json={"cmd": ["true"], "cwd": "/tmp", "project": "BETA"}, + ) + rooted_client.post( + "/submit", + json={ + "cmd": ["true"], + "cwd": "/home/user/beta/sweeps", + "project": "pillar2a1_sweep", + }, + ) + # A third spelling nobody typed: only folding can find these. + rows = rooted_client.get("/jobs", params={"project": "Beta"}).json() + assert len(rows) == 2, [(r["project"], r["project_label"]) for r in rows] + assert {r["project"] for r in rows} == {"beta"} + # And the label as typed still finds the folded job. + rows = rooted_client.get("/jobs", params={"project": "BETA"}).json() + assert len(rows) == 2 + # Positive control: folding must not widen the filter into a no-op. + assert rooted_client.get("/jobs", params={"project": "gamma"}).json() == [] diff --git a/tests/unit/test_project_from_cwd.py b/tests/unit/test_project_from_cwd.py index cc44619..bb036da 100644 --- a/tests/unit/test_project_from_cwd.py +++ b/tests/unit/test_project_from_cwd.py @@ -133,3 +133,15 @@ def test_dotdot_onto_a_prefix_sibling_does_not_match(): def test_dotdot_escaping_the_home_tree_entirely_does_not_match(): """The worst case: a job really running in /tmp, priced at beta's 78.""" assert project_from_cwd(_rooted(), "/home/user/beta/../../tmp") is None + """The worst case: a job really running in /tmp, priced at beta's 78.""" + assert project_from_cwd(_rooted(), "/home/user/beta/../../tmp") is None + + +def test_a_leading_double_slash_in_the_cwd_still_matches(): + """`//home/...` and `/home/...` name the same directory on Linux; the + match must not fail on a spelling difference in the caller's cwd.""" + projects = _projects(beta=["/home/user/beta"]) + assert project_from_cwd(projects, "//home/user/beta/sweeps") == ( + "beta", + "/home/user/beta", + ) diff --git a/tests/unit/test_project_label_column.py b/tests/unit/test_project_label_column.py index b8b09fa..9e3ccc4 100644 --- a/tests/unit/test_project_label_column.py +++ b/tests/unit/test_project_label_column.py @@ -9,6 +9,16 @@ from jobd.db import Base, migrate +def _drop_project_label(engine) -> None: + """Turn a fresh schema into the pre-column one. SQLite refuses to drop a + column an index references, so the index goes first.""" + with engine.begin() as conn: + for idx in inspect(engine).get_indexes("jobs"): + if idx["column_names"] == ["project_label"]: + conn.execute(text(f"DROP INDEX {idx['name']}")) + conn.execute(text("ALTER TABLE jobs DROP COLUMN project_label")) + + def test_migrate_adds_project_label_to_a_pre_existing_table(tmp_path): """The real-execution check for the migration: build a jobs table WITHOUT the column, run migrate, and read the live schema back. A test that only @@ -16,8 +26,7 @@ def test_migrate_adds_project_label_to_a_pre_existing_table(tmp_path): url = f"sqlite:///{tmp_path / 'old.db'}" engine = create_engine(url) Base.metadata.create_all(engine) - with engine.begin() as conn: - conn.execute(text("ALTER TABLE jobs DROP COLUMN project_label")) + _drop_project_label(engine) assert "project_label" not in {c["name"] for c in inspect(engine).get_columns("jobs")} migrate(engine) @@ -36,3 +45,24 @@ def test_migrate_is_idempotent(tmp_path): migrate(engine) cols = {c["name"] for c in inspect(engine).get_columns("jobs")} assert "project_label" in cols + + +def test_project_label_is_indexed_on_a_fresh_table(tmp_path): + """audit 2026-09-02 L-5: `job list --project` ORs on project_label, so an + unindexed column turns every filtered list into a table scan.""" + engine = create_engine(f"sqlite:///{tmp_path / 'fresh.db'}") + Base.metadata.create_all(engine) + indexed = {tuple(i["column_names"]) for i in inspect(engine).get_indexes("jobs")} + assert ("project_label",) in indexed + + +def test_migrate_adds_the_project_label_index_to_a_pre_existing_table(tmp_path): + """An in-place upgrade gets the ALTER for the column; it must get the + index too, or only fresh databases are fast.""" + engine = create_engine(f"sqlite:///{tmp_path / 'old.db'}") + Base.metadata.create_all(engine) + _drop_project_label(engine) + migrate(engine) + indexed = {tuple(i["column_names"]) for i in inspect(engine).get_indexes("jobs")} + assert ("project_label",) in indexed + migrate(engine) # idempotent: no "index already exists" diff --git a/tests/unit/test_reload_takes_the_mutation_lock.py b/tests/unit/test_reload_takes_the_mutation_lock.py new file mode 100644 index 0000000..412e5be --- /dev/null +++ b/tests/unit/test_reload_takes_the_mutation_lock.py @@ -0,0 +1,31 @@ +"""audit 2026-09-02 L-1: `/reload` swapped `state["projects"]` while a +concurrent `set`/`nudge` could be between mutating the old dict and persisting +`state["projects"]` -- the overlay was then written from the NEW dict and the +mutation lost both in memory and on disk. The fix is that reload takes the same +lock the writers hold. +""" + +import threading + +from jobd.broker.projects import projects_mutation_lock + + +def test_reload_waits_for_the_projects_mutation_lock(client): + done = threading.Event() + status: list[int] = [] + + def _reload(): + status.append(client.post("/reload").status_code) + done.set() + + projects_mutation_lock.acquire() + try: + t = threading.Thread(target=_reload, daemon=True) + t.start() + # While a writer holds the lock, reload must not complete. + assert not done.wait(0.5), "reload ran without taking projects_mutation_lock" + finally: + projects_mutation_lock.release() + # Positive control: once the writer is done, reload proceeds normally. + assert done.wait(5.0) + assert status == [200] From 063155a191bb49aa5dab1cf03b88c06047b5a6d8 Mon Sep 17 00:00:00 2001 From: Jaret Arnold <96366172+musharna@users.noreply.github.com> Date: Wed, 2 Sep 2026 12:39:59 -0400 Subject: [PATCH 02/10] fix(mcp): expose project_label, map bad arguments to is_error, dispatch off the event loop audit 2026-09-02 Q-3 plus two LOWs. - tools.py: _LIST_SUMMARY_FIELDS and the submit result carry project_label. The broker filter matches either name, so a list row could arrive under an identity the agent never typed with no field saying why. - server.py: KeyError/TypeError from a tool (missing required argument) become an is_error result of kind invalid_arguments; under mcp 2.x the uncaught exception was a protocol-level 'Internal server error'. _dispatch runs via asyncio.to_thread; it drives a blocking httpx client and a wait=true submit could hold the loop for 270 s. - schemas.py: the project description said 'falls back to _default'; the fallback is now the project whose roots contain cwd, then _default. 5 tests (4 new + 1 widened key-set pin), each watched failing first. --- changelog.d/audit-2026-09-02-mcp.fixed.md | 1 + src/jobd/mcp/schemas.py | 8 +++- src/jobd/mcp/server.py | 25 +++++++++- src/jobd/mcp/tools.py | 7 +++ tests/mcp/test_protocol_roundtrip.py | 58 +++++++++++++++++++++++ tests/mcp/test_tools.py | 52 ++++++++++++++++++++ 6 files changed, 149 insertions(+), 2 deletions(-) create mode 100644 changelog.d/audit-2026-09-02-mcp.fixed.md diff --git a/changelog.d/audit-2026-09-02-mcp.fixed.md b/changelog.d/audit-2026-09-02-mcp.fixed.md new file mode 100644 index 0000000..aff9cfb --- /dev/null +++ b/changelog.d/audit-2026-09-02-mcp.fixed.md @@ -0,0 +1 @@ +- **The MCP server no longer hides a substituted project identity, crashes on a missing argument, or stalls the session behind one slow call.** `jobd_list` rows and the `jobd_submit` result now carry `project_label` (the name as typed, `null` when it equals `project`), so an agent can see that the broker scheduled a job under a different identity than it asked for; previously only `jobd_status` exposed it. A missing or mis-typed argument (`jobd_status {}`) is returned as an `is_error` result of kind `invalid_arguments` with a hint, instead of a protocol-level "Internal server error" with a traceback on stderr. Tool dispatch runs off the event loop, so a `jobd_submit wait=true` (up to 270 s) no longer blocks every other request on the stdio session. The `project` schema description now describes the cwd-roots fallback rather than the old `_default`-only one. diff --git a/src/jobd/mcp/schemas.py b/src/jobd/mcp/schemas.py index c339459..30e5b22 100644 --- a/src/jobd/mcp/schemas.py +++ b/src/jobd/mcp/schemas.py @@ -12,7 +12,13 @@ "command": {"type": "string", "description": "Shell command run by the worker shell."}, "project": { "type": "string", - "description": "Priority lookup key; falls back to _default.", + "description": ( + "Scheduling identity. A registered projects.yaml name (matched " + "case- and -/_-insensitively) prices at its priority; an " + "unregistered name is priced by the project whose roots: contain " + "cwd, else by _default. The result's project_label carries the " + "name as typed when the two differ." + ), }, "cwd": { "type": "string", diff --git a/src/jobd/mcp/server.py b/src/jobd/mcp/server.py index 8f87201..2e501a3 100644 --- a/src/jobd/mcp/server.py +++ b/src/jobd/mcp/server.py @@ -191,7 +191,12 @@ async def _call( t0 = time.monotonic() error_kind: str | None = None try: - payload = _dispatch(name, arguments) + # Off the event loop: `_dispatch` drives a BLOCKING httpx client, + # and a `jobd_submit wait=true` can hold it for up to 270 s. Run + # inline, that stalled every other request on the stdio session + # (audit 2026-09-02). Unknown-tool ValueError is raised inside the + # thread too and propagates through `to_thread` unchanged. + payload = await asyncio.to_thread(_dispatch, name, arguments) if ( isinstance(payload, dict) and "error" in payload @@ -219,6 +224,24 @@ async def _call( content=[types.TextContent(type="text", text=str(e))], is_error=True, ) + except (KeyError, TypeError) as e: + # A missing or mis-typed argument (`jobd_status {}`) raises inside + # the tool. Uncaught, 2.x reports it as a protocol-level "Internal + # server error" with a traceback on stderr, and the is_error + hint + # contract silently stops applying to the most common agent + # mistake (audit 2026-09-02). Same shape as a broker 422. + error_kind = "invalid_arguments" + payload = { + "error": { + "kind": error_kind, + "message": f"missing or malformed argument: {e}", + "hint": f"check the inputSchema for {name}: required fields and types", + } + } + return types.CallToolResult( + content=[types.TextContent(type="text", text=json.dumps(payload))], + is_error=True, + ) finally: _log_call(name, arguments, error_kind, (time.monotonic() - t0) * 1000) diff --git a/src/jobd/mcp/tools.py b/src/jobd/mcp/tools.py index 0620d22..21ce3fb 100644 --- a/src/jobd/mcp/tools.py +++ b/src/jobd/mcp/tools.py @@ -155,6 +155,9 @@ def jobd_submit(client: JobdClient, args: dict) -> dict: "job_id": resp["job_id"], "state": resp["state"], "project": resp.get("project"), + # null when it equals `project`; set when the broker scheduled the job + # under a different identity than the agent typed (fold or cwd root). + "project_label": resp.get("project_label"), "host_pin": resp.get("host_pin"), "queued_at": resp.get("queued_at"), } @@ -268,6 +271,10 @@ def jobd_preempt(client: JobdClient, args: dict) -> dict: _LIST_SUMMARY_FIELDS = ( "job_id", "project", + # The name as typed, when it differs from `project` (the broker's filter + # matches either, so a row can come back under an identity the agent never + # typed; without this column the substitution is invisible here). + "project_label", "state", "host", "exit_code", diff --git a/tests/mcp/test_protocol_roundtrip.py b/tests/mcp/test_protocol_roundtrip.py index f8ccaac..48528cb 100644 --- a/tests/mcp/test_protocol_roundtrip.py +++ b/tests/mcp/test_protocol_roundtrip.py @@ -113,3 +113,61 @@ async def go(): assert result.is_error assert "unknown tool" in result.content[0].text.lower() + + +@respx.mock +def test_missing_required_argument_is_an_error_result_not_a_protocol_crash(): + """audit 2026-09-02: `_call` caught transport and unknown-tool errors but + not the KeyError a missing required argument raises inside a tool, so + `jobd_status {}` -- the most common agent mistake -- reached the client as + a bare protocol-level 'Internal server error' with a traceback on stderr, + and the `is_error` + hint contract silently stopped applying.""" + respx.get("http://broker.test/jobs/7").mock( + return_value=httpx.Response(200, json={"job_id": 7, "state": "running"}) + ) + server = build_server(client=JobdClient(base_url="http://broker.test")) + + async def go(): + async with Client(server) as client: + bad_status = await client.call_tool("jobd_status", {}) + bad_submit = await client.call_tool("jobd_submit", {}) + good = await client.call_tool("jobd_status", {"job_id": 7}) + return bad_status, bad_submit, good + + bad_status, bad_submit, good = _run(go()) + + for bad in (bad_status, bad_submit): + assert bad.is_error, "a missing argument must be an is_error result" + payload = json.loads(bad.content[0].text) + assert payload["error"]["kind"] == "invalid_arguments" + assert "job_id" in json.loads(bad_status.content[0].text)["error"]["message"] + assert not good.is_error, "positive control failed - the harness itself is broken" + + +@respx.mock +def test_concurrent_calls_do_not_serialize_behind_one_slow_broker_request(): + """audit 2026-09-02: `_dispatch` ran the blocking httpx call ON the event + loop, so a `jobd_submit wait=true` (up to 270 s) stalled every other + request on the stdio session. Two 0.4 s broker calls issued together must + finish in well under 0.8 s.""" + import time + + def slow(request): + time.sleep(0.4) + return httpx.Response(200, json={"job_id": 7, "state": "running"}) + + respx.get("http://broker.test/jobs/7").mock(side_effect=slow) + server = build_server(client=JobdClient(base_url="http://broker.test")) + + async def go(): + async with Client(server) as client: + t0 = time.monotonic() + a, b = await asyncio.gather( + client.call_tool("jobd_status", {"job_id": 7}), + client.call_tool("jobd_status", {"job_id": 7}), + ) + return a, b, time.monotonic() - t0 + + a, b, elapsed = _run(go()) + assert not a.is_error and not b.is_error + assert elapsed < 0.7, f"two concurrent calls took {elapsed:.2f}s: dispatch is serialized" diff --git a/tests/mcp/test_tools.py b/tests/mcp/test_tools.py index 31b6cf8..735aa93 100644 --- a/tests/mcp/test_tools.py +++ b/tests/mcp/test_tools.py @@ -454,6 +454,7 @@ def test_jobd_list_summarizes_jobs(): assert set(out["jobs"][0].keys()) == { "job_id", "project", + "project_label", "state", "host", "exit_code", @@ -722,3 +723,54 @@ def test_call_tool_logs_error_kind_for_refusal(tmp_path, monkeypatch): entry = _json.loads((tmp_path / "calls.jsonl").read_text().strip().splitlines()[-1]) assert entry["error_kind"] == "cwd_outside_mount_roots" assert entry["tool"] == "jobd_submit" + + +@respx.mock +def test_jobd_list_carries_the_typed_label_beside_the_identity(): + """audit 2026-09-02 Q-3: the broker filter matches either name, so + `jobd_list(project="pillar2a1_sweep")` returned rows whose `project` was + `jepagame` with no field saying why. The summary hand-lists its columns; + the label has to be one of them or the substitution is invisible here.""" + _mock_jobs_endpoint( + [ + { + "id": 3, + "project": "jepagame", + "project_label": "pillar2a1_sweep", + "state": "queued", + "worker": None, + "exit_code": None, + "submitted_at": "2026-04-26T00:00:00+00:00", + "started_at": None, + } + ] + ) + from jobd.mcp.tools import jobd_list + + client = JobdClient(base_url="http://broker.test") + out = jobd_list(client, {"state": ["queued"]}) + assert out["jobs"][0]["project"] == "jepagame" + assert out["jobs"][0]["project_label"] == "pillar2a1_sweep" + + +@respx.mock +def test_submit_reports_the_typed_label_when_the_identity_was_substituted(): + """Same gap on the submit result: the agent typed one name and the broker + scheduled under another; the reply must carry both.""" + respx.post("http://broker.test/submit").mock( + return_value=httpx.Response( + 200, + json={ + "job_id": 7, + "state": "queued", + "project": "jepagame", + "project_label": "pillar2a1_sweep", + "host_pin": "any", + "queued_at": "2026-04-26T00:00:00Z", + }, + ) + ) + client = JobdClient(base_url="http://broker.test") + out = jobd_submit(client, {"command": "x", "project": "pillar2a1_sweep", "cwd": "/x"}) + assert out["project"] == "jepagame" + assert out["project_label"] == "pillar2a1_sweep" From 7e80d0ba4c9169d38e068031fc6626644be0dd1a Mon Sep 17 00:00:00 2001 From: Jaret Arnold <96366172+musharna@users.noreply.github.com> Date: Wed, 2 Sep 2026 12:42:55 -0400 Subject: [PATCH 03/10] fix(cli,api): explain shows the typed name on a fold; encode project names; list roots audit 2026-09-02 LOWs (quality 6, 8; security 5). - cli.py --explain: the 'submitted as' line lived only inside the matched_root branch, so a rule-1 fold never showed the typed spelling. - cli.py projects set/nudge: quote(name, safe='') so '../reload' can only address /projects/ (it used to run the reload and then claim a write). Test drives the real Typer command against the real app and checks that the on-disk config edit did NOT become visible. - broker/projects.py: _entry_to_yaml_dict emits roots (read surface only; _persist_projects writes priorities alone). cli.py list renders them. - docs/projects-yaml.md: matched_root is surfaced in three places, not two (the dry-run plan's validation.effective_matched_root was undocumented). 4 new tests, each watched failing first. --- changelog.d/audit-2026-09-02-cli.fixed.md | 1 + docs/projects-yaml.md | 10 +++--- src/job_cli/cli.py | 21 ++++++++--- src/jobd/broker/projects.py | 6 ++++ tests/test_cli_projects_write.py | 20 +++++++++++ tests/test_explain.py | 36 +++++++++++++++++++ tests/unit/test_projects_roots_are_visible.py | 35 ++++++++++++++++++ 7 files changed, 120 insertions(+), 9 deletions(-) create mode 100644 changelog.d/audit-2026-09-02-cli.fixed.md create mode 100644 tests/unit/test_projects_roots_are_visible.py diff --git a/changelog.d/audit-2026-09-02-cli.fixed.md b/changelog.d/audit-2026-09-02-cli.fixed.md new file mode 100644 index 0000000..c20db6d --- /dev/null +++ b/changelog.d/audit-2026-09-02-cli.fixed.md @@ -0,0 +1 @@ +- **`job submit --explain` names the typed spelling on a pure fold, `job projects set/nudge` percent-encode the project name, and `GET /projects` / `job projects list` show each project's `roots`.** The dry run printed the typed name only when cwd had supplied the identity, so `--project PROJECT-C` rendered the registered `project-c` and never what was typed. The write commands spliced the name into the URL raw, so `job projects set ../reload 5` normalised to `POST /reload`, ran a config reload, and then reported a write that never happened. And no runtime surface listed the roots a broker had actually loaded; the only way to learn one was a dry run from inside it. The runtime overlay still persists priorities alone, so exposing roots on the read surface cannot leak them into it. diff --git a/docs/projects-yaml.md b/docs/projects-yaml.md index 66d866b..b115ff1 100644 --- a/docs/projects-yaml.md +++ b/docs/projects-yaml.md @@ -756,11 +756,13 @@ debugging: the job that was priced as `beta` — though the rendered table column shows `project`, the scheduling identity. - `matched_root` — the root that supplied the identity — is **not** on the - Job row. It is computed at resolution time and surfaced in exactly two + Job row. It is computed at resolution time and surfaced in three places: the `POST /resolve` response (so `job submit --explain` prints - it, alongside the typed label) and the `cwd_identity_applied` event - recorded when rule 2 fires. It is `None`/absent whenever cwd was not - consulted. + it, alongside the typed label), the `validation.effective_matched_root` + field of a `job submit --dry-run` plan, and the `cwd_identity_applied` + event recorded when rule 2 fires. It is `None`/absent whenever cwd was + not consulted. The roots a broker has loaded are listed by + `GET /projects` / `job projects list`. ### The roots shipped in this file diff --git a/src/job_cli/cli.py b/src/job_cli/cli.py index 9bba976..7d36a38 100644 --- a/src/job_cli/cli.py +++ b/src/job_cli/cli.py @@ -10,6 +10,7 @@ from collections.abc import Callable from datetime import UTC, datetime, timedelta from typing import Any +from urllib.parse import quote import typer @@ -568,11 +569,16 @@ def _row(label: str, fr: dict, fmt: Callable[[Any], str] = str) -> None: # the surface built for that question unable to answer it. Rendered only # when rule 2 fired: `matched_root` is None whenever cwd was not consulted. matched_root = resolved.get("matched_root") + # Not `label`: that name is already bound to a `str` by the row loop + # above, and rebinding it to an `Any | None` is a type error. + submitted_as = resolved.get("project_label") if matched_root: - # Not `label`: that name is already bound to a `str` by the row loop - # above, and rebinding it to an `Any | None` is a type error. - submitted_as = resolved.get("project_label") typer.echo(f" identity from cwd: root {matched_root} (submitted as {submitted_as})") + elif submitted_as and submitted_as != resolved.get("project"): + # Rule 1 alone changed the name (`PROJECT-C` -> `project-c`). Without + # this line the dry run showed only the registered spelling and never + # what was typed (audit 2026-09-02). + typer.echo(f" identity by spelling: registered name (submitted as {submitted_as})") def _stream_wait(job_id: int) -> None: @@ -1142,6 +1148,9 @@ def projects_list(): bits.append(f"needs={list(req['needs'])}") if bits: extras = " defaults: " + " ".join(bits) + roots = entry.get("roots") if isinstance(entry, dict) else None + if roots: + extras += " roots=" + ",".join(roots) typer.echo(f"{name:>30} {pri:>3}{extras}") @@ -1150,7 +1159,9 @@ def projects_set(name: str, priority: int): """Set a project's priority (0-100). Persists as a runtime override; the projects.yaml baseline stays git-owned.""" with _client() as c: - r = c.post(f"/projects/{name}", json={"priority": priority}) + # Percent-encoded: spliced raw, `../reload` normalised to `/reload` and + # ran the config reload instead of a write (audit 2026-09-02). + r = c.post(f"/projects/{quote(name, safe='')}", json={"priority": priority}) r.raise_for_status() _echo_project_write(name, r.json()) @@ -1160,7 +1171,7 @@ def projects_nudge(name: str, delta: int): """Adjust a project's priority by DELTA (may be negative), clamped to 0-100. Persists as a runtime override.""" with _client() as c: - r = c.post(f"/projects/{name}/nudge", json={"delta": delta}) + r = c.post(f"/projects/{quote(name, safe='')}/nudge", json={"delta": delta}) r.raise_for_status() _echo_project_write(name, r.json()) diff --git a/src/jobd/broker/projects.py b/src/jobd/broker/projects.py index e76a9a6..4ed74bd 100644 --- a/src/jobd/broker/projects.py +++ b/src/jobd/broker/projects.py @@ -47,6 +47,12 @@ def _entry_to_yaml_dict(entry: ProjectEntry) -> dict: defaults_dict["escalate_to_arc"] = d.escalate_to_arc if defaults_dict: out["defaults"] = defaults_dict + if entry.roots: + # Read surface only: `_persist_projects` writes priorities alone, so + # exposing roots here cannot leak them into the overlay. Without this + # no runtime surface showed which roots the broker had loaded + # (audit 2026-09-02). + out["roots"] = list(entry.roots) return out diff --git a/tests/test_cli_projects_write.py b/tests/test_cli_projects_write.py index c305c8d..fcffdec 100644 --- a/tests/test_cli_projects_write.py +++ b/tests/test_cli_projects_write.py @@ -116,3 +116,23 @@ def test_an_old_broker_that_folded_says_so_rather_than_crashing(capsys): out = _echo("PROJECT_B", {"project-b": {"priority": 90}}, capsys) assert "90" not in out, f"must not attribute another project's priority: {out!r}" assert "does not report" in out + + +def test_a_project_name_with_a_slash_cannot_reach_another_route(cli, app, sample_projects_yaml): + """audit 2026-09-02: the CLI spliced the name into the URL path unencoded, + so `job projects set ../reload 5` POSTed to `/reload` -- the config reload + ran and the command then reported a write that never happened. The name + must be percent-encoded so it can only ever address `/projects/`.""" + # If a reload fires, this edit becomes visible on GET /projects. + sample_projects_yaml.write_text( + "projects:\n project-b: { priority: 99 }\n _default: { priority: 40 }\n" + ) + result = cli.invoke(cli_mod.app, ["projects", "set", "../reload", "5"]) + assert result.exit_code != 0, result.output + with TestClient(app) as c: + assert c.get("/projects").json()["project-b"]["priority"] == 80, ( + "the write escaped to /reload" + ) + # Positive control in the same test: a legitimate write still lands. + out = _run(cli, "set", "project-b", "5") + assert "project-b -> 5" in out diff --git a/tests/test_explain.py b/tests/test_explain.py index 7d7297a..07ac997 100644 --- a/tests/test_explain.py +++ b/tests/test_explain.py @@ -172,3 +172,39 @@ def post(self, path, *, json=None, params=None): assert "desktop" in r.output assert "[source: project default]" in r.output assert "max_wall_s" in r.output + + +def test_explain_names_the_typed_spelling_when_only_the_fold_changed_it( + client_with_defaults, monkeypatch +): + """audit 2026-09-02: the typed name was printed only inside the + `if matched_root:` branch, so a rule-1 fold (`PROJECT-C` -> `project-c`) + rendered `resolved config for project project-c` and never said what the + operator had actually typed -- the CHANGELOG claimed both were rendered.""" + import job_cli.cli as cli_mod + + real_client = client_with_defaults + + class _ExplainFakeClient: + def __enter__(self): + return self + + def __exit__(self, *_a): + pass + + def post(self, path, *, json=None, params=None): + return real_client.post("/resolve", json=json) + + monkeypatch.setattr(cli_mod, "_client", lambda: _ExplainFakeClient()) + r = CliRunner().invoke( + cli_mod.app, ["submit", "--project", "PROJECT-C", "--explain", "--", "./run.sh"] + ) + assert r.exit_code == 0, r.output + assert "resolved config for project project-c" in r.output + assert "submitted as PROJECT-C" in r.output + # Positive control: an exact spelling has nothing to explain. + r2 = CliRunner().invoke( + cli_mod.app, ["submit", "--project", "project-c", "--explain", "--", "./run.sh"] + ) + assert r2.exit_code == 0, r2.output + assert "submitted as" not in r2.output diff --git a/tests/unit/test_projects_roots_are_visible.py b/tests/unit/test_projects_roots_are_visible.py new file mode 100644 index 0000000..9bdbdb0 --- /dev/null +++ b/tests/unit/test_projects_roots_are_visible.py @@ -0,0 +1,35 @@ +"""audit 2026-09-02: `GET /projects` (and so `job projects list`) never +serialised `roots`, so no runtime surface showed which directories the broker +had actually loaded -- the only way to learn a root was to submit a dry run +from inside it. The overlay writer is unaffected: it persists priorities only. +""" + +from starlette.testclient import TestClient +from typer.testing import CliRunner + +import job_cli.cli as cli_mod + + +def test_get_projects_reports_each_projects_roots(rooted_client): + body = rooted_client.get("/projects").json() + assert body["jepagame"]["roots"] == ["/home/mjarnold/jepagame"] + assert body["orchid-sdxl"]["roots"] == ["/home/mjarnold/orchid-sdxl"] + # A project without roots does not grow an empty key. + assert "roots" not in body["_default"] + + +def test_job_projects_list_renders_roots(rooted_app, monkeypatch): + class _AppClient: + def __enter__(self): + self._c = TestClient(rooted_app) + return self._c + + def __exit__(self, *exc): + self._c.close() + return False + + monkeypatch.setattr(cli_mod, "_client", _AppClient) + r = CliRunner().invoke(cli_mod.app, ["projects", "list"]) + assert r.exit_code == 0, r.output + jepagame_line = next(line for line in r.output.splitlines() if "jepagame" in line) + assert "roots=/home/mjarnold/jepagame" in jepagame_line From cf14e557edb03f7e31abb2e72a301050bee5734f Mon Sep 17 00:00:00 2001 From: Jaret Arnold <96366172+musharna@users.noreply.github.com> Date: Wed, 2 Sep 2026 12:47:10 -0400 Subject: [PATCH 04/10] test(ci): floors for derived guards, editable-install guard, fold-collision coverage audit 2026-09-02 T-1, T-2, T-3 plus tests/CI LOWs. - test_corpus_replay.py: arm 2 gains a non-empty floor counted in jobs (63 pairs / 1129 jobs measured); with canonical_project_name disabled it passed while checking zero rows -- the same fail-open shape as the 07-25 route-table guard. New parity test pins ROOTS to config/projects.yaml (681f60b's drift can no longer recur silently). - test_project_key_collisions.py: the ambiguous-fold branch and the load-time collision warning were executed by no test; a mutation picking the first match survived the suite. Both now killed (verified). - test_deploy_lint.py: the suite must import jobd/job_cli from /src. A non-editable wheel in the venv made every local run test the installed snapshot (0% coverage, vacuous mutation checks). Coverage floor value now lives in pyproject [tool.coverage.report] and the lint checks ci.yml agrees. - test_packaging.py: drop the duplicate server.json version test (the deploy lint already has it). - ci.yml: -rs so always-skipped systemd-scope tests are visible. codeql.yml: scan the 'actions' language too. release.yml: top-level read-only permissions (publishing jobs keep their own blocks); v7.0.1 comment drift fixed. --- .github/workflows/ci.yml | 6 ++-- .github/workflows/codeql.yml | 4 ++- .github/workflows/release.yml | 8 ++++- pyproject.toml | 5 +++ tests/test_corpus_replay.py | 17 +++++++++++ tests/test_deploy_lint.py | 26 ++++++++++++++++ tests/test_packaging.py | 11 ------- tests/unit/test_project_key_collisions.py | 37 +++++++++++++++++++++++ 8 files changed, 99 insertions(+), 15 deletions(-) create mode 100644 tests/unit/test_project_key_collisions.py diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 3d23560..62ecb06 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -59,7 +59,9 @@ jobs: - name: Tests (excluding live broker) if: matrix.python-version != '3.12' - run: uv run pytest -m "not live" -q + # -rs: print each skip's reason. Real-execution tests that need a + # systemd --user scope skip on GitHub runners; silent skips hid that. + run: uv run pytest -m "not live" -q -rs # One matrix leg also enforces a coverage floor (term-missing report, # no external uploader). BRANCH coverage — line coverage alone lets an @@ -70,7 +72,7 @@ jobs: # slowest step buys nothing. - name: Tests with coverage gate (excluding live broker) if: matrix.python-version == '3.12' - run: uv run pytest -m "not live" -q --cov=src/jobd --cov=src/job_cli --cov-branch --cov-report=term-missing --cov-fail-under=83 + run: uv run pytest -m "not live" -q -rs --cov=src/jobd --cov=src/job_cli --cov-branch --cov-report=term-missing --cov-fail-under=83 # mypy is clean and now gates the build: type regressions fail CI. - name: Types (mypy) diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index 4c237e4..8e60dee 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -20,7 +20,9 @@ jobs: strategy: fail-fast: false matrix: - language: ["python"] + # `actions` scans the workflows themselves (untrusted-input injection, + # unpinned actions, over-broad permissions) — six of them ship here. + language: ["python", "actions"] steps: - name: Checkout uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 95572e6..56e1685 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -15,6 +15,12 @@ on: push: tags: ["v*"] +# Read-only default for every job; the three that publish (pypi, docker, +# github-release) declare their own narrower-but-writable blocks below, which +# REPLACE this one rather than extend it. +permissions: + contents: read + env: # See ci.yml: `uv run` re-locks a stale lockfile unless told not to, which # would let the test gate below test something other than the committed lock. @@ -140,7 +146,7 @@ jobs: permissions: contents: write # create the release + upload assets steps: - - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.0 (for CHANGELOG.md) + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 (for CHANGELOG.md) - uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8.0.1 with: name: dist diff --git a/pyproject.toml b/pyproject.toml index b5d4429..8638603 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -123,3 +123,8 @@ extend-immutable-calls = ["typer.Argument", "typer.Option"] [tool.ruff.lint.per-file-ignores] # Nested `with patch.object(...)` is a readable, idiomatic test pattern. "tests/**" = ["SIM117"] + +[tool.coverage.report] +# Branch-coverage floor enforced by ci.yml's coverage leg (the workflow flag +# must agree; tests/test_deploy_lint.py checks). Measured 86.8% on 2026-09-02. +fail_under = 83 diff --git a/tests/test_corpus_replay.py b/tests/test_corpus_replay.py index 93b911e..f40a75d 100644 --- a/tests/test_corpus_replay.py +++ b/tests/test_corpus_replay.py @@ -51,6 +51,7 @@ from jobd.config import ( ProjectEntry, canonical_project_name, + load_projects, project_from_cwd, resolve_effective_config, resolve_priority, @@ -139,10 +140,12 @@ def test_arm2_no_currently_registered_submit_changes_priority(corpus, projects): registered project today must keep its exact priority, through the real resolver.""" changed = [] + checked_jobs = 0 for typed, cwd, n in corpus: before = canonical_project_name(projects, typed) if before not in projects: continue # not a rule-1 job; arm 1's business + checked_jobs += n eff = _eff(projects, typed, cwd) expected_priority = resolve_priority(projects, before, 0) if eff.project != before or eff.priority.value != expected_priority: @@ -150,6 +153,20 @@ def test_arm2_no_currently_registered_submit_changes_priority(corpus, projects): (typed, cwd, before, eff.project, eff.priority.value, expected_priority, n) ) assert not changed, f"{len(changed)} registered submits were repriced: {changed[:5]}" + # Non-empty floor (audit 2026-09-02 T-2): with `canonical_project_name` + # broken so that nothing registers, every row takes the `continue` above + # and `changed` is empty -- this arm passed while checking NOTHING. Counted + # in JOBS like arm 1 (63 rule-1 pairs / 1129 jobs measured 2026-09-02); a + # derived guard needs a floor or it fails open. + assert checked_jobs >= 1000, f"arm 2 checked only {checked_jobs} rule-1 jobs" + + +def test_the_roots_table_mirrors_the_shipped_config(): + """`ROOTS` claims to mirror config/projects.yaml, and an earlier version + listed three of six roots under that same claim (681f60b). Assert it, so + the next drift fails here instead of silently narrowing arm 1.""" + shipped = load_projects(Path(__file__).resolve().parent.parent / "config" / "projects.yaml") + assert {name: entry.roots for name, entry in shipped.items() if entry.roots} == ROOTS def test_arm1_jobs_inside_a_rooted_project_now_get_an_identity(corpus, projects): diff --git a/tests/test_deploy_lint.py b/tests/test_deploy_lint.py index 0e4821b..ca7007f 100644 --- a/tests/test_deploy_lint.py +++ b/tests/test_deploy_lint.py @@ -756,6 +756,32 @@ def test_ci_enforces_a_branch_coverage_floor(): assert any("--cov-branch" in r for r in gated), ( "the coverage gate no longer measures branch coverage" ) + # The VALUE lives in pyproject ([tool.coverage.report] fail_under) so it is + # discoverable from the repo, not only from a workflow line; the workflow + # flag must agree with it (audit 2026-09-02). + import tomllib + + floor = tomllib.loads(_PYPROJECT.read_text())["tool"]["coverage"]["report"]["fail_under"] + flags = {int(m) for r in gated for m in re.findall(r"--cov-fail-under=(\d+)", r)} + assert flags == {floor}, f"ci.yml floors {flags} disagree with pyproject fail_under={floor}" + + +def test_the_suite_imports_jobd_from_the_working_tree(): + """audit 2026-09-02 T-1: a non-editable `jobd` wheel in the venv made every + local pytest run test the installed SNAPSHOT, not src/ -- nine mutation + checks passed vacuously and a coverage run read 0%. CI installs editable + (uv.lock: `source = { editable = "." }`); this pins that shape wherever the + suite runs, and points at the fix when it does not hold.""" + import job_cli + import jobd + + src = _REPO_ROOT / "src" + for mod in (jobd, job_cli): + where = Path(mod.__file__).resolve() + assert src in where.parents, ( + f"{mod.__name__} imports from {where}, not {src}: the venv holds a " + f"non-editable install. Run `uv sync --extra dev --extra worker --extra mcp`." + ) def test_workflow_run_blocks_do_not_splice_github_expressions(): diff --git a/tests/test_packaging.py b/tests/test_packaging.py index 8172f1b..cb1c366 100644 --- a/tests/test_packaging.py +++ b/tests/test_packaging.py @@ -23,17 +23,6 @@ _SERVER_JSON = json.loads((_ROOT / "server.json").read_text()) -def test_server_json_version_matches_pyproject() -> None: - expected = _PYPROJECT["project"]["version"] - assert _SERVER_JSON["version"] == expected, ( - f"server.json top-level version {_SERVER_JSON['version']!r} != pyproject {expected!r}" - ) - for i, pkg in enumerate(_SERVER_JSON.get("packages", [])): - assert pkg["version"] == expected, ( - f"server.json packages[{i}].version {pkg['version']!r} != pyproject {expected!r}" - ) - - def test_server_json_identity() -> None: assert _SERVER_JSON["name"] == "io.github.musharna/jobd" identifiers = {p.get("identifier") for p in _SERVER_JSON.get("packages", [])} diff --git a/tests/unit/test_project_key_collisions.py b/tests/unit/test_project_key_collisions.py new file mode 100644 index 0000000..67cd396 --- /dev/null +++ b/tests/unit/test_project_key_collisions.py @@ -0,0 +1,37 @@ +"""audit 2026-09-02 T-3: the two branches that handle registered names which +fold onto the same key (`Foo` and `foo`) were executed by no test, so a +mutation that picked the first match -- routing one project's jobs at its +neighbour's priority, the failure this matching exists to end -- survived the +whole suite. +""" + +import logging + +from jobd.config import ProjectEntry, canonical_project_name, load_effective_projects + + +def test_an_ambiguous_fold_degrades_to_exact_matching_and_says_so(caplog): + projects = { + "_default": ProjectEntry(priority=40), + "Foo": ProjectEntry(priority=90), + "foo": ProjectEntry(priority=10), + } + with caplog.at_level(logging.WARNING, logger="jobd.config"): + assert canonical_project_name(projects, "FOO") == "FOO" + assert "folds onto more than one" in caplog.text + # Positive control: an exact spelling is still found without a warning. + caplog.clear() + with caplog.at_level(logging.WARNING, logger="jobd.config"): + assert canonical_project_name(projects, "Foo") == "Foo" + assert caplog.text == "" + + +def test_loading_a_table_with_colliding_names_warns(tmp_path, caplog): + projects = tmp_path / "projects.yaml" + projects.write_text( + "projects:\n arf-promoter: { priority: 60 }\n arf_promoter: { priority: 50 }\n" + ) + with caplog.at_level(logging.WARNING, logger="jobd.config"): + load_effective_projects(projects, tmp_path / "overrides.yaml") + assert "differ only by case or -/_" in caplog.text + assert "arf-promoter" in caplog.text and "arf_promoter" in caplog.text From 877930155e042a973712089912dd07b08dbc0865 Mon Sep 17 00:00:00 2001 From: Jaret Arnold <96366172+musharna@users.noreply.github.com> Date: Wed, 2 Sep 2026 12:48:21 -0400 Subject: [PATCH 05/10] docs: shipped-status banners, event catalog, concept-only DOI; scrub a tailnet address MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit audit 2026-09-02 S-1, Q-1, Q-2, Q-4 plus docs LOWs. - docs/plans/...-plan.md:1138 carried the maintainer's ssh login and tailnet IP (missed by #121's scrub, regressing the PL-3 decision that rested on none being in the published tree). Generic host now. The maintainer's DB path is gone from the plan, the design doc, and scripts/export_project_cwd_corpus.py, which now requires JOBD_DB. - CITATION.cff: the per-release DOI named the v0.5.38 record beside version 0.5.42. It cannot be right on a release commit (Zenodo mints it afterwards), so only the concept DOI remains, with the reasoning inline. - Design doc said 'approved, not yet planned'; plan had 44 unchecked boxes and no shipped marker; docs/projects-yaml.md was titled 'Plan' with stale line refs above the current §10 spec; README link text named a path that does not exist. Banners, retitle, and a historical-sections note. - docs/events.md: the six event names added in the delta had no catalog. Every KNOWN_EVENTS name is now documented, pinned two-way by tests/test_events_catalog_doc.py (with a non-empty floor). - config/projects.yaml: header states the decision to track the maintainer's real roots and what a stranger must replace; the jepagame comment no longer overclaims what the corpus shows. --- CITATION.cff | 8 ++- README.md | 2 +- changelog.d/audit-2026-09-02-broker.fixed.md | 2 +- changelog.d/audit-2026-09-02-docs.changed.md | 1 + config/projects.yaml | 17 ++++- docs/events.md | 72 +++++++++++++++++++ .../2026-08-31-cwd-project-identity-design.md | 7 +- .../2026-08-31-cwd-project-identity-plan.md | 7 +- docs/projects-yaml.md | 15 ++-- scripts/export_project_cwd_corpus.py | 7 +- tests/test_events_catalog_doc.py | 26 +++++++ 11 files changed, 148 insertions(+), 16 deletions(-) create mode 100644 changelog.d/audit-2026-09-02-docs.changed.md create mode 100644 docs/events.md create mode 100644 tests/test_events_catalog_doc.py diff --git a/CITATION.cff b/CITATION.cff index db8236e..ca2ef7f 100644 --- a/CITATION.cff +++ b/CITATION.cff @@ -14,13 +14,15 @@ authors: given-names: Jaret orcid: "https://orcid.org/0009-0003-4055-5238" doi: "10.5281/zenodo.21636369" +# Only the concept DOI is recorded here. Zenodo mints a version DOI AFTER the +# GitHub release is published, so a "DOI for this specific release" in this +# file is stale by construction on the commit that ships the release (it named +# the v0.5.38 record beside version 0.5.42 until the 2026-09-02 audit). Each +# release's own DOI is listed on the Zenodo record the concept DOI resolves to. identifiers: - type: doi value: "10.5281/zenodo.21636369" description: "Concept DOI — always resolves to the latest release." - - type: doi - value: "10.5281/zenodo.21831629" - description: "DOI for this specific release." repository-code: "https://github.com/musharna/jobd" url: "https://pypi.org/project/jobd/" license: MIT diff --git a/README.md b/README.md index 6a54ff0..8efdfd5 100644 --- a/README.md +++ b/README.md @@ -227,7 +227,7 @@ Now an agent can "run this overnight," check on it next session, and route GPU w Three optional YAML files under `JOBD_CONFIG_DIR` (defaults shipped in `config/`): -- **`projects.yaml`** — per-project base priority and submit defaults (preemptibility, wall/idle timeouts, host pins, capability requirements). See [docs/plans/projects-yaml.md](https://github.com/musharna/jobd/blob/main/docs/projects-yaml.md) for the full resolution model. +- **`projects.yaml`** — per-project base priority and submit defaults (preemptibility, wall/idle timeouts, host pins, capability requirements). Entries may also declare `roots:` so a job typed with an unregistered run label is priced by the project whose directory it runs in. See [docs/projects-yaml.md](https://github.com/musharna/jobd/blob/main/docs/projects-yaml.md) for the full resolution model and [docs/events.md](https://github.com/musharna/jobd/blob/main/docs/events.md) for the event catalog. - **`profiles.yaml`** — named resource bundles (`--profile gpu-train-large`) the matcher uses to size a job. - **`classifier.yaml`** — rules that auto-suggest a profile from the command string. diff --git a/changelog.d/audit-2026-09-02-broker.fixed.md b/changelog.d/audit-2026-09-02-broker.fixed.md index cf31844..55bd95a 100644 --- a/changelog.d/audit-2026-09-02-broker.fixed.md +++ b/changelog.d/audit-2026-09-02-broker.fixed.md @@ -1,3 +1,3 @@ -- **`job list --project` and `/events?project=` now fold the name the way submit does.** Submit stores a job under its registered spelling (`JEPAGAME` prices as, and is recorded as, `jepagame`), but both read filters compared the raw string, so no single spelling returned all of a project's jobs and a query for `Project_A` found no event written after the fold landed. The filter now matches the folded identity, the typed spelling, and the typed run label. `project_label` gains an index (created by `migrate()` on in-place upgrades too) since the list filter ORs on it. +- **`job list --project` and `/events?project=` now fold the name the way submit does.** Submit stores a job under its registered spelling (`BETA` prices as, and is recorded as, `beta`), but both read filters compared the raw string, so no single spelling returned all of a project's jobs and a query for `Project_A` found no event written after the fold landed. The filter now matches the folded identity, the typed spelling, and the typed run label. `project_label` gains an index (created by `migrate()` on in-place upgrades too) since the list filter ORs on it. - **A project declared with `roots:` but no `priority:` is a load error, not a silent drop.** `load_projects` skipped any entry lacking `priority` before the roots validation ran, so `name: {roots: [...]}` vanished with no log line — the invisible removal the roots validation promises to prevent. `_default` may no longer declare roots either (they were validated, accepted, and never consulted). A leading `//` in a root or cwd is collapsed onto `/`; previously `//home/x` was a valid root that could never match. - **`/reload` takes the projects mutation lock.** A `set`/`nudge` that landed between reload's re-read and its swap of the projects table was persisted from the new table and lost. diff --git a/changelog.d/audit-2026-09-02-docs.changed.md b/changelog.d/audit-2026-09-02-docs.changed.md new file mode 100644 index 0000000..d6ab4a1 --- /dev/null +++ b/changelog.d/audit-2026-09-02-docs.changed.md @@ -0,0 +1 @@ +- **Docs and metadata caught up with what shipped.** `CITATION.cff` records only the concept DOI: the "DOI for this specific release" entry named the v0.5.38 Zenodo record beside `version: 0.5.42`, and it is stale by construction because Zenodo mints a version DOI only after the GitHub release exists. The cwd-identity design and plan under `docs/plans/` carry "shipped in v0.5.42" banners (the plan's 44 unchecked boxes were never status), `docs/projects-yaml.md` is retitled from "Plan" and says which sections are historical, and the README's link text matches its target. New `docs/events.md` catalogs every event name in `KNOWN_EVENTS` with a one-line meaning, pinned two-way by a test. `config/projects.yaml`'s header says plainly that every name and path in it is a pseudonym, where the real ones go (`projects.local.yaml`), and what a fresh clone must replace. A real tailnet address and login that the privacy scrub missed in the plan doc, and the maintainer's database path hard-coded in the corpus export script, are gone; the script now requires `JOBD_DB`. diff --git a/config/projects.yaml b/config/projects.yaml index 716a008..001a12a 100644 --- a/config/projects.yaml +++ b/config/projects.yaml @@ -2,6 +2,18 @@ # This file is OPTIONAL — with no projects.yaml the broker uses the global # default priority for every job. Edit freely for your own projects. # +# WHAT IS IN HERE: every name and path in this file is a PSEUDONYM. The +# `project-a..d` rows are placeholders, and the `alpha`/`beta`/`gamma`/`delta` +# entries with `roots:` mirror the maintainer's fleet with names and home +# directory rewritten, so that CI can replay 3,608 rows of real job history +# (tests/data/project_cwd_corpus.csv, rewritten the same way) through +# tests/test_corpus_replay.py without publishing where anything really lives. +# The real names and roots go in `config/projects.local.yaml` beside this file +# -- gitignored, and read by the broker as an overlay that replaces same-named +# entries and adds new ones (docs/projects-yaml.md, "projects.local.yaml"). +# If you cloned jobd to run your own broker: put YOUR projects there, or +# replace the entries below; `docker-compose.yml` bind-mounts ./config. +# # Change priorities live (persists + reloads) with: # job projects set NAME PRIORITY projects: @@ -36,8 +48,9 @@ projects: roots: # 187 jobs directly in /home/user/beta plus several hundred more # in its .claude/worktrees/* and .worktrees/* subdirectories (matched - # via the same root, component-wise) — every typed name is `beta` - # or a `pillarN...` sweep label. + # via the same root, component-wise). Typed names are `beta`, the + # `pillarN...` sweep labels, and a few others (`synthetic-souls`, + # `ss-1l-emerge`, `beta-2c-*`) that all read as this one project. - /home/user/beta # 12 jobs, one typed label (`pillar1l-emerge`), unregistered — every one # of them falls to _default today. A sibling of the root above (not diff --git a/docs/events.md b/docs/events.md new file mode 100644 index 0000000..7a4fdd7 --- /dev/null +++ b/docs/events.md @@ -0,0 +1,72 @@ +# Event catalog + +Every event the broker or a worker records in `events.jsonl`, by name. The +list is pinned to `KNOWN_EVENTS` in `src/jobd/models.py` by +`tests/test_events_catalog_doc.py`; an event listed in one place but not the +other fails CI. Read them with `job events`, `GET /events`, or the MCP +`jobd_events` tool (whose `event` enum is derived from the same constant). + +A name that is NOT in this catalog still reaches `events.jsonl` (hooks may +emit their own), but its Prometheus counter collapses into the `other` +bucket, so an alert written against it never fires. + +## Job lifecycle (broker) + +| Event | Meaning | +|---|---| +| `job_submitted` | A job row was created (one per array member). | +| `job_dispatched` | The dispatcher assigned the job to a worker. | +| `job_started` | The worker reported the workload running. | +| `job_completed` | The workload reached a terminal state the worker reported (completed / failed / exit code carried in the payload). | +| `job_cancelled` | The job was cancelled by request or by a dependency cascade. | +| `job_orphaned` | Its worker died or restarted while it was in flight; the job is parked for possible resurrection. | +| `job_resurrected` | An orphaned job was re-queued after its worker came back. | +| `job_uncancelled` | A dependency-cascade cancel was reversed because the parent came back. | +| `scheduling_timeout` | The job waited longer than its `scheduling_timeout_s` without a matching worker and was failed. | +| `dispatch_skip` | A dispatch pass considered the job and passed it over (payload names the reason, e.g. no worker advertises a required tag). | +| `admission_blocked` | Admission control refused to dispatch (quota, contention, or exclusion). | +| `auto_preempt` | A higher-priority job caused a preemptible one to be preempted. | +| `checkpoint_complete` | The worker confirmed the workload checkpointed inside its grace window. | +| `cwd_refused` | Submit refused the job because its `cwd` cannot be reached from any eligible worker. | + +## Submit-time warnings (broker) + +`submit_warning` fires once per job whatever warned; the per-cause events +below fire beside it so an alert can name a single cause. + +| Event | Meaning | +|---|---| +| `submit_warning` | The submit response carried at least one warning. | +| `unknown_project` | The typed `--project` was not registered and no `roots:` entry identified `cwd`; the job is priced at `_default`. | +| `preflight_warning` | The preflight check on the command or environment reported something non-fatal. | +| `cwd_route_warning` | `cwd` is reachable from some but not all otherwise-eligible workers. | +| `serialization_warning` | A `depends_on` target was already terminal when the job was submitted. | +| `gpu_contention_warning` | The requested GPU is currently held by another process on the pinned host. | +| `sweep_warning` | The post-commit TOCTOU sweep found and cascaded a dependency that failed during submit. | +| `cwd_identity_applied` | The typed name was not registered, and a project's `roots:` supplied the scheduling identity. Not a warning: payload carries `project`, `project_label`, and `matched_root`. | + +## Sweeper and retention (broker) + +| Event | Meaning | +|---|---| +| `reclaim_suppressed` | The sweep declined its time-based terminal phases because the broker had not been observing the interval (fresh start or a suspend/stall gap). | +| `jobs_pruned` | Terminal job rows older than the retention window were deleted. | +| `logs_pruned` | Job log files older than the retention window were deleted. | +| `env_scrubbed` | Environment blobs on terminal jobs were replaced with `***` after the at-rest window. | + +## Worker lifecycle (broker-observed) + +| Event | Meaning | +|---|---| +| `worker_registered` | A worker registered or re-registered. | +| `worker_offline` | A worker missed heartbeats past the offline threshold. | +| `worker_stale` | A worker missed heartbeats past the stale threshold (still expected back). | +| `version_drift` | A worker has run a different version than the broker for the full drift window. | + +## Worker-posted + +| Event | Meaning | +|---|---| +| `worker_shutdown` | The worker is draining and exiting. | +| `watchdog_fired` | The worker's watchdog escalated a workload that ignored SIGTERM to SIGKILL. | +| `stale_scope_sweep` | The worker found and cleaned a leftover systemd scope from a previous run. | diff --git a/docs/plans/2026-08-31-cwd-project-identity-design.md b/docs/plans/2026-08-31-cwd-project-identity-design.md index cb4cf7c..f4c745a 100644 --- a/docs/plans/2026-08-31-cwd-project-identity-design.md +++ b/docs/plans/2026-08-31-cwd-project-identity-design.md @@ -1,6 +1,9 @@ # cwd-derived project identity -Design, 2026-08-31. Status: approved, not yet planned. +Design, 2026-08-31. **Status: shipped in v0.5.42 (2026-09-01).** Historical design +record; the shipped behaviour diverged in places (lexical `..` collapse, the +stderr substitution note's wording, `matched_root` in the dry-run plan). The +current specification is `docs/projects-yaml.md` §10. ## The problem @@ -21,7 +24,7 @@ v0.5.39–41 closed the _spelling_ half of this (`epsilon` now folds onto `epsilon`; a write reports the name it landed on). The retyped-string mechanism itself is untouched, and it is the larger half. -### Measured, live DB `/home/user/jobd/data/jobd.db`, 2026-08-31 +### Measured, live broker DB, 2026-08-31 3,608 job rows; 249 distinct `(project, cwd)` pairs; 138 distinct project names against 148 distinct cwds. diff --git a/docs/plans/2026-08-31-cwd-project-identity-plan.md b/docs/plans/2026-08-31-cwd-project-identity-plan.md index 887591b..e3ed557 100644 --- a/docs/plans/2026-08-31-cwd-project-identity-plan.md +++ b/docs/plans/2026-08-31-cwd-project-identity-plan.md @@ -1,5 +1,10 @@ # cwd-derived project identity — Implementation Plan +> **Historical.** Executed and shipped in v0.5.42 (2026-09-01). The checkboxes +> below were not maintained during execution and do not reflect status. For +> what actually shipped, read `docs/projects-yaml.md` §10 and the 0.5.42 +> section of `CHANGELOG.md`. + > **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. **Goal:** Let a job's scheduling identity be derived from its `cwd` when the typed `--project` name is not a registered project, so the 241 measured jobs that ran at `_default` 40 inside a registered project's directory get that project's priority. @@ -1121,7 +1126,7 @@ signal.signal( ) signal.alarm(60) -DB = os.environ.get("JOBD_DB", "/home/user/jobd/data/jobd.db") +DB = os.environ["JOBD_DB"] # e.g. /srv/jobd/data/jobd.db on the broker host rows = sqlite3.connect(f"file:{DB}?mode=ro", uri=True).execute( "SELECT project, cwd, COUNT(*) FROM jobs GROUP BY project, cwd ORDER BY project, cwd" ).fetchall() diff --git a/docs/projects-yaml.md b/docs/projects-yaml.md index b115ff1..415680b 100644 --- a/docs/projects-yaml.md +++ b/docs/projects-yaml.md @@ -1,8 +1,13 @@ -# Plan: Per-project defaults file (`projects.yaml` enforcement) +# `projects.yaml` — per-project priorities, defaults, and roots -Design notes for the per-project `defaults:` feature. Read alongside the -live source files (`src/jobd/app.py`, `src/jobd/config.py`, -`src/jobd/models.py`, `src/job_cli/cli.py`). +Sections 1–9 are the original design notes for the per-project `defaults:` +feature (2026-07) and are kept as the rationale record: their `path:line` +references and effort estimates are historical, and submit now lives in +`src/jobd/broker/submit.py` with the precedence cascade in +`src/jobd/config.py:resolve_effective_config`. Section 10 is the current +specification for `roots:` (cwd-derived project identity, shipped v0.5.42). +Read alongside the live source: `src/jobd/config.py`, `src/jobd/models.py`, +`src/job_cli/cli.py`. --- @@ -12,7 +17,7 @@ live source files (`src/jobd/app.py`, `src/jobd/config.py`, `projects.yaml` already lives on the broker host at the path passed to `build_app(projects_path=...)`. In production this resolves to -`/app/config/projects.yaml` (controlled by `JOBD_CONFIG_DIR` in `main.py:13`). +`/app/config/projects.yaml` (controlled by `JOBD_CONFIG_DIR`, read in `main.py`). In the Docker stack this maps to `/srv/jobd/config/projects.yaml` on the broker host, bind-mounted into the container. diff --git a/scripts/export_project_cwd_corpus.py b/scripts/export_project_cwd_corpus.py index ba377b8..96878cb 100644 --- a/scripts/export_project_cwd_corpus.py +++ b/scripts/export_project_cwd_corpus.py @@ -25,7 +25,12 @@ ) signal.alarm(60) -DB = os.environ.get("JOBD_DB", "/home/user/jobd/data/jobd.db") +DB = os.environ.get("JOBD_DB") +if not DB: + # No default: the only sensible one was the maintainer's own path, and a + # script that silently opens someone else's database is worse than one + # that asks. Fail loud. + sys.exit("set JOBD_DB=/path/to/jobd.db (read-only open) before running this export") rows = ( sqlite3.connect(f"file:{DB}?mode=ro", uri=True) .execute("SELECT project, cwd, COUNT(*) FROM jobs GROUP BY project, cwd ORDER BY project, cwd") diff --git a/tests/test_events_catalog_doc.py b/tests/test_events_catalog_doc.py new file mode 100644 index 0000000..26a301f --- /dev/null +++ b/tests/test_events_catalog_doc.py @@ -0,0 +1,26 @@ +"""Every name in KNOWN_EVENTS must be documented in docs/events.md. + +audit 2026-09-02: six event names landed in the delta with no catalog anywhere +but the source constant and the changelog. An operator writing an alert has to +know what `reclaim_suppressed` or `cwd_identity_applied` means; the MCP +`jobd_events` enum derives from KNOWN_EVENTS, so agents saw the names while +humans had nowhere to read them. +""" + +import re +from pathlib import Path + +from jobd.models import KNOWN_EVENTS + +_DOC = Path(__file__).resolve().parent.parent / "docs" / "events.md" + + +def test_every_known_event_is_documented(): + assert _DOC.exists(), "docs/events.md is missing" + documented = set(re.findall(r"^\| `([a-z_]+)`", _DOC.read_text(), re.M)) + missing = sorted(KNOWN_EVENTS - documented) + assert not missing, f"undocumented events: {missing}" + stale = sorted(documented - KNOWN_EVENTS) + assert not stale, f"docs/events.md lists events KNOWN_EVENTS does not: {stale}" + # Non-empty floor: an empty doc and an empty constant would agree. + assert len(documented) >= 30 From b65d77a5085599566a2033d9bf8245dc4782c963 Mon Sep 17 00:00:00 2001 From: Jaret Arnold <96366172+musharna@users.noreply.github.com> Date: Wed, 2 Sep 2026 12:52:37 -0400 Subject: [PATCH 06/10] fix(worker): make the solo-job gate atomic with the reparented-orphan sweep audit 2026-09-02 L-4. _is_solo_in_flight() was a point read released before sweep_and_kill_reparented_orphans scanned /proc; with max_concurrent > 1 the poll loop could _register_in_flight(B) and Popen B in that window, and B's child (ppid == worker, not in A's tracked set) was SIGTERMed as an orphan. Registration already takes _in_flight_lock before any Popen, so _sweep_reparented_orphans_if_solo holds that lock across check + scan; the point-read gate is deleted. Real-threads test asserts no registration can complete while the scan runs, and fails when the lock is released before the scan (verified). The prior gate test is ported to the helper. --- changelog.d/audit-2026-09-02-worker.fixed.md | 1 + src/jobd/worker/job_worker.py | 62 +++++++++------- tests/test_worker.py | 26 +++++-- .../test_orphan_sweep_solo_gate_is_atomic.py | 70 +++++++++++++++++++ 4 files changed, 126 insertions(+), 33 deletions(-) create mode 100644 changelog.d/audit-2026-09-02-worker.fixed.md create mode 100644 tests/unit/test_orphan_sweep_solo_gate_is_atomic.py diff --git a/changelog.d/audit-2026-09-02-worker.fixed.md b/changelog.d/audit-2026-09-02-worker.fixed.md new file mode 100644 index 0000000..2a6a463 --- /dev/null +++ b/changelog.d/audit-2026-09-02-worker.fixed.md @@ -0,0 +1 @@ +- **A worker running two jobs at once can no longer kill the second job's fresh child during the first job's cleanup.** The reparented-orphan `/proc` sweep at job finalize gated on a point read of the in-flight count and then scanned with the lock released. Between the two, the poll loop could register and start job B, whose new child has this worker as parent and is not in job A's tracked set, so the sweep SIGTERMed it. The check and the scan now run under the same lock registration takes before any `Popen`, so B cannot appear inside the window and, if it registered first, the sweep declines. Only reachable with `max_concurrent > 1`. diff --git a/src/jobd/worker/job_worker.py b/src/jobd/worker/job_worker.py index 6f901b9..493086b 100644 --- a/src/jobd/worker/job_worker.py +++ b/src/jobd/worker/job_worker.py @@ -462,17 +462,38 @@ def _effective_owned_pids(tracked_pids: set[int]) -> set[int]: return owned -def _is_solo_in_flight() -> bool: - """True when at most one job (this one) is registered in flight. - - Gates the reparented-orphan /proc sweep in run_job: that sweep treats any - process reparented to this worker but not in tracked_pids as a leak, which - is only sound when no OTHER job is running concurrently (a concurrent - fast-path job's reparented descendant would otherwise look like an orphan - and be killed). See run_job's finalize block. +def _sweep_reparented_orphans_if_solo(job_id: int, tracked_pids: set[int]) -> None: + """Run the reparented-orphan /proc sweep, but only while this job is the + sole one in flight -- and hold `_in_flight_lock` across BOTH the check and + the scan. + + The sweep treats any process whose ppid is this worker and which is not in + `tracked_pids` as a leak. That is sound only when no OTHER job is running: + a concurrent job's fresh Popen child has ppid == this worker too. The gate + used to be a point read released before the scan, so the poll loop could + register job B and Popen it inside the window and B's child was SIGTERMed + (audit 2026-09-02 L-4). `_register_in_flight` takes this same lock BEFORE + any Popen, so with the lock held here B cannot register until the scan is + done, and if B registered first the count is two and the sweep declines. + The scan is a /proc listdir plus a stat read per pid -- milliseconds -- so + the poll loop's registration is delayed by at most that. """ with _in_flight_lock: - return len(_in_flight) <= 1 + if len(_in_flight) > 1: + return + try: + known = _tracked_pids_snapshot(tracked_pids) + killed = _subreaper.sweep_and_kill_reparented_orphans(known) + except Exception as e: + log.error("job %s: subreaper /proc-sweep error: %s", job_id, e) + return + if killed: + log.info( + "job %s: subreaper /proc-sweep reaped %s reparented orphan PID(s): %s", + job_id, + len(killed), + killed, + ) def _reserve_and_dispatch( @@ -1370,23 +1391,12 @@ def reap_stragglers(self, tracked_pids: set[int]) -> None: # Concurrency guard (max_concurrent > 1): this /proc sweep is GLOBAL — it # has no way to tell job A's leaked orphan from job B's still-running # fast-path descendant (which also reparents to this worker). Only sweep - # when this is the sole in-flight job; a concurrent job's orphan is reaped - # at the next idle moment instead. cgroup-walk above is per-scope and - # stays unconditional, so scope-wrapped jobs lose no cleanup. - if _REAPER_OK and _is_solo_in_flight(): - try: - killed = _subreaper.sweep_and_kill_reparented_orphans( - _tracked_pids_snapshot(tracked_pids) - ) - if killed: - log.info( - "job %s: subreaper /proc-sweep reaped %s reparented orphan PID(s): %s", - self.job_id, - len(killed), - killed, - ) - except Exception as e: - log.error("job %s: subreaper /proc-sweep error: %s", self.job_id, e) + # when this is the sole in-flight job, atomically with that check (see + # the helper); a concurrent job's orphan is reaped at the next idle + # moment instead. cgroup-walk above is per-scope and stays + # unconditional, so scope-wrapped jobs lose no cleanup. + if _REAPER_OK: + _sweep_reparented_orphans_if_solo(self.job_id, tracked_pids) def final_state(self, rc: int) -> str: if self.got_signal == "cancel": diff --git a/tests/test_worker.py b/tests/test_worker.py index c4128cc..19d7c29 100644 --- a/tests/test_worker.py +++ b/tests/test_worker.py @@ -1247,20 +1247,32 @@ def test_own_scope_job_not_counted_as_foreign_vram(tmp_path, monkeypatch): _reset_in_flight() -def test_is_solo_in_flight_gates_reparented_orphan_sweep(): +def test_the_reparented_orphan_sweep_runs_only_for_a_solo_job(monkeypatch): """P3.5: the reparented-orphan /proc sweep must only run when this is the sole in-flight job. With a concurrent job registered, the global sweep - could kill the other job's reparented fast-path descendant, so the gate - returns False and run_job skips it.""" + could kill the other job's reparented fast-path descendant. The gate is + now atomic with the scan (audit 2026-09-02 L-4; see + tests/unit/test_orphan_sweep_solo_gate_is_atomic.py), so this drives the + helper and observes whether the scan ran.""" + scans: list[set[int]] = [] + monkeypatch.setattr( + job_worker._subreaper, + "sweep_and_kill_reparented_orphans", + lambda known: scans.append(set(known)) or [], + ) _reset_in_flight() try: - assert job_worker._is_solo_in_flight() is True # nothing in flight + job_worker._sweep_reparented_orphans_if_solo(960, {1}) + assert len(scans) == 1 # nothing in flight job_worker._register_in_flight({"id": 960, "vram_gb": 0, "ram_gb": 0, "cpus": 0}) - assert job_worker._is_solo_in_flight() is True # just me + job_worker._sweep_reparented_orphans_if_solo(960, {1}) + assert len(scans) == 2 # just me job_worker._register_in_flight({"id": 961, "vram_gb": 0, "ram_gb": 0, "cpus": 0}) - assert job_worker._is_solo_in_flight() is False # a concurrent job exists + job_worker._sweep_reparented_orphans_if_solo(960, {1}) + assert len(scans) == 2 # a concurrent job exists: no scan job_worker._unregister_in_flight(961) - assert job_worker._is_solo_in_flight() is True # back to solo + job_worker._sweep_reparented_orphans_if_solo(960, {1}) + assert len(scans) == 3 # back to solo finally: _reset_in_flight() diff --git a/tests/unit/test_orphan_sweep_solo_gate_is_atomic.py b/tests/unit/test_orphan_sweep_solo_gate_is_atomic.py new file mode 100644 index 0000000..7713377 --- /dev/null +++ b/tests/unit/test_orphan_sweep_solo_gate_is_atomic.py @@ -0,0 +1,70 @@ +"""audit 2026-09-02 L-4: the reparented-orphan /proc sweep at job finalize is +sound only while THIS job is the sole one in flight, and it gated on a point +read of `_in_flight`. Between that read and the scan the poll loop could +register job B and Popen it; B's fresh child has ppid == this worker and is not +in A's tracked set, so the sweep SIGTERMed it. Registration already takes +`_in_flight_lock` before any Popen, so holding that lock across gate + scan +makes them atomic: B cannot register while the scan runs, and if B registered +first the gate sees two jobs and declines. + +Real threads, real lock; the only stub is the /proc scan itself. +""" + +import threading + +import jobd.worker.job_worker as jw + + +def _reset_in_flight(*ids): + with jw._in_flight_lock: + jw._in_flight.clear() + for i in ids: + jw._in_flight[i] = {"vram_gb": 0.0, "ram_gb": 0.0, "cpus": 0} + + +def test_no_job_can_register_while_the_solo_sweep_is_scanning(monkeypatch): + _reset_in_flight(1) + monkeypatch.setattr(jw, "_REAPER_OK", True) + scanning = threading.Event() + release = threading.Event() + swept: list[set[int]] = [] + + def fake_scan(known: set[int]) -> list[int]: + scanning.set() + assert release.wait(5), "test harness never released the scan" + swept.append(set(known)) + return [] + + monkeypatch.setattr(jw._subreaper, "sweep_and_kill_reparented_orphans", fake_scan) + + sweeper = threading.Thread( + target=jw._sweep_reparented_orphans_if_solo, args=(1, {111}), daemon=True + ) + sweeper.start() + assert scanning.wait(5), "the solo sweep did not run for a lone in-flight job" + + registered = threading.Event() + + def register_b(): + jw._register_in_flight({"id": 2, "vram_gb": 0, "ram_gb": 0, "cpus": 0}) + registered.set() + + threading.Thread(target=register_b, daemon=True).start() + assert not registered.wait(0.3), "job B registered while the solo sweep was scanning /proc" + + release.set() + sweeper.join(5) + assert registered.wait(5), "registration never completed after the sweep finished" + assert swept == [{111}] + try: + # Positive control: with two jobs in flight the sweep must not run at all. + calls: list[set[int]] = [] + monkeypatch.setattr( + jw._subreaper, + "sweep_and_kill_reparented_orphans", + lambda known: calls.append(known) or [], + ) + jw._sweep_reparented_orphans_if_solo(1, {111}) + assert calls == [] + finally: + _reset_in_flight() From 9206f0f5b6f389e92b88be9f8d59ed29cebbbda5 Mon Sep 17 00:00:00 2001 From: Jaret Arnold <96366172+musharna@users.noreply.github.com> Date: Wed, 2 Sep 2026 13:27:38 -0400 Subject: [PATCH 07/10] test: map the new roots-visibility test to the #129 pseudonyms The audit branch was written against the pre-#129 tree; after the rebase this one new file still named the real home directory and project, which tests/test_no_private_paths.py now forbids. --- tests/unit/test_projects_roots_are_visible.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/unit/test_projects_roots_are_visible.py b/tests/unit/test_projects_roots_are_visible.py index 9bdbdb0..69ca9c0 100644 --- a/tests/unit/test_projects_roots_are_visible.py +++ b/tests/unit/test_projects_roots_are_visible.py @@ -12,8 +12,8 @@ def test_get_projects_reports_each_projects_roots(rooted_client): body = rooted_client.get("/projects").json() - assert body["jepagame"]["roots"] == ["/home/mjarnold/jepagame"] - assert body["orchid-sdxl"]["roots"] == ["/home/mjarnold/orchid-sdxl"] + assert body["beta"]["roots"] == ["/home/user/beta"] + assert body["gamma"]["roots"] == ["/home/user/gamma"] # A project without roots does not grow an empty key. assert "roots" not in body["_default"] @@ -31,5 +31,5 @@ def __exit__(self, *exc): monkeypatch.setattr(cli_mod, "_client", _AppClient) r = CliRunner().invoke(cli_mod.app, ["projects", "list"]) assert r.exit_code == 0, r.output - jepagame_line = next(line for line in r.output.splitlines() if "jepagame" in line) - assert "roots=/home/mjarnold/jepagame" in jepagame_line + beta_line = next(line for line in r.output.splitlines() if "beta" in line) + assert "roots=/home/user/beta" in beta_line From c903a3d99cae7571ebda9a3d9e650a5af9605a89 Mon Sep 17 00:00:00 2001 From: Jaret Arnold <96366172+musharna@users.noreply.github.com> Date: Wed, 2 Sep 2026 13:27:38 -0400 Subject: [PATCH 08/10] test(privacy): guard that config/projects.local.yaml is gitignored Found while rebasing onto #129: its .gitignore rule was appended as ONE line holding literal backslash-n sequences, so 'git check-ignore config/projects.local.yaml' printed nothing and the real-roots overlay showed as '??'. The rule itself is corrected in the companion privacy fix by the session that owns #129; this adds the positive control that would have caught it: a test asserting git check-ignore accepts the path (watched failing on the broken rule). --- changelog.d/audit-2026-09-02-gitignore.security.md | 1 + tests/test_no_private_paths.py | 12 ++++++++++++ 2 files changed, 13 insertions(+) create mode 100644 changelog.d/audit-2026-09-02-gitignore.security.md diff --git a/changelog.d/audit-2026-09-02-gitignore.security.md b/changelog.d/audit-2026-09-02-gitignore.security.md new file mode 100644 index 0000000..ea6bfb9 --- /dev/null +++ b/changelog.d/audit-2026-09-02-gitignore.security.md @@ -0,0 +1 @@ +- **`config/projects.local.yaml` is gitignored, and a test now keeps it so.** The rule added in #129 landed as a single line containing literal `\n` sequences, so the pattern matched nothing and the file that exists to hold the operator's real project roots and names showed up as untracked, one `git add -A` away from being published. The rule is corrected in the companion privacy fix; `tests/test_no_private_paths.py` now asserts `git check-ignore` accepts the path, so it cannot silently regress. diff --git a/tests/test_no_private_paths.py b/tests/test_no_private_paths.py index fe535b2..f529478 100644 --- a/tests/test_no_private_paths.py +++ b/tests/test_no_private_paths.py @@ -63,3 +63,15 @@ def test_scanner_reports_every_planted_hit(tmp_path): found = {h.split(": ", 1)[1] for h in hits} assert found == set(FORBIDDEN), sorted(set(FORBIDDEN) - found) assert all(h.split(":")[1].isdigit() for h in hits) + + +def test_the_private_overlay_is_gitignored(): + """The real roots and names live in config/projects.local.yaml. If git does + not ignore that path, the deploy step that copies it beside projects.yaml + turns the next `git add -A` into a publication of everything this file + exists to keep private. (#129 appended the rule as one line containing + literal backslash-n sequences, so the pattern never matched.)""" + r = subprocess.run( + ["git", "check-ignore", "-q", "config/projects.local.yaml"], cwd=ROOT, check=False + ) + assert r.returncode == 0, "config/projects.local.yaml is not gitignored" From 29ee40a6b979d548ce3a63c2c7f233962bbffcf6 Mon Sep 17 00:00:00 2001 From: Jaret Arnold <96366172+musharna@users.noreply.github.com> Date: Wed, 2 Sep 2026 13:27:38 -0400 Subject: [PATCH 09/10] test(mcp): use the pseudonymous project name in the new label tests --- tests/mcp/test_tools.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/mcp/test_tools.py b/tests/mcp/test_tools.py index 735aa93..53028ca 100644 --- a/tests/mcp/test_tools.py +++ b/tests/mcp/test_tools.py @@ -729,13 +729,13 @@ def test_call_tool_logs_error_kind_for_refusal(tmp_path, monkeypatch): def test_jobd_list_carries_the_typed_label_beside_the_identity(): """audit 2026-09-02 Q-3: the broker filter matches either name, so `jobd_list(project="pillar2a1_sweep")` returned rows whose `project` was - `jepagame` with no field saying why. The summary hand-lists its columns; + `beta` with no field saying why. The summary hand-lists its columns; the label has to be one of them or the substitution is invisible here.""" _mock_jobs_endpoint( [ { "id": 3, - "project": "jepagame", + "project": "beta", "project_label": "pillar2a1_sweep", "state": "queued", "worker": None, @@ -749,7 +749,7 @@ def test_jobd_list_carries_the_typed_label_beside_the_identity(): client = JobdClient(base_url="http://broker.test") out = jobd_list(client, {"state": ["queued"]}) - assert out["jobs"][0]["project"] == "jepagame" + assert out["jobs"][0]["project"] == "beta" assert out["jobs"][0]["project_label"] == "pillar2a1_sweep" @@ -763,7 +763,7 @@ def test_submit_reports_the_typed_label_when_the_identity_was_substituted(): json={ "job_id": 7, "state": "queued", - "project": "jepagame", + "project": "beta", "project_label": "pillar2a1_sweep", "host_pin": "any", "queued_at": "2026-04-26T00:00:00Z", @@ -772,5 +772,5 @@ def test_submit_reports_the_typed_label_when_the_identity_was_substituted(): ) client = JobdClient(base_url="http://broker.test") out = jobd_submit(client, {"command": "x", "project": "pillar2a1_sweep", "cwd": "/x"}) - assert out["project"] == "jepagame" + assert out["project"] == "beta" assert out["project_label"] == "pillar2a1_sweep" From caf4fea68142a8309e6433608b0654b16c686994 Mon Sep 17 00:00:00 2001 From: Jaret Arnold <96366172+musharna@users.noreply.github.com> Date: Wed, 2 Sep 2026 15:07:32 -0400 Subject: [PATCH 10/10] test(mcp): pin the live summary key set to the source tuple The `jobd_list` summary shape was written down in three places: the `_LIST_SUMMARY_FIELDS` tuple, the synthetic assertion in test_tools.py, and the live-broker assertion in test_live.py. Adding `project_label` to the source updated the first two; the third went stale and only surfaced in CI's live leg, because the live suite is deselected on any machine without a broker -- the copy that never runs locally is the copy that drifts. Give the live test a module-level LIST_SUMMARY_KEYS and assert it equals `_LIST_SUMMARY_FIELDS` from the default suite, so the next field added to the source fails on a laptop instead of on a pull request. Verified: removing project_label from LIST_SUMMARY_KEYS reproduces CI's exact diff in the new guard. Live suite re-run against a real broker+worker (RUN_LIVE_JOBD=1, loopback broker on 8799): 9 passed. --- tests/mcp/test_live.py | 28 +++++++++++++++++++--------- tests/mcp/test_tools.py | 14 ++++++++++++++ 2 files changed, 33 insertions(+), 9 deletions(-) diff --git a/tests/mcp/test_live.py b/tests/mcp/test_live.py index 38d8401..d295ff9 100644 --- a/tests/mcp/test_live.py +++ b/tests/mcp/test_live.py @@ -30,6 +30,24 @@ LIVE = os.environ.get("RUN_LIVE_JOBD") == "1" JOBD_URL = os.environ.get("JOBD_URL", "http://127.0.0.1:8765") +# The keys `jobd_list` promises in each summary row. Hand-maintained here on +# purpose — this is the wire contract as an agent sees it, asserted against a +# real broker. It is pinned against jobd.mcp.tools._LIST_SUMMARY_FIELDS by +# test_tools.py so a field added to the source cannot drift away from the +# contract while this file sits deselected. (2026-09-02: project_label was +# added to the source and only the live copy went stale, because the live +# suite is the one that never runs on a laptop.) +LIST_SUMMARY_KEYS = { + "job_id", + "project", + "project_label", + "state", + "host", + "exit_code", + "queued_at", + "started_at", +} + def _broker_reachable() -> bool: try: @@ -104,15 +122,7 @@ def test_live_submit_status_list_jobget_cancel_full_round_trip(): ids = {j["job_id"] for j in lst["jobs"]} assert job_id in ids, f"submitted job_id {job_id} not in list: {ids}" # summary shape — only the keys jobd_list promises. - assert set(lst["jobs"][0].keys()) == { - "job_id", - "project", - "state", - "host", - "exit_code", - "queued_at", - "started_at", - } + assert set(lst["jobs"][0].keys()) == LIST_SUMMARY_KEYS # Full record — exercises xlate_job_info on /jobs/ (the broker's # full info, incl. scheduling internals). jobd_status is the only tool diff --git a/tests/mcp/test_tools.py b/tests/mcp/test_tools.py index 53028ca..afa9a93 100644 --- a/tests/mcp/test_tools.py +++ b/tests/mcp/test_tools.py @@ -466,6 +466,20 @@ def test_jobd_list_summarizes_jobs(): assert out["jobs"][1]["job_id"] == 1 +def test_the_live_suite_pins_the_same_summary_keys_as_the_source(): + """The `jobd_list` summary shape exists in three places: the source tuple, + the synthetic assertion above, and the live-broker test. The live one is + deselected on any machine without a broker, so a field added to the source + reaches CI's live leg as a red diff nobody saw locally — which is exactly + what `project_label` did on 2026-09-02. Pin the live copy here, where it + runs in the default suite. + """ + from jobd.mcp.tools import _LIST_SUMMARY_FIELDS + from tests.mcp.test_live import LIST_SUMMARY_KEYS + + assert set(_LIST_SUMMARY_FIELDS) == LIST_SUMMARY_KEYS + + def _job_row(i: int, state: str) -> dict: return { "id": i,