From 54a867a6e99784670fa6612d3477d5d31e6397f9 Mon Sep 17 00:00:00 2001 From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com> Date: Tue, 22 Sep 2026 12:20:13 +0530 Subject: [PATCH] fix(skill_manage): batch success rows carry the linter findings and org-sharing note MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Since operations[] became the only call shape (#97295) the batch's success path rebuilt each row as {name, action, file_path, success} and dropped every advisory key the per-op handler attached — lint_warnings/lint_hint from the create linter and the org_sharing note. The model never saw a finding. Carry those keys onto the row, mirroring what the failure path already does for teaching payloads. --- tests/tools/test_skill_manage_batch.py | 11 +++++++++++ tools/skill_manager_batch.py | 11 +++++++++-- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/tests/tools/test_skill_manage_batch.py b/tests/tools/test_skill_manage_batch.py index 7e1c105339..0abf45f3de 100644 --- a/tests/tools/test_skill_manage_batch.py +++ b/tests/tools/test_skill_manage_batch.py @@ -54,6 +54,17 @@ class TestSkillManageBatch(unittest.TestCase): for rel in ("SKILL.md", "references/a.md", "scripts/r.py"): self.assertTrue(os.path.exists(os.path.join(base, rel)), rel) + def test_advisory_findings_ride_on_the_op_result(self): + # operations[] is the only call shape, so a finding the per-op handler attaches must + # survive into the batch row or the linter is silent for every model call. + from tools.skill_linter import _BODY_SOFT_BUDGET_CHARS + self._call("probe", [{"action": "create", "content": SK.format(n="probe")}]) + r = self._call("probe", [{"action": "patch", "old_string": "Step 1.", + "new_string": "- rule; why.\n" * (_BODY_SOFT_BUDGET_CHARS // 12 + 1)}]) + self.assertTrue(r["success"], r) + rules = {w["rule"] for w in r["results"][0]["lint_warnings"]} + self.assertIn("oversized-body", rules) + def test_midbatch_failure_rolls_back_existing_skill(self): self._call("probe", [{"action": "create", "content": SK.format(n="probe")}]) r = self._call("probe", [ diff --git a/tools/skill_manager_batch.py b/tools/skill_manager_batch.py index 5b710935f9..f64b2080a8 100644 --- a/tools/skill_manager_batch.py +++ b/tools/skill_manager_batch.py @@ -181,6 +181,9 @@ def _rollback(snapshots, find_skill): return ("; ".join(notes) if notes else "all touched skills rolled back"), bool(notes) +_ADVISORY_KEYS = ("lint_warnings", "lint_hint", "org_sharing") + + def _skill_manage_batch(operations, default_name: str = None, task_id: str = None, session_id: str = None) -> str: """Apply operations atomically: every touched skill is snapshotted first and any @@ -249,8 +252,12 @@ def _skill_manage_batch(operations, default_name: str = None, task_id: str = Non if k not in ("success", "error") and v is not None: fail.setdefault(k, v) return json.dumps(fail, ensure_ascii=False) - results.append({"name": names[i], "action": op["action"], - "file_path": op.get("file_path"), "success": True}) + entry = {"name": names[i], "action": op["action"], + "file_path": op.get("file_path"), "success": True} + # Advisory payloads (linter findings, org-sharing note) ride on the op result; the + # compact success row otherwise hides them and the model never sees a finding. + entry.update({k: parsed[k] for k in _ADVISORY_KEYS if parsed.get(k) is not None}) + results.append(entry) finally: _smt._skill_gate_bypass.reset(token) if rollback_failed: