Repository navigation
Board management tools - #10
Conversation
gpapakyriakopoulos
left a comment
There was a problem hiding this comment.
Thanks for the board-management work. I reviewed the current head (bd3b972) and am requesting changes for the following regressions before merge:
- Fix the new task column transaction shape.
_build_task_transactions()sends{"type": "column", "value": column_phid}, while the existingManiphestClient.create_column_transaction()uses a list of column PHIDs, as its API contract and workboard test specify. Bothpha_task_updateandpha_task_bulk_updateuse the new builder, so column moves are likely to fail on Phorge. Please reuse the existing helper or list shape and correct the new stub test that currently expects a string. - Make bulk dry-run previews complete. The apply path includes project membership, column, and comment transactions, but
would_changeonly compares priority, status, owner, and points. For example, a project-only dry run returns an empty preview even though the apply call writes the change. Show every requested write in the preview, including operations whose current value is unavailable. - Deduplicate bulk targets by task PHID before applying. Supplying the same task as
T1and its PHID makes twoedit_taskcalls, which can post a comment twice. Numeric aliases such asT1and1can also produce a falsetask not foundresult. Normalize identifiers and edit each resolved task once. - Preserve or version the column-search response contract.
pha_workboard_search_columnschangescolumnsfrom the previous result object (dataand cursor/pagination metadata) to a list. It also returnshas_morewithout anafterinput or a continuation cursor. Please keep existing callers working and provide a way to fetch the next page.
Please also put a hard server-side ceiling on pha_task_aggregate.max_tasks and its page/work budget. A targeted run of the registered handler made 100 API requests and retained 10,000 tasks when the caller supplied max_tasks=10000; a shared deployment with a large task set can consume substantial API and server resources. The hidden-column search loop likewise needs a total scan budget: with include_hidden=False, a targeted run made 51 page requests to return one visible column at limit=1.
The focused tests, Ruff, and compileall pass, but the added tests use stubs/mocks and do not verify these upstream API contracts. Please add regression tests for the preview, alias deduplication, and pagination behavior, plus a live Phorge integration check for column moves and column edits. I could not run live integration tests because no Phorge test container was running.
One additional compatibility check: ProjectClient removes several public column helper methods and changes the edit_column signature. If external Python callers are supported, keep wrappers or document the breaking API change.
|
Thanks. Addressed in fixups 9396584..a69effd.
Budgets: Tests: unit regressions cover the preview, alias dedup, cursor, scan budget and ceiling.
|
gpapakyriakopoulos
left a comment
There was a problem hiding this comment.
Thanks for addressing the review feedback and adding regression coverage. I verified the fixes for task aggregation limits, column transactions, bulk preview and duplicate handling, and workboard pagination. The focused checks and CI pass. Approved.
Add pha_task_edit_comment, backed by the new maniphest.comment.edit Conduit API, which rewrites an existing comment identified by its transaction PHID as returned by pha_task_get_transactions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The column client called project.column.create, project.column.edit and project.column.delete with transaction lists. None of those endpoints existed, so every method was dead code. Replace them with a single edit_column() taking flat parameters, matching the project.column.edit API added to our fork, which creates a column when no PHID is given and edits one otherwise. delete_column, update_column_name and update_column_limit are removed: Phabricator has no notion of deleting a column, and the remaining two are one edit_column() call each. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Five additions to the task and workboard tool surface, all sharing the same transaction and paging helpers: - pha_task_update takes points, column_phid and comment. maniphest.edit has always accepted them; the tool did not expose them. - pha_task_bulk_update applies one change set to up to 500 tasks. It defaults to dry_run, returning the diff against each task's current values so a job can propose a change without holding write access. - pha_task_aggregate counts tasks by column, priority, status, owner, project, points, staleness or month, paging server-side and returning counts only. - pha_task_search_advanced takes a fields projection, so callers can ask for the handful of fields they need instead of the whole task payload. - pha_workboard_search_columns pages past the first 100 columns and reports is_hidden and proxy_phid; pha_workboard_edit_column creates, renames, limits, reorders and hides columns, also dry_run by default. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Wrap dashboard.panel.edit, the only dashboard endpoint upstream exposes, so an agent can keep a hand-built dashboard's panels current: rename one, repoint a query panel at another saved query, or refresh the text of a panel holding the task standard. Like the other write tools it defaults to dry_run. The tool edits only; it takes no panel_type and cannot create. PhabricatorDash- boardPanelEditEngine sets the panel type from setPanelType(), which only the web controllers call, so a panel created over Conduit would have no type and no properties. There is also no dashboard.panel.search and PhabricatorDashboardPanel does not implement PhabricatorConduitResultInterface, so panels cannot be read back: the caller needs the panel PHID from its UI page, and a dry run reports the transactions it would send rather than a diff. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
a69effd to
16edaac
Compare
MCP tools for managing Maniphest workboards. Pairs with skroutz-internal/phabricator#15 — the Conduit half. Neither does anything without the other.
pha_task_edit_comment— wrapsmaniphest.comment.edit.pha_workboard_edit_columnandpha_workboard_search_columns— create, rename, hide and reorder columns; surfaceis_hiddenandproxy_phid, and page past 100 so one call can enumerate a board of 177 columns.pha_task_update— addspoints,column_phidandcomment. Transaction building moved into_build_task_transactions, shared with the bulk path.pha_task_bulk_update— batched edits,dry_rundefaults to true.pha_task_aggregate— server-side counts grouped by column, priority, owner, points or staleness, so a board can be measured without pulling every task.pha_task_search_advanced—fieldsprojection to trim the payload.pha_dashboard_edit_panel— wraps the existingdashboard.panel.edit.Rebased onto current main: the pagination work this branch carried is already in via 8d87fd1, and
due_datefrom e651020 is folded into the shared transaction builder. 229 unit tests pass.Breaking:
ProjectClient.create_column,delete_column,update_column_nameandupdate_column_limitare removed, andedit_columnnow takes flat parameters (column_phid,project_phid,name,hidden,limit,sequence). The removed methods called endpoints that exist on no server.