Skip to content

P0: main CI red — CI failure on dc231f1 #3262

Description

@github-actions

P0: main went red

Workflow: CI concluded failure on main.

Failing run: https://github.com/edobry/minsky/actions/runs/32657875194
HEAD SHA: dc231f19ce6b2ad744942ca7ad7be83394097916
Head commit: fix(mt#4459): Split --arg on the first separator only, so a value keeps its own equals signs

Summary

minsky mcp call --arg key=value silently truncated any value containing a second =, and
reported success.

The cause is at src/commands/mcp/direct-client.ts: the parser used arg.split("=", 2), which
reads like Python's str.split(sep, maxsplit) — cap the number of SPLITS, keep the remainder — but
JavaScript's String.prototype.split(sep, limit) caps the length of the result array and
discards everything past it. So content=a=b=c marshalled as content -> "a", and the loop's
key && value !== undefined guard passed happily on the truncated value.

Found while writing an operational memory through this path during an MCP daemon outage: the
memory's content and description were both stored cut off mid-sentence, under a normal success
payload with a populated record. Nothing at the call site indicated loss.

This matters more than an ordinary parsing bug because minsky mcp call is the documented
fallback when the MCP daemon is unavailable
— it is reached precisely when the normal path is
broken. It also fails OPEN where its sibling gotcha fails CLOSED: passing a numeric param raises a
visible Zod error, so that one teaches itself; this one does not.

Changes

  • Extract the marshalling as a pure, exported parseToolArgs() (the pure-decision-core pattern,
    mt#3629) so it is testable without spawning a server — everything else in that module is IO.
  • Split on indexOf of the first separator and slice, so the value keeps any later =.
    Malformed input is unchanged: no separator (-1) and a leading separator (0) are both still
    skipped rather than written under a bogus key.
  • Document both marshalling rules in mcp call --help: the first-separator rule, and that every
    value is sent as a string (so a numeric or boolean param fails schema validation) — with a
    pointer to native CLI commands as the preferred path for long or structured content.

src/generated/completion-manifest.json is regenerated by the pre-commit hook because the help text
changed.

Verification

  • Negative control recorded (per the test-first evidence requirement): the 7 new tests were run
    against the extracted-but-unfixed parser and 3 failed, showing "a" where "a=b=c" was
    expected. After the one-line fix, 7 pass, 0 fail.
  • Live end-to-end on the real path, not just the unit:
    bun run ./src/cli.ts mcp call debug_echo --arg 'message=alpha=beta=gamma' returns
    "message": "alpha=beta=gamma". Pre-fix this would have been "alpha".
  • mcp call --help rendered from the session build and read in full.
  • validate_typecheck: 0 errors.
  • eslint clean on the three changed source files.

Scope note

src/commands/mcp/inspect-command.ts declares its own --arg option but forwards each pair
verbatim to the inspector CLI as --tool-arg without splitting, so it does not share this
defect and is deliberately untouched.

Schema-aware numeric coercion is out of scope and documented instead — resolved during planning
and recorded on the task. Coercing correctly would require this thin direct client to fetch each
tool's schema before marshalling, which it deliberately does not do.

Related

  • mt#4458 — the same silent-success-with-content-loss class on tasks_spec_patch, filed from the
    same session; still PLANNING, its mechanism unverified.
  • mt#3731 — sibling CLI param-marshalling defect (array params), different mechanism.
  • mt#1576 / mt#4455 — the daemon idle-timeout that forces callers onto this path.

Co-Authored-By: minsky-ai[bot] <minsky-ai[bot]@users.noreply.github.com>

What this means

A push to main triggered CI and the workflow above did not conclude success. Per
CLAUDE.md user preference ("main must never be broken"), this is severity-1.

Diagnostic checklist

  1. Open the failing run URL above; identify which job/step failed.
  2. Check whether the offending PR was merged with a known-failing required check
    (operator-API bypass via gh api PUT /merge despite enforce_admins).
  3. Confirm enforce_admins is currently enabled:
    gh api repos/edobry/minsky/branches/main/protection --jq .enforce_admins.enabled
    
    Expected: true post-mt#1938. If false, that is itself a separate finding.

Recovery

  1. Open a hotfix branch off current main.
  2. Apply the smallest fix that turns CI green (often a formatter pass or a config
    flip).
  3. Land via the standard Minsky session flow:
    tasks_create → session_start → session_commit → session_pr_create → /review-pr → session_pr_merge.
  4. Verify the post-merge main build is green within ~5 minutes.
  5. Close this issue with a link to the hotfix PR.

Cross-references

Metadata

Metadata

Assignees

No one assigned

    Labels

    main-redAuto-filed by .github/workflows/main-watch.yml when main CI fails (mt#1938)p0Severity 0: production breakage requiring immediate attention

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions