fix(launchpad): drop double-apostrophe in the projector's args refusal, and test what it says (#239) - #282
Conversation
…ssage (#239) persona {name!r}'s MCP server {name!r} rendered as "'the-professor''s" -- two adjacent apostrophes from stacking repr() quoting with a possessive. Reworded to "MCP server {name!r} for persona {name!r}" instead. Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
…y say (#239) Both MCP refusal tests only asserted that ProjectionError was raised, so the doubled apostrophe in "persona 'bot''s MCP server ..." was invisible to the suite -- the wording fix in the previous commit had no guard behind it. Three changes, all to what is asserted rather than to the projector: - The plurality and args refusals now assert the server names and the dropped args appear in the message. A refusal an operator cannot act on is barely better than a silent drop. - Fixture servers renamed "a"/"b" -> "alpha-tools"/"beta-tools". A one-letter name is a substring of half the words in these messages, so asserting on it would pass whether or not the name was ever interpolated. - A new test asserts no refusal message contains "''". Verified it fails against the pre-fix wording ("persona 'bot''s ...") and passes after -- the two tests above cannot see it, because both assert on names and args rather than on the punctuation between them. Signed-off-by: Serina Mcfall <serina.mcfall@gmail.com>
benmitchell11
left a comment
There was a problem hiding this comment.
Independent pass. No existing reviews on this PR yet.
The bug is real and the fix is correct. Diffed against origin/launchpad (base 678008ea4): project-pack.py:152-153 changed the MCP-args refusal from
f"persona {persona.get('name')!r}'s MCP server {server.get('name')!r} "to
f"MCP server {server.get('name')!r} for persona "
f"{persona.get('name')!r} declares args {server['args']!r}, but "{name!r}'s really does render as 'bot''s for a persona named bot — repr()'s closing quote landing directly against the possessive 's. Confirmed by hand: f"{('bot')!r}'s" → "'bot''s".
The new test genuinely locks in the fix, verified red/green myself (not just trusting the PR body's pasted output): I swapped project-pack.py back to the 678008ea4 version, ran test_refusal_messages_do_not_stack_repr_quoting_into_a_possessive in isolation, and got the exact failure the PR body claims:
AssertionError: "''" unexpectedly found in "persona 'bot''s MCP server 'alpha-tools' declares args [...]"
Restored the fix and reran the full suite (python3 -m unittest discover -s launchpad/agents -p "test_project_pack.py" -v): 19/21 pass, including the new test and the reworded/strengthened test_mcp_server_with_args_fails_loudly_rather_than_dropping_them. The 2 failures (test_single_mcp_server_absolute_command_left_untouched, test_single_mcp_server_relative_command_resolved_against_pack_dir) are pre-existing, untouched-by-this-diff Windows path-separator artifacts (PosixPath-style assertions like /usr/bin/tools-server failing against Windows' \-joined Path) — not introduced by this PR and not present in its diff.
Also independently checked the "one instance in the whole file" claim rather than trusting it: grepped project-pack.py for the !r}' pattern myself — zero hits on the fix branch, and the plurality-refusal message a few lines above (persona {name!r} declares {n} MCP servers ({names})) doesn't have the same shape since declares intervenes rather than a possessive, so it's correctly left alone. Agree with the PR's decision not to touch it in this PR.
One very minor accuracy nit, not blocking: the PR body says "The file has eight raise ProjectionError sites"; I count seven (grep -n "raise ProjectionError" project-pack.py → lines 55, 76, 91, 98, 112, 141, 151). Doesn't change the scan's conclusion (still 0 other !r}' hits), just a miscount in the writeup.
Clean, appropriately narrow, and the regression test does real work. No blockers.
serina-mcfall
left a comment
There was a problem hiding this comment.
Independent review — no blockers, nothing to fix
I am an agent and do not approve — posting as a comment. Note this is @serina-mcfall's own PR, so it needs a different approver regardless.
The bug is real and still present on launchpad
$ git show origin/launchpad:launchpad/agents/project-pack.py | sed -n '152p'
f"persona {persona.get('name')!r}'s MCP server {server.get('name')!r} "
!r yields 'bot', the possessive appends 's, and the operator reads persona 'bot''s MCP server .... Not on origin/launchpad in any form yet — I checked.
The fix is the right shape
It restructures the sentence to MCP server {name!r} for persona {name!r} rather than escaping around !r or dropping the quoting. That removes the possessive entirely instead of patching its symptom, so the defect class cannot recur at this site.
The regression test is earned, and I verified that rather than assuming it
This is the part worth calling out. test_refusal_messages_do_not_stack_repr_quoting_into_a_possessive asserts assertNotIn("''", str(caught.exception)) — aimed at the defect class, not the specific wording. I reverted only the message change on a scratch copy and re-ran:
FAIL: test_refusal_messages_do_not_stack_repr_quoting_into_a_possessive
AssertionError: "''" unexpectedly found in "persona 'bot''s MCP server 'alpha-tools' declares args ['-y', 'server'], but ..."
Ran 21 tests
FAILED (failures=1)
It fails on the old code with the exact bug output and passes on the fix. A regression test that has been shown to fail is worth more than three that have not.
Two further things the suite gets right:
- The fixtures use
alpha-tools/beta-toolswith a comment explaining why nota/b— "a single letter is a substring of half the words in these messages, so asserting on it would pass whether or not the name was ever interpolated." That is the tautological-assertion trap identified before it was walked into. - The new test's docstring states why the two sibling tests could not catch this: they assert on names and args, not on the punctuation between them. Naming the gap the new test closes is exactly what makes it reviewable.
Full suite: Ran 21 tests ... OK.
One housekeeping note, not a finding
A local branch feat/issue-239-projector-step2 carries commit 709c77096, a near-duplicate of this PR's 634396821 with the same headline. It has no open PR, so there is no competing change and nothing to reconcile — but that branch is now redundant with this one and can be dropped once this merges, to avoid someone later resurrecting a second copy of the same fix.
No blockers, no high/medium/low findings. mergeable currently reads UNKNOWN, which is usually GitHub still computing — worth a glance before merging.
Summary
Two commits against merged STEP 2 code (#260): a wording fix to one projector refusal message, and the test assertions that make that class of defect visible to the suite at all.
Related issue
Refs #239
Issue type
Bug
Agent provenance
Objective
launchpad/agents/project-pack.pyonlaunchpadrenders its args refusal as:'bot''sis two adjacent apostrophes —repr()'s closing quote butted against apossessive. The message an operator reads when the projector refuses to project
their pack is the whole interface of that refusal, so it should not look corrupt.
The commit fixing it was written before #260 merged but never made it into that
PR, so
launchpadstill carries the defect.Impacted components
launchpad/agents/project-pack.py
launchpad/agents/test_project_pack.py
Approach and rejected alternatives
Reworded to
MCP server {name!r} for persona {name!r}— drops the possessiverather than dropping
!r. Rejected removing the!rand hand-quoting instead:repr()is what makes an empty or whitespace-only persona name visible in themessage, and that is exactly the case where a human most needs to see it.
The second commit is the more substantive half. Both MCP refusal tests asserted
only that
ProjectionErrorwas raised, never what it said, so the doubledapostrophe was invisible to a green suite — the wording fix on its own would have
had no guard behind it. Three changes, all to assertions rather than to the
projector:
args that would have been dropped, actually appear in the message. A refusal
an operator cannot act on is barely better than the silent drop it replaced.
a/b→alpha-tools/beta-tools. Rejected keepingthe one-letter names:
"a"is a substring of half the words in these messages,so
assertIn("a", message)would pass whether or not the name was everinterpolated. The rename is what makes the new assertions mean anything.
''. It is separate from thetwo above on purpose: both of those assert on names and args, so neither can
see the punctuation between them.
Verification
Suite, on the fix:
The assertion that matters is the one that fails on the old wording. Reverted
project-pack.pyto678008ea4(the merge commit of #260) and re-ran, leavingthe new tests in place:
Red against the defect, green against the fix. That is the only evidence that
distinguishes this test from one that cannot fail.
The file has eight
raise ProjectionErrorsites, so a wording fix to one of theminvites the question of whether the others share the shape. Scanned both refs for
a
repr()interpolation immediately followed by an apostrophe (!r}'):One instance in the whole file, and this PR is it.
Not verified
Professor's real pack via
buzz pack inspect --format json— these are unitassertions on message construction only. The pack has exactly one MCP server
and no args, so neither refusal path is reachable with the real pack anyway;
both are fixture-only today.
names appearing. It reads
persona 'bot' declares 2 MCP servers (a, b)—bare, un-
repr()'d names inside parentheses while the persona name isrepr()'d. That inconsistency is real and deliberate-looking but I did notconfirm it was intended, and I did not change it.
ProjectionErrormessages have no assertions on their text.The
!r}'scan above proves none of them carries this defect, which is anarrower claim than their wording being right. Only the two MCP refusals are
covered by content assertions.