Add a new optional field to Connection for the Spark History Server
base URL. The field is stored as part of the connection but is not
yet consumed by any tool — for now it's a labeled place to record
where SHS lives on the cluster, and a hook for future SHS-specific
tools. To actually fetch SHS endpoints today, use fetch_url with the
SHS host added to url_allowlist.
The field is Optional[str], default None. Same nullability as
yarn_rm_url. The SHS host and the YARN RM host are usually
different, so this is independent of yarn_rm_url.
Schema:
- models.py: Connection.history_server_url (str | None, default None)
- requests.py:
- SaveConnectionRequest.history_server_url (str | None, default None)
- UpdateConnectionRequest.history_server_url (str | None, default None)
- tools/connections.py: save_connection signature gains
history_server_url with the _UNSET sentinel pattern (same as
yarn_rm_url, url_allowlist, etc.) so the upsert path correctly
distinguishes "not provided" from "explicitly None".
- tools/connections.py: update_connection docstring lists the new
field in the mutable fields set.
Tests (4 new in tests/unit/test_connection_tools.py):
- test_save_connection_with_history_server_url
- test_save_connection_history_server_url_defaults_to_none
- test_update_connection_changes_history_server_url
- test_update_connection_keeps_history_server_url_when_omitted
Tests: 398 passed (was 394, +4 net).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Three small but high-leverage text corrections. No code or test
changes — these only affect what the LLM sees in tools/list and
Pydantic schemas.
1) FetchUrlRequest.connection_name description
Old: "The Connection's yarn_rm_url defines the allowed host domain."
New: explicitly says url_allowlist is the host gate, NOT yarn_rm_url.
yarn_rm_url is only used by get_external_* tools. Without this
fix the LLM would try to control fetch scope via yarn_rm_url
(a no-op) instead of url_allowlist.
2) Connection.url_allowlist description
Added: "Set or change via save_connection (pass url_allowlist on
create) or update_connection (PATCH the field on an existing
connection)." Tells the LLM which tools populate the field,
instead of leaving it to guess.
3) /fetch_url route description
Old: "30s timeout, redirects followed."
New: "30s timeout, redirects followed, response body capped at
1 MB (the response includes a truncated boolean when this
kicks in)." LLM previously had no way to discover the 1MB
cap or the truncated indicator; it would just see
short responses and assume that was the full body.
Tests: 394 passed, no test changes (descriptions are not asserted).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- fetch_url: remove the 1MB body cap and the streaming helper. The
manual redirect loop with allowlist re-check (the SSRF fix) is kept
intact; the body is now read in full via resp.text. Drop the now-
meaningless `truncated` field from FetchUrlResult and the tests that
asserted on it. Switched from httpx.stream() back to httpx.get()
for the redirect loop — cleaner without the body cap.
- datetime: replace deprecated datetime.utcnow() with
datetime.now(timezone.utc) in submit.py (3 sites) and
core/job_writer.py (1 site). Update the stale comment in
core/pending_store.py that referenced the old call.
- Clean up an unused `from common.config import settings` import in
submit.py that ruff flagged.
Co-Authored-By: Claude <noreply@anthropic.com>
- fetch_url: revalidate allowlist on every redirect hop (fixes SSRF where
302 to disallowed host / 169.254.169.254 / file:// bypassed the
url_allowlist). Stream response body with iter_bytes and cap at 1MB
so a multi-GB response from an allowlisted host cannot OOM the service.
Reuses the manual-redirect-loop pattern from yarn_client.
- list_applications: stop swallowing 404 (YARN returns 200+empty for
"no match"; 404 means the RM doesn't support the endpoint — surface
the YarnError instead of hiding it as an empty result). Add
Field(ge=1, le=10000) to ListApplicationsRequest.limit so a runaway
limit is rejected at the Pydantic layer with 422.
- save_connection: PATCH semantics for existing records. Re-route to
update_connection when the name already exists so partial updates
(e.g. only master) no longer wipe url_allowlist back to []. Uses an
_UNSET sentinel in the tool function to distinguish "omitted" from
"None" without breaking the existing parameter list.
- README: drop leading space on 5 new connection-tool table rows that
was breaking GitHub Flavored Markdown table continuity.
- Indentation: normalize connections.py and requests.py to 4-space
indent (auth_password/auth_principal/auth_keytab were 3-space).
Co-Authored-By: Claude <noreply@anthropic.com>
Add a new MCP tool that queries YARN's /ws/v1/cluster/apps endpoint
through a named Connection, returning a list of ApplicationSummary
records. Bypasses the local JobStore — useful for enumerating apps
that were not submitted through this service.
API:
list_applications(
connection_name: str, # required, which YARN cluster
state: str | None = None, # YARN state filter: NEW/NEW_SAVING/
# SUBMITTED/ACCEPTED/RUNNING/
# FINISHED/FAILED/KILLED
queue: str | None = None, # YARN queue filter
limit: int = 100, # cap on returned apps (YARN has no
# offset-based pagination; combine
# state/queue filters for big clusters)
) -> list[ApplicationSummary]
Implementation:
- yarn_client.list_applications(config, *, state, queue, limit) -> list[dict]
Returns raw YARN app dicts; raises YarnError on 4xx/5xx; returns
[] on 404 (no apps match). Uses the existing _request helper,
which now accepts a "params" kwarg for query strings (one-line
additive change).
- external_jobs.list_applications(connection_name, state, queue, limit)
-> list[ApplicationSummary]. Looks up the Connection, builds the
YarnClientConfig, calls the yarn_client function, maps each raw
YARN dict to ApplicationSummary (mirroring the manual field-mapping
style of get_job_result). The yarn_client function is imported
as "list_applications_yarn" to avoid name collision.
- ApplicationSummary: 12-field Pydantic model with snake_case names
(application_id, name, user, queue, state, final_status,
application_type, application_tags, started_time, finished_time,
tracking_url, progress). Unused YARN fields (memorySeconds,
vcoreSeconds, preemptedResource*, etc.) are not exposed.
- ListApplicationsRequest: Pydantic body model with Field(description=)
for LLM-facing schema.
- /list_applications route registered with operation_id=
"list_applications", placed next to the other external YARN tools.
Tests:
- 8 new unit tests in test_external_jobs.py (happy path, state/queue/
limit pass-through, default limit, empty list, missing connection,
full field mapping).
- test_mcp_routes.py: assert 23 tool routes.
- README: list_applications row added to the Spark Executor table.
Tests: 390 passed (was 382, +8 net).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two changes:
1) Drop every check except the allowlist lookup.
Old _validate_url_host did: scheme check, host-presence check,
IP-literal check, empty-allowlist check, then glob match.
New _validate_url_host does: parse host, return on glob match,
raise on miss. That's it. The only remaining structural check is
'the URL must have a host' (otherwise the glob has nothing to
test against).
Security implication: scheme (file://, gopher://, ftp://) and
IP literals (10.0.0.1, ::1) are NO LONGER rejected by the
validator. The allowlist is the single source of truth. If the
user writes ['*.*.*.*'], they have opted in to 4-label hosts
including IP literals; if they write ['ccam*'], they get ccam1-
ccam99 and nothing else. The default ['ccam*'] / [] pattern is
tight by construction.
Removed: import ipaddress, the scheme/IP rejection branches, the
'allowlist empty' explicit branch (the empty list naturally
matches nothing).
2) Rename allowed_url_hosts -> url_allowlist.
The previous name was a verbose double-negative ('allowed ... hosts').
The new name is short, modern (allowlist > whitelist), and matches
the pattern of the field (URL hosts allowed). Renamed in:
- Connection (models.py)
- SaveConnectionRequest, UpdateConnectionRequest, FetchUrlRequest
(requests.py)
- _validate_url_host, _host_matches_any_glob parameters
(fetch_url.py)
- save_connection / update_connection call sites and error
messages (connections.py, fetch_url.py)
- Route descriptions (server.py)
- All test files
- README
No backward-compat alias: the field was added in 0291f36 and
hasn't shipped, so no production migration. Local dev data
(data/connections.json, gitignored) with allowed_url_hosts set
will be silently dropped by Pydantic v2 (default for extra
fields is ignore) — those connections lose their allowlist and
fetch_url will reject everything until re-saved.
Test cleanup:
- Removed 9 obsolete tests (scheme/IP/suffix rejection)
- Renamed allowed_url_hosts -> url_allowlist in 11 surviving tests
- Added 4 new tests documenting the 'allowlist is the only gate'
model: IP literal accepted, HTTPS accepted, no-host rejected,
empty allowlist rejected, error message mentions url_allowlist
-1 obsolete test, net -4 from 386 -> 382 tests passing.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Part 1 — fetch_url validation simplification
Now that Connection.allowed_url_hosts exists, it's the ONLY check.
The old 2-label suffix-overlap rule against yarn_rm_url is gone.
- Connection.allowed_url_hosts: list[str] = Field(default_factory=list)
(was list[str] | None = None). Default empty list means the
connection has no fetch access; the user must explicitly opt in
via save_connection or update_connection.
- _validate_url_host drops the yarn_rm_url parameter and the
_host_suffix_overlap helper. Now: scheme/host/IP checks, then
empty-reject, then glob match. Reject everything else.
- fetch_url's error message now points the user at
Connection.allowed_url_hosts as the fix.
- The label-by-label glob check from the previous commit is kept
(SSRF guard: 'ccam*' matches 'ccam50' but NOT 'ccam50.evil.com').
Part 2 — update_connection tool
PATCH-style update for an existing Connection record. Only the
fields the caller provides are changed. Same pattern as
update_pending_job: req.model_dump(exclude_none=True), with
'name' popped before passing to the store.
To CLEAR a field (e.g. drop auth_password), use delete_connection
+ save_connection. This is YAGNI; the alternative (model_fields_set
to distinguish 'omitted' from 'null') adds surface for bugs.
- ConnectionStore.update(name, **fields): get, model_copy(update=...),
save. Lock + atomic write. Re-validates the patched record.
- update_connection tool function: passes fields through to the
store, logs which fields were changed.
- UpdateConnectionRequest Pydantic model: 12 mutable fields + name.
- /update_connection route with operation_id='update_connection'.
- 9 new unit tests in test_connection_tools.py (PATCH, dict/list
replace-not-merge, unknown name 404, validation of patched record,
disk persistence).
- test_fetch_url.py: existing 21 tests updated; 3 new tests for
the empty/None/missing allowed_url_hosts cases.
- test_mcp_routes.py: assert 22 tool routes.
- README: update_connection row added; fetch_url row updated.
Tests: 386 passed (up from 377, +9 net).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The default 'URL host shares >= 2 labels of suffix with yarn_rm_url
host' rule is too strict for clusters whose hostnames are single-label
(e.g. 'ccam1' through 'ccam99'). The user's cluster is reachable at
http://ccam1:8088, and they want fetch_url to work for any ccamN
host — but ccam1 and ccam50 share 0 suffix labels, so the existing
rule rejects everything.
Add an opt-in allowlist field on Connection:
allowed_url_hosts: list[str] | None
Each entry is an fnmatch glob pattern. The URL host is allowed if it
matches ANY pattern, regardless of the suffix rule. Save a Connection
with ['ccam*'] to allow ccam1, ccam2, ..., ccam99.
SSRF safety: the implementation does NOT use vanilla fnmatch on the
flat host string (because '*' in fnmatch crosses '.', so 'ccam*' would
match 'ccam50.evil.com' — a security hole). Instead, both the pattern
and the host are split on '.' and matched LABEL-BY-LABEL with the
label counts required to match exactly. So 'ccam*' matches 'ccam50'
but NOT 'ccam50.evil.com' (different label counts).
Test coverage:
- 8 new tests in tests/unit/test_fetch_url.py (glob allow, glob deny,
dot-boundary SSRF test, multiple globs, empty list, None fallback,
error message hint)
- All 21 fetch_url tests pass; 377 total.
- spark_executor/models.py: Connection.allowed_url_hosts (with description)
- spark_executor/tools/requests.py: SaveConnectionRequest.allowed_url_hosts
- spark_executor/tools/fetch_url.py: new _host_matches_any_glob helper;
_validate_url_host now takes allowed_hosts and checks globs BEFORE
the suffix rule
- tests/unit/test_fetch_url.py: 8 new tests
- README.md: fetch_url row mentions the allowlist
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Add a new MCP tool that lets the agent fetch URLs on the cluster's
network (YARN tracking UI, Spark History Server, NodeManager web UIs)
when the agent is on a different network and cannot reach those hosts
directly.
The MCP service runs on the YARN RM node, so it can reach every host
the cluster knows about — the agent just needs a way to ask.
Security: SSRF guard via host suffix overlap
- URL host must share >= 2 labels of suffix with the named
Connection's yarn_rm_url host (e.g. yarn_rm_url='rm.prod.internal'
allows 'http://nm01.prod.internal/...')
- IP literals (10.0.0.1, ::1) rejected
- Non-http(s) schemes (file://, gopher://, ftp://) rejected
- Connection with no yarn_rm_url cannot use this tool
- Reuses Connection.auth_for_httpx() and verify_for_httpx() so the
agent does not need cluster credentials
- Response body capped at 1 MB (truncated=true if larger)
- 30s timeout, follows redirects, loguru INFO audit log on every call
- spark_executor/tools/fetch_url.py: new tool + 2 helpers
(_host_suffix_overlap, _validate_url_host)
- spark_executor/models.py: FetchUrlResult Pydantic model
- spark_executor/tools/requests.py: FetchUrlRequest with descriptions
- spark_executor/server.py: /fetch_url route, operation_id='fetch_url'
- tests/unit/test_fetch_url.py: 13 unit tests covering all guards,
truncation, auth/SSL pass-through, redirect follow
- tests/integration/test_mcp_routes.py: assert 21 tool routes
- README.md: 1 row in Spark Executor 工具 table
Tests: 369 passed (up from 356).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Add 3 new MCP tools for inspecting YARN applications NOT submitted
through this service: get_external_job_logs, get_external_job_status,
get_external_job_result. Each takes application_id + connection_name
and queries YARN directly, bypassing the local JobStore.
- spark_executor/tools/external_jobs.py: 3 tool functions
- spark_executor/tools/requests.py: 3 new Pydantic body models
(ExternalJobLogsRequest, ExternalJobStatusRequest,
ExternalJobResultRequest)
- spark_executor/server.py: 3 new POST routes with explicit operation_id
- tests/unit/test_external_jobs.py: 7 unit tests
- tests/integration/test_mcp_routes.py: assert 20 tool routes
- README.md: list the 3 new tools
To make the LLM pick the right tool and not guess at field values,
also:
- Add Pydantic field descriptions for 22 fields across 8 request models
(SaveConnectionRequest, UpdatePendingJobRequest, GetJobLogsRequest,
JobIdRequest, PendingIdRequest, ConnectionNameRequest, plus the new
ExternalJob*Request models).
- Update 12 route descriptions with cross-references, prerequisite
context, and 400 behavior notes.
- Refactor _unknown_job_error: an input that looks like a YARN
application_id (starts with 'application_') now returns HTTP 400
(ValueError) with a hint message naming the right external tool;
other not-found cases still return 404 (KeyError). This catches the
common LLM mistake of passing application_id to the internal
get_job_* / kill_job tools.
- 4 new unit tests for the 400 behavior.
Tests: 356 passed (up from 242).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Dockerfile: COPY files_mcp package, mkdir -p /app/data/files at build
time, set ENV FILES_MCP_ROOT=/app/data/files as the default sandbox
root (overridable at runtime).
- docker-compose.yml: surface FILES_MCP_ROOT in the env block with the
same default, so operators can override it via .env or -e.
FILES_MCP_ROOT must exist at process start (path_guard._init_root
checks is_dir()), so the directory is created during build rather than
deferred to entrypoint — fail-fast at image build beats a runtime
RuntimeError on every container start.
Default lives under /app/data so it rides the existing data volume
(./data:/app/data) and persists across container restarts alongside
connections.json / pending_jobs.json / logs/.
Verified locally: docker compose config --quiet passes; FILES_MCP_ROOT
resolves to /app/data/files in the rendered config. Full image build
not run (docker daemon unavailable in this environment); please run
`docker compose up -d --build` to confirm in your environment.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds tests/integration/test_main_lifespan.py with 5 cases that exercise
the end-to-end main.app lifespan:
- default (no MCP_SERVICES) mounts both services in BUILTIN order
- MCP_SERVICES=files mounts only files
- MCP_SERVICES='' mounts zero
- MCP_SERVICES=bogus raises RuntimeError at startup
- /health on the root app is unaffected by MCP filtering
Tests use TestClient and mock main.init_mcp_server to record mount
calls, since fastapi-mcp's HTTP endpoints 404 for bare GETs regardless
of mount state (the MCP transport requires an initialize handshake
before any tool call). Mocking avoids that coupling.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- main.py: drops the hard-coded McpService class and MCP_SERVICES list.
Lifespan now calls discover_and_filter() which does import + env
filtering. Unset MCP_SERVICES reproduces today's behavior exactly.
- spark_executor/service.py: new file exporting SERVICE = McpService(
name='spark_executor', app=spark_executor.app, mount_path='/spark-executor-mcp').
- files_mcp/service.py: new file exporting SERVICE = McpService(
name='files', app=files_mcp.app, mount_path='/files-mcp').
The McpService dataclass itself moved to common/mcp_service.py in the
prior commit. 327 existing tests pass unchanged.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds common/mcp_service.py with the McpService dataclass (moved from
main.py), BUILTIN_SERVICES list, _filter_by_env, and discover_and_filter.
MCP_SERVICES env var acts as a whitelist (unset=all, empty=zero).
Adds tests/unit/test_mcp_service.py with 12 cases covering: load order,
broken-import handling (warn vs raise by env), missing/wrong-typed
SERVICE, duplicate name and duplicate mount_path detection, env filter
(unset/empty/subset/order), and unknown-name error.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Self-review caught a contradiction: the spec's prose said
'MCP_SERVICES="" (empty) -> zero services mount' but the code example
in the spec used 'if not raw: return services' which conflates unset
and empty (both fall through to 'all mount').
Fix: _filter_by_env now uses 'MCP_SERVICES' not in os.environ to
distinguish unset (backward compat: all mount) from empty (explicit
choice: zero mount). Test cases already specified the correct behavior.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds docs/superpowers/specs/2026-06-30-pluggable-mcp-design.md describing
a refactor of main.py: McpService dataclass moves to common/mcp_service.py
alongside BUILTIN_SERVICES list and discover_and_filter() helper. Each
service package gets a service.py exporting a SERVICE McpService instance.
Deployments opt in/out via MCP_SERVICES env whitelist (unset = all).
Branch: feat/files-mcp (continues from file-MCP service work; same branch,
not pushed; can be split into separate PRs later)
Backward compat: 0 existing tools / tests / dependencies changed;
unsetting MCP_SERVICES reproduces today's behavior exactly.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- main.py: import files_app, append McpService entry to MCP_SERVICES.
Lifespan already iterates the list, so no edit to the lifespan function.
- tests/conftest.py: autouse fixture rebinds files_mcp.core.path_guard._ROOT
to a per-test tmp_path so the sandbox is fresh for every test.
- files_mcp/__init__.py: drop the try/except around the server import now
that server.py exists, restoring fail-fast on a missing/corrupt server
module in production. The _init_root() try/except stays, since pytest
collection can import the package before FILES_MCP_ROOT is set.
Verified via uv run pytest: 327 tests pass, no regressions.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds files_mcp/server.py (FastAPI title=Files MCP, 9 POST routes, and
3 exception handlers: KeyError->404, ValueError->400, FileExistsError->409).
The third handler is the only one not present in spark_executor.server;
handlers are registered on this app only, so spark_executor is unaffected.
Adds tests/integration/test_files_mcp_routes.py with 12 end-to-end HTTP
tests covering CRUD happy paths, error codes (404/400/409), and
introspection (list_dir, stat, search, move, copy).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds files_mcp/tools/requests.py with 9 Pydantic body models (one per
tool, since fastapi-mcp passes args as JSON body) and
files_mcp/tools/files.py with 9 thin wrappers that call path_guard
then fs_ops and return model_dump() form. Adds tests/unit/test_files_tool.py
covering happy path, sandbox escape, and KeyError/FileExistsError
propagation for each wrapper.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds files_mcp/models.py (FileEntry, OperationResult) and
files_mcp/core/fs_ops.py with 9 pure file operations on already-resolved
Path objects: create/read/update/delete/list_dir/stat/search/move/copy.
All writes go through tempfile.mkstemp + os.replace for atomicity;
read enforces utf-8 + 1MB cap; create/update enforce strict presence
semantics (create: absent or overwrite; update: present). 39 unit tests
in tests/unit/test_fs_ops.py.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds the files_mcp/ sibling package and core/path_guard.py which
resolves arbitrary input paths and rejects any that escape the
FILES_MCP_ROOT sandbox (handles .., ~, symlinks, NUL, empty). Adds
tests/unit/test_path_guard.py covering accept, escape modes,
symlink escape, and _init_root failure paths.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds docs/superpowers/plans/2026-06-30-files-mcp.md — 5-task
implementation plan covering scaffold+path_guard, models+fs_ops,
business wrappers+request models, server+routes+integration tests,
and main.py+conftest wiring. 77 new tests across 3 unit files and
1 integration file; verified acceptance includes the spark_executor
test suite (242 existing) still passing unchanged.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Adds docs/superpowers/specs/2026-06-30-files-mcp-design.md describing a
sibling FastAPI sub-app (mount: /files-mcp) that exposes 9 file operations
(create/read/update/delete/list_dir/stat/search/move/copy) over fastapi-mcp,
sandboxed to FILES_MCP_ROOT with utf-8 text only and atomic writes.
Branch: feat/files-mcp
Mount path: /files-mcp
New package: files_mcp/
Backward compat: 0 existing tools / tests / dependencies changed.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Audit of the previous 'salvage application_id from stderr' change
(e4fb97c) revealed the fix was incomplete:
- The pending record now had pending.application_id set on failure.
- But get_job_logs / get_job_status / kill_job all look up the
application_id through JobStore.get_either(application_id), which
scans the on-disk jobs.json for a Job record matching that
application_id. No Job record was being created on the failure
path, so the recovered application_id was unreachable through
the public tool surface — get_job_logs would 404 with 'No Job
found for id=...application_xxx'.
Fix: when confirm_submit_job's failure branch successfully extracts
an application_id from stderr, it now also creates a Job record (same
shape as the success path: 12-char hex job_id, application_id,
script_path, queue, submit_time, connection, yarn_rm_url). The Job's
only job in this case is to act as the lookup index from the
LLM-facing tools into the underlying YARN app.
The job_id is generated even when application_id recovery fails (so
we always set pending.job_id), but the Job record is only put when
application_id is non-None. That keeps the 'spark-submit died locally
before reaching YARN' case clean: no Job record, get_job_logs gets
the standard 'no logs available' error from yarn_client, not a
404 from JobStore.
Tests:
- test_confirm_recovers_application_id_from_failed_spark_submit now
asserts both that pending.application_id is set AND that a Job
record exists and is findable by both job_id AND application_id.
- test_confirm_failed_spark_submit_without_application_id_keeps_it_none
asserts the negative case: no application_id, no Job record.
Fixture fix: _fresh() now rebinds submit.job_store to use tmp_path.
Previously the test wrote to ./data/jobs.json (the real data dir),
which left stale records between test runs and broke any test that
scanned by application_id. This is a latent bug exposed by the new
assertions; fixing it here also makes every other confirm test
hermetic.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Spark-submit can fail AFTER YARN has already accepted the application
(lost RM connection, PySpark script exited non-zero on the cluster
side, auth expired mid-submit, ...). In those cases the stderr still
contains 'Submitted application <id>', but the previous
confirm_submit_job implementation threw away that signal and recorded
the pending as FAILED with no application_id. The user had no way to
fetch the YARN logs of the failed job.
Fix:
- run_spark_submit now attaches the failed CompletedProcess to the
SparkSubmitError as .result, so callers can still inspect stderr.
- confirm_submit_job's failure branch tries to parse
(application_id, tracking_url) from exc.result.stderr. If found,
it writes them onto the pending record before re-raising. The
status is still FAILED and the error message is still set — the
user sees the failure normally, but the pending now also has a
log target they can pass to get_application_logs.
- If the stderr has no application_id (e.g. spark-submit failed
locally before reaching YARN), the parse attempt returns None and
the pending is FAILED with no log target — exactly the previous
behavior in that case.
Tests:
- test_confirm_marks_failed_on_spark_submit_error: existing test
now also asserts application_id is None (no result attached).
- test_confirm_recovers_application_id_from_failed_spark_submit:
failed run with 'Submitted application X' in stderr → pending is
FAILED + application_id is set.
- test_confirm_failed_spark_submit_without_application_id_keeps_it_none:
failed run with no application_id in stderr → pending is FAILED
+ application_id is None.
Full suite 248 passed (+2 from 246).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
write_job_file now validates that the incoming code string is encodable
as UTF-8 BEFORE doing any file I/O. A lone surrogate (U+D800..U+DFFF) —
typically the result of a broken decode somewhere upstream in the
agent's pipeline — cannot be represented in UTF-8, and would otherwise
either silently produce mojibake on disk or crash with an opaque
UnicodeEncodeError from open().
Behavior:
- try: code.encode('utf-8')
- on UnicodeEncodeError: log a warning and raise ValueError with a
message that points the agent at 're-generate the code as plain
Unicode'. ValueError -> HTTP 400 via the existing server.py
exception handler, so the agent gets a clean error and no file is
written.
- normal CJK / emoji / accented Latin still work — the encode check
is essentially free.
Tests:
- test_write_accepts_valid_utf8_including_cjk_and_emoji: positive
case for the common LLM output patterns.
- test_write_rejects_lone_surrogate: negative case, also asserts
that tmp_path is empty (no half-written .py left behind).
Full suite 246 passed (+2 from 244).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
confirm_submit_job no longer retries on SparkSubmitError. A failed
single attempt is recorded as-is:
- pending.status = 'FAILED' with the error message in pending.error
- the function re-raises the SparkSubmitError so the caller (and the
agent) sees the failure immediately
- the agent can call get_pending_job to see the persisted FAILED
state, including the captured error
The 'retry' code path was hiding real failures behind transient
recovery: a spark-submit that died because the script is broken looks
the same as one that died because YARN was momentarily unreachable.
Without retries, the agent gets a clean FAILED state and a clear
exception to act on, instead of a delayed, ambiguous result.
Manual recovery is still possible: re-calling confirm_submit_job on a
FAILED pending resets it to PENDING and runs a single fresh attempt
(see test_confirm_resets_failed_then_succeeds). The reset path is
useful for 'I fixed the script, try again' flows; it just is no longer
the default on every failure.
Removed:
- the for-attempt loop and time.sleep in submit.py
- import time in submit.py and test_submit_tool.py
- confirm_max_retries and confirm_retry_delay_seconds from
common/config.py (env vars, snapshot, reload)
- 2 retry tests (test_confirm_retries_spark_submit_failure_then_succeeds,
test_confirm_exhausts_retries_and_sets_failed)
- test_confirm_marks_failed_on_spark_submit_error updated to assert
the new single-attempt + raise contract
- test_confirm_resets_failed_and_retries renamed to
test_confirm_resets_failed_then_succeeds (no more retry loop to
exercise)
Full suite 244 passed (3 fewer than before, matching the removed
tests).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
YARN's 307 Location header is always a fully-qualified absolute URL.
The previous implementation used urllib.parse.urljoin to handle the
hypothetical case of a relative Location, but that's a defensive
codepath for a scenario that does not occur in practice. Drop it:
- Remove 'from urllib.parse import urljoin'.
- _request_following_redirects now assigns the Location value
straight to the next request URL.
- Update the docstring to reflect the new contract.
- Remove the test that exercised the relative-Location path.
Full suite 246 passed (1 fewer than the previous commit, matching the
test that was removed).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
httpx follows redirects by default, but it does NOT forward the
Authorization header across host boundaries. In YARN deployments where
the ResourceManager 307-redirects the NodeManager log fetch to a
different host (load balancer, Knox, NM selection), the follow-up
request lands unauthenticated and returns 401/403.
Replace the _request call in _fetch_logs_via_am_container with
_request_following_redirects, which:
- Walks the 3xx Location chain (up to 3 hops).
- Re-applies auth + verify on every hop.
- Resolves relative Location URLs against the current request URL.
- Raises YarnError on 3xx-without-Location (misconfigured server) and
on hop count overflow (redirect loop protection).
301/302/303/307/308 are all treated as 'follow the Location' per
RFC 7231 — the method/body handling for the rest is up to httpx when
we re-issue the request.
Tests: 4 new (cross-host 307, no-Location 307, two-hop chain, relative
Location). Full suite 247 passed.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The previous commit (32ee167) added a regex-based HTML directory-listing
parser as a fallback when the NodeManager wraps /stdout in HTML (Knox,
wrapped vendor YARN builds). On review, the parser is too brittle to
trust in production:
- Apache-style listings vary across Hadoop distributions; some add
extra rows, icons, or anchors that change the regex hit set.
- Some clusters return the listing but 4xx each individual log file
(auth layer in the way), so the fallback silently returns empty.
- The behavior the agent observes for the same application_id becomes
environment-dependent in ways that are hard to reason about.
Revert to the simple, predictable path: GET /apps/{appid}, read
amContainerLogs, GET {amContainerLogs}/stdout, return its text. If the
cluster wraps /stdout in HTML, the agent gets a clear
'log-aggregation-enable' error and can ask the user to enable log
aggregation or use a different fetch mechanism — much easier to debug
than a half-parsed directory listing.
Removes:
- _looks_like_html, _parse_log_listing_html helpers
- The bare-URL fallback path and its try/except wrapper
- 4 unit tests that exercised the parser
Full suite back to 243 passed (matches fd0036c).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Some YARN deployments (Knox-proxied, wrapped vendor builds) return an
HTML directory listing page for /node/containerlogs/.../stdout instead of
the raw log bytes. The previous fix assumed a clean text/plain response
and failed every log fetch on those clusters.
Two new private helpers in yarn_client.py:
- _looks_like_html(text): cheap prolog sniff (<!doctype html, <html,
<?xml) on the first ~200 chars.
- _parse_log_listing_html(html): extract plain log-file names via a
href="..." regex; drop ../, absolute paths, sort toggles (?C=N;O=D),
and absolute URLs.
_fetch_logs_via_am_container now does:
1. GET /apps/{appid} (unchanged).
2. Read app.amContainerLogs.
3. Fast path: GET {amContainerLogs}/stdout. If 2xx and NOT HTML, return.
4. Fallback: GET {amContainerLogs} (bare URL), parse the HTML listing,
fetch each log file individually, and concatenate with === filename ===
headers in document order.
5. Raise _logs_unavailable_error if both paths yield nothing.
Tests: 3 new (test_logs_falls_back_to_directory_listing_when_stdout_returns_html,
test_logs_html_filter_strips_sort_links_and_parent_dir,
test_logs_html_filter_handles_prelaunch_files,
test_logs_raises_when_directory_listing_empty) — full suite 247 passed.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The previous _fetch_logs_via_nodemanager walked
/apps/{appid}/appattempts -> /apps/{appid}/appattempts/{id}/containers,
but YARN ResourceManager does not expose the second hop. Container
info lives on NodeManager and is not reachable from RM REST, so the
fallback returned _logs_unavailable_error on every cluster without
/aggregated-logs (Hadoop 2.x, log aggregation disabled).
Replace the walk with: GET /apps/{appid} -> read app.amContainerLogs
-> GET {amContainerLogs}/stdout. The URL is provided directly by the
app response, so no container enumeration is needed.
Returns AM (driver) container logs only. Executor container logs
still require yarn.log-aggregation-enable=true (primary /aggregated-logs
path); the existing _logs_unavailable_error message already says that,
so the contract is preserved.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Three changes the user wanted in the operation skill, all aimed at
making the LLM agent pause and ask the human instead of silently
choosing defaults or auto-confirming:
1. At task submission, ASK for connection (which one to use) and
app_name. The service has no default for app_name and rejects
implicit queue/memory/cores/num_executors defaults, so the
agent has to surface these to the user anyway — better to do
it deliberately than to call a tool, get 400, and re-ask.
2. Before prepare_submit_job, ASK for the submit parameters
(queue, executor_memory, executor_cores, num_executors,
extra_args). The agent tells the user the server-side defaults
it WOULD use so they can approve or override, instead of
picking on their behalf.
3. Before confirm_submit_job, present a one-screen summary of
exactly what will run (connection, app_name, script, queue,
resources, tracking URL) and wait for the user's explicit
affirmative ('yes', 'confirm', 'go', 'y', even an emoji).
A non-answer or 'wait' / 'let me think' is NOT consent. The
agent NEVER calls confirm_submit_job without that go-ahead.
Where the changes live in the skill:
* §0 Mindset — added a 4th 'must' about not auto-confirming,
cross-referencing the new §0.1.
* §0.1 Mandatory user confirmations — new sub-section, spells out
the three checkpoints in detail (what to ask, what counts as
consent, what doesn't).
* §2 The canonical happy path — the numbered list now interleaves
CHECKPOINT 1/2/3 lines with the tool calls, so the pauses are
visually unmistakable.
* §8 Common pitfalls — two new rows: 'auto-confirming without
explicit yes' and 'picking defaults on the user's behalf'.
* §9 End-to-end example — the word-count walkthrough now shows
the full ask flow with sample agent / user dialogue at each
checkpoint, ending in 'only NOW may you call confirm_submit_job'.
The example also adds a 'kill_job: ASK THE USER FIRST' note, since
killing a running YARN app is the same class of irreversible action
as confirming a new one.
File: 449 → 557 lines (+108).
No code changes; tests not affected.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The MCP tool was named `generate_job_file` from Stage 2 but it does
NOT generate PySpark code — the calling LLM writes the code in its own
context, and this tool only persists it to a file under
SPARK_EXECUTOR_JOBS_DIR so `spark-submit` can see it. The misleading
`generate_` prefix sent agents (and humans) looking for a code
generator that doesn't exist.
This commit folds three related polish changes into one (split later
with rebase -i if you want them as separate history):
1. The rename itself:
- `tools/generate.py` → `tools/write_job.py`
- `generate_job_file` → `write_job_file`
- `GenerateJobFileRequest` → `WriteJobFileRequest`
- `/generate_job_file` route → `/write_job_file`
- `operation_id="generate_job_file"` → `operation_id="write_job_file"`
The internal helper `core.job_writer.write_job_file` (which just
writes bytes to disk with no SQL guard) is imported with an
`_write_to_disk` alias to avoid the name collision with the
MCP-exposed function in the same module.
The description for the tool now explicitly states 'this tool
does NOT generate PySpark code. The calling LLM is expected to
have already written the code; this tool only persists it.'
2. Skill for LLM agents operating the service
(`docs/superpowers/skills/spark-executor-mcp-operate/SKILL.md`,
449 lines). Covers the 16 tools, the two-step prepare/confirm
flow, the dual-ID contract (job_id vs application_id), the
PendingSubmission state machine, the Connection profile, the
job-file workflow, the error reference, common pitfalls, and a
full end-to-end word-count example.
3. Default `executor_memory` lowered 4G → 2G
(`_DEFAULTS_TO_CONFIRM` in `server.py`). Mirrors the matching
change in `test_mcp_routes.py` and the 5 unit tests that
reference the default. Aligns with the lighter workloads the
service is sized for in its current container profile.
Also tracked in git for the first time:
- `docs/superpowers/plans/2026-06-24-spark-executor-mcp.md`
(the original Stage 1/2/3 design plan, updated to use the new
tool name throughout).
Test rename:
- `tests/unit/test_generate_tool.py` → `test_write_job_tool.py`
- the new test file picks up an extra assertion that the SQL guard
rejects a `DROP TABLE` statement at write time.
243 tests pass (was 242; +1 new SQL-guard assertion). Zero regressions.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Two user-reported bugs, same root cause: the in-memory JobStore + the
'job_id must be the 12-char hex' tool contract.
Bug 1: 'Unknown job_id' reported frequently
JobStore was a process-local dict (spark_executor/core/job_store.py).
Under gunicorn workers > 1, a job created by confirm_submit_job
landing on worker A was invisible to worker B, so a follow-up
get_job_status / get_job_result / get_job_logs / kill_job landing on
a different worker returned 'Unknown job_id'. Same multi-worker
problem that bit the MCP session layer; only the affected data was
different.
Bug 2: 'get_job_logs frequently confuses job_id and application_id'
confirm_submit_job returns BOTH identifiers in SubmitResult, but
get_job_logs (and friends) only accepted the local 12-char job_id
and never said so in their description. When the agent passed the
YARN application_id, the error message itself was misleading:
'Unknown job_id: application_17400000001_0001' — the agent had
passed an id, just the wrong kind.
This change fixes both at the root:
* JobStore is now JSON-backed at data/jobs.json (atomic tempfile +
os.replace), with cross-process safety via fcntl.flock on a sibling
.lock file. Stage 3's SQLite migration is still planned; the file
format is intentionally simple so it is a straight
'for j in read_all(): db.insert(j)'.
* New JobStore.get_either(uid) looks up by job_id first, then
application_id. All four job-lifecycle tools (get_job_status,
get_job_result, get_job_logs, kill_job) call get_either instead
of get(job_id), so the agent can pass either identifier and get
the same answer.
* The 'neither matched' KeyError now spells out both id forms and
what they look like, so the agent isn't left guessing.
* server.py tool descriptions for the four job tools explicitly
state 'job_id accepts BOTH identifiers' so this is visible to the
LLM at tool-selection time, not only at error time.
Tests:
* test_job_store.py: tmp_path isolation, persistence across
instances, human-readable JSON, corrupt-file resilience,
get_either (by job_id, by application_id, collision preference,
unknown), put idempotency.
* test_{logs,status,kill,result}_tool.py: per-test tmp_path fixture,
'accepts application_id' regression for each tool, and an
explicit assertion that the unknown-id error message mentions
BOTH id forms. test_result_raises_keyerror_for_unknown_job's
match pattern updated for the new message.
242 tests pass (was 226; +16 new). Zero regressions.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
fastapi-mcp uses each route's OpenAPI operationId as the MCP tool name
exposed to LLM clients (fastapi_mcp/server.py:619-620). When the
operation_id is omitted, FastAPI auto-generates a name like
`prepare_submit_job_prepare_submit_job_post` (function name + path +
method), which is verbose and ugly for the agent.
This change adds an explicit `operation_id=` to all 17 MCP tool
routes in spark_executor/server.py, named after the handler function
(stripping the leading underscore). Concretely, the LLM client now sees
tool names like `prepare_submit_job`, `confirm_submit_job`,
`get_job_status`, etc. — same as the route paths.
`/health` (the only non-MCP GET route) is left untouched; its
auto-generated name (`health_check_health_get`) is never exposed.
Also adds a regression test
`test_every_mcp_route_has_explicit_clean_operation_id` in
tests/integration/test_mcp_routes.py that:
* fetches the live OpenAPI spec,
* asserts every MCP tool route has an operationId,
* rejects FastAPI's auto-generated format (e.g. operationId ending
in `_post` / `_get`),
* asserts no two routes share an operationId (fastapi-mcp keys
its tool registry by name, so duplicates would shadow silently).
All 226 tests pass.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
MCP sessions live in a per-process in-memory dict on whatever worker
handled the initialize request (StreamableHTTPSessionManager._server_instances).
gunicorn round-robins HTTP requests across workers, so the same
mcp-session-id frequently lands on a different worker and returns
'Session not found' / 'Invalid or expired session ID'. fastapi-mcp
hardcodes stateless=False, and gunicorn has no built-in sticky-session
support, so there is no in-process workaround.
This change keeps the existing workers=2 default (the project does not
want to give up the modest parallelism it gives) but adds:
* A comment block above workers= explaining the constraint, the
workers=1 fix, and the nginx sticky-session escape hatch.
* An on_starting(server) hook that logs a WARNING whenever
workers > 1, naming the symptom and the fix so the misconfig is
loud at deploy time rather than silent at request time.
* Three regression tests in tests/unit/test_gunicorn_config.py
asserting the hook exists, is silent for workers=1, and warns
(with the expected text) for workers>1.
* A 'MCP session affinity' note in CLAUDE.md key conventions so
the constraint is recorded for future maintainers.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
docker-entrypoint.sh now:
- Reads SPARK_EXECUTOR_SPARK_SUBMIT_BIN with default spark-submit and
validates that the configured binary resolves in PATH.
- If the configured binary is an absolute path, prepends its directory
to PATH so subprocess.run can find it.
- Still validates JAVA_HOME/bin/java and SPARK_HOME/bin/spark-submit.
- Writes /etc/profile.d/spark_executor_env.sh with the resolved
JAVA_HOME, SPARK_HOME, SPARK_EXECUTOR_SPARK_SUBMIT_BIN, and PATH so
interactive `docker exec` shells see the same environment as the
main gunicorn process.
This fixes two reported issues:
1. SPARK_EXECUTOR_SPARK_SUBMIT_BIN was ignored because the binary was
not guaranteed to be on PATH.
2. `docker exec` shells did not see JAVA_HOME/SPARK_HOME in PATH.
Add an update_pending_job MCP tool that lets callers modify parameters
of a PENDING submission before confirm_submit_job:
- queue, executor_memory, executor_cores, num_executors
- app_name
- extra_args
- script_path (re-validates file existence and SQL guard)
Only PENDING submissions can be updated; SUBMITTED/CANCELLED/FAILED are
rejected with HTTP 400.
Delete compression="gz" from both debug and info file sinks so logs
are kept as plain text files. Simplifies log inspection and avoids the
small CPU overhead of nightly gzip compression.
tao.chen