fix(sre): repair yertle-sre status, and guard the allowlist against rot - #24
Open
albertcmiller1 wants to merge 1 commit into
Open
fix(sre): repair yertle-sre status, and guard the allowlist against rot#24albertcmiller1 wants to merge 1 commit into
yertle-sre status, and guard the allowlist against rot#24albertcmiller1 wants to merge 1 commit into
Conversation
… rot Audit of src/yertle/ for places that bypass yertle_client turned up a live regression I introduced in #20. `probe_yertle` ran `["yertle", "orgs", "--format", "json"]`. When `orgs` became a group that argv became a usage error (exit 2), so `yertle-sre status` reported "not authenticated — run `yertle login`" to authenticated users, and pointed them at a command that is not how anyone authenticates here. I updated two of the three subprocess call sites in #20 and missed this one. Its tests passed because they mocked `run_cli`, which happily "succeeds" for a command that does not exist — the same mock-hides-the-bug shape as the SRE allowlist last week and the invented hierarchy fixtures this morning. Third instance today, so this commit also closes the class. probe_yertle now calls the SDK in-process. That deletes the string that can drift, answers the question the probe actually asks (are these credentials good) without a process spawn, and reports which backend it reached — more useful than a bare "authenticated". `aws` and `gh` keep shelling out because they genuinely are external CLIs. Its tests now mock the wire layer and cover missing credentials and an unreachable API. New: test_allowlist_only_names_commands_the_cli_actually_has asserts YERTLE_READ_COMMANDS against the Typer app's registered commands and groups, so the allowlist cannot name a command the CLI lacks. Verified it fails on a reintroduced "canvas" rather than passing vacuously. This is the guard proposed twice and not written; the third occurrence earned it. Audit result for the other two bypasses, both deliberate: - mcp/server.py uses raw httpx because FastMCP.from_openapi owns the transport and wants an httpx.AsyncClient; the generated client is sync and cannot be handed to it. - sre/tools/yertle.py shells out on purpose — run_cli is where the timeout and output truncation live, and truncation matters for model context. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UJhtswytWsBWUbDxPkMvUT
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Came out of auditing
src/yertle/for places that bypassyertle_client. Turned up a live regression I introduced in #20.The bug
probe_yertleran["yertle", "orgs", "--format", "json"]. Whenorgsbecame a group, that argv became a usage error (exit 2), so:…for a fully authenticated user, pointing them at a command that isn't how anyone authenticates here. I updated two of the three subprocess call sites in #20 and missed this one.
Its tests passed because they mocked
run_cli, which happily "succeeds" for a command that doesn't exist. Same mock-hides-the-bug shape as the SRE allowlist last week and the invented hierarchy fixtures this morning — third instance today, so this closes the class rather than just the instance.The fix
probe_yertlenow calls the SDK in-process. That deletes the string that can drift, answers the question the probe actually asks — are these credentials good — without a process spawn, and reports which backend it reached:awsandghkeep shelling out, because they genuinely are external CLIs. Tests now mock the wire layer and cover missing credentials and an unreachable API.The guard
test_allowlist_only_names_commands_the_cli_actually_hasassertsYERTLE_READ_COMMANDSagainst the Typer app's registered commands and groups, so the allowlist can't name a command the CLI lacks. Verified it fails on a reintroduced"canvas"rather than passing vacuously.This is the invariant I proposed twice and didn't write. The third occurrence earned it.
Audit result
Only two places in
src/yertle/bypassyertle_client, and both are deliberate:mcp/server.py— rawhttpx, becauseFastMCP.from_openapiowns the transport and wants anhttpx.AsyncClient; the generated client is sync and can't be handed to it.sre/tools/yertle.py— shells out on purpose.run_cliis where the timeout and output truncation live, and truncation matters for model context.One asymmetry worth noting for TODO #15: the MCP server fetches the live
/openapi.jsonat startup, while the SDK and CLI use the pinned generated client. Two freshness models in one package — MCP is never stale but its tool surface can change silently under it; the SDK is stable but can drift behind the backend.Verification
make checkclean, 145 tests.yertle-sre statusverified against a real backend.🤖 Generated with Claude Code
https://claude.ai/code/session_01UJhtswytWsBWUbDxPkMvUT