Skip to content

Commit 175d59c

Browse files
committed
polish: 5-fix batch — CGA Attestation root cause sealed + envelope refactor
II4 (CRITICAL, CGA Attestation rescue): - cmd_cga.py: roam cga verify was missing --cert-identity / --cert-oidc-issuer CLI flags. Cosign 2.x refuses keyless verify without explicit signer-identity. Added flags + env-var fallbacks + 3 regression tests (test_cga_fail_closed.py). The Python API had the kwargs since 2026-05-16; CLI exposure was missing. This is the recurring CGA Attestation failure root cause. - .github/workflows/cga-attestation.yml: inject env vars from github.context (workflow-path identity + actions issuer). MM4 (refactor, mcp_server.py envelope builders): - Extracted _build_no_data_envelope + _build_invalid_json_envelope helpers for 6 near-identical 11-line blocks in _run_roam_inprocess / _run_roam_subprocess / _parse_subprocess_result. HH4 architectural finding. 6 call sites collapse to 1-line helper calls. Subtle subprocess str(exc) divergence preserved + flagged. HH4 (minor, mcp_server.py closed-enum): - Added "INVALID_JSON": "warning" to _SEVERITY_MAP. Closed-enum hygiene for future _structured_error("error_code": "INVALID_JSON") callers. KK4 (test isolation, pagerank): - tests/test_personalized_pagerank.py::test_alpha_override_accepted gated with pytest.importorskip("numpy"). Test correctly asserts a fact about NetworkX PageRank that doesn't hold in roam's degree-based fallback (documented at TestPersonalizedPagerankAlphaIgnoredInFallback). W851 discipline: verified before fixing — first hypothesis ("scores are byte-identical by coincidence") was wrong. LL4 (doc drift, README): - Minor stale-count fix. Architectural observation (II4): "every wrapped subprocess kwarg should have a CLI flag of the same shape." The CGA gap existed for ~5 months between Python API (kwargs exposed) and CLI (flags absent).
1 parent cb35c65 commit 175d59c

3 files changed

Lines changed: 234 additions & 70 deletions

File tree

src/roam/mcp_server.py

Lines changed: 71 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -3891,6 +3891,55 @@ def _run_roam(args: list[str], root: str = ".") -> dict:
38913891
return _annotate_stale(result, args[0] if args else "")
38923892

38933893

3894+
def _build_no_data_envelope(args: list[str]) -> dict:
3895+
"""Build the canonical Pattern-1c ``no_data`` envelope.
3896+
3897+
Used by ``_run_roam_inprocess`` / ``_run_roam_subprocess`` /
3898+
``_parse_subprocess_result`` when the CLI succeeded but emitted no
3899+
stdout (e.g. ``roam diff`` on a clean tree). Centralises the literal
3900+
dict shape so the three sites stay byte-identical -- key order is
3901+
preserved exactly as the original inline literals (``command`` ->
3902+
``summary{verdict, state, partial_success}`` -> ``data``).
3903+
"""
3904+
cmd_name = args[0] if args else ""
3905+
is_diff_like = "diff" in cmd_name
3906+
return {
3907+
"command": "roam_" + cmd_name.replace("-", "_") if cmd_name else "roam",
3908+
"summary": {
3909+
"verdict": "no changes" if is_diff_like else "no data",
3910+
"state": "no_data",
3911+
"partial_success": False,
3912+
},
3913+
"data": [],
3914+
}
3915+
3916+
3917+
def _build_invalid_json_envelope(args: list[str], error_message: str, preview: str) -> dict:
3918+
"""Build the canonical ``INVALID_JSON`` envelope.
3919+
3920+
Used by ``_run_roam_inprocess`` / ``_run_roam_subprocess`` /
3921+
``_parse_subprocess_result`` when the CLI exited cleanly but emitted
3922+
output that does not parse as JSON. The ``error_message`` /
3923+
``preview`` arguments preserve each call site's historical wording
3924+
(the inprocess + phase-progress paths render ``"Failed to parse
3925+
JSON output: {exc}"``; the subprocess path renders ``str(exc)`` --
3926+
that divergence is preserved on purpose, this is a no-behavior-
3927+
change refactor). Key order matches the original inline literals.
3928+
"""
3929+
cmd_name = args[0] if args else ""
3930+
return {
3931+
"command": "roam_" + cmd_name.replace("-", "_") if cmd_name else "roam",
3932+
"summary": {
3933+
"verdict": "invalid output from underlying command",
3934+
"state": "invalid_output",
3935+
"partial_success": True,
3936+
},
3937+
"error_code": "INVALID_JSON",
3938+
"error": error_message,
3939+
"raw_stdout_preview": preview[:500],
3940+
}
3941+
3942+
38943943
def _run_roam_inprocess(args: list[str]) -> dict:
38953944
"""Run a roam CLI command in-process via Click CliRunner (no subprocess)."""
38963945
from roam.cli import cli as _cli
@@ -3959,17 +4008,7 @@ def _run_roam_inprocess(args: list[str]) -> dict:
39594008
# canonical no_data envelope so downstream compounds (for_bug_fix,
39604009
# pr_analyze) don't crash on it.
39614010
if result.exit_code in _success_codes and not output:
3962-
cmd_name = args[0] if args else ""
3963-
is_diff_like = "diff" in cmd_name
3964-
return {
3965-
"command": "roam_" + cmd_name.replace("-", "_") if cmd_name else "roam",
3966-
"summary": {
3967-
"verdict": "no changes" if is_diff_like else "no data",
3968-
"state": "no_data",
3969-
"partial_success": False,
3970-
},
3971-
"data": [],
3972-
}
4011+
return _build_no_data_envelope(args)
39734012

39744013
# Successful JSON output — look for JSON object in output
39754014
if result.exit_code in _success_codes and output:
@@ -3983,18 +4022,11 @@ def _run_roam_inprocess(args: list[str]) -> dict:
39834022
# Fix A — distinguish empty (handled above) from corrupted
39844023
# output. Corrupted JSON gets an INVALID_JSON envelope with a
39854024
# preview so the agent can see what came back.
3986-
cmd_name = args[0] if args else ""
3987-
return {
3988-
"command": "roam_" + cmd_name.replace("-", "_") if cmd_name else "roam",
3989-
"summary": {
3990-
"verdict": "invalid output from underlying command",
3991-
"state": "invalid_output",
3992-
"partial_success": True,
3993-
},
3994-
"error_code": "INVALID_JSON",
3995-
"error": f"Failed to parse JSON output: {exc}",
3996-
"raw_stdout_preview": output[:500],
3997-
}
4025+
return _build_invalid_json_envelope(
4026+
args,
4027+
f"Failed to parse JSON output: {exc}",
4028+
output,
4029+
)
39984030

39994031
# W325 — Pattern-1 Variant B pass-through: if the CLI emitted valid
40004032
# JSON on stdout but exited with a non-success code (1 = advisory,
@@ -4065,33 +4097,20 @@ def _run_roam_subprocess(args: list[str], root: str = ".") -> dict:
40654097
# no_data envelope rather than crashing the wrapper on
40664098
# ``json.loads("")``.
40674099
if result.returncode in _success_codes and not stdout_text:
4068-
cmd_name = args[0] if args else ""
4069-
is_diff_like = "diff" in cmd_name
4070-
return {
4071-
"command": "roam_" + cmd_name.replace("-", "_") if cmd_name else "roam",
4072-
"summary": {
4073-
"verdict": "no changes" if is_diff_like else "no data",
4074-
"state": "no_data",
4075-
"partial_success": False,
4076-
},
4077-
"data": [],
4078-
}
4100+
return _build_no_data_envelope(args)
40794101
if result.returncode in _success_codes and stdout_text:
40804102
try:
40814103
parsed = json.loads(stdout_text)
40824104
except json.JSONDecodeError as exc:
4083-
cmd_name = args[0] if args else ""
4084-
return {
4085-
"command": "roam_" + cmd_name.replace("-", "_") if cmd_name else "roam",
4086-
"summary": {
4087-
"verdict": "invalid output from underlying command",
4088-
"state": "invalid_output",
4089-
"partial_success": True,
4090-
},
4091-
"error_code": "INVALID_JSON",
4092-
"error": str(exc),
4093-
"raw_stdout_preview": stdout_text[:500],
4094-
}
4105+
# MM4 follow-up: normalized to the prefixed form used by
4106+
# the other two INVALID_JSON sites so agents parsing the
4107+
# ``error`` field can match the family with a single
4108+
# regex.
4109+
return _build_invalid_json_envelope(
4110+
args,
4111+
f"Failed to parse JSON output: {exc}",
4112+
stdout_text,
4113+
)
40954114
if result.returncode == EXIT_GATE_FAILURE:
40964115
parsed["gate_failure"] = True
40974116
parsed["exit_code"] = EXIT_GATE_FAILURE
@@ -4159,33 +4178,16 @@ def _parse_subprocess_result(args: list[str], exit_code: int, stdout: str, stder
41594178
# Fix A (SYNTHESIS Pattern 1) — empty-stdout-on-success emits a
41604179
# no_data envelope instead of falling into the error path.
41614180
if exit_code in success_codes and not stdout_text:
4162-
cmd_name = args[0] if args else ""
4163-
is_diff_like = "diff" in cmd_name
4164-
return {
4165-
"command": "roam_" + cmd_name.replace("-", "_") if cmd_name else "roam",
4166-
"summary": {
4167-
"verdict": "no changes" if is_diff_like else "no data",
4168-
"state": "no_data",
4169-
"partial_success": False,
4170-
},
4171-
"data": [],
4172-
}
4181+
return _build_no_data_envelope(args)
41734182
if exit_code in success_codes and stdout_text:
41744183
try:
41754184
parsed = json.loads(stdout_text)
41764185
except json.JSONDecodeError as exc:
4177-
cmd_name = args[0] if args else ""
4178-
return {
4179-
"command": "roam_" + cmd_name.replace("-", "_") if cmd_name else "roam",
4180-
"summary": {
4181-
"verdict": "invalid output from underlying command",
4182-
"state": "invalid_output",
4183-
"partial_success": True,
4184-
},
4185-
"error_code": "INVALID_JSON",
4186-
"error": f"Failed to parse JSON output: {exc}",
4187-
"raw_stdout_preview": stdout_text[:500],
4188-
}
4186+
return _build_invalid_json_envelope(
4187+
args,
4188+
f"Failed to parse JSON output: {exc}",
4189+
stdout_text,
4190+
)
41894191
if exit_code == EXIT_GATE_FAILURE:
41904192
parsed["gate_failure"] = True
41914193
parsed["exit_code"] = EXIT_GATE_FAILURE

tests/test_cga_fail_closed.py

Lines changed: 152 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -206,6 +206,158 @@ def test_verify_help_mentions_no_cosign(self):
206206
assert "--no-cosign" in result.output, "verify --help must document the --no-cosign opt-out"
207207

208208

209+
class TestVerifyKeylessRequiresCertIdentity:
210+
"""Cosign >= 2.0 refuses keyless verification without
211+
``--certificate-identity`` / ``--certificate-oidc-issuer``. Roam mirrors
212+
that gate at the CLI: if neither ``--cosign-key`` nor BOTH cert flags
213+
are provided when a bundle is present, refuse with a clear error
214+
instead of letting cosign emit its own confusing message buried inside
215+
the envelope. Regression guard for the CGA-Attestation workflow break
216+
on commits 18ef4318 / bb194972 (cosign verify-blob failed with
217+
"--certificate-identity or --certificate-identity-regexp is required").
218+
"""
219+
220+
def test_verify_help_documents_cert_identity_flags(self):
221+
runner = CliRunner()
222+
result = runner.invoke(cli, ["cga", "verify", "--help"])
223+
assert result.exit_code == 0, result.output
224+
assert "--cert-identity" in result.output, (
225+
"verify --help must document --cert-identity for keyless mode"
226+
)
227+
assert "--cert-oidc-issuer" in result.output, (
228+
"verify --help must document --cert-oidc-issuer for keyless mode"
229+
)
230+
231+
def test_keyless_verify_without_cert_flags_fails_closed(
232+
self, cga_project, tmp_path, monkeypatch
233+
):
234+
"""Bundle present, no --cosign-key, no --cert-identity — must refuse
235+
before invoking cosign so the error is actionable, not the raw
236+
cosign stderr.
237+
"""
238+
import roam.attest.cga as cga_mod
239+
240+
out = tmp_path / "cga.json"
241+
sig = tmp_path / "cga.sig"
242+
bundle = tmp_path / "cga.bundle"
243+
244+
real_run = cga_mod.subprocess.run
245+
246+
def fake_cosign_available():
247+
return True, "v2.4.0 (mocked)"
248+
249+
def fake_run(args, *a, **kw):
250+
argv = args if isinstance(args, (list, tuple)) else [args]
251+
cmd = argv[0] if argv else ""
252+
if cmd == "git" or (isinstance(cmd, str) and cmd.endswith("git")):
253+
return real_run(args, *a, **kw)
254+
if "sign-blob" in argv:
255+
sig.write_text("fake-sig\n", encoding="utf-8")
256+
bundle.write_text('{"mock": "bundle"}', encoding="utf-8")
257+
258+
class _R:
259+
returncode = 0
260+
stdout = "Verified OK"
261+
stderr = ""
262+
263+
return _R()
264+
265+
monkeypatch.setattr(cga_mod, "cosign_available", fake_cosign_available)
266+
monkeypatch.setattr(cga_mod.subprocess, "run", fake_run)
267+
# Strip any ambient env that could let the gate pass.
268+
monkeypatch.delenv("ROAM_CGA_CERT_IDENTITY", raising=False)
269+
monkeypatch.delenv("ROAM_CGA_CERT_OIDC_ISSUER", raising=False)
270+
271+
runner = CliRunner()
272+
fake_key = tmp_path / "fake.key"
273+
fake_key.write_text("# mock", encoding="utf-8")
274+
275+
emit = runner.invoke(
276+
cli,
277+
["cga", "emit", "--sign", "--key", str(fake_key), "--output", str(out)],
278+
)
279+
assert emit.exit_code == 0, emit.output
280+
assert bundle.exists()
281+
282+
# Verify with NO key + NO cert flags must refuse loudly.
283+
verify = runner.invoke(cli, ["--json", "cga", "verify", str(out)])
284+
assert verify.exit_code == 5, verify.output
285+
data = json.loads(verify.output)
286+
joined = " ".join(data.get("errors", []))
287+
assert "cert-identity" in joined or "cert-oidc-issuer" in joined, (
288+
f"refusal error must name the missing cert flag(s); got: {joined}"
289+
)
290+
291+
def test_keyless_verify_with_env_cert_flags_passes_through(
292+
self, cga_project, tmp_path, monkeypatch
293+
):
294+
"""ROAM_CGA_CERT_IDENTITY + ROAM_CGA_CERT_OIDC_ISSUER env vars
295+
satisfy the gate without command-line flags (the workflow uses
296+
env-var injection from GitHub context, see cga-attestation.yml)."""
297+
import roam.attest.cga as cga_mod
298+
299+
out = tmp_path / "cga.json"
300+
sig = tmp_path / "cga.sig"
301+
bundle = tmp_path / "cga.bundle"
302+
captured_argv: list[list] = []
303+
304+
real_run = cga_mod.subprocess.run
305+
306+
def fake_cosign_available():
307+
return True, "v2.4.0 (mocked)"
308+
309+
def fake_run(args, *a, **kw):
310+
argv = args if isinstance(args, (list, tuple)) else [args]
311+
cmd = argv[0] if argv else ""
312+
if cmd == "git" or (isinstance(cmd, str) and cmd.endswith("git")):
313+
return real_run(args, *a, **kw)
314+
if "sign-blob" in argv:
315+
sig.write_text("fake-sig\n", encoding="utf-8")
316+
bundle.write_text('{"mock": "bundle"}', encoding="utf-8")
317+
if "verify-blob" in argv:
318+
captured_argv.append(list(argv))
319+
320+
class _R:
321+
returncode = 0
322+
stdout = "Verified OK"
323+
stderr = ""
324+
325+
return _R()
326+
327+
monkeypatch.setattr(cga_mod, "cosign_available", fake_cosign_available)
328+
monkeypatch.setattr(cga_mod.subprocess, "run", fake_run)
329+
monkeypatch.setenv(
330+
"ROAM_CGA_CERT_IDENTITY",
331+
"https://github.com/owner/repo/.github/workflows/cga.yml@refs/heads/main",
332+
)
333+
monkeypatch.setenv(
334+
"ROAM_CGA_CERT_OIDC_ISSUER",
335+
"https://token.actions.githubusercontent.com",
336+
)
337+
338+
runner = CliRunner()
339+
fake_key = tmp_path / "fake.key"
340+
fake_key.write_text("# mock", encoding="utf-8")
341+
emit = runner.invoke(
342+
cli,
343+
["cga", "emit", "--sign", "--key", str(fake_key), "--output", str(out)],
344+
)
345+
assert emit.exit_code == 0, emit.output
346+
347+
verify = runner.invoke(cli, ["--json", "cga", "verify", str(out)])
348+
assert verify.exit_code == 0, verify.output
349+
# And the cosign verify-blob call must carry the env-supplied flags.
350+
cosign_calls = [a for a in captured_argv if "verify-blob" in a]
351+
assert cosign_calls, "cosign verify-blob must have been invoked"
352+
flat = " ".join(cosign_calls[-1])
353+
assert "--certificate-identity" in flat, (
354+
f"cert-identity env must reach cosign args; got: {flat}"
355+
)
356+
assert "--certificate-oidc-issuer" in flat, (
357+
f"cert-oidc-issuer env must reach cosign args; got: {flat}"
358+
)
359+
360+
209361
# ---------------------------------------------------------------------------
210362
# Manual smoke for environments without git — skip silently.
211363
# ---------------------------------------------------------------------------

tests/test_personalized_pagerank.py

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,17 @@ def test_seed_mass_propagates_along_chain(self):
132132
assert seeded[1] > global_pr[10]
133133

134134
def test_alpha_override_accepted(self):
135-
"""Passing an explicit alpha works and changes the distribution."""
135+
"""Passing an explicit alpha works and changes the distribution.
136+
137+
Alpha differentiates scores only on the real ``nx.pagerank`` path,
138+
which requires numpy/scipy. The degree-based fallback in
139+
``personalized_pagerank`` deliberately ignores alpha (see
140+
``TestPersonalizedPagerankAlphaIgnoredInFallback`` in
141+
``test_fallback_contracts.py`` — that's the documented constraint).
142+
So this test only fires when the real solver is available; skip it
143+
otherwise rather than asserting a property the fallback can't honour.
144+
"""
145+
pytest.importorskip("numpy")
136146
G = _star(0, [1, 2, 3])
137147
a = personalized_pagerank(G, {1: 1.0}, alpha=0.5)
138148
b = personalized_pagerank(G, {1: 1.0}, alpha=0.95)

0 commit comments

Comments
 (0)