Conversation
…stant-count swap must not slip a verb in
Adds (6b)/(6c) to the L4 section. L4_COUNT==31 fails closed on a verb ADDED
and the per-name arms pin the L4/field-notes verbs individually, but a rename
or swap that keeps the count at 31 and touches a verb no arm names (analyze,
grep, the edit trio, ...) passes every existing arm — CONTRIBUTING §2 shape 7
('true but narrower'). (6b) pins the full sorted advertised roster so ANY
add/remove/rename reddens until a human signs for it; (6c) is the mutation
control proving (6b) can see a new verb, plus a named tripwire asserting no
shell/exec-shaped verb is advertised (--run-trace stays CLI-only).
Context: security-audit follow-up. Both audit findings closed clean — M1
(report XSS) not-a-bug, verified end-to-end: jsonesc::escapeHtml \uXXXX-escapes
<>& at emission and all 8 innerHTML sinks escape repo-derived values or
interpolate integers/constexpr vocab. L1 (--run-trace reachable from MCP) was
never a vuln: no such verb exists. This arm is invariant-hardening so that
stays true, not a vuln fix.
Verified on a real build: planted lego->run_trace in kMcpVerbTable + the
tools/list stanza (constant-count swap) — L4_COUNT==31 stayed green, (6b)
reddened with the diff, (6c) tripwire reddened; reverted, gate ALL PASS.
New arm in an existing gate file — no regression.sh or gate-count ride-along.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: redhat-et/ripwire/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe MCP verb check now pins the 31 advertised tools, compares them with the live ChangesMCP roster validation
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Other Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Hi @pt-act — thank you for this, and for the writeup. The failure-mode analysis is exactly right: an What's good, with evidence I re-derived myself (not just re-reading your PR description):
Needed before merge:
Smaller / non-blocking:
What happens next: once Dogfood gapsThis was a diff-review + shell-gate task (which file changed, does it pass, plant three mutations, |
|
Hi @pt-act — thank you again, and sorry for the wait. Your (6b)/(6c) arms were exactly right and needed What we did: re-derived Landed on Landed in #306 (integration train 13), now on |
…s roster PR redhat-et#291 (@pt-act) pinned the pre-train-10 31-verb MCP roster in test/mcpverbscheck.sh's (6b)/(6c) arms. Train 10 (redhat-et#300) added two MCP verbs, affected and rank_by, taking the live roster to 33, which reddened the PR's own gate on rebase. EXPECTED_VERBS is by design a hand-maintained pin, but the VALUE was derived from the live binary rather than typed from memory: built HEAD, called tools/list over --mcp, and took the sorted 33-name output verbatim. Reproved the arm's whole point on this binary before committing: planted an add, a remove, and a count-preserving rename (lego -> run_probe) in src/mcp.h's tools/list stanza (not the kMcpVerbTable mirror near the top of that file, which is not what's served over the wire) and confirmed all three redden (6b), with the rename case also proving the pre-existing count-only arm stays green while (6b) catches it — the gap this PR exists to close. Credit: @pt-act (PR redhat-et#291, mechanism and (6b)/(6c) arms). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…fest pin on the merged tree fix 3 re-captures a Java/Kotlin `import` as @reference.import, which removes the only reference train 12's redhat-et#60 module-scope mint had to own in a .kt file. kotlincheck's §1 counts fall back to 3/31/14, its census row to symbols="23", and §1a inverts: the arm that pinned "exactly ONE module-scope owner, owning the import line" now pins ZERO owners, the import still VISIBLE on --uses with role="import", and no <file-scope> in_id anywhere in the fixture. §7a's mutation arm asserts --callers=square falls 1 -> 0 again, keeping train 12's named-edge form, which is the better arm either way. queries/java/tags.scm's conflict resolved toward fix 3 with train 12's redhat-et#60 consequence note kept as the record of the call this answers. qschemetrip.hash re-derived with UPDATE_GOLDEN=1: kParserVer 118 -> 119 and computeDelta's two new clone-group demotions both move manifest text. NO kQSnapCacheScheme bump, measured rather than argued -- a blob written by ae6e3e7 is refused by this build cold and warm ("cache ...-lean.bin: parser-version -- not used"), and both runs answer 31/14 against the base's 32/15. printf_parity.manifest: help_all only (UPDATE_GOLDEN_EXPECT matched), from fixes 1 and 4's --help lines. docs/COMMANDS.md regenerated through docs/docs_commands_build.py. CHANGELOG: the four honesty fixes, and the three contributions credited to @pt-act (redhat-et#291), @s0undt3ch (redhat-et#298) and @llvm-x86 (redhat-et#302). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2 participants
…stant-count swap must not slip a verb in
Adds arms (6b)/(6c) to
test/mcpverbscheck.sh(L4 section, after thepack_task-absentassertion — reuses the in-scope
l4_field/LIST_OUT2; no second server spawn).The gap
L4_COUNT==31fails closed on a verb added, and the per-name arms pin the L4/field-notesverbs individually — but a rename or swap that keeps the count at 31 and touches a verb no
arm names (
analyze,grep, the edit trio, …) passes every existing arm. "Same count + thesenames present" is strictly weaker than "the roster is exactly this set". A verb that reaches a
subprocess (a hypothetical
run_traceMCP twin of the CLI-only--run-trace) could replace anunnamed read verb with every existing arm green.
The arms
updates
EXPECTED_VERBSconsciously — the point being that a new MCP verb, above all one thatreaches an exec, is signed for, never a silent drift. The one-line ride-along on every
verb-changing PR is deliberate: it is the sign-off.
run_traceinjected into the live list mustdisturb the comparison — guards the vacuous-arm shape, per §2's "an arm that cannot fail")
plus a named tripwire asserting no shell/exec-shaped verb is advertised (
--run-tracestaysCLI-only;
runtracecheck.showns the CLI side).New arm in an existing gate file, per §6.4 — no new gate, so no
regression.shloop move, nogatecount_build.pyride-along (verified:manifestcheckrc=0,gatecountcheckrc=0).Verification (real build, g++ 13.3 / cmake 4.4, ubuntu-24.04)
Live
tools/listfrom the built binary matches the pinned set exactly (31 verbs, sorted,byte-for-byte).
Plant (the gap, demonstrated, not argued): renamed
lego→run_traceinkMcpVerbTable+ thetools/liststanza (constant-count swap) and rebuilt:PASS tools/list shows exactly 31 verbs <- the old count arm alone stays green
FAIL (6b) advertised roster DRIFTED from the pinned set — …
FAIL (6c) an MCP verb named like a shell/exec entry point is advertised — --run-trace must stay CLI-only
Reverted the plant; gate
ALL PASSwith the new arms green.Context
This came out of a security audit of the MCP surface. Both audit findings closed clean and
neither is this PR: the HTML-report XSS candidate was verified not-a-bug end-to-end
(
jsonesc::escapeHtmlescapes at emission; all 8innerHTMLsinks escape repo-derived valuesor interpolate integers/constexpr vocab), and
--run-trace's MCP reachability was confirmedabsent (no such verb;
from_traceparses pasted text and executes nothing). This PR is thedurable artifact of that audit: invariant-hardening so the second property stays true by gate,
not by absence.
Summary by CodeRabbit