Skip to content

fix(launchpad): drop double-apostrophe in the projector's args refusal, and test what it says (#239) - #282

Open
serina-mcfall wants to merge 2 commits into
launchpadfrom
fix/projector-args-message-apostrophe
Open

fix(launchpad): drop double-apostrophe in the projector's args refusal, and test what it says (#239)#282
serina-mcfall wants to merge 2 commits into
launchpadfrom
fix/projector-args-message-apostrophe

Conversation

@serina-mcfall

Copy link
Copy Markdown

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

Field Value
Harness / provider Claude Code
Model claude-opus-5
Session reference N/A — harness does not expose a stable run id/URL for this session
Initiating human @serina-mcfall

Objective

launchpad/agents/project-pack.py on launchpad renders its args refusal as:

persona 'bot''s MCP server 'alpha-tools' declares args ['-y', 'server'], but ...

'bot''s is two adjacent apostrophes — repr()'s closing quote butted against a
possessive. 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 launchpad still 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 possessive
rather than dropping !r. Rejected removing the !r and hand-quoting instead:
repr() is what makes an empty or whitespace-only persona name visible in the
message, 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 ProjectionError was raised, never what it said, so the doubled
apostrophe 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:

  • The plurality and args refusals now assert that the server names, and the
    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.
  • Fixture servers renamed a/balpha-tools/beta-tools. Rejected keeping
    the 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 ever
    interpolated. The rename is what makes the new assertions mean anything.
  • One new test asserts no refusal message contains ''. It is separate from the
    two above on purpose: both of those assert on names and args, so neither can
    see the punctuation between them.

Verification

Suite, on the fix:

$ python3 -m unittest test_project_pack
.....................
----------------------------------------------------------------------
Ran 21 tests in 0.063s

OK

The assertion that matters is the one that fails on the old wording. Reverted
project-pack.py to 678008ea4 (the merge commit of #260) and re-ran, leaving
the new tests in place:

$ git checkout 678008ea4 -- launchpad/agents/project-pack.py
$ python3 -m unittest test_project_pack
FAIL: test_refusal_messages_do_not_stack_repr_quoting_into_a_possessive (servers=1)
----------------------------------------------------------------------
AssertionError: "''" unexpectedly found in "persona 'bot''s MCP server
'alpha-tools' declares args ['-y', 'server'], but BUZZ_ACP_MCP_COMMAND is a bare
command string -- buzz-acp's build_mcp_servers() always spawns it with an empty
args list. Projecting only the command would silently drop the required
arguments."

Ran 21 tests in 0.044s

FAILED (failures=1)

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 ProjectionError sites, so a wording fix to one of them
invites the question of whether the others share the shape. Scanned both refs for
a repr() interpolation immediately followed by an apostrophe (!r}'):

origin/launchpad (buggy): 1 hit(s)
   152: f"persona {persona.get('name')!r}'s MCP server {server.get('name')!r} "
fix branch: 0 hit(s)

One instance in the whole file, and this PR is it.

Not verified

  • No end-to-end projector run. Nothing here was exercised against The
    Professor's real pack via buzz pack inspect --format json — these are unit
    assertions 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.
  • The plurality message's wording is untouched and unverified beyond the two
    names appearing.
    It reads persona 'bot' declares 2 MCP servers (a, b)
    bare, un-repr()'d names inside parentheses while the persona name is
    repr()'d. That inconsistency is real and deliberate-looking but I did not
    confirm it was intended, and I did not change it.
  • The six other ProjectionError messages have no assertions on their text.
    The !r}' scan above proves none of them carries this defect, which is a
    narrower claim than their wording being right. Only the two MCP refusals are
    covered by content assertions.

…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>
@serina-mcfall serina-mcfall added the by:agent Filed or authored by an AI agent, not a human label Aug 21, 2026
@serina-mcfall
serina-mcfall marked this pull request as ready for review August 21, 2026 05:12

@benmitchell11 benmitchell11 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 botrepr()'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 serina-mcfall left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-tools with a comment explaining why not a/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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

by:agent Filed or authored by an AI agent, not a human

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants