fix(server): fail job explicitly on unknown execution mode (#881)

[akshatmalik-bruh]

Here is the exact filled-out content ready to copy and paste into your GitHub PR:

---

Summary

• Validates job mode against VALID_JOB_MODES = {"sync", "full_resync", "initial_index", "index_only"} in execute_job().
• Raises a ValueError on unknown modes so execute_job() explicitly fails the job and records the error message in the DB instead of silently running partial work and updating baseline commit state.
• Preserves "sync" as default fallback when mode is missing or empty.

Related Issues

Fixes #881

Test Plan

• Tests pass (pytest) - Added unit tests test_execute_job_fails_on_unknown_mode and test_execute_job_defaults_to_sync_when_mode_absent in tests/unit/server/test_job_executor.py.
• Lint passes (ruff check .)

Checklist

• My code follows the project's code style
• I have added tests for new functionality
• All existing tests still pass
• I have updated documentation if needed (n/a — internal validation change)

---

[repowise-bot]

✅ Health: 7.4 (unchanged)

📋 At a glance
2 hotspots touched · 1 new finding introduced · 2 files with recent fix history.

Files & modules (2)

• packages (1 file)
  • .../server/job_executor.py
• tests (1 file)
  • .../server/test_job_executor.py

📌 Before you merge

• Run .../server/test_generate.py, .../server/test_initial_index.py, .../server/conftest.py: they depend on the changed files

🗺️ Change map

flowchart LR
  subgraph PR ["Changed in this PR (2 with dependents)"]
    f_packages_server_src_repowise_server_job_executor_py[".../server/job_executor.py 🔥"]:::changed
    f_tests_unit_server_test_job_executor_py[".../server/test_job_executor.py"]:::changed
  end
  f_packages_cli_src_repowise_cli_commands_init_cmd_command_py[".../init_cmd/command.py"]
  f_packages_server_src_repowise_server_job_executor_py --> f_packages_cli_src_repowise_cli_commands_init_cmd_command_py
  f_packages_cli_src_repowise_cli_commands_init_cmd_persistence_py[".../init_cmd/persistence.py"]
  f_packages_server_src_repowise_server_job_executor_py --> f_packages_cli_src_repowise_cli_commands_init_cmd_persistence_py
  f_packages_cli_src_repowise_cli_commands_init_cmd_workspace_py[".../init_cmd/workspace.py"]
  f_packages_server_src_repowise_server_job_executor_py --> f_packages_cli_src_repowise_cli_commands_init_cmd_workspace_py
  f_packages_cli_src_repowise_cli_commands_restyle_cmd_py[".../commands/restyle_cmd.py"]
  f_packages_server_src_repowise_server_job_executor_py --> f_packages_cli_src_repowise_cli_commands_restyle_cmd_py
  more(["+12 more dependents"])
  PR --> more
  t_tests_unit_server_test_generate_py(["✅ .../server/test_generate.py"]):::guard
  t_tests_unit_server_test_generate_py -.-> f_packages_server_src_repowise_server_job_executor_py
  t_tests_unit_server_conftest_py(["✅ .../server/conftest.py"]):::guard
  t_tests_unit_server_conftest_py -.-> f_tests_unit_server_test_job_executor_py
  classDef changed fill:#dbeafe,stroke:#1d4ed8,color:#1e3a5f
  classDef warn fill:#fef3c7,stroke:#b45309,color:#78350f
  classDef guard fill:#dcfce7,stroke:#15803d,color:#14532d

Loading

_{Solid arrows: code that imports the changed files (16 direct dependents, from the last indexed snapshot). Dashed: history/tests.}

⚠️ Change risk: moderate _{(riskier than 40% of this repo's commits)}
This change's risk is driven by:

• more lines added than baseline

🩹 Review priority _{(files here with the most recent bug-fix history — defects cluster, so review these first)}

• .../server/job_executor.py — fixed 4× in the last ~6 months
• .../server/test_job_executor.py — fixed 3× in the last ~6 months

🔎 More signals (1)

🔥 Hotspots touched (2)

• .../server/test_job_executor.py — 7 commits/90d, 1 dependents · primary owner: Raghav Chamadiya (52%)
• .../server/job_executor.py — 19 commits/90d, 19 dependents · primary owner: Raghav Chamadiya (75%)

---

📊 Full report · ⭐ Star Repowise · 📥 Install bot · Last updated 2026-07-26 12:11 UTC
_{Silence on a single PR with [skip repowise] in the title · Per-repo toggle on repowise.dev/settings?tab=bot}

[swati510]

This is a small robustness point rather than a real bug. The mode comes out of parsed JSON, so it is not guaranteed to be a string, and because VALID_JOB_MODES is a set, a non-empty list or dictionary would make the membership test on the next line raise an unhashable-type error instead of the helpful message you have written. The job would still be recorded as failed, but the message stored in the database would be confusing to whoever reads it later. Would you consider coercing the value when you read it, for example mode = str(config.get("mode") or "sync"), so that the friendly message is always the one that gets recorded?

[akshatmalik-bruh]

Done! Changed to mode = str(config.get("mode") or "sync") so any non-string JSON value (e.g. a list or dict) is coerced before the set membership test. This ensures the friendly ValueError with the invalid mode message is always the one recorded in the DB, rather than a confusing unhashable type error.

[swati510]

Thank you for naming the accepted modes in one place. One thing that might be worth capturing here in a comment: routers/pages.py line 121 creates job rows with the mode "single_page", and those rows are never handed to execute_job, which is why they are not in this set. Without that note, a future reader who wires up the page regeneration endpoint will hit an "invalid job mode" failure and have to rediscover the history. This is only a comment suggestion, since the linked issue says the dormant single_page endpoint deserves its own ticket.

[akshatmalik-bruh]

Added a comment explaining that "single_page" is intentionally excluded — routers/pages.py (line 121) creates those job rows but they are never dispatched to execute_job. Also noted the dormant endpoint deserves its own dedicated handling. Thanks for flagging — this will save future readers a confusing debugging session!

[swati510]

The description says "a missing or empty mode string", but the test only covers the missing case, since the configuration is an empty dictionary. The empty-string case is the one that actually exercises the or "sync" fallback, and it is the case most likely to break in a future refactor of that line. Could you either add a second case with config={"mode": ""} or adjust the description so that it matches what is being tested?

[akshatmalik-bruh]

Good point! I've split this into two separate tests: test_execute_job_defaults_to_sync_when_mode_absent (covers config={}) and a new test_execute_job_defaults_to_sync_when_mode_empty_string (covers config={"mode": ""}) which directly exercises the or "sync" fallback branch. Also updated the docstring of the original test to only say "missing" instead of "missing or empty".

[swati510]

This assertion shows that the pipeline ran, but it would pass for any accepted mode, so it does not quite prove that the fallback is "sync". Because the provider lookup is forced to fail in this test, the behaviour that is specific to "sync", namely the incremental page regeneration, never runs, so nothing here distinguishes "sync" from, say, "index_only". Checking the generate_docs keyword would not help either, since it is computed as false for "sync" and for "index_only" alike. What would distinguish them is that "sync" attempts the provider lookup while "index_only" skips that block entirely, so asserting that the patched get_chat_provider_instance was called would pin the mode down. Another option would be to let the patched provider return a mock client and then assert that the sync-only incremental page regeneration ran.

[akshatmalik-bruh]

Great catch! I've captured the get_chat_provider_instance mock as get_provider_mock and added get_provider_mock.assert_called_once() after the run. Since "sync" always enters the provider lookup block while "index_only" skips it entirely, this assertion specifically pins the fallback mode down to "sync". Applied the same pattern to both the absent-mode and empty-string tests.

[akshatmalik-bruh]

Hi @swati510 @RaghavChamadiya , just following up on this PR when you get a chance to take a look. Thanks!

[akshatmalik-bruh]

Resolved merge conflicts with main and verified tests pass. Ready for review whenever you get a chance!

[RaghavChamadiya]

Thanks for the quick turnaround on Swati's points. The coercion to str(...), the split empty-string test, and the get_provider_mock.assert_called_once() assertion all address exactly what was raised, and that last one is a genuinely better test than what it replaced.

One blocking item, and it comes from the merge rather than from anything you wrote.

generate and single_page are now dispatched modes, and this allowlist rejects them. Since #881 was filed, execute_job gained a real branch for both:

# job_executor.py:407 on this branch
if mode in ("generate", "single_page"):
    ...
    await _run_generate_job(...)
    return

Your merge commit pulled that in, but VALID_JOB_MODES (line 115) was not updated, and the validation at line 353 runs before the dispatch at 407. Both modes reach the executor for real:

• routers/repos.py:667 builds {"mode": "generate", ...}, launched via _launch_job_task. That is POST /repos/{id}/generate, the HTTP repowise generate.
• routers/pages.py:147 builds {"mode": "single_page", ...}, same path.

So after merge every generate and every per-page regenerate fails with Invalid job mode 'generate'. The endpoint that the issue described as dormant is live now.

To be clear about where this came from: the single_page note was Swati's suggestion on 2026-07-20 and it was correct then, and my own issue text said the same thing. The dispatch landed in between.

The fix:

1. Add "generate" and "single_page" to VALID_JOB_MODES.
2. Replace the exclusion note with the opposite, something like: these two are handled by the scoped-generation branch below, so validation has to run before it and accept them.
3. Add a test that a {"mode": "generate"} job still reaches _run_generate_job. That is the regression guard, and its absence is why CI stays green on this: nothing currently drives a generate job through execute_job.

One process note. The branch is 5 commits behind main again, and this is the second time the gap has hidden something. Could you rebase onto main rather than merging? A rebase would have put your validation line directly against the new dispatch branch and made the conflict visible instead of silently compatible. Worth a look at whether anything else moved under the PR while you were waiting.

Everything else here is ready. Fix the allowlist and I think this goes in.

[akshatmalik-bruh]

Thanks for catching that @RaghavChamadiya ! All items have been updated:

1. Updated VALID_JOB_MODES Allowlist: Added "generate" and "single_page" to VALID_JOB_MODES in job_executor.py.
2. Updated Documentation: Updated the inline comment above VALID_JOB_MODES to document that "generate" and "single_page" are valid modes dispatched to _run_generate_job.
3. Regression Guard Test: Added test_execute_job_dispatches_generate_mode in tests/unit/server/test_job_executor.py to ensure jobs with mode="generate" pass validation and reach _run_generate_job.
4. Rebased onto Main: Rebased the branch cleanly on top of latest main. All 23 unit tests in test_job_executor.py pass cleanly.

[RaghavChamadiya]

LGTM