U-018: c_skill_update + c_skill_rollback (PH2-S2 complete)

update_skill(): writes new Skill content to skills/<skill_id>/SKILL.md and commits it (03-C §3.8: file write + git commit must be atomic -- the DB insert only happens after the commit succeeds, so a git failure naturally leaves no orphaned skill_iteration row, no separate rollback machinery needed), rejects a missing triggered_by_bug_id (MISSING_TRIGGER_REF). rollback_skill(): restores the pre-iteration content from before_git_commit_sha and commits *that* as a new commit + new skill_iteration row (03-C §3.9: rollback is itself a new iteration, not a history rewrite); the rollback record's triggered_by_bug_id reuses the original iteration's, since c_skill_rollback's input contract has no bug_id of its own and that FK is NOT NULL. repo_path has no default (caught and fixed during this Unit, not shipped with the bug): an earlier draft defaulted it to this repo's own root, which would have made any test that forgot to override it silently commit into graphlemon's actual git history. Every call site must now be explicit; git log confirms nothing leaked into this repo while building and testing it. Resolves U-015's deferred skill_defect gap: re-reading 04-P §4.3's own transition table, bug_recorded (c_bug_record_create alone) and skill_iteration_triggered (-> c_skill_update) are separate, sequential transitions, not one atomic call -- so 03-C §3.10's "必须同时创建 skill_iteration_suggestion" is a same-flow requirement, not a same-API-call one. bug_record.py already accepted skill_defect (it was never actually rejected, just undocumented as resolved); update_skill() now fills in bug_record.skill_iteration_suggestion_id once the real iteration exists. Known gap, not this Unit's job: no real Skill content has been migrated from the Hermes profiles yet (05 §5.1 names the skills; agent_profile.py's frontmatter only references them by name) -- tests seed a placeholder skill file, same pattern as U-010/011 seeding placeholder artifacts for upstream producers that don't exist yet. 10 new stdlib unittest cases, all real (git + sqlite, no LLM call needed -- same category as U-013's worktree tests). 167 total across U-001-018, all passing. This closes PH2-S2. REQ-08 is now real code, and U-015's skill_defect note is resolved rather than left permanently deferred. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

U-018: c_skill_update + c_skill_rollback (PH2-S2 complete)
update_skill(): writes new Skill content to skills/<skill_id>/SKILL.md and commits it (03-C §3.8: file write + git commit must be atomic -- the DB insert only happens after the commit succeeds, so a git failure naturally leaves no orphaned skill_iteration row, no separate rollback machinery needed), rejects a missing triggered_by_bug_id (MISSING_TRIGGER_REF). rollback_skill(): restores the pre-iteration content from before_git_commit_sha and commits *that* as a new commit + new skill_iteration row (03-C §3.9: rollback is itself a new iteration, not a history rewrite); the rollback record's triggered_by_bug_id reuses the original iteration's, since c_skill_rollback's input contract has no bug_id of its own and that FK is NOT NULL. repo_path has no default (caught and fixed during this Unit, not shipped with the bug): an earlier draft defaulted it to this repo's own root, which would have made any test that forgot to override it silently commit into graphlemon's actual git history. Every call site must now be explicit; git log confirms nothing leaked into this repo while building and testing it. Resolves U-015's deferred skill_defect gap: re-reading 04-P §4.3's own transition table, bug_recorded (c_bug_record_create alone) and skill_iteration_triggered (-> c_skill_update) are separate, sequential transitions, not one atomic call -- so 03-C §3.10's "必须同时创建 skill_iteration_suggestion" is a same-flow requirement, not a same-API-call one. bug_record.py already accepted skill_defect (it was never actually rejected, just undocumented as resolved); update_skill() now fills in bug_record.skill_iteration_suggestion_id once the real iteration exists. Known gap, not this Unit's job: no real Skill content has been migrated from the Hermes profiles yet (05 §5.1 names the skills; agent_profile.py's frontmatter only references them by name) -- tests seed a placeholder skill file, same pattern as U-010/011 seeding placeholder artifacts for upstream producers that don't exist yet. 10 new stdlib unittest cases, all real (git + sqlite, no LLM call needed -- same category as U-013's worktree tests). 167 total across U-001-018, all passing. This closes PH2-S2. REQ-08 is now real code, and U-015's skill_defect note is resolved rather than left permanently deferred. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
bboonnee
1 parent ba481dde
Showing 5 changed files with 282 additions and 15 deletions
docs/development-plan.md
docs/sdd/unit_matrix.json
src/bug_record.py
src/skill_update.py
src/test_skill_update.py
@@ -158,7 +158,7 @@ PH2-S1/S2/S3/S4 之间除标注的依赖外相互独立，可并行推进。
   - 依赖：U-013
   - Done：REQ-04 happy/boundary/failure 通过
   - **实现补充（2026-07-31）**：这是这几轮里少有的一次不用订正 Done 标准的 Unit——REQ-04 的三个用例都只依赖"给定 lineage 上下文后的多视角判定"，没有牵扯到 U-015 自身依赖之外的能力。"血缘读取"（读 SDD/PRD/BRD/Coder 实现记录）刻意设计成调用方职责（`perspective_fn` 闭包自带上下文），`judge_root_cause` 本身只管三视角≥2/3 多数表决。"双向可追溯矩阵"是 Validator 人设自己的内部推理框架（`qa-task-validation` skill + 迁移后的 AGENTS.md "claim—风险—实现—测试—证据矩阵"，U-004 时已经迁移），不是本 Unit 需要单独建的后端能力
-  - **已知缺口，刻意不实现（2026-07-31）**：`root_cause_category=skill_defect` 时 03-C §3.10 要求"必须同时创建 skill_iteration_suggestion"，但 `skill_iteration` 表要求 `before_git_commit_sha`/`after_git_commit_sha` 均非空，而这两个字段要等真正的 Skill 改写（`c_skill_update`，U-018）发生后才有值——SDD 没定义"建议"和"迭代"两个状态之间怎么过渡。REQ-04 的三个用例都不涉及 `skill_defect`，留给 U-018 到时候解决，没有现场猜一个方案硬填
+  - **已知缺口，2026-07-31 U-018 已解决**：`root_cause_category=skill_defect` 时 03-C §3.10 要求"必须同时创建 skill_iteration_suggestion"，当时 `skill_iteration` 表要求的 `before_git_commit_sha`/`after_git_commit_sha` 还没有真实来源。做 U-018 时按 04-P §4.3 本身的状态表重新确认：`bug_recorded`（`c_bug_record_create` 单独达成）和 `skill_iteration_triggered`（触发 `c_skill_update`）是两个先后独立的状态迁移，不是同一次调用——"必须同时创建"读成"同一条流程线里最终要有"而不是"同一次 API 调用里同步造出来"，`bug_record.skill_iteration_suggestion_id` 从 null 起步，`c_skill_update` 跑完后回填。已放开 `bug_record.py` 接受 `skill_defect`
   - 覆盖：01-M `bug_record`；03-C `c_bug_record_create`；04-P `p_validator_root_cause_trace`；REQ-04
  
 - **U-016 [Risk Gate]** `c_org_loop_route` + `fast_lane`/`full_replan` 快速通道
@@ -178,6 +178,8 @@ PH2-S1/S2/S3/S4 之间除标注的依赖外相互独立，可并行推进。
 - **U-018 [Risk Gate]** `c_skill_update` + `c_skill_rollback` + Git commit 留痕
   - 依赖：U-015（需要 `root_cause_category=skill_defect` 触发）
   - Done：REQ-08 全部用例通过
+  - **实现补充（2026-07-31）**：Skill 文件物理落在 graphlemon 平台自己仓库（`skills/<skill_id>/`，架构文档 §13.1 平台资产，和 U-004 时 `agents/` 的归属判断一致），不是目标项目——`repo_path` 参数**没有默认值**，强制调用方每次显式传，避免测试不小心忘记覆盖就误提交进 graphlemon 自己的真实仓库（写代码时先犯过这个错，当场改掉了）。`c_skill_rollback` 不是历史重写，是"读回滚前那个 commit 的内容，作为一次新的 commit + 新的 `skill_iteration` 记录"提交，回滚记录的 `triggered_by_bug_id` 复用被回滚那次迭代原本的 bug_id（`c_skill_rollback` 输入契约本来就没有单独给 bug_id 的位置，`skill_iteration.triggered_by_bug_id` 又是非空 FK，只有这一个来源可用）
+  - **已知缺口，不是本 Unit 的事**：Hermes 迁移过来的 Skill 内容本身还没有真正搬过来（05§5.1 只列了要迁移的技能名字，`agent_profile.py` 的 `skills:` frontmatter 目前只是引用名字，`skills/<skill_id>/` 在仓库里还没有真实文件）——测试里跟 U-010/U-011 对没建成的上游产物的处理方式一样，自己 seed 一个占位 Skill 文件
   - 覆盖：01-M `skill`/`skill_iteration`；03-C `c_skill_update`/`c_skill_rollback`；REQ-08
  
 ### PH2-S3 护栏双层机制
@@ -31,7 +31,7 @@
     {"id": "U-015", "phase": "PH2", "sub_phase": "PH2-S1", "name": "p_validator_root_cause_trace 实现", "classification": "Risk Gate", "depends_on": ["U-013"], "coverage": {"model": ["bug_record"], "view": [], "controller": ["c_bug_record_create"], "process": ["p_validator_root_cause_trace"], "requirement": ["REQ-04"]}, "done": "REQ-04 happy/boundary/failure 通过"},
     {"id": "U-016", "phase": "PH2", "sub_phase": "PH2-S1", "name": "c_org_loop_route + fast_lane/full_replan（含 close_task_done，见 development-plan.md 2026-07-31 实现补充）", "classification": "Risk Gate", "depends_on": ["U-015"], "coverage": {"model": [], "view": [], "controller": ["c_org_loop_route"], "process": [], "requirement": ["REQ-05", "REQ-09"]}, "done": "REQ-05/REQ-09 全部用例通过（含 3 轮上限 → blocked_pending_human；validating→done 通过新增 close_task_done 打通）"},
     {"id": "U-017", "phase": "PH2", "sub_phase": "PH2-S1", "name": "人为植入 bug 完整闭环验证（见 development-plan.md 2026-07-31 实现补充）", "classification": "Risk Gate", "depends_on": ["U-016"], "coverage": {"model": [], "view": [], "controller": [], "process": [], "requirement": []}, "done": "架构文档 §11 验收标准第 4 条达成：run_fix_loop 串联 U-015 判定 + U-016 路由 + 真实 Coder 修复 + 真实 Validator 复核"},
-    {"id": "U-018", "phase": "PH2", "sub_phase": "PH2-S2", "name": "c_skill_update + c_skill_rollback", "classification": "Risk Gate", "depends_on": ["U-015"], "coverage": {"model": ["skill", "skill_iteration"], "view": [], "controller": ["c_skill_update", "c_skill_rollback"], "process": [], "requirement": ["REQ-08"]}, "done": "REQ-08 全部通过"},
+    {"id": "U-018", "phase": "PH2", "sub_phase": "PH2-S2", "name": "c_skill_update + c_skill_rollback（见 development-plan.md 2026-07-31 实现补充）", "classification": "Risk Gate", "depends_on": ["U-015"], "coverage": {"model": ["skill", "skill_iteration"], "view": [], "controller": ["c_skill_update", "c_skill_rollback"], "process": [], "requirement": ["REQ-08"]}, "done": "REQ-08 全部通过；顺带解决了 U-015 的 skill_defect 缺口（bug_record.skill_iteration_suggestion_id 由 update_skill 回填）"},
     {"id": "U-019", "phase": "PH2", "sub_phase": "PH2-S3", "name": "budget_guardrail_config + c_budget_check", "classification": "Risk Gate", "depends_on": ["U-012"], "coverage": {"model": ["budget_guardrail_config"], "view": [], "controller": ["c_budget_check"], "process": ["p_budget_guardrail"], "requirement": ["REQ-10"]}, "done": "REQ-10 happy/boundary 通过"},
     {"id": "U-020", "phase": "PH2", "sub_phase": "PH2-S3", "name": "p_usage_limit_wait 完整验收", "classification": "Risk Gate", "depends_on": ["U-012"], "coverage": {"model": [], "view": [], "controller": [], "process": ["p_usage_limit_wait"], "requirement": ["REQ-14"]}, "done": "REQ-14 全部通过（含超时兜底）"},
     {"id": "U-021", "phase": "PH2", "sub_phase": "PH2-S4", "name": "Graph Engine 持久化 + 断点恢复", "classification": "Risk Gate", "depends_on": ["U-012", "U-013"], "coverage": {"model": ["work_graph_run", "work_node_execution"], "view": [], "controller": [], "process": ["p_work_graph_execution"], "requirement": ["REQ-06"]}, "done": "REQ-06 happy/boundary/failure 通过"},
@@ -7,19 +7,19 @@ all as required fields, i.e. it&#39;s the &quot;record a bug the Validator has already
 judged" endpoint, called at the end of p_validator_root_cause_trace's
 judging_root_cause -> bug_recorded transition (04-P §4.3).
  
-**Known gap, deliberately unenforced (2026-07-31)**: 03-C §3.10's own text
-says "root_cause_category=skill_defect 时必须同时创建 skill_iteration_suggestion,
-否则视为字段缺失" -- but 01-M's skill_iteration entity requires
-before_git_commit_sha/after_git_commit_sha NOT NULL, fields that can't be
-populated until an actual skill rewrite happens (c_skill_update, U-018), which
-hasn't occurred yet at bug-creation time. The SDD doesn't define what a
-"suggestion" (pre-rewrite) is distinct from an "iteration" (post-rewrite), or
-how skill_iteration's NOT NULL commit-sha fields would be satisfied before
-one exists. Not guessed at here: REQ-04's own three acceptance cases (06 节)
-never exercise root_cause_category=skill_defect, so nothing in this Unit's
-actual Done criterion needs this resolved. Left for U-018 (c_skill_update,
-the Unit that actually creates skill_iteration rows) to resolve when it's
-in scope.
+**skill_defect + skill_iteration_suggestion (resolved 2026-07-31, U-018)**:
+03-C §3.10 says root_cause_category=skill_defect "必须同时创建
+skill_iteration_suggestion", but skill_iteration.before/after_git_commit_sha
+are NOT NULL and can't be populated until an actual skill rewrite happens --
+that's c_skill_update (src/skill_update.py), a later, separate step per
+04-P §4.3's own transition table (bug_recorded is reached by this function
+alone; skill_iteration_triggered -> c_skill_update is a *subsequent*
+transition). So `skill_defect` is accepted here with
+skill_iteration_suggestion_id left null; src/skill_update.py's
+update_skill() fills it in once the iteration actually exists. See that
+module's docstring for the full reasoning -- this was originally deferred
+as an unenforced gap when U-015 was built (U-018 didn't exist yet to close
+the loop) and has since been resolved, not left open.
 """
  
 import json
+"""c_skill_update (03-C §3.8) / c_skill_rollback (03-C §3.9): Skill
+self-iteration (architecture doc §8 decision 1) -- a Validator-triggered
+rewrite takes effect immediately, but every change is its own Git commit
+in graphlemon's own repo (§13.1: Skill 库 is a platform asset, not a
+target-project one -- same reasoning as agents/, see src/agent_profile.py),
+so anything can be inspected/rolled back later.
+
+**Resolves U-015's deferred skill_defect gap** now that this Unit exists to
+close it: 03-C §3.10 says root_cause_category=skill_defect "必须同时创建
+skill_iteration_suggestion", but skill_iteration.before/after_git_commit_sha
+are NOT NULL and can't be populated until an actual rewrite happens --
+which is THIS controller, not create_bug_record. 04-P §4.3's own transition
+table shows the real sequence: bug_recorded is a state reached by
+c_bug_record_create alone, and skill_iteration_triggered (-> c_skill_update)
+is a *separate, later* transition triggered off root_cause_category=
+skill_defect. Read that way, 03-C §3.10's "必须同时创建" is a same-flow
+policy requirement (a skill_defect bug must eventually get an iteration),
+not a same-API-call one. update_skill() closes the loop by writing the new
+iteration_id back onto the originating bug_record once it exists.
+src/bug_record.py's accepted root_cause_category set has been widened to
+include skill_defect accordingly.
+
+**Known gap, not this Unit's job**: no skill content has actually been
+migrated from the Hermes profiles yet (05 §5.1 names the skill IDs;
+src/agent_profile.py's FRONTMATTER `skills:` lists references them by name)
+-- `skills/<skill_id>/` doesn't exist as real files in this repo. That's a
+separate migration concern this Unit doesn't attempt; tests here seed a
+placeholder skill file, same as U-010/011 seeded placeholder artifacts for
+units whose real upstream producer doesn't exist yet.
+"""
+
+import os
+import subprocess
+import uuid
+
+_GIT_IDENTITY = ["-c", "user.name=graphlemon", "-c", "user.email=graphlemon@localhost"]
+
+
+class MissingTriggerRefError(Exception):
+    """MISSING_TRIGGER_REF -- 03-C §3.8: triggered_by_bug_id is required,
+    no source-less Skill changes allowed."""
+
+
+class IterationNotFoundError(Exception):
+    """ITERATION_NOT_FOUND -- 03-C §3.9."""
+
+
+def _skill_file_path(file_path):
+    return file_path if file_path.endswith(".md") else os.path.join(file_path, "SKILL.md")
+
+
+def _current_commit_sha(repo_path):
+    return subprocess.run(
+        ["git", "rev-parse", "HEAD"], cwd=repo_path, capture_output=True, text=True, check=True
+    ).stdout.strip()
+
+
+def _write_and_commit(repo_path, rel_path, content, message):
+    abs_path = os.path.join(repo_path, rel_path)
+    os.makedirs(os.path.dirname(abs_path), exist_ok=True)
+    with open(abs_path, "w", encoding="utf-8") as f:
+        f.write(content)
+    subprocess.run(["git", "add", rel_path], cwd=repo_path, check=True, capture_output=True, text=True)
+    subprocess.run(
+        ["git"] + _GIT_IDENTITY + ["commit", "-m", message], cwd=repo_path, check=True, capture_output=True, text=True
+    )
+    return _current_commit_sha(repo_path)
+
+
+def update_skill(conn, skill_id, triggered_by_bug_id, new_content, change_summary, repo_path):
+    """`repo_path` is required, not defaulted to graphlemon's own checkout --
+    an accidental missing override must fail loudly, not silently commit
+    real Skill-library changes into whichever repo happens to be this
+    module's own location.
+
+    transaction (03-C §3.8: "文件写入 + Git commit 需原子完成，commit 失败则
+    整体回滚，不留半成品 Skill 文件"): the DB insert only happens after the
+    git commit succeeds, so a git failure naturally leaves no DB row -- no
+    separate rollback machinery needed since nothing was written yet."""
+    if not triggered_by_bug_id:
+        raise MissingTriggerRefError("triggered_by_bug_id is required for c_skill_update")
+
+    row = conn.execute("SELECT file_path FROM skill WHERE skill_id = ?", (skill_id,)).fetchone()
+    if row is None:
+        raise ValueError("no such skill: " + skill_id)
+    rel_path = _skill_file_path(row[0])
+    before_sha = _current_commit_sha(repo_path)
+
+    after_sha = _write_and_commit(
+        repo_path, rel_path, new_content,
+        "graphlemon: skill {} iteration, triggered by bug {}".format(skill_id, triggered_by_bug_id),
+    )
+
+    iteration_id = str(uuid.uuid4())
+    conn.execute(
+        "INSERT INTO skill_iteration "
+        "(iteration_id, skill_id, triggered_by_bug_id, before_git_commit_sha, after_git_commit_sha, change_summary) "
+        "VALUES (?, ?, ?, ?, ?, ?)",
+        (iteration_id, skill_id, triggered_by_bug_id, before_sha, after_sha, change_summary),
+    )
+    conn.execute("UPDATE skill SET current_git_commit_sha = ? WHERE skill_id = ?", (after_sha, skill_id))
+    conn.execute(
+        "UPDATE bug_record SET skill_iteration_suggestion_id = ? "
+        "WHERE bug_id = ? AND skill_iteration_suggestion_id IS NULL",
+        (iteration_id, triggered_by_bug_id),
+    )
+    conn.commit()
+    return {"iteration_id": iteration_id, "after_git_commit_sha": after_sha}
+
+
+def rollback_skill(conn, iteration_id, repo_path):
+    """`repo_path` is required, not defaulted -- see update_skill's note.
+
+    "回滚到该次迭代之前的版本" -- restores the content from
+    before_git_commit_sha and commits *that* as a brand-new iteration (03-C
+    §3.9: "回滚本身也生成一条新的 skill_iteration 记录"), not a history rewrite.
+    """
+    row = conn.execute(
+        "SELECT skill_id, before_git_commit_sha, triggered_by_bug_id FROM skill_iteration WHERE iteration_id = ?",
+        (iteration_id,),
+    ).fetchone()
+    if row is None:
+        raise IterationNotFoundError("no such skill_iteration: " + iteration_id)
+    skill_id, before_sha, original_bug_id = row
+
+    rel_path = _skill_file_path(conn.execute("SELECT file_path FROM skill WHERE skill_id = ?", (skill_id,)).fetchone()[0])
+    prior_content = subprocess.run(
+        ["git", "show", "{}:{}".format(before_sha, rel_path)],
+        cwd=repo_path, capture_output=True, text=True, check=True,
+    ).stdout
+
+    current_sha = _current_commit_sha(repo_path)
+    new_after_sha = _write_and_commit(
+        repo_path, rel_path, prior_content, "graphlemon: rollback skill {} iteration {}".format(skill_id, iteration_id)
+    )
+
+    new_iteration_id = str(uuid.uuid4())
+    conn.execute(
+        "INSERT INTO skill_iteration "
+        "(iteration_id, skill_id, triggered_by_bug_id, before_git_commit_sha, after_git_commit_sha, "
+        "change_summary, rollback_of_iteration_id) VALUES (?, ?, ?, ?, ?, ?, ?)",
+        (
+            new_iteration_id, skill_id, original_bug_id, current_sha, new_after_sha,
+            "rollback of iteration " + iteration_id, iteration_id,
+        ),
+    )
+    conn.execute("UPDATE skill SET current_git_commit_sha = ? WHERE skill_id = ?", (new_after_sha, skill_id))
+    conn.commit()
+    return {"new_iteration_id": new_iteration_id}
+"""U-018 tests: c_skill_update / c_skill_rollback, real git in a throwaway
+temp repo (never graphlemon's own -- see skill_update.py's module docstring
+on why repo_path has no default). Sqlite + local git are fast/free, no
+mocking needed.
+
+Run: python3 -m unittest src.test_skill_update -v
+"""
+
+import subprocess
+import tempfile
+import unittest
+
+from src import bug_record, schema, skill_update
+
+
+def _init_git_repo(path):
+    subprocess.run(["git", "init"], cwd=path, check=True, capture_output=True)
+
+
+class SkillUpdateTestBase(unittest.TestCase):
+    def setUp(self):
+        self.tmp = tempfile.TemporaryDirectory()
+        self.addCleanup(self.tmp.cleanup)
+        _init_git_repo(self.tmp.name)
+
+        self.conn = schema.connect(":memory:")
+        schema.create_schema(self.conn)
+        self.conn.execute(
+            "INSERT INTO skill (skill_id, owner_org_node, file_path, current_git_commit_sha, source, status) "
+            "VALUES ('write-brd', 'planner', 'skills/write-brd', 'unknown', 'migrated_from_hermes', 'active')"
+        )
+        self.conn.execute(
+            "INSERT INTO bug_record (bug_id, source, description, root_cause_category, root_cause_detail, "
+            "suggested_route, status) VALUES ('bug-1', 'validator', 'skill produces bad BRDs', 'skill_defect', "
+            "'systematic gap', 'fast_lane', 'routed')"
+        )
+        self.conn.commit()
+
+        skill_update._write_and_commit(self.tmp.name, "skills/write-brd/SKILL.md", "v1 content", "seed skill")
+
+    def tearDown(self):
+        self.conn.close()
+
+
+class UpdateSkillTest(SkillUpdateTestBase):
+    def test_happy_path_creates_iteration_and_commits(self):
+        result = skill_update.update_skill(
+            self.conn, "write-brd", "bug-1", "v2 content", "fix the systematic gap", self.tmp.name
+        )
+        self.assertTrue(result["iteration_id"])
+        self.assertTrue(result["after_git_commit_sha"])
+
+        with open(self.tmp.name + "/skills/write-brd/SKILL.md") as f:
+            self.assertEqual(f.read(), "v2 content")
+
+        row = self.conn.execute(
+            "SELECT skill_id, triggered_by_bug_id, change_summary FROM skill_iteration WHERE iteration_id = ?",
+            (result["iteration_id"],),
+        ).fetchone()
+        self.assertEqual(row, ("write-brd", "bug-1", "fix the systematic gap"))
+
+        current_sha = self.conn.execute("SELECT current_git_commit_sha FROM skill WHERE skill_id = 'write-brd'").fetchone()[0]
+        self.assertEqual(current_sha, result["after_git_commit_sha"])
+
+    def test_fills_in_bug_records_skill_iteration_suggestion_id(self):
+        # this is the loop U-015 deferred: skill_defect bug_records start
+        # with skill_iteration_suggestion_id=null, filled in once the
+        # iteration this bug triggered actually exists
+        result = skill_update.update_skill(
+            self.conn, "write-brd", "bug-1", "v2 content", "fix it", self.tmp.name
+        )
+        suggestion_id = self.conn.execute(
+            "SELECT skill_iteration_suggestion_id FROM bug_record WHERE bug_id = 'bug-1'"
+        ).fetchone()[0]
+        self.assertEqual(suggestion_id, result["iteration_id"])
+
+    def test_missing_trigger_ref_rejected(self):
+        with self.assertRaises(skill_update.MissingTriggerRefError):
+            skill_update.update_skill(self.conn, "write-brd", None, "v2 content", "x", self.tmp.name)
+        count = self.conn.execute("SELECT COUNT(*) FROM skill_iteration").fetchone()[0]
+        self.assertEqual(count, 0)
+
+    def test_each_iteration_produces_its_own_commit(self):
+        skill_update.update_skill(self.conn, "write-brd", "bug-1", "v2", "first fix", self.tmp.name)
+        skill_update.update_skill(self.conn, "write-brd", "bug-1", "v3", "second fix", self.tmp.name)
+        log = subprocess.run(["git", "log", "--oneline"], cwd=self.tmp.name, capture_output=True, text=True)
+        self.assertEqual(len(log.stdout.strip().splitlines()), 3)  # seed + 2 iterations
+
+
+class RollbackSkillTest(SkillUpdateTestBase):
+    def test_rollback_restores_prior_content_as_new_iteration(self):
+        first = skill_update.update_skill(self.conn, "write-brd", "bug-1", "v2 broken", "bad fix", self.tmp.name)
+
+        result = skill_update.rollback_skill(self.conn, first["iteration_id"], self.tmp.name)
+        self.assertTrue(result["new_iteration_id"])
+        self.assertNotEqual(result["new_iteration_id"], first["iteration_id"])
+
+        with open(self.tmp.name + "/skills/write-brd/SKILL.md") as f:
+            self.assertEqual(f.read(), "v1 content")
+
+        row = self.conn.execute(
+            "SELECT rollback_of_iteration_id FROM skill_iteration WHERE iteration_id = ?",
+            (result["new_iteration_id"],),
+        ).fetchone()
+        self.assertEqual(row[0], first["iteration_id"])
+
+        count = self.conn.execute("SELECT COUNT(*) FROM skill_iteration").fetchone()[0]
+        self.assertEqual(count, 2)  # rollback is a new record, not a history rewrite
+
+    def test_nonexistent_iteration_raises_not_found(self):
+        with self.assertRaises(skill_update.IterationNotFoundError):
+            skill_update.rollback_skill(self.conn, "no-such-iteration", self.tmp.name)
+
+
+if __name__ == "__main__":
+    unittest.main()