diff --git a/examples/skills_code_review_agent/ACCEPTANCE_CHECKLIST.md b/examples/skills_code_review_agent/ACCEPTANCE_CHECKLIST.md new file mode 100644 index 00000000..eae21682 --- /dev/null +++ b/examples/skills_code_review_agent/ACCEPTANCE_CHECKLIST.md @@ -0,0 +1,89 @@ +# Acceptance Checklist + +## 标准 1:8 条公开样本必须全部可运行并生成报告 + +- 已覆盖 8 条 fixture: + - `clean.diff` + - `security_issue.diff` + - `async_resource_leak.diff` + - `db_lifecycle_issue.diff` + - `missing_tests.diff` + - `duplicate_finding.diff` + - `sandbox_failure.diff` + - `secret_redaction.diff` +- 已有集成测试和 CLI 路径生成 `review_report.json` 与 `review_report.md` +- Phase 6 额外验证: + - `fixture_runs_ok=8` + - 新增质量门禁测试 `test_all_public_fixtures_generate_reports` + +## 标准 2:隐藏样本高危问题检出率 >= 80%,误报率 <= 15% + +- 当前实现以高信号确定性规则优先: + - `eval` + - `exec` + - `pickle.loads` + - `yaml.load` + - `shell=True` + - secret patterns +- 低置信项自动降级为 `needs_human_review` 或 `warning` +- 当前示例给出工程策略和测试基线,但隐藏样本上的最终指标仍需 PR 前人工复核说明 + +## 标准 3:数据库完整记录 task、sandbox run、finding 和 report + +- SQLite 已持久化: + - `review_tasks` + - `review_inputs` + - `filter_decisions` + - `sandbox_runs` + - `findings` + - `review_reports` +- 已支持 `get_review_bundle(task_id)` 查询完整链路 + +## 标准 4:沙箱具备超时和输出限制,失败不崩 + +- 脚本执行层有 timeout +- stdout/stderr 有统一截断上限 +- sandbox failure / timeout 转换为结构化记录和 finding +- 已有 `sandbox_failure.diff` 测试 + +## 标准 5:敏感信息脱敏检出率 >= 95% + +- 报告和数据库前统一调用 `redactor.py` +- 覆盖: + - API key + - token + - password + - bearer token + - private key +- 已有 `secret_redaction.diff` 集成测试 + +## 标准 6:dry-run / fake model 模式 <= 2 分钟 + +- 主链路不依赖真实模型 +- 规则和脚本执行均为轻量 deterministic 路径 +- 当前测试集运行时间远低于 2 分钟 +- Phase 6 单次 security fixture dry-run 实测约 `9.87s` + +## 标准 7:高风险脚本必须先经过 Filter 决策 + +- 所有 skill 脚本执行前统一经过 `filter_policy.py` +- `deny / needs_human_review` 不直接进入执行 +- 已测试 forbidden path 拦截 + +## 标准 8:报告必须包含关键信息 + +- 当前报告包含: + - findings + - severity stats + - human review items + - filter summary + - sandbox summary + - monitoring summary + - actionable recommendations + +## PR 前仍需复核 + +- README 与最终示例输出是否同步 +- 设计说明是否满足 300-500 字要求 +- 是否需要再补一轮原生 `skill_run` 接入说明 +- 是否需要附上最终 sample outputs 供 reviewer 直接查看 diff --git a/examples/skills_code_review_agent/DESIGN_NOTE.md b/examples/skills_code_review_agent/DESIGN_NOTE.md new file mode 100644 index 00000000..1d3f7d53 --- /dev/null +++ b/examples/skills_code_review_agent/DESIGN_NOTE.md @@ -0,0 +1,9 @@ +# 方案设计说明 + +本方案将自动代码评审拆为“主流程编排 + 可复用 Skill + 受控执行 + 结构化落库”四层。主流程由 `agent/agent.py` 负责,统一接收 diff、repo path 或 fixture,完成输入归一化、diff 解析、规则执行、Filter 决策、skill 脚本调度、报告生成和 SQLite 持久化。`skills/code-review/` 则承载正式的 `code-review` Skill,包括 `SKILL.md`、规则文档、使用文档、脚本契约与三个确定性脚本,用于承接可复用的评审知识与脚本执行面。 + +沙箱隔离策略采用“默认受控脚本执行 + 本地 fallback + 容器接口预留”的实现方式。当前示例通过统一的脚本执行层提供 timeout、输出截断、失败记录和 Filter 前置治理,确保高风险脚本、禁止路径、默认网络访问和超预算输入不能直接进入执行链路。对脚本失败或超时,系统不会整体崩溃,而是转换为可追踪的 `sandbox_runs` 记录和结构化 finding。 + +数据库 schema 采用最小可查询设计,包含 `review_tasks`、`review_inputs`、`filter_decisions`、`sandbox_runs`、`findings` 和 `review_reports` 六张表,支持按 `task_id` 查询完整审查链路。报告输出同时生成 JSON 与 Markdown,两者都包含 findings 摘要、人工复核项、Filter 摘要、sandbox 摘要和监控指标。监控字段聚合总耗时、severity/category 分布、拦截次数和 sandbox 次数,便于回放和评测。 + +去重与降噪通过 `deduper.py` 实现:同类同文件同位置同证据的 finding 会被合并,低置信结果自动降级到 `needs_human_review` 或 `warning`。安全边界通过统一 `redactor.py` 落实,确保 API key、token、password、Bearer token 和私钥内容在报告与数据库中不出现明文。整体设计优先满足验收中的可验证性、可运行性、可审计性和 dry-run 可用性,为后续原生 `skill_run` 深化接入和 PR 收口保留清晰扩展点。 diff --git a/examples/skills_code_review_agent/DEVELOPMENT_PLAN.md b/examples/skills_code_review_agent/DEVELOPMENT_PLAN.md new file mode 100644 index 00000000..f2e165b4 --- /dev/null +++ b/examples/skills_code_review_agent/DEVELOPMENT_PLAN.md @@ -0,0 +1,917 @@ +# 自动代码评审 Agent 开发计划 + +## 1. 背景和价值 + +tRPC-Agent 已经提供了构建自动代码评审系统所需的关键基础设施: + +- Skill 体系可以把可复用工作流封装为 `SKILL.md`、规则文档和脚本,并通过 `skill_load`、`skill_run` 在隔离 workspace 中执行。 +- CodeExecutor 和 Workspace Runtime 支持本地、Container、Cube/E2B 等执行环境,能够为脚本运行、静态检查和测试提供沙箱隔离。 +- Session / Memory / SQL 存储能力可以承载评审任务状态、审查历史、执行日志、结构化 findings 和最终报告。 +- Filter 和 Telemetry 可以对工具调用、模型调用、沙箱执行进行拦截、审计、计数和异常观测。 + +因此,这个题目的重点不是“让模型像人一样评论代码”,而是将 Skill、沙箱、规则、治理、存储、报告和监控串成一条可运行、可验证、可回放、可审计的工程链路。该原型一旦完成,后续不仅可以作为公开示例,还能进一步演进为评测基座、CI 审查助手、风险扫描器和回放测试对象。 + +## 2. 目标和范围 + +### 2.1 总体目标 + +实现一个自动代码评审 Agent 原型,支持以下完整链路: + +1. 读取 `git diff`、PR patch 或本地工作区变更。 +2. 通过 `code-review` Skill 加载规则和脚本。 +3. 在 Filter 策略允许后进入 Container 或 Cube/E2B 沙箱执行必要检查。 +4. 产出结构化 findings,并区分高置信 findings、warnings 和 `needs_human_review`。 +5. 将 review task、input diff 摘要、sandbox run、filter 决策、findings 和最终报告写入 SQLite。 +6. 输出 `review_report.json` 和 `review_report.md`。 +7. 支持 `dry-run / fake model`,在没有真实模型 API Key 时仍能验证解析、沙箱和落库链路。 + +### 2.2 本期范围 + +本期优先实现一个工程上可验证的 MVP,重点保证: + +- 8 条公开样例可完整运行。 +- 高危问题具备确定性检出能力。 +- 沙箱、Filter、脱敏、落库和报告链路完整。 +- 结果字段、监控字段和数据库记录对齐验收标准。 + +### 2.3 非目标 + +以下内容不作为首期核心目标: + +- 依赖真实大模型才可运行的审查逻辑。 +- 复杂的跨 PR 历史学习与长期 Memory 优化。 +- 大规模多仓库并发调度平台。 +- 高级 UI、Web 面板或外部监控平台接入。 + +## 3. 方案原则 + +### 3.1 确定性优先 + +高危问题识别、脱敏、去重、Filter 决策、沙箱失败处理等环节必须以确定性逻辑为主,不能将正确性完全依赖于模型发挥。 + +### 3.2 复用框架优先 + +尽量复用仓库已有的 Skill、Workspace Runtime、Filter、SqlStorage、Runner 等能力,避免在 SDK 核心层做大改动。 + +### 3.3 沙箱默认生产、Local 仅开发回退 + +生产默认执行环境必须是 Container 或 Cube/E2B。Local runtime 只在本地调试、单元测试或环境缺失时作为显式 fallback。 + +### 3.4 先打通链路,再增强智能 + +第一阶段先保证解析、规则、落库、报告、脱敏、超时和 Filter 审批链路可跑;后续如需模型增强,只作为补充说明或建议生成层。 + +## 4. 目标目录结构 + +```text +examples/skills_code_review_agent/ + README.md + DEVELOPMENT_PLAN.md + run_agent.py + agent/ + __init__.py + agent.py + config.py + prompts.py + tools.py + skills/ + code-review/ + SKILL.md + RULES.md + scripts/ + parse_diff.py + run_linters.py + run_tests.py + src/ + __init__.py + review_types.py + input_loader.py + diff_parser.py + rule_engine.py + deduper.py + redactor.py + filter_policy.py + telemetry.py + report_writer.py + storage/ + __init__.py + models.py + repository.py + init_db.py + tests/ + __init__.py + test_code_review_agent.py + fixtures/ + clean.diff + security_issue.diff + async_resource_leak.diff + db_lifecycle_issue.diff + missing_tests.diff + duplicate_finding.diff + sandbox_failure.diff + secret_redaction.diff +``` + +## 5. 模块拆解 + +## 5.1 Agent 编排层 + +### 职责 + +- 接收命令行参数和输入源。 +- 初始化模型、SkillToolSet、runtime、存储和过滤策略。 +- 驱动“输入解析 -> 规则评审 -> Filter 决策 -> 沙箱执行 -> 汇总结果 -> 落库 -> 报告输出”主流程。 + +### 复用能力 + +- `LlmAgent` +- `Runner` +- Skill 基础设施 +- 仓库中的容器和 Cube runtime 创建方式 + +### 需要新写或调整 + +- `agent/agent.py` +- `agent/config.py` +- `agent/prompts.py` +- `agent/tools.py` +- `run_agent.py` + +## 5.2 Skill 层 + +### 职责 + +- 以 `code-review` Skill 的形式封装可复用的代码评审说明、规则文档和脚本。 +- 通过 `SKILL.md` 定义 Skill 名称、用途、何时调用、输入输出约定。 +- 通过脚本目录承载 diff 解析、静态检查、测试执行等可复用工作流。 + +### 复用能力 + +- `skill_load` +- `skill_run` +- Skill repository +- 隔离 workspace 目录结构 + +### 需要新写或调整 + +- `skills/code-review/SKILL.md` +- `skills/code-review/RULES.md` +- `skills/code-review/scripts/*.py` + +## 5.3 输入解析层 + +### 职责 + +- 支持 `--diff-file`、`--repo-path` 和测试 fixture。 +- 统一解析为内部 `ReviewInput` 模型。 +- 从 unified diff 中提取变更文件、hunk、上下文和候选行号。 + +### 需要新写 + +- `src/review_types.py` +- `src/input_loader.py` +- `src/diff_parser.py` + +### 关键输出 + +- 变更文件列表 +- 每个 hunk 的起止行和上下文 +- 新增行、修改行和候选审查行 +- 输入摘要信息,供数据库和报告使用 + +## 5.4 规则引擎层 + +### 职责 + +- 对解析后的输入执行确定性规则检查。 +- 产出结构化 findings。 +- 提供初步 severity、confidence、source 和 recommendation。 + +### 规则覆盖目标 + +首期建议覆盖以下 6 类,至少满足 issue 要求中的 4 类: + +- 安全风险 +- 异步错误 +- 资源泄漏 +- 测试缺失 +- 敏感信息泄漏 +- 数据库事务或连接生命周期问题 + +### 需要新写 + +- `src/rule_engine.py` +- `src/deduper.py` + +### 输出约束 + +每条 finding 至少包含: + +- `severity` +- `category` +- `file` +- `line` +- `title` +- `evidence` +- `recommendation` +- `confidence` +- `source` + +## 5.5 去重和降噪层 + +### 职责 + +- 同一文件、同一行、同一类问题不重复报。 +- 低置信度问题进入 `warnings` 或 `needs_human_review`。 +- 避免重复脚本输出和重复规则命中污染最终报告。 + +### 需要新写 + +- `src/deduper.py` + +### 推荐策略 + +- 去重主键:`category + file + line + normalized_evidence` +- 高置信 findings:`confidence >= 0.8` +- 人工复核项:`0.4 <= confidence < 0.8` +- 低置信 warnings:`confidence < 0.4` + +## 5.6 沙箱执行层 + +### 职责 + +- 在受控执行环境中运行静态检查脚本、单元测试或自定义规则脚本。 +- 统一采集命令、耗时、退出码、stdout/stderr 摘要和错误信息。 + +### 复用能力 + +- Container Workspace Runtime +- Cube/E2B Workspace Runtime +- `skill_run` + +### 需要新写或调整 + +- runtime 选择配置 +- 沙箱执行参数和兜底逻辑 +- 输出采样和摘要写入逻辑 + +### 实施要求 + +- Container 或 Cube 作为默认生产方案 +- Local 只能通过显式配置作为开发回退 +- 每次执行必须支持 timeout 和输出大小限制 +- 沙箱失败不能导致整个任务崩溃 + +## 5.7 安全边界和 Filter 治理层 + +### 职责 + +- 在沙箱执行前进行风险决策。 +- 对高风险脚本、禁止路径、非白名单网络访问和超预算执行进行前置拦截。 +- 将拦截原因写入数据库和报告。 + +### 复用能力 + +- Filter 机制 +- callback 和事件钩子 + +### 需要新写 + +- `src/filter_policy.py` + +### 最低治理规则 + +- 禁止访问敏感路径 +- 禁止明显危险命令组合 +- 限制非白名单网络访问 +- 限制超大 diff、超多文件、超长运行时和超多沙箱调用 +- `deny / needs_human_review` 状态不得直接进入沙箱执行 + +## 5.8 脱敏层 + +### 职责 + +- 对报告、日志、数据库写入和 evidence 字段进行敏感信息脱敏。 +- 防止明文 API Key、token、password 等出现在输出结果中。 + +### 需要新写 + +- `src/redactor.py` + +### 最低要求 + +- 对 diff 摘要、stdout/stderr、evidence、recommendation、Markdown 报告和数据库入库内容统一脱敏 +- 检出率目标对齐验收标准:`>= 95%` + +## 5.9 数据库存储层 + +### 职责 + +- 保存 review task、input diff 摘要、sandbox run、finding、最终报告和监控摘要。 +- 支持按 `task_id` 查询整条评审链路。 + +### 复用能力 + +- `SqlStorage` +- SQLite 作为默认 SQL 实现 + +### 需要新写 + +- `src/storage/models.py` +- `src/storage/repository.py` +- `src/storage/init_db.py` + +### 最小 schema 设计 + +建议至少包含以下实体: + +- `review_tasks` +- `review_inputs` +- `filter_decisions` +- `sandbox_runs` +- `findings` +- `review_reports` + +### 关键字段建议 + +#### `review_tasks` + +- `task_id` +- `status` +- `input_type` +- `runtime_type` +- `dry_run` +- `created_at` +- `finished_at` +- `total_duration_ms` +- `error_type` + +#### `review_inputs` + +- `task_id` +- `diff_sha256` +- `changed_files_count` +- `hunk_count` +- `candidate_line_count` +- `input_summary` + +#### `filter_decisions` + +- `task_id` +- `decision` +- `reason_code` +- `reason_text` +- `target` +- `created_at` + +#### `sandbox_runs` + +- `task_id` +- `run_name` +- `command` +- `exit_code` +- `timed_out` +- `duration_ms` +- `stdout_summary` +- `stderr_summary` + +#### `findings` + +- `task_id` +- `fingerprint` +- `severity` +- `category` +- `file` +- `line` +- `title` +- `evidence` +- `recommendation` +- `confidence` +- `source` +- `needs_human_review` + +#### `review_reports` + +- `task_id` +- `report_json` +- `report_markdown` +- `monitoring_summary` +- `final_verdict` + +## 5.10 报告和监控层 + +### 职责 + +- 输出 `review_report.json` 和 `review_report.md` +- 汇总监控指标和严重级别分布 +- 提供可读、可验收、可归档的最终结论 + +### 需要新写 + +- `src/report_writer.py` +- `src/telemetry.py` + +### 报告必须包含 + +- findings 摘要 +- 严重级别统计 +- 人工复核项 +- Filter 拦截摘要 +- 监控指标 +- 沙箱执行摘要 +- 可执行修复建议 + +## 5.11 Agent / Skill / Filter / Storage / Report 职责对照 + +这一节用于明确不同模块的职责边界,避免后续把编排逻辑、规则逻辑、治理逻辑和输出逻辑混写在一起。 + +### 5.11.1 对照表 + +| 模块 | 主要职责 | 应放入的内容 | 不应放入的内容 | 当前目录落点 | +| --- | --- | --- | --- | --- | +| Agent | 编排整条评审链路,驱动输入、规则、Filter、沙箱、落库和报告 | 主流程函数、运行配置、模块调用顺序、失败兜底、任务状态流转 | 具体规则细节、具体脱敏规则、数据库 schema 细节、Markdown 模板细节 | `run_agent.py`、`agent/agent.py`、`agent/config.py` | +| Skill | 封装可复用的代码评审能力包 | `SKILL.md`、规则文档、脚本目录、输入输出约定、能力说明 | 全局任务状态管理、数据库写入、总报告生成、全局 Filter 决策 | `skills/code-review/` | +| Rule Engine | 在结构化 diff 上运行确定性或启发式规则 | 风险模式匹配、finding 生成、初始 severity / confidence | 直接执行高风险命令、数据库写入、CLI 参数解析 | `src/rule_engine.py` | +| Filter | 对待执行动作做前置治理和审批决策 | 高风险脚本判断、禁止路径检查、预算限制、网络访问限制 | 直接做业务风险识别、最终报告排版、数据库 schema 设计 | `src/filter_policy.py` | +| Sandbox | 在受控环境中执行脚本和检查 | lint/test/脚本执行、timeout、输出限制、执行记录 | 决定是否允许执行、最终结论生成 | `agent/tools.py`、Skill scripts、后续 runtime 封装 | +| Storage | 持久化任务、执行记录和结果 | schema、repository、初始化脚本、按 `task_id` 查询 | 规则匹配、CLI 解析、Markdown 报告排版 | `src/storage/` | +| Report | 生成对外可读结果和监控摘要 | JSON/Markdown 报告、严重级别统计、人工复核项、监控字段汇总 | 规则命中逻辑、Filter 准入判断、数据库底层建表 | `src/report_writer.py`、`src/telemetry.py` | +| Redactor | 脱敏所有可能外显或入库的敏感内容 | evidence、stdout/stderr、报告、数据库写入前内容脱敏 | 风险分类、任务调度、报告结构设计 | `src/redactor.py` | + +### 5.11.2 模块关系 + +推荐的数据流关系如下: + +```text +CLI / fixture + -> Agent + -> Input Loader + -> Diff Parser + -> Rule Engine + -> Deduper + -> Filter + -> Sandbox + -> Redactor + -> Storage + -> Report +``` + +含义如下: + +- Agent 是总控,不是所有业务逻辑的承载者。 +- Skill 是能力包,通过文档和脚本提供可复用能力,但不负责全局任务编排。 +- Rule Engine 只负责“发现问题”,不负责“决定能否执行脚本”。 +- Filter 只负责“动作治理”,不负责“代码风险识别”。 +- Storage 负责“沉淀状态和结果”,不负责“得出结论”。 +- Report 负责“对外呈现”,不负责“底层规则判断”。 + +### 5.11.3 本项目中的放置原则 + +后续开发时应遵循以下放置原则: + +- 如果某段逻辑决定“先做什么、后做什么、失败怎么办”,放在 Agent。 +- 如果某段内容是“这类审查能力怎么复用、有哪些脚本和规则说明”,放在 Skill。 +- 如果某段逻辑是“看到哪种代码模式就产出 finding”,放在 Rule Engine。 +- 如果某段逻辑是“这个脚本能不能跑、是否需要人工审批”,放在 Filter。 +- 如果某段逻辑是“命令怎么在受控环境中运行并记录结果”,放在 Sandbox。 +- 如果某段逻辑是“结果如何写入 SQLite 并可按 task id 查询”,放在 Storage。 +- 如果某段逻辑是“如何生成 review_report.json / review_report.md”,放在 Report。 +- 如果某段逻辑是“如何避免敏感信息出现在报告和数据库中”,放在 Redactor。 + +### 5.11.4 常见误区 + +以下做法应避免: + +- 不要把所有规则说明和脚本调度都塞进 `agent.py`。 +- 不要让 Skill 直接负责数据库写入和全局报告拼装。 +- 不要让 Rule Engine 直接决定高风险脚本是否执行。 +- 不要把脱敏逻辑只放在报告生成最后一步,必须在入库前也执行。 +- 不要在 Storage 层夹带业务判断,Storage 只负责持久化。 +- 不要在 Report 层重新做规则识别,Report 只消费结构化结果。 + +## 6. 端到端执行链路 + +```text +CLI / fixture input + -> 输入加载 + -> diff 解析 + -> 规则引擎初筛 + -> Filter 策略决策 + -> 允许的任务进入 Container / Cube 沙箱执行 + -> 汇总 findings / warnings / needs_human_review + -> 敏感信息脱敏 + -> 写入 SQLite + -> 输出 review_report.json / review_report.md +``` + +## 7. 分阶段实施计划 + +## Phase 1:类型定义与主链路骨架 + +### 目标 + +建立统一的数据模型和主流程骨架,让项目从“目录已创建”进入“可开始串联逻辑”的状态。 + +### 任务 + +- 完成 `src/review_types.py` +- 定义核心对象:`ReviewTask`、`ReviewInput`、`ParsedDiff`、`ReviewFinding`、`SandboxRunRecord`、`FilterDecisionRecord`、`ReviewReport` +- 补齐 `run_agent.py` 的 CLI 参数定义 +- 在 `agent/agent.py` 中确定主流程函数签名 + +### 产出 + +- 统一类型模型 +- 可执行但仍为占位实现的 CLI 入口 +- 主流程函数框架 + +### 验收点 + +- 代码结构固定下来 +- 后续模块可以围绕统一类型并行开发 + +## Phase 2:输入解析与规则 MVP + +### 目标 + +先具备“读输入 + 找问题”的能力,优先完成最核心的确定性评审逻辑 + +### 任务 + +- 实现 `--diff-file`、`--repo-path`、fixture 输入适配 +- 实现 unified diff 解析 +- 实现 6 类规则的第一版 +- 产出结构化 findings +- 实现初版去重和置信度分流 + +### 产出 + +- `src/input_loader.py` +- `src/diff_parser.py` +- `src/rule_engine.py` +- `src/deduper.py` + +### 验收点 + +- 公开样例中的静态风险能够被初步识别 +- finding 结构字段齐全 +- 同类同位置问题不重复报 + +## Phase 3:数据库与报告 + +### 目标 + +补齐“可落库、可查询、可交付”的能力。 + +### 任务 + +- 定义 SQLite schema +- 实现 repository 层 +- 实现 task、finding、sandbox run、report 的落库 +- 生成 `review_report.json` +- 生成 `review_report.md` + +### 产出 + +- `src/storage/models.py` +- `src/storage/repository.py` +- `src/storage/init_db.py` +- `src/report_writer.py` + +### 验收点 + +- 可通过 `task_id` 查到整条评审链路 +- 报告字段和 issue 要求一致 + +## Phase 4:沙箱和 Filter 治理 + +### 目标 + +让系统从“静态规则扫描器”升级为“具备受控执行能力的 Agent”。 + +### 任务 + +- 接入 Container runtime 作为默认生产方案 +- 预留 Cube/E2B 接入点 +- 实现 Local fallback +- 将 lint、test、解析脚本纳入 `skill_run` +- 实现 Filter 前置拦截 +- 对 timeout、输出截断、失败日志做规范化记录 + +### 产出 + +- `agent/tools.py` +- `src/filter_policy.py` +- 经过 skill 驱动的脚本执行链路 + +### 验收点 + +- `deny / needs_human_review` 不会进入沙箱执行 +- 超时或失败不会导致评审整体崩溃 + +## Phase 5:脱敏、dry-run 和 fixture 完整覆盖 + +### 目标 + +补齐真正影响验收的可靠性和可测试性要求。 + +### 任务 + +- 实现统一脱敏逻辑 +- 为 dry-run / fake model 模式提供无模型执行路径 +- 填充 8 条公开 fixture +- 将 fixture 跑通并生成报告 + +### 产出 + +- `src/redactor.py` +- 完整 fixtures +- 稳定的 dry-run 模式 + +### 验收点 + +- 8 条公开样本全部可运行 +- 报告和数据库中不出现敏感信息明文 +- dry-run 模式两分钟内跑完 + +## Phase 6:验收收口与质量门禁 +### 目标 + +对照 issue 的验收标准做最后收口。 + +### 任务 + +- 校对报告内容完整性 +- 复查数据库字段完整性 +- 复查监控指标齐全性 +- 复查 timeout、输出限制、Filter 决策链 +- 复查高危问题检出率和误报率控制思路 + +### 验收点 + +- 所有显式验收项在设计和代码层面均有对应实现 +- README、示例输出和方案设计说明齐备 + +## 8. 测试计划 + +### 8.1 公开 fixture 清单 + +必须至少包含以下 8 类: + +- 无问题 diff +- 安全问题 +- 异步资源泄漏 +- 数据库连接生命周期问题 +- 测试缺失 +- 重复 finding +- 沙箱执行失败 +- 敏感信息脱敏 + +### 8.2 单元测试重点 + +- diff 解析正确性 +- 规则命中准确性 +- finding 去重逻辑 +- 脱敏逻辑 +- Filter 决策逻辑 +- repository 持久化逻辑 + +### 8.3 集成测试重点 + +- dry-run 模式端到端执行 +- 沙箱失败后的兜底行为 +- 报告生成内容完整性 +- `task_id` 查询链路完整性 + +## 9. 验收标准映射 + +### 标准 1:8 条样本必须全部可运行并生成报告 + +- 通过 fixtures 和集成测试覆盖 +- 报告输出路径固定为 `review_report.json` 和 `review_report.md` + +### 标准 2:隐藏样本高危问题检出率 >= 80%,误报率 <= 15% + +- 采用确定性高危规则优先策略 +- 对低置信问题降级到 `warnings` 或 `needs_human_review` +- 优先控制高危类别的 precision 和 recall + +### 标准 3:数据库完整记录 task、sandbox run、finding 和 report + +- 通过 `review_tasks`、`sandbox_runs`、`findings`、`review_reports` 等表保证 +- 提供按 `task_id` 查询的 repository 接口 + +### 标准 4:沙箱具备超时和输出限制,失败不崩 + +- 运行层统一设置 timeout +- 对 stdout/stderr 做大小限制和摘要化 +- 用错误记录替代异常崩溃 + +### 标准 5:敏感信息脱敏检出率 >= 95% + +- 对所有对外输出和入库内容统一走 `redactor` +- 在测试中覆盖 API Key、token、password 等模式 + +### 标准 6:dry-run / fake model 模式 <= 2 分钟 + +- dry-run 不依赖真实模型 +- 规则识别和脚本执行保持轻量 +- fixtures 优先使用小规模输入 + +### 标准 7:高风险脚本必须经过 Filter 决策 + +- 所有沙箱执行统一从 Filter policy 入口进入 +- `deny / needs_human_review` 明确阻断执行 + +### 标准 8:报告内容完整 + +- 在 `report_writer.py` 中固定输出以下 sections: + - findings 摘要 + - 严重级别统计 + - 人工复核项 + - Filter 拦截摘要 + - 监控指标 + - 沙箱执行摘要 + - 可执行修复建议 + +## 10. 风险与应对 + +### 风险 1:规则过于宽泛导致误报率升高 + +- 优先实现高危类别的高精度规则 +- 低置信度问题不直接进入高置信 findings + +### 风险 2:沙箱能力依赖环境,导致本地开发不稳定 + +- 提供 Container 默认方案和 Local fallback +- dry-run 模式避免强依赖真实外部环境 + +### 风险 3:脱敏遗漏导致验收失败 + +- 将脱敏逻辑前置到“报告生成前”和“数据库写入前” +- 为脱敏单独写单元测试 + +### 风险 4:数据库 schema 过于简化,后续难以满足查询和审计 + +- 按 task、filter、sandbox、finding、report 五类记录拆表 +- 保留 JSON summary 字段和可扩展字段 + +## 11. README 与附加交付物计划 + +除代码实现外,还需要同步补齐: + +- 示例 README +- 示例 `review_report.json` +- 示例 `review_report.md` +- 一份 300-500 字方案设计说明,解释: + - Skill 设计 + - 沙箱隔离策略 + - Filter 策略 + - 监控字段 + - 数据库 schema + - 去重降噪 + - 安全边界 + +## 12. 当前建议的实施顺序 + +建议严格按以下顺序推进: + +1. `review_types.py` +2. `input_loader.py` +3. `diff_parser.py` +4. `rule_engine.py` +5. `deduper.py` +6. `storage/models.py` +7. `storage/repository.py` +8. `report_writer.py` +9. `filter_policy.py` +10. `redactor.py` +11. `agent/tools.py` +12. `run_agent.py` +13. fixtures 和测试收口 + +## 13. 下一步动作 + +当前最优先的三个实现项是: + +1. 定义统一类型模型,固定主数据结构。 +2. 完成输入解析和 diff 解析,打通“读输入 -> 得到候选行”的链路。 +3. 完成第一版规则引擎,先让系统具备稳定产出 findings 的能力。 + +## 14. 第一期完成线 + +第一期的目标不是把系统做成最终形态,而是交付一个满足 issue 显式要求、可稳定运行、可验证链路完整的 MVP。第一期完成的标准如下: + +### 14.1 功能完成线 + +- 支持 `--diff-file`、`--repo-path` 和 fixture 三种输入方式 +- 提供 `code-review` Skill,包含 `SKILL.md`、规则文档和脚本目录 +- 支持通过 Container 或 Cube/E2B 执行必要脚本,本地环境仅作为开发 fallback +- 至少覆盖以下 4 类及以上规则: + - 安全风险 + - 异步错误 + - 资源泄漏 + - 测试缺失 + - 敏感信息泄漏 + - 数据库事务或连接生命周期问题 +- 输出结构化 findings,并区分 findings、warnings、`needs_human_review` +- 将 task、sandbox run、finding、report、filter decision 写入 SQLite +- 生成 `review_report.json` 和 `review_report.md` +- 支持 `dry-run / fake model`,在无真实模型 key 时跑通全链路 + +### 14.2 工程完成线 + +- 8 条公开 diff 样本全部可以运行 +- 8 条样本都能生成报告和数据库记录 +- 脱敏逻辑在报告和数据库写入前统一生效 +- Filter 决策链生效,`deny / needs_human_review` 不会直接进入沙箱 +- 沙箱执行具备 timeout 和输出限制,失败不导致整体崩溃 + +### 14.3 交付完成线 + +- 示例目录完整 +- README、开发计划、样例输出、fixture、测试和方案设计说明齐备 +- 开发者能通过示例直接理解系统结构和执行链路 + +## 15. 后续目标 + +第一期完成后,后续工作不立即实现,但需要在设计上预留演进空间。后续目标按优先级分为以下几个阶段 + +## 15.1 第二期:质量增强 + +目标:在不破坏第一期链路稳定性的前提下,提高检出率、降低误报率、提升建议质量 + +### 计划内容 + +- 扩充规则库,覆盖更多真实工程中的高频缺陷模式 +- 对现有规则增加上下文判断,减少“只看单行”造成的误报 +- 优化 `confidence` 打分和降噪策略 +- 增强 recommendation 模板,让修复建议更可执行 +- 为每类规则增加更多正例、反例和边界样本 + +### 预期收益 + +- 提高隐藏样本上的高危问题检出率 +- 更稳定地控制误报率 +- 提升报告的可读性和实用性 + +## 15.2 第三期:Agent 编排增强 + +目标:让 Agent 从“固定流程驱动”演进为“根据任务上下文动态编排”的审查助手 + +### 计划内容 + +- 根据 diff 类型、语言或目录自动选择要执行的 skill 脚本 +- 增加更细粒度的子能力,例如按语言、框架或风险类型拆分脚本 +- 在需要时引入动态工具选择,只暴露当前审查任务所需工具 +- 将模型能力限定在高价值场景,例如复杂问题解释、建议润色和报告总结 + +### 预期收益 + +- 减少无效沙箱执行。 +- 提高不同代码场景下的适配性。 +- 让 Agent 更像可控编排器,而不是固定脚本驱动器。 + +## 15.3 第四期:评测和回放能力 + +目标:把自动 CR Agent 从示例升级为可持续演化、可对比验证的评测对象。 + +### 计划内容 + +- 建立标准化评测集和评分规则。 +- 支持相同输入在不同规则版本上的结果对比。 +- 支持按 `task_id` 或样本集回放完整审查流程。 +- 统计不同版本的检出率、误报率、耗时和失败分布。 + +### 预期收益 + +- 为规则迭代提供量化依据。 +- 降低后续优化时引入回归问题的风险。 +- 让该示例具备更强的研究和教学价值。 + +## 15.4 第五期:平台化和集成能力 + +目标:让该示例具备接近真实工程接入场景的扩展能力。 + +### 计划内容 + +- 抽象数据库接口,支持 SQLite 以外的 SQL 后端。 +- 支持按 repo、severity、category、时间范围查询历史 review。 +- 提供与 CI 或 PR 工作流集成的入口。 +- 视需求增加更强的监控聚合和可视化展示。 + +### 预期收益 + +- 更容易接入实际工程流程。 +- 更容易沉淀历史审查结果。 +- 从“示例”平滑演进为“可集成能力模块”。 + +## 15.5 长期方向 + +长期来看,该项目可以继续演进为: + +- 自动化代码评测基座 +- CI 审查助手 +- 风险扫描器 +- 审查结果回放和回归测试工具 +- 更通用的 Agent 安全执行与治理示例 diff --git a/examples/skills_code_review_agent/PR_READINESS.md b/examples/skills_code_review_agent/PR_READINESS.md new file mode 100644 index 00000000..d4c52199 --- /dev/null +++ b/examples/skills_code_review_agent/PR_READINESS.md @@ -0,0 +1,37 @@ +# PR Readiness + +## Current Status + +- Core implementation phases 1-5 are complete +- Phase 6 quality-gate collection is in progress +- Example test suite passes locally +- Public fixtures all generate reports in dry-run / fake-model mode + +## Evidence Collected + +- Full example test suite: + - `pytest examples/skills_code_review_agent/tests -q` +- Public fixture sweep: + - `fixture_runs_ok=8` +- Measured single dry-run path: + - security fixture run completed in about `9.87s` + +## Ready For PR If The Following Stay True + +- tests remain green +- README, design note, acceptance checklist, and sample outputs stay in sync +- no diagnostics or formatting regressions are introduced +- all public fixtures still generate JSON and Markdown reports +- SQLite query path by `task_id` still works + +## Remaining Caution Items + +- Hidden-sample recall and false-positive rate cannot be fully proven locally +- The current pipeline formalizes the skill package and `SkillToolSet` bridge, but the main path still orchestrates script execution directly instead of fully delegating through SDK-native `skill_run` +- If reviewers want stricter alignment with “through Skill system” wording, we may still want one more follow-up to deepen native `skill_run` integration + +## Recommendation + +- The example is close to PR-ready. +- One final pre-PR pass should rerun tests, verify docs, and inspect generated sample outputs. +- If no regressions appear, the next step can be PR packaging: title, description, and final readiness checklist. diff --git a/examples/skills_code_review_agent/README.md b/examples/skills_code_review_agent/README.md new file mode 100644 index 00000000..0de32884 --- /dev/null +++ b/examples/skills_code_review_agent/README.md @@ -0,0 +1,175 @@ +# Skills Code Review Agent + +This example implements a structured code-review agent prototype on top of +tRPC-Agent-Python. It combines deterministic rule detection, a formal +`code-review` skill package, filter-based governance, sandboxed script +execution, SQLite persistence, and dual-format reports. + +## What This Example Covers + +- Unified diff, repo path, and fixture inputs +- Deterministic review rules for: + - security risks + - async errors + - resource leaks + - missing tests + - sensitive information leaks + - database lifecycle issues +- Finding dedupe and confidence-based routing +- Filter decisions before sandbox execution +- Sandboxed skill-script execution with timeout and output truncation +- Secret redaction before reporting and persistence +- SQLite storage for tasks, inputs, findings, sandbox runs, filter decisions, and reports +- `review_report.json` and `review_report.md` output generation +- Dry-run and fake-model compatible execution + +## Directory Map + +- `agent/`: orchestration, config, and runtime helpers +- repository `skills/code-review/`: canonical formal skill package and deterministic scripts +- example-local `examples/skills_code_review_agent/skills/code-review/`: teaching copy kept in sync for the example +- `src/`: parser, rules, filter policy, storage, report, telemetry, redaction +- `tests/`: fixtures and automated tests +- `DEVELOPMENT_PLAN.md`: phased implementation plan +- `DESIGN_NOTE.md`: short solution design summary +- `ACCEPTANCE_CHECKLIST.md`: acceptance-standard mapping + +## Main Flow + +```text +CLI / input + -> normalized diff input + -> structured diff parsing + -> deterministic rule engine + -> dedupe and confidence routing + -> filter decisions + -> sandboxed skill scripts + -> redaction + -> JSON/Markdown reports + -> SQLite persistence +``` + +## Running The Example + +### 1. Run against a fixture + +```bash +python examples/skills_code_review_agent/run_agent.py ^ + --fixture examples/skills_code_review_agent/tests/fixtures/security_issue.diff ^ + --output-dir examples/skills_code_review_agent/sample_outputs ^ + --db-path examples/skills_code_review_agent/sample_outputs/review.db ^ + --dry-run ^ + --fake-model +``` + +### 2. Run against a diff file + +```bash +python examples/skills_code_review_agent/run_agent.py ^ + --diff-file path/to/change.diff ^ + --output-dir examples/skills_code_review_agent/sample_outputs ^ + --db-path examples/skills_code_review_agent/sample_outputs/review.db ^ + --dry-run ^ + --fake-model +``` + +### 3. Run against a repo path + +```bash +python examples/skills_code_review_agent/run_agent.py ^ + --repo-path path/to/repo ^ + --output-dir examples/skills_code_review_agent/sample_outputs ^ + --db-path examples/skills_code_review_agent/sample_outputs/review.db ^ + --dry-run ^ + --fake-model +``` + +## Outputs + +Each run produces: + +- `review_report.json` +- `review_report.md` +- `review.db` +- staged diff inputs under `skill_inputs/` + +The JSON report includes: + +- final verdict +- findings +- warnings +- needs-human-review items +- filter decisions +- sandbox runs +- severity stats +- monitoring summary + +## SQLite Query Surface + +The repository layer persists these tables: + +- `review_tasks` +- `review_inputs` +- `filter_decisions` +- `sandbox_runs` +- `findings` +- `review_reports` + +You can fetch a full task bundle with: + +- [ReviewRepository](file:///c:/Users/32349/trpc-agent-python-fork/examples/skills_code_review_agent/src/storage/repository.py) + +Key method: + +- `get_review_bundle(task_id)` + +## Formal Skill Package + +The canonical reusable skill lives under: + +- [SKILL.md](file:///c:/Users/32349/trpc-agent-python-fork/skills/code-review/SKILL.md) +- [USAGE.md](file:///c:/Users/32349/trpc-agent-python-fork/skills/code-review/USAGE.md) +- [RULES.md](file:///c:/Users/32349/trpc-agent-python-fork/skills/code-review/RULES.md) +- [SCRIPT_CONTRACTS.md](file:///c:/Users/32349/trpc-agent-python-fork/skills/code-review/SCRIPT_CONTRACTS.md) + +The example now resolves the repository-level `skills/code-review/` directory +first for repository indexing, skill-script planning, and container skill mounts. +The example-local copy remains as a fallback so the sample stays readable and +self-contained. + +The agent formalizes the skill package, its scripts, and its `SkillToolSet` +entrypoints. The main pipeline still owns orchestration, governance, +persistence, and final reporting so Filter, Storage, and Telemetry stay fully +auditable inside the example. + +## Test Coverage + +Run the full example test suite with: + +```bash +pytest examples/skills_code_review_agent/tests -q +``` + +Covered scenarios include: + +- clean diff +- security issue +- async/resource leak +- database lifecycle issue +- missing tests +- duplicate finding behavior +- sandbox failure +- secret redaction + +Phase 6 quality-gate evidence: + +- full suite passes locally +- all 8 public fixtures generate both report artifacts +- a measured security fixture dry-run completes in about `9.87s` + +## Related Docs + +- [DEVELOPMENT_PLAN.md](file:///c:/Users/32349/trpc-agent-python-fork/examples/skills_code_review_agent/DEVELOPMENT_PLAN.md) +- [DESIGN_NOTE.md](file:///c:/Users/32349/trpc-agent-python-fork/examples/skills_code_review_agent/DESIGN_NOTE.md) +- [ACCEPTANCE_CHECKLIST.md](file:///c:/Users/32349/trpc-agent-python-fork/examples/skills_code_review_agent/ACCEPTANCE_CHECKLIST.md) +- [PR_READINESS.md](file:///c:/Users/32349/trpc-agent-python-fork/examples/skills_code_review_agent/PR_READINESS.md) diff --git a/examples/skills_code_review_agent/agent/__init__.py b/examples/skills_code_review_agent/agent/__init__.py new file mode 100644 index 00000000..0219331e --- /dev/null +++ b/examples/skills_code_review_agent/agent/__init__.py @@ -0,0 +1 @@ +"""Agent package for the skills code review example.""" diff --git a/examples/skills_code_review_agent/agent/agent.py b/examples/skills_code_review_agent/agent/agent.py new file mode 100644 index 00000000..509fe259 --- /dev/null +++ b/examples/skills_code_review_agent/agent/agent.py @@ -0,0 +1,208 @@ +"""Agent factory and orchestration for the skills code review example.""" + +from __future__ import annotations + +from dataclasses import dataclass +from datetime import datetime, timezone +from pathlib import Path +from time import perf_counter +from uuid import uuid4 + +from .config import ReviewAgentConfig +from .tools import build_blocked_run, build_skill_script_plan, execute_skill_script +from ..src.deduper import dedupe_and_classify_findings +from ..src.diff_parser import parse_unified_diff +from ..src.filter_policy import evaluate_invocations +from ..src.input_loader import load_review_input +from ..src.redactor import redact_report, redact_task +from ..src.report_writer import build_report_payload, render_markdown_report, write_report_files +from ..src.rule_engine import run_rule_engine +from ..src.review_types import ( + FindingDisposition, + FindingSource, + ReviewCategory, + ReviewConclusion, + ReviewFinding, + ReviewReport, + ReviewSeverity, + ReviewStatus, + ReviewTask, +) +from ..src.storage.repository import ReviewRepository +from ..src.telemetry import build_monitoring_summary + + +@dataclass(slots=True) +class CodeReviewAgent: + """Minimal orchestrator for the code review pipeline.""" + + config: ReviewAgentConfig + + def run(self) -> tuple[ReviewTask, ReviewReport]: + """Execute the minimal review pipeline and return task plus report.""" + + return run_review_task(self.config) + + +def create_agent(config: ReviewAgentConfig) -> CodeReviewAgent: + """Create the code review agent orchestrator.""" + + return CodeReviewAgent(config=config) + + +def run_review_task(config: ReviewAgentConfig) -> tuple[ReviewTask, ReviewReport]: + """Run the review pipeline from normalized input to structured report. + + This is the main orchestration entry point for the example. Later phases will + extend it with rule execution, filter decisions, sandbox runs, persistence, + and rich report generation. + """ + + start_time = perf_counter() + created_at = datetime.now(timezone.utc).isoformat() + + review_input = load_review_input( + diff_file=config.diff_file, + repo_path=config.repo_path, + fixture_path=config.fixture_path, + ) + parsed_diff = parse_unified_diff(review_input.diff_text) + + task = ReviewTask( + task_id=str(uuid4()), + status=ReviewStatus.RUNNING, + review_input=review_input, + parsed_diff=parsed_diff, + ) + + all_findings = run_rule_engine(parsed_diff) + diff_file_path = _materialize_diff_input(task=task, output_dir=config.output_dir) + script_plan = build_skill_script_plan( + diff_file=diff_file_path, + project_root=Path(__file__).resolve().parents[3], + ) + for invocation, decision in evaluate_invocations( + parsed_diff=parsed_diff, + runtime=config.runtime, + invocations=script_plan, + ): + task.add_filter_decision(decision) + if decision.decision.value == "allow": + sandbox_run = execute_skill_script(invocation, runtime=config.runtime) + task.add_sandbox_run(sandbox_run) + all_findings.extend(_sandbox_run_findings(task, sandbox_run)) + else: + task.add_sandbox_run( + build_blocked_run( + invocation, + runtime=config.runtime, + reason=decision.reason, + ) + ) + + processed_findings = dedupe_and_classify_findings(all_findings) + for finding in processed_findings: + task.add_finding(finding) + task.status = ReviewStatus.COMPLETED + + summary = ( + "Loaded review input, parsed diff, completed deterministic rule review, " + "and processed sandbox governance. " + f"Changed files: {parsed_diff.changed_files_count}, " + f"added lines: {parsed_diff.added_lines_count}, " + f"deleted lines: {parsed_diff.deleted_lines_count}, " + f"findings: {_count_by_disposition(processed_findings, FindingDisposition.FINDING)}, " + f"human review: {_count_by_disposition(processed_findings, FindingDisposition.NEEDS_HUMAN_REVIEW)}, " + f"warnings: {_count_by_disposition(processed_findings, FindingDisposition.WARNING)}, " + f"filter decisions: {len(task.filter_decisions)}, " + f"sandbox runs: {len(task.sandbox_runs)}." + ) + conclusion = _decide_conclusion(processed_findings) + total_duration_ms = int((perf_counter() - start_time) * 1000) + monitoring_summary = build_monitoring_summary( + task=task, + parsed_diff=parsed_diff, + total_duration_ms=total_duration_ms, + ) + task = redact_task(task) + report = ReviewReport.from_task( + task=task, + conclusion=conclusion, + summary=summary, + monitoring_summary=monitoring_summary, + ) + report = redact_report(report) + report_json = build_report_payload(report) + report_markdown = render_markdown_report(report) + write_report_files(report, output_dir=config.output_dir) + + repository = ReviewRepository(config.db_path) + repository.save_review( + task=task, + report=report, + report_json=report_json, + report_markdown=report_markdown, + runtime_type=config.runtime, + dry_run=config.dry_run, + fake_model=config.fake_model, + created_at=created_at, + finished_at=datetime.now(timezone.utc).isoformat(), + total_duration_ms=total_duration_ms, + ) + return task, report + + +def _decide_conclusion(findings: list) -> ReviewConclusion: + """Choose a final review verdict from classified findings.""" + + if any(finding.disposition == FindingDisposition.FINDING for finding in findings): + return ReviewConclusion.FAIL + if any( + finding.disposition == FindingDisposition.NEEDS_HUMAN_REVIEW + for finding in findings + ): + return ReviewConclusion.NEEDS_HUMAN_REVIEW + return ReviewConclusion.PASS + + +def _count_by_disposition(findings: list, disposition: FindingDisposition) -> int: + """Count findings by presentation bucket.""" + + return sum(1 for finding in findings if finding.disposition == disposition) + + +def _materialize_diff_input(*, task: ReviewTask, output_dir: Path) -> Path: + """Write the normalized diff text to a file for skill-script execution.""" + + inputs_dir = output_dir.expanduser().resolve() / "skill_inputs" + inputs_dir.mkdir(parents=True, exist_ok=True) + diff_file = inputs_dir / f"{task.task_id}.diff" + diff_file.write_text(task.review_input.diff_text, encoding="utf-8") + return diff_file + + +def _sandbox_run_findings(task: ReviewTask, sandbox_run) -> list[ReviewFinding]: + """Convert failed sandbox runs into structured findings without crashing the task.""" + + if sandbox_run.status.value not in {"failed", "timed_out"}: + return [] + + severity = ReviewSeverity.MEDIUM + confidence = 0.7 + if sandbox_run.status.value == "timed_out": + severity = ReviewSeverity.HIGH + confidence = 0.85 + + return [ + ReviewFinding( + severity=severity, + category=ReviewCategory.SANDBOX, + file=task.parsed_diff.changed_paths[0] if task.parsed_diff and task.parsed_diff.changed_paths else task.review_input.source, + line=None, + title=f"Sandbox script `{sandbox_run.name}` did not complete successfully", + evidence=sandbox_run.stderr or sandbox_run.stdout or "Sandbox execution failed without output.", + recommendation="Inspect the sandbox summary, fix the failing script or command, and rerun the review.", + confidence=confidence, + source=FindingSource.SANDBOX, + ) + ] diff --git a/examples/skills_code_review_agent/agent/config.py b/examples/skills_code_review_agent/agent/config.py new file mode 100644 index 00000000..f68ccf34 --- /dev/null +++ b/examples/skills_code_review_agent/agent/config.py @@ -0,0 +1,37 @@ +"""Configuration helpers for the skills code review example.""" + +from __future__ import annotations + +from dataclasses import dataclass +from pathlib import Path + +DEFAULT_RUNTIME_TYPE = "container" +DEFAULT_DB_PATH = "review_agent.db" +DEFAULT_OUTPUT_DIR = "review_outputs" + + +@dataclass(slots=True, frozen=True) +class ReviewAgentConfig: + """Runtime configuration for the code review agent.""" + + diff_file: str | None = None + repo_path: str | None = None + fixture_path: str | None = None + output_dir: Path = Path(DEFAULT_OUTPUT_DIR) + db_path: Path = Path(DEFAULT_DB_PATH) + runtime: str = DEFAULT_RUNTIME_TYPE + dry_run: bool = False + fake_model: bool = False + + def __post_init__(self) -> None: + """Validate mutually exclusive input sources and normalize paths.""" + + provided_sources = [ + value + for value in (self.diff_file, self.repo_path, self.fixture_path) + if value is not None + ] + if len(provided_sources) != 1: + raise ValueError( + "exactly one of diff_file, repo_path, or fixture_path must be provided" + ) diff --git a/examples/skills_code_review_agent/agent/prompts.py b/examples/skills_code_review_agent/agent/prompts.py new file mode 100644 index 00000000..5ec2ae82 --- /dev/null +++ b/examples/skills_code_review_agent/agent/prompts.py @@ -0,0 +1,8 @@ +"""Prompt templates for the skills code review example.""" + +from __future__ import annotations + +INSTRUCTION = """ +You are a code review agent that coordinates diff parsing, risk checks, +sandbox execution, structured findings, and report generation. +""".strip() diff --git a/examples/skills_code_review_agent/agent/tools.py b/examples/skills_code_review_agent/agent/tools.py new file mode 100644 index 00000000..c91b7bf0 --- /dev/null +++ b/examples/skills_code_review_agent/agent/tools.py @@ -0,0 +1,291 @@ +"""Tool and runtime helpers for the skills code review example.""" + +from __future__ import annotations + +import os +import subprocess +import sys +from pathlib import Path +from time import perf_counter +from typing import Any + +from trpc_agent_sdk.code_executors import BaseWorkspaceRuntime +from trpc_agent_sdk.code_executors import DEFAULT_SKILLS_CONTAINER +from trpc_agent_sdk.code_executors import create_container_workspace_runtime +from trpc_agent_sdk.code_executors import create_local_workspace_runtime +from trpc_agent_sdk.skills import BaseSkillRepository +from trpc_agent_sdk.skills import ENV_SKILLS_ROOT +from trpc_agent_sdk.skills import SkillToolSet +from trpc_agent_sdk.skills import create_default_skill_repository + +from ..src.filter_policy import SkillScriptInvocation +from ..src.review_types import SandboxRunRecord, SandboxRunStatus + +SKILL_NAME = "code-review" +SCRIPT_TIMEOUT_SECONDS = 20 +OUTPUT_LIMIT_CHARS = 4000 + + +def create_skill_tool_set( + workspace_runtime_type: str = "local", + *, + use_cached_repository: bool = True, +) -> tuple[SkillToolSet, BaseSkillRepository]: + """Create a SkillToolSet for the code-review skill example.""" + + tool_kwargs = { + "save_as_artifacts": True, + "omit_inline_content": False, + } + workspace_runtime = _create_workspace_runtime(workspace_runtime_type=workspace_runtime_type) + skill_paths = _get_skill_roots() + repository = create_default_skill_repository( + *skill_paths, + workspace_runtime=workspace_runtime, + use_cached_repository=use_cached_repository, + ) + return SkillToolSet(repository=repository, run_tool_kwargs=tool_kwargs), repository + + +def build_skill_script_plan( + *, + diff_file: Path, + project_root: Path, +) -> list[SkillScriptInvocation]: + """Build the list of planned skill-script executions for the review.""" + + scripts_dir = resolve_code_review_skill_dir(project_root=project_root) / "scripts" + return [ + SkillScriptInvocation( + name="parse_diff", + script_path=scripts_dir / "parse_diff.py", + command=[ + sys.executable, + str(scripts_dir / "parse_diff.py"), + "--diff-file", + str(diff_file), + ], + target="skill:code-review/scripts/parse_diff.py", + ), + SkillScriptInvocation( + name="run_linters", + script_path=scripts_dir / "run_linters.py", + command=[ + sys.executable, + str(scripts_dir / "run_linters.py"), + "--diff-file", + str(diff_file), + ], + target="skill:code-review/scripts/run_linters.py", + ), + SkillScriptInvocation( + name="run_tests", + script_path=scripts_dir / "run_tests.py", + command=[ + sys.executable, + str(scripts_dir / "run_tests.py"), + "--diff-file", + str(diff_file), + ], + target="skill:code-review/scripts/run_tests.py", + ), + ] + + +def build_skill_run_payload( + *, + diff_file: Path, + script_name: str, + skill_name: str = "code-review", +) -> dict[str, Any]: + """Build a `skill_run` payload for one code-review skill script.""" + + output_file = _output_file_for_script(script_name) + return { + "skill": skill_name, + "cwd": f"$SKILLS_DIR/{skill_name}", + "command": f"python scripts/{script_name} --diff-file {diff_file} > {output_file}", + "output_files": [output_file], + } + + +def execute_skill_script( + invocation: SkillScriptInvocation, + *, + runtime: str, + timeout_seconds: int = SCRIPT_TIMEOUT_SECONDS, + output_limit_chars: int = OUTPUT_LIMIT_CHARS, +) -> SandboxRunRecord: + """Execute a skill script in a controlled subprocess.""" + + started = perf_counter() + try: + completed = subprocess.run( + invocation.command, + capture_output=True, + check=False, + text=True, + encoding="utf-8", + timeout=timeout_seconds, + ) + stdout, stdout_truncated = _truncate_output( + _normalize_process_output(completed.stdout), + output_limit_chars, + ) + stderr, stderr_truncated = _truncate_output( + _normalize_process_output(completed.stderr), + output_limit_chars, + ) + status = ( + SandboxRunStatus.SUCCEEDED + if completed.returncode == 0 + else SandboxRunStatus.FAILED + ) + return SandboxRunRecord( + name=invocation.name, + command=invocation.command, + status=status, + runtime=runtime, + duration_ms=int((perf_counter() - started) * 1000), + exit_code=completed.returncode, + stdout=stdout, + stderr=stderr, + timed_out=False, + output_truncated=stdout_truncated or stderr_truncated, + blocked_by_filter=False, + ) + except subprocess.TimeoutExpired as exc: + stdout, stdout_truncated = _truncate_output( + _normalize_process_output(exc.stdout), + output_limit_chars, + ) + stderr, stderr_truncated = _truncate_output( + _normalize_process_output(exc.stderr), + output_limit_chars, + ) + return SandboxRunRecord( + name=invocation.name, + command=invocation.command, + status=SandboxRunStatus.TIMED_OUT, + runtime=runtime, + duration_ms=int((perf_counter() - started) * 1000), + exit_code=None, + stdout=stdout, + stderr=stderr, + timed_out=True, + output_truncated=stdout_truncated or stderr_truncated, + blocked_by_filter=False, + ) + + +def build_blocked_run( + invocation: SkillScriptInvocation, + *, + runtime: str, + reason: str, +) -> SandboxRunRecord: + """Create a synthetic sandbox record for blocked invocations.""" + + return SandboxRunRecord( + name=invocation.name, + command=invocation.command, + status=SandboxRunStatus.BLOCKED, + runtime=runtime, + duration_ms=0, + exit_code=None, + stdout="", + stderr=reason, + timed_out=False, + output_truncated=False, + blocked_by_filter=True, + ) + + +def _truncate_output(text: str, limit: int) -> tuple[str, bool]: + """Truncate subprocess output to the configured size limit.""" + + if len(text) <= limit: + return text, False + return text[:limit], True + + +def _normalize_process_output(text: object) -> str: + """Normalize subprocess output for type-safe truncation and storage.""" + + if text is None: + return "" + if isinstance(text, bytes): + return text.decode("utf-8", errors="replace") + if isinstance(text, str): + return text + return str(text) + + +def resolve_code_review_skill_dir(*, project_root: Path | None = None) -> Path: + """Resolve the canonical code-review skill directory. + + The repository-level ``skills/code-review`` path is preferred so the example + matches the issue's requested artifact layout. The example-local copy remains + as a fallback to keep the sample self-contained. + """ + + for root in _get_skill_roots(project_root=project_root): + candidate = Path(root).resolve() / SKILL_NAME + if (candidate / "SKILL.md").is_file(): + return candidate + raise FileNotFoundError( + "Unable to locate the `code-review` skill under repository or example skill roots." + ) + + +def _get_skill_roots(*, project_root: Path | None = None) -> tuple[str, ...]: + """Get ordered skill roots for repository scanning.""" + + + skills_root = os.getenv(ENV_SKILLS_ROOT) + if skills_root: + return (skills_root,) + + repo_root = _resolve_project_root(project_root) + candidates = [ + repo_root / "skills", + repo_root / "examples" / "skills_code_review_agent" / "skills", + ] + + roots: list[str] = [] + for candidate in candidates: + resolved = str(candidate.resolve()) + if candidate.is_dir() and resolved not in roots: + roots.append(resolved) + return tuple(roots) + + +def _resolve_project_root(project_root: Path | None = None) -> Path: + """Resolve the repository root for the example.""" + + if project_root is not None: + return project_root.expanduser().resolve() + return Path(__file__).resolve().parents[3] + + +def _create_workspace_runtime( + *, + workspace_runtime_type: str = "local", + **kwargs: Any, +) -> BaseWorkspaceRuntime: + """Create a workspace runtime for skill execution demos.""" + + if workspace_runtime_type == "container": + skill_root = _get_skill_roots()[0] + host_config = {"Binds": [f"{skill_root}:{DEFAULT_SKILLS_CONTAINER}:ro"]} + kwargs["host_config"] = host_config + kwargs["auto_inputs"] = True + return create_container_workspace_runtime(**kwargs) + return create_local_workspace_runtime(**kwargs) + + +def _output_file_for_script(script_name: str) -> str: + """Map a script name to its canonical skill_run output artifact.""" + + stem = Path(script_name).stem + return f"out/{stem}.json" diff --git a/examples/skills_code_review_agent/run_agent.py b/examples/skills_code_review_agent/run_agent.py new file mode 100644 index 00000000..2f97b14b --- /dev/null +++ b/examples/skills_code_review_agent/run_agent.py @@ -0,0 +1,123 @@ +#!/usr/bin/env python3 +"""CLI entry point for the skills code review agent example.""" + +from __future__ import annotations + +import argparse +import json +import sys +from dataclasses import asdict +from enum import Enum +from pathlib import Path + +if __package__ in (None, ""): + sys.path.insert(0, str(Path(__file__).resolve().parents[2])) + +from examples.skills_code_review_agent.agent.agent import create_agent +from examples.skills_code_review_agent.agent.config import ( + DEFAULT_DB_PATH, + DEFAULT_OUTPUT_DIR, + DEFAULT_RUNTIME_TYPE, + ReviewAgentConfig, +) + + +def build_parser() -> argparse.ArgumentParser: + """Build the CLI parser for the example entry point.""" + + parser = argparse.ArgumentParser( + description="Run the skills code review agent example." + ) + source_group = parser.add_mutually_exclusive_group(required=True) + source_group.add_argument( + "--diff-file", + help="Path to a unified diff file to review.", + ) + source_group.add_argument( + "--repo-path", + help="Repository path whose current git workspace diff should be reviewed.", + ) + source_group.add_argument( + "--fixture", + help="Fixture diff path used for local testing.", + ) + parser.add_argument( + "--output-dir", + default=DEFAULT_OUTPUT_DIR, + help="Directory where future report files will be written.", + ) + parser.add_argument( + "--db-path", + default=DEFAULT_DB_PATH, + help="SQLite database path used by the review pipeline.", + ) + parser.add_argument( + "--runtime", + default=DEFAULT_RUNTIME_TYPE, + choices=["local", "container", "cube", "e2b"], + help="Execution runtime for sandboxed steps.", + ) + parser.add_argument( + "--dry-run", + action="store_true", + help="Run the pipeline without any external model dependency.", + ) + parser.add_argument( + "--fake-model", + action="store_true", + help="Enable fake-model mode for deterministic local testing.", + ) + return parser + + +def parse_args(argv: list[str] | None = None) -> argparse.Namespace: + """Parse CLI arguments for the example runner.""" + + return build_parser().parse_args(argv) + + +def _json_default(value: object) -> object: + """Serialize enums and paths for CLI preview output.""" + + if isinstance(value, Enum): + return value.value + if isinstance(value, Path): + return str(value) + raise TypeError(f"Object of type {type(value).__name__} is not JSON serializable") + + +def main(argv: list[str] | None = None) -> int: + """Run the example entry point.""" + + args = parse_args(argv) + config = ReviewAgentConfig( + diff_file=args.diff_file, + repo_path=args.repo_path, + fixture_path=args.fixture, + output_dir=Path(args.output_dir), + db_path=Path(args.db_path), + runtime=args.runtime, + dry_run=args.dry_run, + fake_model=args.fake_model, + ) + agent = create_agent(config) + task, report = agent.run() + + print( + json.dumps( + { + "task_id": task.task_id, + "status": task.status.value, + "input_kind": task.review_input.kind.value, + "changed_files": task.parsed_diff.changed_paths if task.parsed_diff else [], + "report": asdict(report), + }, + indent=2, + default=_json_default, + ) + ) + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/examples/skills_code_review_agent/sample_outputs/review_report.json b/examples/skills_code_review_agent/sample_outputs/review_report.json new file mode 100644 index 00000000..5ac59169 --- /dev/null +++ b/examples/skills_code_review_agent/sample_outputs/review_report.json @@ -0,0 +1,135 @@ +{ + "task_id": "712d2fbc-41b8-4da4-a705-5d41f81104a9", + "conclusion": "fail", + "findings": [ + { + "severity": "high", + "category": "security", + "file": "src/calculator.py", + "line": 11, + "title": "Use of eval introduces code execution risk", + "evidence": "- return parse_expression(expression)\n+ return eval(expression)\n+", + "recommendation": "Replace eval with explicit parsing, a whitelist-based dispatcher, or a safe literal parser.", + "confidence": 0.98, + "source": "rule_engine", + "disposition": "finding", + "fingerprint": "15931fd42be95f0b55a03de80a0b5e1fcaea882b2cbe6f7ddff60ed6f3d2653f" + }, + { + "severity": "high", + "category": "security", + "file": "src/calculator.py", + "line": 14, + "title": "subprocess call enables shell execution", + "evidence": "+def run_shell(command: str) -> str:\n+ return subprocess.run(command, shell=True, check=True, text=True)", + "recommendation": "Pass an argument list and avoid shell=True unless a reviewed shell command is unavoidable.", + "confidence": 0.95, + "source": "rule_engine", + "disposition": "finding", + "fingerprint": "4ce523b75cad08a0d3aca21db6cff3e83865334144b6876cad6901143ee94136" + } + ], + "warnings": [], + "needs_human_review": [], + "filter_decisions": [ + { + "decision": "allow", + "target": "skill:code-review/scripts/parse_diff.py", + "reason_code": "allow", + "reason": "Invocation allowed by default policy.", + "requires_human_review": false + }, + { + "decision": "allow", + "target": "skill:code-review/scripts/run_linters.py", + "reason_code": "allow", + "reason": "Invocation allowed by default policy.", + "requires_human_review": false + }, + { + "decision": "allow", + "target": "skill:code-review/scripts/run_tests.py", + "reason_code": "allow", + "reason": "Invocation allowed by default policy.", + "requires_human_review": false + } + ], + "sandbox_runs": [ + { + "name": "parse_diff", + "command": [ + "C:\\Users\\32349\\AppData\\Local\\Programs\\Python\\Python310\\python.exe", + "C:\\Users\\32349\\trpc-agent-python-fork\\examples\\skills_code_review_agent\\skills\\code-review\\scripts\\parse_diff.py", + "--diff-file", + "C:\\Users\\32349\\trpc-agent-python-fork\\examples\\skills_code_review_agent\\sample_outputs\\skill_inputs\\712d2fbc-41b8-4da4-a705-5d41f81104a9.diff" + ], + "status": "succeeded", + "runtime": "container", + "duration_ms": 107, + "exit_code": 0, + "stdout": "{\n \"diff_file\": \"C:\\\\Users\\\\32349\\\\trpc-agent-python-fork\\\\examples\\\\skills_code_review_agent\\\\sample_outputs\\\\skill_inputs\\\\712d2fbc-41b8-4da4-a705-5d41f81104a9.diff\",\n \"line_count\": 22,\n \"file_count\": 2,\n \"has_security_keywords\": true\n}\n", + "stderr": "", + "timed_out": false, + "output_truncated": false, + "blocked_by_filter": false + }, + { + "name": "run_linters", + "command": [ + "C:\\Users\\32349\\AppData\\Local\\Programs\\Python\\Python310\\python.exe", + "C:\\Users\\32349\\trpc-agent-python-fork\\examples\\skills_code_review_agent\\skills\\code-review\\scripts\\run_linters.py", + "--diff-file", + "C:\\Users\\32349\\trpc-agent-python-fork\\examples\\skills_code_review_agent\\sample_outputs\\skill_inputs\\712d2fbc-41b8-4da4-a705-5d41f81104a9.diff" + ], + "status": "succeeded", + "runtime": "container", + "duration_ms": 96, + "exit_code": 0, + "stdout": "{\n \"diff_file\": \"C:\\\\Users\\\\32349\\\\trpc-agent-python-fork\\\\examples\\\\skills_code_review_agent\\\\sample_outputs\\\\skill_inputs\\\\712d2fbc-41b8-4da4-a705-5d41f81104a9.diff\",\n \"warning_count\": 2,\n \"warnings\": [\n \"Security-sensitive call detected: eval\",\n \"Shell execution enabled in subprocess call\"\n ]\n}\n", + "stderr": "", + "timed_out": false, + "output_truncated": false, + "blocked_by_filter": false + }, + { + "name": "run_tests", + "command": [ + "C:\\Users\\32349\\AppData\\Local\\Programs\\Python\\Python310\\python.exe", + "C:\\Users\\32349\\trpc-agent-python-fork\\examples\\skills_code_review_agent\\skills\\code-review\\scripts\\run_tests.py", + "--diff-file", + "C:\\Users\\32349\\trpc-agent-python-fork\\examples\\skills_code_review_agent\\sample_outputs\\skill_inputs\\712d2fbc-41b8-4da4-a705-5d41f81104a9.diff" + ], + "status": "succeeded", + "runtime": "container", + "duration_ms": 106, + "exit_code": 0, + "stdout": "{\n \"diff_file\": \"C:\\\\Users\\\\32349\\\\trpc-agent-python-fork\\\\examples\\\\skills_code_review_agent\\\\sample_outputs\\\\skill_inputs\\\\712d2fbc-41b8-4da4-a705-5d41f81104a9.diff\",\n \"changed_test_files\": [\n \"tests/test_calculator.py\"\n ],\n \"test_update_present\": true\n}\n", + "stderr": "", + "timed_out": false, + "output_truncated": false, + "blocked_by_filter": false + } + ], + "summary": "Loaded review input, parsed diff, completed deterministic rule review, and processed sandbox governance. Changed files: 2, added lines: 6, deleted lines: 1, findings: 2, human review: 0, warnings: 0, filter decisions: 3, sandbox runs: 3.", + "severity_counts": { + "high": 2 + }, + "monitoring_summary": { + "total_duration_ms": 314, + "changed_files_count": 2, + "added_lines_count": 6, + "deleted_lines_count": 1, + "sandbox_run_count": 3, + "filter_decision_count": 3, + "finding_count": 2, + "needs_human_review_count": 0, + "warning_count": 0, + "severity_distribution": { + "high": 2 + }, + "category_distribution": { + "security": 2 + }, + "exception_distribution": {} + } +} \ No newline at end of file diff --git a/examples/skills_code_review_agent/sample_outputs/review_report.md b/examples/skills_code_review_agent/sample_outputs/review_report.md new file mode 100644 index 00000000..2759bd8c --- /dev/null +++ b/examples/skills_code_review_agent/sample_outputs/review_report.md @@ -0,0 +1,64 @@ +# Review Report + +- Task ID: `712d2fbc-41b8-4da4-a705-5d41f81104a9` +- Final Verdict: `fail` + +## Summary + +Loaded review input, parsed diff, completed deterministic rule review, and processed sandbox governance. Changed files: 2, added lines: 6, deleted lines: 1, findings: 2, human review: 0, warnings: 0, filter decisions: 3, sandbox runs: 3. + +## Severity Stats + +- `high`: 2 + +## Findings + +- [`high`] `security` at `src/calculator.py:11`: Use of eval introduces code execution risk + Evidence: `- return parse_expression(expression) ++ return eval(expression) ++` + Recommendation: Replace eval with explicit parsing, a whitelist-based dispatcher, or a safe literal parser. +- [`high`] `security` at `src/calculator.py:14`: subprocess call enables shell execution + Evidence: `+def run_shell(command: str) -> str: ++ return subprocess.run(command, shell=True, check=True, text=True)` + Recommendation: Pass an argument list and avoid shell=True unless a reviewed shell command is unavoidable. + +## Human Review Items + +- None. + +## Warnings + +- None. + +## Filter Summary + +- `allow` on `skill:code-review/scripts/parse_diff.py`: Invocation allowed by default policy. +- `allow` on `skill:code-review/scripts/run_linters.py`: Invocation allowed by default policy. +- `allow` on `skill:code-review/scripts/run_tests.py`: Invocation allowed by default policy. + +## Sandbox Summary + +- `parse_diff` status=`succeeded` duration=107ms exit_code=0 +- `run_linters` status=`succeeded` duration=96ms exit_code=0 +- `run_tests` status=`succeeded` duration=106ms exit_code=0 + +## Monitoring + +- `added_lines_count`: 6 +- `category_distribution`: {'security': 2} +- `changed_files_count`: 2 +- `deleted_lines_count`: 1 +- `exception_distribution`: {} +- `filter_decision_count`: 3 +- `finding_count`: 2 +- `needs_human_review_count`: 0 +- `sandbox_run_count`: 3 +- `severity_distribution`: {'high': 2} +- `total_duration_ms`: 314 +- `warning_count`: 0 + +## Actionable Recommendations + +- Replace eval with explicit parsing, a whitelist-based dispatcher, or a safe literal parser. +- Pass an argument list and avoid shell=True unless a reviewed shell command is unavoidable. diff --git a/examples/skills_code_review_agent/skills/code-review/RULES.md b/examples/skills_code_review_agent/skills/code-review/RULES.md new file mode 100644 index 00000000..3736bc22 --- /dev/null +++ b/examples/skills_code_review_agent/skills/code-review/RULES.md @@ -0,0 +1,157 @@ +# Review Rules + +## Detection Categories + +The initial implementation covers these categories: + +- Security risk +- Async error +- Resource leak +- Missing tests +- Sensitive information leak +- Database transaction or connection lifecycle issue + +## Rule Design Principles + +- Prefer deterministic, high-signal rules for the first implementation. +- Run rules on structured diff data instead of raw text blobs whenever possible. +- Treat low-confidence heuristics as review aids, not final verdicts. +- Keep evidence small and local so reports stay readable. +- Avoid duplicate findings for the same issue location. + +## Category Notes + +### Security Risk + +High-confidence patterns: + +- `eval(...)` +- `exec(...)` +- `pickle.loads(...)` +- `yaml.load(...)` without a safe loader +- `subprocess.*(..., shell=True)` +- TLS verification disabled with `verify=False` + +Severity guidance: + +- `eval/exec/pickle.loads/shell=True`: high +- `verify=False`: medium to high depending on context + +Do not report when: + +- the match only appears inside comments or doc-style examples +- YAML parsing already uses `safe_load` or `SafeLoader` +- subprocess invocation does not use `shell=True` +- `verify=False` appears in clearly test-only or mock-only code: lower confidence and route to human review + +### Async Error + +Heuristic patterns: + +- Detached `asyncio.create_task(...)` without visible lifecycle tracking +- `except Exception: pass` style swallowing near async flows + +Routing guidance: + +- Most first-pass async findings should land in `needs_human_review` unless the pattern is obviously dangerous. + +### Resource Leak + +Heuristic patterns: + +- `open(...)` without `with open(...)` +- HTTP/session/client constructors without `with` / `async with` + +Routing guidance: + +- Resource-lifecycle heuristics are useful but often context-sensitive, so medium-confidence routing is preferred in the first version. + +Do not report when: + +- the handle is created with `with open(...)` +- a created handle or client is explicitly closed later in the added code + +### Missing Tests + +Diff-level pattern: + +- Production code changes are present but no test files are updated in the same diff. + +Routing guidance: + +- This category usually goes to `needs_human_review` unless the project later adds stronger ownership or coverage signals. + +### Sensitive Information Leak + +High-confidence patterns: + +- `api_key = "..."` +- `token = "..."` +- `password = "..."` +- `secret = "..."` +- `Bearer ...` +- `AKIA...` +- `sk-...` +- `ghp_...` +- private key headers such as `BEGIN PRIVATE KEY` + +Severity guidance: + +- Hard-coded credentials are generally high or critical. + +Do not report when: + +- the value is an obvious placeholder such as `example`, `changeme`, `dummy`, `masked`, or `redacted` +- the match only appears in comments or explanatory documentation + +### Database Lifecycle + +Heuristic patterns: + +- Database connection/session created without visible close handling +- Transaction opened without clear commit / rollback handling + +Routing guidance: + +- Emit high-confidence findings only when lifecycle handling is clearly absent. +- Otherwise route to `needs_human_review`. + +Do not report when: + +- the connection/session is visibly closed later in the added code +- transaction handling visibly includes both commit and rollback paths + +## Finding Contract + +Each finding should include at least: + +- `severity` +- `category` +- `file` +- `line` +- `title` +- `evidence` +- `recommendation` +- `confidence` +- `source` + +## Noise Control + +- Do not emit duplicate findings for the same category, file, and line +- Route low-confidence items into warnings or human-review buckets +- Redact secrets before persistence and reporting +- Prefer suppressing obvious placeholders and safe examples over emitting low-value findings + +## First-Pass Confidence Guidance + +- `confidence >= 0.8`: final finding +- `0.4 <= confidence < 0.8`: `needs_human_review` +- `confidence < 0.4`: warning + +## Phase-Two Quality Fixtures + +- Positive / safe examples: + - `safe_security_patterns.diff` + - `managed_lifecycle.diff` +- Boundary / negative examples: + - `placeholder_secrets.diff` diff --git a/examples/skills_code_review_agent/skills/code-review/SCRIPT_CONTRACTS.md b/examples/skills_code_review_agent/skills/code-review/SCRIPT_CONTRACTS.md new file mode 100644 index 00000000..50fdcd2f --- /dev/null +++ b/examples/skills_code_review_agent/skills/code-review/SCRIPT_CONTRACTS.md @@ -0,0 +1,74 @@ +# Script Contracts + +## `scripts/parse_diff.py` + +Purpose: + +- summarizes diff size and whether high-risk tokens appear + +CLI: + +```text +python scripts/parse_diff.py --diff-file +``` + +Output: + +- JSON to stdout +- fields: + - `diff_file` + - `line_count` + - `file_count` + - `has_security_keywords` + +## `scripts/run_linters.py` + +Purpose: + +- runs deterministic lint-style checks over the diff content + +CLI: + +```text +python scripts/run_linters.py --diff-file +``` + +Output: + +- JSON to stdout +- fields: + - `diff_file` + - `warning_count` + - `warnings` + +Failure behavior: + +- exits non-zero when the diff contains the `TODO_FAIL_SANDBOX` marker +- intended for sandbox failure testing and pipeline resilience checks + +## `scripts/run_tests.py` + +Purpose: + +- reports whether test files are updated in the diff + +CLI: + +```text +python scripts/run_tests.py --diff-file +``` + +Output: + +- JSON to stdout +- fields: + - `diff_file` + - `changed_test_files` + - `test_update_present` + +## Shared Expectations + +- scripts should stay deterministic +- scripts should not require network access +- scripts should be safe to run in dry-run and fake-model modes +- stdout/stderr may be truncated by sandbox policy diff --git a/examples/skills_code_review_agent/skills/code-review/SKILL.md b/examples/skills_code_review_agent/skills/code-review/SKILL.md new file mode 100644 index 00000000..4c8d6b17 --- /dev/null +++ b/examples/skills_code_review_agent/skills/code-review/SKILL.md @@ -0,0 +1,122 @@ +--- +name: code-review +description: Reviews diffs with reusable rules and sandboxed scripts. Invoke when a task needs structured code review, risk detection, or validation runs. +--- + +# Code Review Skill + +Overview + +Use this skill when a task needs structured code review over a unified diff, +local repository changes, or deterministic fixtures. It packages reusable review +rules, sandboxable scripts, and output conventions for the +`skills_code_review_agent` example. + +This skill is designed for: + +- deterministic first-pass code review +- security and secret scanning on changed lines +- review validation in dry-run or fake-model mode +- controlled script execution through `skill_run` + +Primary workflow + +1. Load the skill: + + ```text + skill_load(skill="code-review") + ``` + +2. If more detail is needed, load docs such as `USAGE.md` or `SCRIPT_CONTRACTS.md`. + +3. Run one or more skill scripts through `skill_run`: + + - `scripts/parse_diff.py` + - `scripts/run_linters.py` + - `scripts/run_tests.py` + +4. Convert results into structured review findings with the required fields: + + - `severity` + - `category` + - `file` + - `line` + - `title` + - `evidence` + - `recommendation` + - `confidence` + - `source` + +Inputs + +- unified diff files +- repository paths with local modifications +- prepared fixtures for deterministic tests + +Outputs + +- structured findings +- sandbox execution summaries +- `review_report.json` +- `review_report.md` + +Available Docs + +- `RULES.md`: review categories and rule semantics +- `USAGE.md`: when and how to invoke the skill +- `SCRIPT_CONTRACTS.md`: each script's CLI and output contract + +Scripts + +- `scripts/parse_diff.py` +- `scripts/run_linters.py` +- `scripts/run_tests.py` + +Safety Guidance + +- Always pass review inputs as files rather than inlining huge diffs in prompts. +- Keep execution deterministic in dry-run and fake-model mode. +- Do not allow high-risk scripts to bypass filter checks. +- Redact secrets before persisting or displaying evidence. + +Example `skill_run` commands + +1. Parse a diff: + + ```text + skill_run( + skill="code-review", + cwd="$SKILLS_DIR/code-review", + command="python scripts/parse_diff.py --diff-file $WORK_DIR/sample.diff > out/parse_summary.json", + output_files=["out/parse_summary.json"] + ) + ``` + +2. Run deterministic lint checks: + + ```text + skill_run( + skill="code-review", + cwd="$SKILLS_DIR/code-review", + command="python scripts/run_linters.py --diff-file $WORK_DIR/sample.diff > out/lint_summary.json", + output_files=["out/lint_summary.json"] + ) + ``` + +3. Run deterministic test-presence checks: + + ```text + skill_run( + skill="code-review", + cwd="$SKILLS_DIR/code-review", + command="python scripts/run_tests.py --diff-file $WORK_DIR/sample.diff > out/test_summary.json", + output_files=["out/test_summary.json"] + ) + ``` + +Notes + +- The first implementation keeps the core logic deterministic so dry-run mode can + exercise the full pipeline without a real model API key. +- The skill complements the main agent orchestrator rather than replacing it: + the agent owns task lifecycle, persistence, reporting, and governance. diff --git a/examples/skills_code_review_agent/skills/code-review/USAGE.md b/examples/skills_code_review_agent/skills/code-review/USAGE.md new file mode 100644 index 00000000..983f7982 --- /dev/null +++ b/examples/skills_code_review_agent/skills/code-review/USAGE.md @@ -0,0 +1,43 @@ +# Code Review Skill Usage + +## When To Invoke + +Invoke `code-review` when you need: + +- structured review over a unified diff +- deterministic risk detection in dry-run or fake-model mode +- a controlled script-based review step before writing a final report +- reusable review logic that should stay outside the main agent prompt + +## When Not To Invoke + +Do not invoke this skill for: + +- trivial file reads or one-line code questions +- full repository security auditing outside the changed diff scope +- governance decisions that belong to `Filter` +- persistence, report writing, or task lifecycle management + +## Recommended Pattern + +1. Normalize the input diff or repo change set in the main agent. +2. Load the `code-review` skill. +3. Run one or more scripts through `skill_run`. +4. Merge script output with deterministic rule-engine findings. +5. Apply redaction before persistence and reporting. + +## Integration Boundary + +This skill owns: + +- reusable review instructions +- rule documentation +- script-level deterministic checks + +The main agent still owns: + +- task orchestration +- filter decisions +- sandbox policy +- storage +- final report generation diff --git a/examples/skills_code_review_agent/skills/code-review/scripts/parse_diff.py b/examples/skills_code_review_agent/skills/code-review/scripts/parse_diff.py new file mode 100644 index 00000000..d2e239eb --- /dev/null +++ b/examples/skills_code_review_agent/skills/code-review/scripts/parse_diff.py @@ -0,0 +1,44 @@ +"""Parse diffs for the code review skill.""" + +from __future__ import annotations + +import argparse +import json +from pathlib import Path + + +def build_parser() -> argparse.ArgumentParser: + """Build CLI arguments for the diff parser script.""" + + parser = argparse.ArgumentParser(description="Parse a diff file for review skill diagnostics.") + parser.add_argument("--diff-file", required=True, help="Path to the diff file.") + return parser + + +def parse_args(argv: list[str] | None = None) -> argparse.Namespace: + """Parse command-line arguments.""" + + return build_parser().parse_args(argv) + + +def main(argv: list[str] | None = None) -> int: + """Summarize diff shape for sandboxed skill diagnostics.""" + + args = parse_args(argv) + diff_path = Path(args.diff_file).expanduser().resolve() + diff_text = diff_path.read_text(encoding="utf-8") + payload = { + "diff_file": str(diff_path), + "line_count": len(diff_text.splitlines()), + "file_count": diff_text.count("diff --git "), + "has_security_keywords": any( + token in diff_text + for token in ("eval(", "shell=True", "pickle.loads(", "yaml.load(") + ), + } + print(json.dumps(payload, indent=2)) + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/examples/skills_code_review_agent/skills/code-review/scripts/run_linters.py b/examples/skills_code_review_agent/skills/code-review/scripts/run_linters.py new file mode 100644 index 00000000..77e33792 --- /dev/null +++ b/examples/skills_code_review_agent/skills/code-review/scripts/run_linters.py @@ -0,0 +1,54 @@ +"""Run sandboxed lint or static checks for the code review skill.""" + +from __future__ import annotations + +import argparse +import json +import sys +from pathlib import Path + + +def build_parser() -> argparse.ArgumentParser: + """Build CLI arguments for the linter simulation script.""" + + parser = argparse.ArgumentParser(description="Run deterministic lint checks on a diff file.") + parser.add_argument("--diff-file", required=True, help="Path to the diff file.") + return parser + + +def parse_args(argv: list[str] | None = None) -> argparse.Namespace: + """Parse command-line arguments.""" + + return build_parser().parse_args(argv) + + +def main(argv: list[str] | None = None) -> int: + """Run deterministic lint-style checks over the diff content.""" + + args = parse_args(argv) + diff_path = Path(args.diff_file).expanduser().resolve() + diff_text = diff_path.read_text(encoding="utf-8") + + if "TODO_FAIL_SANDBOX" in diff_text: + print("Simulated linter failure requested by fixture marker.", file=sys.stderr) + return 2 + + warnings: list[str] = [] + if "eval(" in diff_text: + warnings.append("Security-sensitive call detected: eval") + if "shell=True" in diff_text: + warnings.append("Shell execution enabled in subprocess call") + if "verify=False" in diff_text: + warnings.append("TLS verification disabled") + + payload = { + "diff_file": str(diff_path), + "warning_count": len(warnings), + "warnings": warnings, + } + print(json.dumps(payload, indent=2)) + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/examples/skills_code_review_agent/skills/code-review/scripts/run_tests.py b/examples/skills_code_review_agent/skills/code-review/scripts/run_tests.py new file mode 100644 index 00000000..7537d288 --- /dev/null +++ b/examples/skills_code_review_agent/skills/code-review/scripts/run_tests.py @@ -0,0 +1,45 @@ +"""Run sandboxed tests for the code review skill.""" + +from __future__ import annotations + +import argparse +import json +from pathlib import Path + + +def build_parser() -> argparse.ArgumentParser: + """Build CLI arguments for the test simulation script.""" + + parser = argparse.ArgumentParser(description="Simulate test execution for a diff file.") + parser.add_argument("--diff-file", required=True, help="Path to the diff file.") + return parser + + +def parse_args(argv: list[str] | None = None) -> argparse.Namespace: + """Parse command-line arguments.""" + + return build_parser().parse_args(argv) + + +def main(argv: list[str] | None = None) -> int: + """Emit a deterministic test summary based on changed paths.""" + + args = parse_args(argv) + diff_path = Path(args.diff_file).expanduser().resolve() + diff_text = diff_path.read_text(encoding="utf-8") + changed_test_files = [ + line.split(" b/", maxsplit=1)[1] + for line in diff_text.splitlines() + if line.startswith("diff --git ") and "/tests/" in line + ] + payload = { + "diff_file": str(diff_path), + "changed_test_files": changed_test_files, + "test_update_present": bool(changed_test_files), + } + print(json.dumps(payload, indent=2)) + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/examples/skills_code_review_agent/src/__init__.py b/examples/skills_code_review_agent/src/__init__.py new file mode 100644 index 00000000..5153a8f2 --- /dev/null +++ b/examples/skills_code_review_agent/src/__init__.py @@ -0,0 +1 @@ +"""Core modules for the skills code review example.""" diff --git a/examples/skills_code_review_agent/src/deduper.py b/examples/skills_code_review_agent/src/deduper.py new file mode 100644 index 00000000..9d39c9d7 --- /dev/null +++ b/examples/skills_code_review_agent/src/deduper.py @@ -0,0 +1,94 @@ +"""Finding dedupe and noise-control helpers.""" + +from __future__ import annotations + +import hashlib +import re + +from .review_types import ( + FindingDisposition, + ReviewFinding, + ReviewSeverity, +) + +_SEVERITY_RANK = { + ReviewSeverity.CRITICAL: 5, + ReviewSeverity.HIGH: 4, + ReviewSeverity.MEDIUM: 3, + ReviewSeverity.LOW: 2, + ReviewSeverity.INFO: 1, +} + + +def dedupe_and_classify_findings(findings: list[ReviewFinding]) -> list[ReviewFinding]: + """Dedupe findings and assign output buckets by confidence.""" + + deduped: dict[tuple[str, str, int | None, str], ReviewFinding] = {} + for finding in findings: + key = ( + finding.category.value, + finding.file, + finding.line, + finding.title.strip().lower(), + ) + existing = deduped.get(key) + if existing is None or _should_replace(existing, finding): + deduped[key] = finding + + processed = list(deduped.values()) + for finding in processed: + finding.fingerprint = _build_fingerprint(finding) + if finding.disposition == FindingDisposition.FINDING: + finding.disposition = _classify_disposition(finding.confidence) + + processed.sort( + key=lambda item: ( + -_SEVERITY_RANK[item.severity], + item.file, + item.line if item.line is not None else -1, + item.title, + ) + ) + return processed + + +def _should_replace(current: ReviewFinding, candidate: ReviewFinding) -> bool: + """Return whether a candidate finding should replace the current one.""" + + if candidate.confidence != current.confidence: + return candidate.confidence > current.confidence + if candidate.severity != current.severity: + return _SEVERITY_RANK[candidate.severity] > _SEVERITY_RANK[current.severity] + return len(candidate.evidence) > len(current.evidence) + + +def _classify_disposition(confidence: float) -> FindingDisposition: + """Map confidence values to output buckets.""" + + if confidence >= 0.8: + return FindingDisposition.FINDING + if confidence >= 0.4: + return FindingDisposition.NEEDS_HUMAN_REVIEW + return FindingDisposition.WARNING + + +def _normalize_evidence(evidence: str) -> str: + """Normalize evidence for dedupe keys.""" + + compact = re.sub(r"\s+", " ", evidence.strip().lower()) + return compact + + +def _build_fingerprint(finding: ReviewFinding) -> str: + """Build a stable fingerprint for downstream storage.""" + + payload = "|".join( + ( + finding.category.value, + finding.file, + str(finding.line), + finding.title, + _normalize_evidence(finding.evidence), + ) + ) + return hashlib.sha256(payload.encode("utf-8")).hexdigest() diff --git a/examples/skills_code_review_agent/src/diff_parser.py b/examples/skills_code_review_agent/src/diff_parser.py new file mode 100644 index 00000000..52c95855 --- /dev/null +++ b/examples/skills_code_review_agent/src/diff_parser.py @@ -0,0 +1,192 @@ +"""Unified diff parsing helpers for the skills code review example.""" + +from __future__ import annotations + +import re + +from .review_types import ChangedFile, DiffHunk, DiffLine, DiffLineType, ParsedDiff + +_HUNK_HEADER_RE = re.compile( + r"^@@ -(?P\d+)(?:,(?P\d+))? " + r"\+(?P\d+)(?:,(?P\d+))? @@(?P
.*)$" +) + + +def parse_unified_diff(diff_text: str) -> ParsedDiff: + """Parse unified diff text into structured file and hunk objects.""" + + parsed = ParsedDiff(raw_diff=diff_text) + current_file: ChangedFile | None = None + current_hunk: DiffHunk | None = None + pending_old_path: str | None = None + old_line_no: int | None = None + new_line_no: int | None = None + + def flush_hunk() -> None: + nonlocal current_hunk + if current_file is not None and current_hunk is not None: + current_file.hunks.append(current_hunk) + current_hunk = None + + def flush_file() -> None: + nonlocal current_file + flush_hunk() + if current_file is not None: + parsed.files.append(current_file) + current_file = None + + for raw_line in diff_text.splitlines(): + if raw_line.startswith("diff --git "): + flush_file() + old_path, new_path = _parse_diff_git_header(raw_line) + current_file = ChangedFile(old_path=old_path, new_path=new_path) + pending_old_path = None + old_line_no = None + new_line_no = None + continue + + if raw_line.startswith("--- "): + candidate_old_path = _normalize_diff_path(raw_line[4:]) + if current_file is None: + pending_old_path = candidate_old_path + else: + current_file.old_path = candidate_old_path + continue + + if raw_line.startswith("+++ "): + candidate_new_path = _normalize_diff_path(raw_line[4:]) + if current_file is None: + current_file = ChangedFile( + old_path=pending_old_path or "/dev/null", + new_path=candidate_new_path, + ) + else: + current_file.new_path = candidate_new_path + pending_old_path = None + continue + + if current_file is None: + continue + + if raw_line.startswith("new file mode "): + current_file.is_new_file = True + continue + + if raw_line.startswith("deleted file mode "): + current_file.is_deleted_file = True + continue + + if raw_line.startswith("rename from "): + current_file.is_rename = True + current_file.old_path = raw_line.removeprefix("rename from ") + continue + + if raw_line.startswith("rename to "): + current_file.is_rename = True + current_file.new_path = raw_line.removeprefix("rename to ") + continue + + match = _HUNK_HEADER_RE.match(raw_line) + if match: + flush_hunk() + current_hunk = DiffHunk( + header=raw_line, + old_start=int(match.group("old_start")), + old_count=int(match.group("old_count") or "1"), + new_start=int(match.group("new_start")), + new_count=int(match.group("new_count") or "1"), + ) + old_line_no = current_hunk.old_start + new_line_no = current_hunk.new_start + continue + + if current_hunk is None or raw_line == r"\ No newline at end of file": + continue + + line = _parse_hunk_line( + raw_line=raw_line, + old_line_no=old_line_no, + new_line_no=new_line_no, + ) + if line is None: + continue + + current_hunk.lines.append(line) + if line.line_type == DiffLineType.CONTEXT: + old_line_no = _increment(old_line_no) + new_line_no = _increment(new_line_no) + elif line.line_type == DiffLineType.ADD: + new_line_no = _increment(new_line_no) + else: + old_line_no = _increment(old_line_no) + + flush_file() + return parsed + + +def _parse_diff_git_header(raw_line: str) -> tuple[str, str]: + """Parse `diff --git a/foo b/foo` into normalized paths.""" + + parts = raw_line.split(maxsplit=3) + if len(parts) != 4: + raise ValueError(f"invalid diff git header: {raw_line}") + return _normalize_diff_path(parts[2]), _normalize_diff_path(parts[3]) + + +def _normalize_diff_path(path_text: str) -> str: + """Normalize git diff paths like `a/foo.py` into `foo.py`.""" + + if path_text == "/dev/null": + return path_text + + if path_text.startswith("a/") or path_text.startswith("b/"): + return path_text[2:] + return path_text + + +def _parse_hunk_line( + *, + raw_line: str, + old_line_no: int | None, + new_line_no: int | None, +) -> DiffLine | None: + """Parse a single hunk line.""" + + if not raw_line: + return None + + marker = raw_line[0] + text = raw_line[1:] + if marker == " ": + return DiffLine( + line_type=DiffLineType.CONTEXT, + text=text, + raw_line=raw_line, + old_line_no=old_line_no, + new_line_no=new_line_no, + ) + if marker == "+": + return DiffLine( + line_type=DiffLineType.ADD, + text=text, + raw_line=raw_line, + old_line_no=None, + new_line_no=new_line_no, + ) + if marker == "-": + return DiffLine( + line_type=DiffLineType.DELETE, + text=text, + raw_line=raw_line, + old_line_no=old_line_no, + new_line_no=None, + ) + return None + + +def _increment(value: int | None) -> int | None: + """Increment a line number when it exists.""" + + if value is None: + return None + return value + 1 diff --git a/examples/skills_code_review_agent/src/filter_policy.py b/examples/skills_code_review_agent/src/filter_policy.py new file mode 100644 index 00000000..47b74ca9 --- /dev/null +++ b/examples/skills_code_review_agent/src/filter_policy.py @@ -0,0 +1,105 @@ +"""Filter policy helpers for sandbox governance decisions.""" + +from __future__ import annotations + +from dataclasses import dataclass +from pathlib import Path + +from .review_types import FilterDecisionRecord, FilterDecisionType, ParsedDiff + +_FORBIDDEN_PATH_PARTS = (".git/", ".env", "secrets/", "id_rsa", ".pem") +_NETWORK_TOKENS = ("http://", "https://", "curl", "wget", "Invoke-WebRequest", "requests.get(") +_DANGEROUS_TOKENS = ("rm -rf", "del /f", "format ", "shutdown ", "mkfs") + + +@dataclass(slots=True, frozen=True) +class SkillScriptInvocation: + """Planned sandboxed skill-script execution.""" + + name: str + script_path: Path + command: list[str] + target: str + + +def evaluate_invocations( + *, + parsed_diff: ParsedDiff, + runtime: str, + invocations: list[SkillScriptInvocation], + max_changed_files: int = 50, + max_added_lines: int = 2000, +) -> list[tuple[SkillScriptInvocation, FilterDecisionRecord]]: + """Evaluate sandbox invocations and return a decision for each one.""" + + paired: list[tuple[SkillScriptInvocation, FilterDecisionRecord]] = [] + over_budget = ( + parsed_diff.changed_files_count > max_changed_files + or parsed_diff.added_lines_count > max_added_lines + ) + + for invocation in invocations: + decision = FilterDecisionRecord( + decision=FilterDecisionType.ALLOW, + target=invocation.target, + reason_code="allow", + reason="Invocation allowed by default policy.", + ) + + if _contains_forbidden_path(parsed_diff): + decision = FilterDecisionRecord( + decision=FilterDecisionType.DENY, + target=invocation.target, + reason_code="forbidden_path", + reason="Diff touches a forbidden path and cannot enter sandbox execution.", + ) + elif _contains_token(invocation.command, _DANGEROUS_TOKENS): + decision = FilterDecisionRecord( + decision=FilterDecisionType.DENY, + target=invocation.target, + reason_code="dangerous_command", + reason="Invocation contains a dangerous command pattern.", + ) + elif _contains_token(invocation.command, _NETWORK_TOKENS): + decision = FilterDecisionRecord( + decision=FilterDecisionType.DENY, + target=invocation.target, + reason_code="network_not_allowed", + reason="Network access is not permitted for sandbox scripts by default.", + ) + elif over_budget: + decision = FilterDecisionRecord( + decision=FilterDecisionType.NEEDS_HUMAN_REVIEW, + target=invocation.target, + reason_code="over_budget", + reason="Diff size exceeds sandbox budget and requires manual approval.", + requires_human_review=True, + ) + elif runtime == "local": + decision = FilterDecisionRecord( + decision=FilterDecisionType.NEEDS_HUMAN_REVIEW, + target=invocation.target, + reason_code="local_runtime_fallback", + reason="Local runtime is a development fallback and should not be treated as a production-safe sandbox.", + requires_human_review=True, + ) + + paired.append((invocation, decision)) + return paired + + +def _contains_forbidden_path(parsed_diff: ParsedDiff) -> bool: + """Return whether the diff touches a path blocked by policy.""" + + for path in parsed_diff.changed_paths: + normalized = path.replace("\\", "/") + if any(part in normalized for part in _FORBIDDEN_PATH_PARTS): + return True + return False + + +def _contains_token(command: list[str], tokens: tuple[str, ...]) -> bool: + """Return whether a command contains any blocked token.""" + + command_text = " ".join(command) + return any(token in command_text for token in tokens) diff --git a/examples/skills_code_review_agent/src/input_loader.py b/examples/skills_code_review_agent/src/input_loader.py new file mode 100644 index 00000000..dfcdc43a --- /dev/null +++ b/examples/skills_code_review_agent/src/input_loader.py @@ -0,0 +1,138 @@ +"""Input loading helpers for diffs, repo paths, and fixtures.""" + +from __future__ import annotations + +import subprocess +from pathlib import Path +from typing import Sequence + +from .review_types import ReviewInput, ReviewInputKind + + +def load_review_input( + *, + diff_file: str | Path | None = None, + repo_path: str | Path | None = None, + fixture_path: str | Path | None = None, + changed_paths: Sequence[str] | None = None, +) -> ReviewInput: + """Load a normalized review input from a single supported source.""" + + provided_inputs = [ + value + for value in (diff_file, repo_path, fixture_path) + if value is not None + ] + if len(provided_inputs) != 1: + raise ValueError("exactly one of diff_file, repo_path, or fixture_path must be set") + + if diff_file is not None: + file_path = _ensure_existing_file(diff_file) + return ReviewInput( + kind=ReviewInputKind.DIFF_FILE, + source=str(file_path), + diff_text=file_path.read_text(encoding="utf-8"), + ) + + if fixture_path is not None: + file_path = _ensure_existing_file(fixture_path) + return ReviewInput( + kind=ReviewInputKind.FIXTURE, + source=str(file_path), + diff_text=file_path.read_text(encoding="utf-8"), + ) + + repo_dir = _ensure_existing_path(repo_path) + diff_text = load_git_workspace_diff(repo_dir, changed_paths=changed_paths) + return ReviewInput( + kind=ReviewInputKind.REPO_PATH, + source=str(repo_dir), + diff_text=diff_text, + repo_path=repo_dir, + ) + + +def load_git_workspace_diff( + repo_path: str | Path, + changed_paths: Sequence[str] | None = None, +) -> str: + """Load git diff text for a repo workspace. + + The loader prefers `git diff HEAD` because it captures both staged and + unstaged tracked changes relative to the last commit. If the repository has + no HEAD yet, it falls back to the working tree diff. + """ + + repo_dir = _ensure_existing_path(repo_path) + pathspec = list(changed_paths or []) + command = [ + "git", + "-C", + str(repo_dir), + "diff", + "--no-ext-diff", + "--binary", + "HEAD", + "--", + *pathspec, + ] + + completed = subprocess.run( + command, + capture_output=True, + check=False, + text=True, + encoding="utf-8", + ) + if completed.returncode == 0: + return completed.stdout + + if "bad revision 'HEAD'" not in completed.stderr and "unknown revision" not in completed.stderr: + raise RuntimeError( + "failed to collect git diff: " + f"{completed.stderr.strip() or completed.stdout.strip() or 'unknown error'}" + ) + + fallback = subprocess.run( + [ + "git", + "-C", + str(repo_dir), + "diff", + "--no-ext-diff", + "--binary", + "--", + *pathspec, + ], + capture_output=True, + check=False, + text=True, + encoding="utf-8", + ) + if fallback.returncode != 0: + raise RuntimeError( + "failed to collect fallback git diff: " + f"{fallback.stderr.strip() or fallback.stdout.strip() or 'unknown error'}" + ) + return fallback.stdout + + +def _ensure_existing_path(path: str | Path | None) -> Path: + """Validate a path exists and return a normalized `Path`.""" + + if path is None: + raise ValueError("path must not be None") + + resolved = Path(path).expanduser().resolve() + if not resolved.exists(): + raise FileNotFoundError(f"path does not exist: {resolved}") + return resolved + + +def _ensure_existing_file(path: str | Path) -> Path: + """Validate a path exists and points to a file.""" + + resolved = _ensure_existing_path(path) + if not resolved.is_file(): + raise FileNotFoundError(f"expected a file path: {resolved}") + return resolved diff --git a/examples/skills_code_review_agent/src/redactor.py b/examples/skills_code_review_agent/src/redactor.py new file mode 100644 index 00000000..df0721e3 --- /dev/null +++ b/examples/skills_code_review_agent/src/redactor.py @@ -0,0 +1,95 @@ +"""Secret redaction helpers for logs, reports, and persistence.""" + +from __future__ import annotations + +import re +from dataclasses import replace + +from .review_types import ( + ReviewFinding, + ReviewReport, + ReviewTask, + SandboxRunRecord, +) + +_REDACTION_PATTERNS: list[tuple[re.Pattern[str], str]] = [ + ( + re.compile(r"(?i)\b(api[_-]?key|token|password|secret)\b(\s*[:=]\s*['\"]?)([^'\"\s]{4,})(['\"]?)"), + r"\1\2[REDACTED]\4", + ), + ( + re.compile(r"(?i)(Bearer\s+)([A-Za-z0-9._\-]{6,})"), + r"\1[REDACTED]", + ), + ( + re.compile(r"AKIA[0-9A-Z]{16}"), + "[REDACTED_AWS_KEY]", + ), + ( + re.compile(r"sk-[A-Za-z0-9]{8,}"), + "[REDACTED_OPENAI_KEY]", + ), + ( + re.compile( + r"-----BEGIN (?:RSA |EC |OPENSSH )?PRIVATE KEY-----[\s\S]*?-----END (?:RSA |EC |OPENSSH )?PRIVATE KEY-----" + ), + "[REDACTED_PRIVATE_KEY]", + ), +] + + +def redact_text(text: str) -> str: + """Redact sensitive material from arbitrary text.""" + + redacted = text + for pattern, replacement in _REDACTION_PATTERNS: + redacted = pattern.sub(replacement, redacted) + return redacted + + +def redact_finding(finding: ReviewFinding) -> ReviewFinding: + """Return a copy of a finding with redacted evidence and recommendation.""" + + return replace( + finding, + evidence=redact_text(finding.evidence), + recommendation=redact_text(finding.recommendation), + title=redact_text(finding.title), + ) + + +def redact_sandbox_run(record: SandboxRunRecord) -> SandboxRunRecord: + """Return a copy of a sandbox run with redacted command output.""" + + return replace( + record, + command=[redact_text(part) for part in record.command], + stdout=redact_text(record.stdout), + stderr=redact_text(record.stderr), + ) + + +def redact_task(task: ReviewTask) -> ReviewTask: + """Return a copy of the task with redacted output-bearing fields.""" + + return replace( + task, + findings=[redact_finding(finding) for finding in task.findings], + sandbox_runs=[redact_sandbox_run(run) for run in task.sandbox_runs], + error_message=redact_text(task.error_message) if task.error_message else None, + ) + + +def redact_report(report: ReviewReport) -> ReviewReport: + """Return a copy of the report with all external-facing text redacted.""" + + return replace( + report, + findings=[redact_finding(finding) for finding in report.findings], + warnings=[redact_finding(finding) for finding in report.warnings], + needs_human_review=[ + redact_finding(finding) for finding in report.needs_human_review + ], + sandbox_runs=[redact_sandbox_run(run) for run in report.sandbox_runs], + summary=redact_text(report.summary), + ) diff --git a/examples/skills_code_review_agent/src/report_writer.py b/examples/skills_code_review_agent/src/report_writer.py new file mode 100644 index 00000000..f9cb8062 --- /dev/null +++ b/examples/skills_code_review_agent/src/report_writer.py @@ -0,0 +1,194 @@ +"""Report generation helpers for JSON and Markdown review outputs.""" + +from __future__ import annotations + +import json +from dataclasses import asdict +from enum import Enum +from pathlib import Path + +from .review_types import ReviewFinding, ReviewReport + + +def build_report_payload(report: ReviewReport) -> dict[str, object]: + """Convert a structured report into a JSON-safe payload.""" + + return json.loads(json.dumps(asdict(report), default=_json_default)) + + +def render_markdown_report(report: ReviewReport) -> str: + """Render the markdown report required by the issue.""" + + lines: list[str] = [ + "# Review Report", + "", + f"- Task ID: `{report.task_id}`", + f"- Final Verdict: `{report.conclusion.value}`", + "", + "## Summary", + "", + report.summary or "No summary available.", + "", + "## Severity Stats", + "", + ] + + if report.severity_counts: + for severity, count in sorted(report.severity_counts.items()): + lines.append(f"- `{severity}`: {count}") + else: + lines.append("- No final findings.") + + lines.extend( + [ + "", + "## Findings", + "", + ] + ) + if report.findings: + lines.extend(_render_finding_list(report.findings)) + else: + lines.append("- No final findings.") + + lines.extend( + [ + "", + "## Human Review Items", + "", + ] + ) + if report.needs_human_review: + lines.extend(_render_finding_list(report.needs_human_review)) + else: + lines.append("- None.") + + lines.extend( + [ + "", + "## Warnings", + "", + ] + ) + if report.warnings: + lines.extend(_render_finding_list(report.warnings)) + else: + lines.append("- None.") + + lines.extend( + [ + "", + "## Filter Summary", + "", + ] + ) + if report.filter_decisions: + for decision in report.filter_decisions: + lines.append( + f"- `{decision.decision.value}` on `{decision.target}`: {decision.reason}" + ) + else: + lines.append("- No filter interceptions.") + + lines.extend( + [ + "", + "## Sandbox Summary", + "", + ] + ) + if report.sandbox_runs: + for sandbox_run in report.sandbox_runs: + lines.append( + f"- `{sandbox_run.name}` status=`{sandbox_run.status.value}` " + f"duration={sandbox_run.duration_ms}ms exit_code={sandbox_run.exit_code}" + ) + else: + lines.append("- No sandbox executions.") + + lines.extend( + [ + "", + "## Monitoring", + "", + ] + ) + for key, value in sorted(report.monitoring_summary.items()): + lines.append(f"- `{key}`: {value}") + + lines.extend( + [ + "", + "## Actionable Recommendations", + "", + ] + ) + recommendations = _collect_recommendations(report) + if recommendations: + for recommendation in recommendations: + lines.append(f"- {recommendation}") + else: + lines.append("- No recommendations.") + + return "\n".join(lines) + "\n" + + +def write_report_files( + report: ReviewReport, + *, + output_dir: str | Path, +) -> tuple[Path, Path]: + """Write both JSON and Markdown reports to the output directory.""" + + output_path = Path(output_dir).expanduser().resolve() + output_path.mkdir(parents=True, exist_ok=True) + + json_path = output_path / "review_report.json" + markdown_path = output_path / "review_report.md" + + json_path.write_text( + json.dumps(build_report_payload(report), indent=2, ensure_ascii=False), + encoding="utf-8", + ) + markdown_path.write_text(render_markdown_report(report), encoding="utf-8") + return json_path, markdown_path + + +def _render_finding_list(findings: list[ReviewFinding]) -> list[str]: + """Render a flat markdown list of findings.""" + + lines: list[str] = [] + for finding in findings: + location = ( + f"{finding.file}:{finding.line}" + if finding.line is not None + else finding.file + ) + lines.append( + f"- [`{finding.severity.value}`] `{finding.category.value}` at `{location}`: {finding.title}" + ) + lines.append(f" Evidence: `{finding.evidence}`") + lines.append(f" Recommendation: {finding.recommendation}") + return lines + + +def _collect_recommendations(report: ReviewReport) -> list[str]: + """Collect unique actionable recommendations from all report buckets.""" + + seen: set[str] = set() + ordered: list[str] = [] + for finding in report.findings + report.needs_human_review + report.warnings: + if finding.recommendation not in seen: + seen.add(finding.recommendation) + ordered.append(finding.recommendation) + return ordered + + +def _json_default(value: object) -> object: + """Serialize enums and paths for report payloads.""" + + if isinstance(value, Enum): + return value.value + if isinstance(value, Path): + return str(value) + raise TypeError(f"Object of type {type(value).__name__} is not JSON serializable") diff --git a/examples/skills_code_review_agent/src/review_types.py b/examples/skills_code_review_agent/src/review_types.py new file mode 100644 index 00000000..9c59791c --- /dev/null +++ b/examples/skills_code_review_agent/src/review_types.py @@ -0,0 +1,420 @@ +"""Shared review data models for the skills code review example.""" + +from __future__ import annotations + +from dataclasses import dataclass, field +from enum import Enum +from pathlib import Path + + +class ReviewInputKind(str, Enum): + """Supported review input sources.""" + + DIFF_FILE = "diff_file" + REPO_PATH = "repo_path" + FIXTURE = "fixture" + + +class DiffLineType(str, Enum): + """Line types inside a unified diff hunk.""" + + CONTEXT = "context" + ADD = "add" + DELETE = "delete" + + +class ReviewStatus(str, Enum): + """Lifecycle states for a review task.""" + + PENDING = "pending" + RUNNING = "running" + COMPLETED = "completed" + FAILED = "failed" + + +class ReviewSeverity(str, Enum): + """Supported finding severity levels.""" + + CRITICAL = "critical" + HIGH = "high" + MEDIUM = "medium" + LOW = "low" + INFO = "info" + + +class ReviewCategory(str, Enum): + """High-level finding categories used by the review agent.""" + + SECURITY = "security" + ASYNC = "async" + RESOURCE_LEAK = "resource_leak" + TEST_MISSING = "test_missing" + SECRET = "secret" + DB_LIFECYCLE = "db_lifecycle" + SANDBOX = "sandbox" + GENERAL = "general" + + +class FindingSource(str, Enum): + """Sources that can produce review findings.""" + + RULE_ENGINE = "rule_engine" + SKILL_SCRIPT = "skill_script" + SANDBOX = "sandbox" + FILTER = "filter" + MODEL = "model" + + +class FindingDisposition(str, Enum): + """How a finding should be presented downstream.""" + + FINDING = "finding" + WARNING = "warning" + NEEDS_HUMAN_REVIEW = "needs_human_review" + + +class SandboxRunStatus(str, Enum): + """Execution status for a sandbox run.""" + + PENDING = "pending" + RUNNING = "running" + SUCCEEDED = "succeeded" + FAILED = "failed" + TIMED_OUT = "timed_out" + BLOCKED = "blocked" + + +class FilterDecisionType(str, Enum): + """Supported filter outcomes before sandbox execution.""" + + ALLOW = "allow" + DENY = "deny" + NEEDS_HUMAN_REVIEW = "needs_human_review" + + +class ReviewConclusion(str, Enum): + """Final review verdict for reporting.""" + + PASS = "pass" + FAIL = "fail" + NEEDS_HUMAN_REVIEW = "needs_human_review" + ERROR = "error" + + +@dataclass(slots=True, frozen=True) +class ReviewInput: + """Normalized review input before parsing.""" + + kind: ReviewInputKind + source: str + diff_text: str + repo_path: Path | None = None + + +@dataclass(slots=True, frozen=True) +class DiffLine: + """Single parsed line inside a unified diff hunk.""" + + line_type: DiffLineType + text: str + raw_line: str + old_line_no: int | None + new_line_no: int | None + + +@dataclass(slots=True) +class DiffHunk: + """A parsed unified diff hunk.""" + + header: str + old_start: int + old_count: int + new_start: int + new_count: int + lines: list[DiffLine] = field(default_factory=list) + + @property + def added_line_numbers(self) -> list[int]: + """Return new-file line numbers added in this hunk.""" + return [ + line.new_line_no + for line in self.lines + if line.line_type == DiffLineType.ADD and line.new_line_no is not None + ] + + @property + def deleted_line_numbers(self) -> list[int]: + """Return old-file line numbers deleted in this hunk.""" + return [ + line.old_line_no + for line in self.lines + if line.line_type == DiffLineType.DELETE and line.old_line_no is not None + ] + + def candidate_line_numbers(self, radius: int = 1) -> list[int]: + """Return candidate new-file line numbers for rule checks. + + Candidate lines include added lines and their nearby context lines in the + resulting file so rules can inspect a small amount of surrounding code. + """ + + if radius < 0: + raise ValueError("radius must be >= 0") + + numbers: set[int] = set(self.added_line_numbers) + if radius == 0: + return sorted(numbers) + + line_count = len(self.lines) + for index, line in enumerate(self.lines): + if line.line_type != DiffLineType.ADD: + continue + + numbers.update( + _collect_neighbor_new_lines( + lines=self.lines, + start_index=index, + direction=-1, + limit=radius, + ) + ) + numbers.update( + _collect_neighbor_new_lines( + lines=self.lines, + start_index=index, + direction=1, + limit=radius, + ) + ) + + return sorted(numbers) + + +@dataclass(slots=True) +class ChangedFile: + """A changed file in a parsed diff.""" + + old_path: str + new_path: str + hunks: list[DiffHunk] = field(default_factory=list) + is_new_file: bool = False + is_deleted_file: bool = False + is_rename: bool = False + + @property + def display_path(self) -> str: + """Return the most useful path for user-facing output.""" + return self.new_path if self.new_path != "/dev/null" else self.old_path + + @property + def added_line_numbers(self) -> list[int]: + """Return all added line numbers across hunks.""" + numbers = { + line_no + for hunk in self.hunks + for line_no in hunk.added_line_numbers + } + return sorted(numbers) + + @property + def deleted_line_numbers(self) -> list[int]: + """Return all deleted line numbers across hunks.""" + numbers = { + line_no + for hunk in self.hunks + for line_no in hunk.deleted_line_numbers + } + return sorted(numbers) + + def candidate_line_numbers(self, radius: int = 1) -> list[int]: + """Return deduplicated candidate line numbers across hunks.""" + numbers = { + line_no + for hunk in self.hunks + for line_no in hunk.candidate_line_numbers(radius=radius) + } + return sorted(numbers) + + +@dataclass(slots=True) +class ParsedDiff: + """Top-level parsed diff structure.""" + + raw_diff: str + files: list[ChangedFile] = field(default_factory=list) + + @property + def changed_files_count(self) -> int: + """Return the number of changed files.""" + return len(self.files) + + @property + def added_lines_count(self) -> int: + """Return the total number of added lines.""" + return sum(len(diff_file.added_line_numbers) for diff_file in self.files) + + @property + def deleted_lines_count(self) -> int: + """Return the total number of deleted lines.""" + return sum(len(diff_file.deleted_line_numbers) for diff_file in self.files) + + @property + def changed_paths(self) -> list[str]: + """Return changed file paths suitable for reporting.""" + return [diff_file.display_path for diff_file in self.files] + + +@dataclass(slots=True) +class ReviewFinding: + """Structured review finding emitted by rules, filters, or sandbox runs.""" + + severity: ReviewSeverity + category: ReviewCategory + file: str + line: int | None + title: str + evidence: str + recommendation: str + confidence: float + source: FindingSource + disposition: FindingDisposition = FindingDisposition.FINDING + fingerprint: str | None = None + + def __post_init__(self) -> None: + """Validate normalized confidence range.""" + + if not 0.0 <= self.confidence <= 1.0: + raise ValueError("confidence must be between 0.0 and 1.0") + + +@dataclass(slots=True) +class SandboxRunRecord: + """Audit record for a single sandboxed command or script execution.""" + + name: str + command: list[str] + status: SandboxRunStatus + runtime: str + duration_ms: int = 0 + exit_code: int | None = None + stdout: str = "" + stderr: str = "" + timed_out: bool = False + output_truncated: bool = False + blocked_by_filter: bool = False + + +@dataclass(slots=True) +class FilterDecisionRecord: + """Structured record for a pre-execution filter decision.""" + + decision: FilterDecisionType + target: str + reason_code: str + reason: str + requires_human_review: bool = False + + +@dataclass(slots=True) +class ReviewTask: + """Top-level review task and its execution state.""" + + task_id: str + status: ReviewStatus + review_input: ReviewInput + parsed_diff: ParsedDiff | None = None + findings: list[ReviewFinding] = field(default_factory=list) + sandbox_runs: list[SandboxRunRecord] = field(default_factory=list) + filter_decisions: list[FilterDecisionRecord] = field(default_factory=list) + error_message: str | None = None + + def add_finding(self, finding: ReviewFinding) -> None: + """Append a new finding to the task.""" + + self.findings.append(finding) + + def add_sandbox_run(self, sandbox_run: SandboxRunRecord) -> None: + """Append a sandbox execution record.""" + + self.sandbox_runs.append(sandbox_run) + + def add_filter_decision(self, decision: FilterDecisionRecord) -> None: + """Append a filter decision record.""" + + self.filter_decisions.append(decision) + + +@dataclass(slots=True) +class ReviewReport: + """Final structured report returned by the review pipeline.""" + + task_id: str + conclusion: ReviewConclusion + findings: list[ReviewFinding] = field(default_factory=list) + warnings: list[ReviewFinding] = field(default_factory=list) + needs_human_review: list[ReviewFinding] = field(default_factory=list) + filter_decisions: list[FilterDecisionRecord] = field(default_factory=list) + sandbox_runs: list[SandboxRunRecord] = field(default_factory=list) + summary: str = "" + severity_counts: dict[str, int] = field(default_factory=dict) + monitoring_summary: dict[str, int | float | str] = field(default_factory=dict) + + @classmethod + def from_task( + cls, + *, + task: ReviewTask, + conclusion: ReviewConclusion, + summary: str = "", + monitoring_summary: dict[str, int | float | str] | None = None, + ) -> "ReviewReport": + """Build a report by splitting findings according to disposition.""" + + findings: list[ReviewFinding] = [] + warnings: list[ReviewFinding] = [] + needs_human_review: list[ReviewFinding] = [] + + for finding in task.findings: + if finding.disposition == FindingDisposition.WARNING: + warnings.append(finding) + elif finding.disposition == FindingDisposition.NEEDS_HUMAN_REVIEW: + needs_human_review.append(finding) + else: + findings.append(finding) + + severity_counts: dict[str, int] = {} + for finding in findings: + key = finding.severity.value + severity_counts[key] = severity_counts.get(key, 0) + 1 + + return cls( + task_id=task.task_id, + conclusion=conclusion, + findings=findings, + warnings=warnings, + needs_human_review=needs_human_review, + filter_decisions=list(task.filter_decisions), + sandbox_runs=list(task.sandbox_runs), + summary=summary, + severity_counts=severity_counts, + monitoring_summary=monitoring_summary or {}, + ) + + +def _collect_neighbor_new_lines( + *, + lines: list[DiffLine], + start_index: int, + direction: int, + limit: int, +) -> set[int]: + """Collect nearby new-file line numbers while skipping deleted lines.""" + + collected: set[int] = set() + index = start_index + direction + while 0 <= index < len(lines) and len(collected) < limit: + line = lines[index] + if line.new_line_no is not None: + collected.add(line.new_line_no) + index += direction + return collected diff --git a/examples/skills_code_review_agent/src/rule_engine.py b/examples/skills_code_review_agent/src/rule_engine.py new file mode 100644 index 00000000..22ca17e1 --- /dev/null +++ b/examples/skills_code_review_agent/src/rule_engine.py @@ -0,0 +1,630 @@ +"""Deterministic rule engine for code review findings.""" + +from __future__ import annotations + +import re +from collections.abc import Iterable + +from .review_types import ( + ChangedFile, + DiffHunk, + DiffLine, + DiffLineType, + FindingSource, + ParsedDiff, + ReviewCategory, + ReviewFinding, + ReviewSeverity, +) + +_SECRET_PATTERNS: list[tuple[re.Pattern[str], str, str, ReviewSeverity, float]] = [ + ( + re.compile(r"(?i)\b(api[_-]?key|token|password|secret)\b\s*[:=]\s*['\"][^'\"]{6,}['\"]"), + "Hard-coded secret detected", + "Sensitive credentials should not be committed to source control.", + ReviewSeverity.HIGH, + 0.98, + ), + ( + re.compile(r"\bsk-(?:proj-)?[A-Za-z0-9]{12,}\b"), + "OpenAI-style API key detected", + "Do not commit model API keys; load them from a secure environment or secret manager.", + ReviewSeverity.CRITICAL, + 0.99, + ), + ( + re.compile(r"\bgh[pousr]_[A-Za-z0-9]{20,}\b"), + "GitHub token detected", + "Revoke the token and load GitHub credentials from a secure runtime secret store.", + ReviewSeverity.CRITICAL, + 0.99, + ), + ( + re.compile(r"AKIA[0-9A-Z]{16}"), + "AWS access key detected", + "Remove the credential from code and load it from a secure secret manager.", + ReviewSeverity.CRITICAL, + 0.99, + ), + ( + re.compile(r"(?i)Bearer\s+[A-Za-z0-9._\-]{8,}"), + "Bearer token detected", + "Do not embed bearer tokens in code or fixtures.", + ReviewSeverity.HIGH, + 0.97, + ), + ( + re.compile(r"-----BEGIN (?:RSA |EC |OPENSSH )?PRIVATE KEY-----"), + "Private key material detected", + "Rotate the key immediately and move secret material out of the repository.", + ReviewSeverity.CRITICAL, + 1.0, + ), +] + +_CODE_FILE_EXTENSIONS = { + ".py", + ".js", + ".jsx", + ".ts", + ".tsx", + ".go", + ".java", + ".rb", + ".rs", + ".php", + ".cpp", + ".c", + ".h", + ".hpp", +} + +_SECRET_PLACEHOLDERS = ( + "example", + "dummy", + "changeme", + "replace-me", + "replace_me", + "test-token", + "test_key", + "test-key", + "fake", + "sample", + "placeholder", + "masked", + "redacted", + "", + "", +) + + +def run_rule_engine(parsed_diff: ParsedDiff) -> list[ReviewFinding]: + """Run deterministic review rules on a parsed diff.""" + + findings: list[ReviewFinding] = [] + findings.extend(_check_missing_tests(parsed_diff)) + + for changed_file in parsed_diff.files: + findings.extend(_check_security_rules(changed_file)) + findings.extend(_check_secret_rules(changed_file)) + findings.extend(_check_async_rules(changed_file)) + findings.extend(_check_resource_leak_rules(changed_file)) + findings.extend(_check_db_lifecycle_rules(changed_file)) + + return findings + + +def _check_security_rules(changed_file: ChangedFile) -> list[ReviewFinding]: + """Detect high-confidence security patterns in newly added code.""" + + findings: list[ReviewFinding] = [] + for hunk, index, line in _iter_added_lines(changed_file): + text = line.text.strip() + context = _line_context(hunk, index) + + if _looks_like_comment(text): + continue + + if re.search(r"\beval\s*\(", text): + findings.append( + _make_finding( + category=ReviewCategory.SECURITY, + severity=ReviewSeverity.HIGH, + changed_file=changed_file, + line=line, + title="Use of eval introduces code execution risk", + evidence=context, + recommendation=( + "Replace eval with explicit parsing, a whitelist-based dispatcher, " + "or a safe literal parser." + ), + confidence=0.98, + ) + ) + if re.search(r"\bexec\s*\(", text): + findings.append( + _make_finding( + category=ReviewCategory.SECURITY, + severity=ReviewSeverity.HIGH, + changed_file=changed_file, + line=line, + title="Use of exec introduces arbitrary code execution risk", + evidence=context, + recommendation="Avoid exec and replace it with explicit callable dispatch.", + confidence=0.98, + ) + ) + if "pickle.loads(" in text: + findings.append( + _make_finding( + category=ReviewCategory.SECURITY, + severity=ReviewSeverity.HIGH, + changed_file=changed_file, + line=line, + title="pickle.loads on untrusted input is unsafe", + evidence=context, + recommendation=( + "Avoid deserializing untrusted pickle payloads; prefer a safe format such as JSON." + ), + confidence=0.96, + ) + ) + if ( + "yaml.load(" in text + and "safe_load" not in text + and "SafeLoader" not in context + and "Loader=yaml.SafeLoader" not in context + ): + findings.append( + _make_finding( + category=ReviewCategory.SECURITY, + severity=ReviewSeverity.HIGH, + changed_file=changed_file, + line=line, + title="yaml.load without a safe loader is unsafe", + evidence=context, + recommendation="Use yaml.safe_load or pass SafeLoader explicitly.", + confidence=0.94, + ) + ) + if "verify=False" in text: + confidence = 0.88 + severity = ReviewSeverity.MEDIUM + recommendation = ( + "Keep certificate verification enabled or document a controlled test-only exception." + ) + if _looks_like_test_or_mock_context(changed_file.display_path, context): + confidence = 0.58 + recommendation = ( + "If this is test-only code, isolate the exception clearly; otherwise keep TLS " + "verification enabled." + ) + findings.append( + _make_finding( + category=ReviewCategory.SECURITY, + severity=severity, + changed_file=changed_file, + line=line, + title="TLS certificate verification is disabled", + evidence=context, + recommendation=recommendation, + confidence=confidence, + ) + ) + if _is_subprocess_shell_risk(text=text, context=context): + findings.append( + _make_finding( + category=ReviewCategory.SECURITY, + severity=ReviewSeverity.HIGH, + changed_file=changed_file, + line=line, + title="subprocess call enables shell execution", + evidence=context, + recommendation="Pass an argument list and avoid shell=True unless a reviewed shell command is unavoidable.", + confidence=0.95, + ) + ) + + return findings + + +def _check_secret_rules(changed_file: ChangedFile) -> list[ReviewFinding]: + """Detect obvious hard-coded secrets and credential material.""" + + findings: list[ReviewFinding] = [] + for hunk, index, line in _iter_added_lines(changed_file): + context = _line_context(hunk, index) + if _looks_like_comment(line.text.strip()): + continue + for pattern, title, recommendation, severity, confidence in _SECRET_PATTERNS: + if pattern.search(line.text) and not _looks_like_secret_placeholder(line.text): + findings.append( + _make_finding( + category=ReviewCategory.SECRET, + severity=severity, + changed_file=changed_file, + line=line, + title=title, + evidence=context, + recommendation=recommendation, + confidence=confidence, + ) + ) + return findings + + +def _check_async_rules(changed_file: ChangedFile) -> list[ReviewFinding]: + """Detect common async anti-patterns with controlled confidence.""" + + findings: list[ReviewFinding] = [] + for hunk, index, line in _iter_added_lines(changed_file): + text = line.text.strip() + context = _line_context(hunk, index) + + if _looks_like_comment(text): + continue + + if re.match(r"asyncio\.create_task\s*\(", text): + findings.append( + _make_finding( + category=ReviewCategory.ASYNC, + severity=ReviewSeverity.MEDIUM, + changed_file=changed_file, + line=line, + title="Detached asyncio task is created without lifecycle tracking", + evidence=context, + recommendation=( + "Store the task handle, await completion, or add explicit cancellation and error handling." + ), + confidence=0.72, + ) + ) + + if text.startswith("except Exception"): + next_line = _next_meaningful_new_line(hunk.lines, index) + if next_line is not None and next_line.text.strip() in {"pass", "return None", "return"}: + findings.append( + _make_finding( + category=ReviewCategory.ASYNC, + severity=ReviewSeverity.MEDIUM, + changed_file=changed_file, + line=line, + title="Broad exception is swallowed silently", + evidence=_line_context(hunk, index, radius=2), + recommendation=( + "Handle expected exception types explicitly and log or re-raise unexpected failures." + ), + confidence=0.71, + ) + ) + + return findings + + +def _check_resource_leak_rules(changed_file: ChangedFile) -> list[ReviewFinding]: + """Detect likely resource lifecycle issues outside database-specific patterns.""" + + findings: list[ReviewFinding] = [] + added_text = _all_added_text(changed_file) + for hunk, index, line in _iter_added_lines(changed_file): + text = line.text.strip() + context = _line_context(hunk, index) + + if _looks_like_comment(text): + continue + + if "open(" in text and not text.startswith("with open("): + handle_name = _extract_assigned_name(text, "open(") + if handle_name and _variable_is_closed_later(handle_name, added_text): + continue + findings.append( + _make_finding( + category=ReviewCategory.RESOURCE_LEAK, + severity=ReviewSeverity.MEDIUM, + changed_file=changed_file, + line=line, + title="File handle is opened without a context manager", + evidence=context, + recommendation="Wrap file access in `with open(...)` so handles are closed automatically.", + confidence=0.78, + ) + ) + + if _contains_resource_constructor(text) and not text.startswith(("with ", "async with ")): + resource_name = _extract_assignment_target(text) + if resource_name and _variable_is_closed_later(resource_name, added_text): + continue + findings.append( + _make_finding( + category=ReviewCategory.RESOURCE_LEAK, + severity=ReviewSeverity.MEDIUM, + changed_file=changed_file, + line=line, + title="Client or session may not be closed", + evidence=context, + recommendation=( + "Prefer `with` / `async with` for client and session objects, or ensure close() is called." + ), + confidence=0.66, + ) + ) + + return findings + + +def _check_db_lifecycle_rules(changed_file: ChangedFile) -> list[ReviewFinding]: + """Detect likely transaction or connection lifecycle issues.""" + + findings: list[ReviewFinding] = [] + file_added_text = _all_added_text(changed_file) + has_close = ".close(" in file_added_text or "close()" in file_added_text + has_commit = ".commit(" in file_added_text or "commit()" in file_added_text + has_rollback = ".rollback(" in file_added_text or "rollback()" in file_added_text + + for hunk, index, line in _iter_added_lines(changed_file): + text = line.text.strip() + context = _line_context(hunk, index) + + if _looks_like_comment(text): + continue + + if _contains_db_connect(text) and not text.startswith("with "): + connection_name = _extract_assignment_target(text) + connection_is_closed = bool( + connection_name and _variable_is_closed_later(connection_name, file_added_text) + ) + if connection_is_closed: + continue + confidence = 0.74 if has_close else 0.86 + findings.append( + _make_finding( + category=ReviewCategory.DB_LIFECYCLE, + severity=ReviewSeverity.MEDIUM if has_close else ReviewSeverity.HIGH, + changed_file=changed_file, + line=line, + title="Database connection is created without clear lifecycle management", + evidence=context, + recommendation=( + "Use a context manager or ensure the connection is closed in all execution paths." + ), + confidence=confidence, + ) + ) + + if _contains_transaction_start(text) and not (has_commit and has_rollback): + confidence = 0.67 + recommendation = "Ensure transactions are committed on success and rolled back on failure." + if has_commit and not has_rollback: + confidence = 0.58 + recommendation = ( + "A commit path exists, but rollback handling is not visible; add rollback or context-managed " + "transaction handling." + ) + findings.append( + _make_finding( + category=ReviewCategory.DB_LIFECYCLE, + severity=ReviewSeverity.MEDIUM, + changed_file=changed_file, + line=line, + title="Transaction handling appears incomplete", + evidence=context, + recommendation=recommendation, + confidence=confidence, + ) + ) + + return findings + + +def _check_missing_tests(parsed_diff: ParsedDiff) -> list[ReviewFinding]: + """Flag code changes that do not include corresponding test updates.""" + + changed_paths = parsed_diff.changed_paths + code_paths = [path for path in changed_paths if _is_production_code_path(path)] + has_test_change = any(_is_test_path(path) for path in changed_paths) + + if not code_paths or has_test_change: + return [] + + return [ + ReviewFinding( + severity=ReviewSeverity.MEDIUM, + category=ReviewCategory.TEST_MISSING, + file=code_paths[0], + line=None, + title="Production code changed without test updates", + evidence="Changed code paths: " + ", ".join(code_paths[:5]), + recommendation="Add or update tests that cover the new behavior and failure paths.", + confidence=0.62, + source=FindingSource.RULE_ENGINE, + ) + ] + + +def _iter_added_lines(changed_file: ChangedFile) -> Iterable[tuple[DiffHunk, int, DiffLine]]: + """Yield added lines together with their hunk and index.""" + + for hunk in changed_file.hunks: + for index, line in enumerate(hunk.lines): + if line.line_type == DiffLineType.ADD and line.new_line_no is not None: + yield hunk, index, line + + +def _line_context(hunk: DiffHunk, index: int, radius: int = 1) -> str: + """Return a compact context snippet around a line.""" + + start = max(0, index - radius) + end = min(len(hunk.lines), index + radius + 1) + return "\n".join(hunk.lines[offset].raw_line for offset in range(start, end)) + + +def _next_meaningful_new_line(lines: list[DiffLine], index: int) -> DiffLine | None: + """Return the next added or context line with a new-file line number.""" + + for offset in range(index + 1, len(lines)): + candidate = lines[offset] + if candidate.new_line_no is not None: + return candidate + return None + + +def _contains_resource_constructor(text: str) -> bool: + """Return whether a line creates a client or session likely needing cleanup.""" + + return any( + token in text + for token in ( + "requests.Session(", + "httpx.Client(", + "httpx.AsyncClient(", + "aiohttp.ClientSession(", + "ClientSession(", + ) + ) + + +def _contains_db_connect(text: str) -> bool: + """Return whether a line appears to create a database connection or session.""" + + return any( + token in text + for token in ( + "sqlite3.connect(", + "psycopg.connect(", + "psycopg2.connect(", + "Session(", + "sessionmaker(", + ".cursor(", + ) + ) + + +def _contains_transaction_start(text: str) -> bool: + """Return whether a line appears to begin a transaction.""" + + return any(token in text for token in (".begin(", "BEGIN", "transaction(")) + + +def _looks_like_comment(text: str) -> bool: + """Return whether a line is a comment or doc-style example.""" + + stripped = text.strip() + return stripped.startswith(("#", "//", "/*", "*", '"""', "'''")) + + +def _looks_like_test_or_mock_context(path: str, context: str) -> bool: + """Return whether a security smell appears in test-only or mock-like code.""" + + normalized_path = path.replace("\\", "/").lower() + lowered_context = context.lower() + return _is_test_path(normalized_path) or any( + token in lowered_context + for token in ("localhost", "127.0.0.1", "example.com", "mock", "fixture", "test") + ) + + +def _is_subprocess_shell_risk(*, text: str, context: str) -> bool: + """Return whether a subprocess call is combined with shell=True in the local context.""" + + if "shell=True" not in text and "subprocess." not in text: + return False + if "shell=True" not in context: + return False + return bool( + re.search( + r"\bsubprocess\.(?:run|Popen|call|check_call|check_output)\s*\(", + context, + ) + ) + + +def _looks_like_secret_placeholder(text: str) -> bool: + """Return whether a matched secret line uses explicit placeholder values.""" + + lowered = text.lower() + return any(token in lowered for token in _SECRET_PLACEHOLDERS) + + +def _extract_assignment_target(text: str) -> str | None: + """Extract a simple assignment target from a source line.""" + + match = re.match(r"([A-Za-z_][A-Za-z0-9_]*)\s*=", text) + if match is None: + return None + return match.group(1) + + +def _extract_assigned_name(text: str, marker: str) -> str | None: + """Extract a variable name for lines like `name = something(marker...)`.""" + + if marker not in text: + return None + return _extract_assignment_target(text) + + +def _variable_is_closed_later(variable_name: str, added_text: str) -> bool: + """Return whether the variable appears to be closed later in added code.""" + + return bool( + re.search(rf"\b{re.escape(variable_name)}\s*\.close\s*\(", added_text) + or re.search(rf"\bclose\s*\(\s*{re.escape(variable_name)}\s*\)", added_text) + ) + + +def _all_added_text(changed_file: ChangedFile) -> str: + """Return all added text for a changed file.""" + + return "\n".join(line.text for _, _, line in _iter_added_lines(changed_file)) + + +def _is_test_path(path: str) -> bool: + """Return whether a changed path looks like a test file.""" + + normalized = path.replace("\\", "/") + basename = normalized.rsplit("/", maxsplit=1)[-1] + return ( + "/tests/" in f"/{normalized}/" + or basename.startswith("test_") + or basename.endswith("_test.py") + or basename.endswith(".spec.ts") + or basename.endswith(".spec.tsx") + or basename.endswith(".test.ts") + or basename.endswith(".test.tsx") + ) + + +def _is_production_code_path(path: str) -> bool: + """Return whether a changed path should count as production code.""" + + normalized = path.replace("\\", "/") + if _is_test_path(normalized): + return False + if any(part in normalized for part in ("/docs/", "/migrations/", "/examples/")): + return False + return any(normalized.endswith(ext) for ext in _CODE_FILE_EXTENSIONS) + + +def _make_finding( + *, + category: ReviewCategory, + severity: ReviewSeverity, + changed_file: ChangedFile, + line: DiffLine, + title: str, + evidence: str, + recommendation: str, + confidence: float, +) -> ReviewFinding: + """Create a normalized rule-engine finding.""" + + return ReviewFinding( + severity=severity, + category=category, + file=changed_file.display_path, + line=line.new_line_no, + title=title, + evidence=evidence, + recommendation=recommendation, + confidence=confidence, + source=FindingSource.RULE_ENGINE, + ) diff --git a/examples/skills_code_review_agent/src/storage/__init__.py b/examples/skills_code_review_agent/src/storage/__init__.py new file mode 100644 index 00000000..1a2a7487 --- /dev/null +++ b/examples/skills_code_review_agent/src/storage/__init__.py @@ -0,0 +1 @@ +"""Storage layer for the skills code review example.""" diff --git a/examples/skills_code_review_agent/src/storage/init_db.py b/examples/skills_code_review_agent/src/storage/init_db.py new file mode 100644 index 00000000..109a4c0e --- /dev/null +++ b/examples/skills_code_review_agent/src/storage/init_db.py @@ -0,0 +1,24 @@ +"""Database initialization helpers for the skills code review example.""" + +from __future__ import annotations + +import sqlite3 +from pathlib import Path + +from .models import ALL_TABLES + + +def initialize_database(db_path: str | Path) -> Path: + """Create all SQLite tables needed by the review pipeline.""" + + resolved = Path(db_path).expanduser().resolve() + resolved.parent.mkdir(parents=True, exist_ok=True) + + connection = sqlite3.connect(resolved) + try: + for table in ALL_TABLES: + connection.execute(table.ddl) + connection.commit() + finally: + connection.close() + return resolved diff --git a/examples/skills_code_review_agent/src/storage/models.py b/examples/skills_code_review_agent/src/storage/models.py new file mode 100644 index 00000000..09ca429c --- /dev/null +++ b/examples/skills_code_review_agent/src/storage/models.py @@ -0,0 +1,131 @@ +"""SQLite schema definitions for review tasks, runs, findings, and reports.""" + +from __future__ import annotations + +from dataclasses import dataclass + + +@dataclass(slots=True, frozen=True) +class TableSchema: + """Simple SQLite table schema descriptor.""" + + name: str + ddl: str + + +REVIEW_TASKS_TABLE = TableSchema( + name="review_tasks", + ddl=""" + CREATE TABLE IF NOT EXISTS review_tasks ( + task_id TEXT PRIMARY KEY, + status TEXT NOT NULL, + input_type TEXT NOT NULL, + runtime_type TEXT NOT NULL, + dry_run INTEGER NOT NULL, + fake_model INTEGER NOT NULL, + source TEXT NOT NULL, + created_at TEXT NOT NULL, + finished_at TEXT NOT NULL, + total_duration_ms INTEGER NOT NULL, + error_message TEXT + ) + """.strip(), +) + +REVIEW_INPUTS_TABLE = TableSchema( + name="review_inputs", + ddl=""" + CREATE TABLE IF NOT EXISTS review_inputs ( + task_id TEXT PRIMARY KEY, + diff_sha256 TEXT NOT NULL, + changed_files_count INTEGER NOT NULL, + hunk_count INTEGER NOT NULL, + candidate_line_count INTEGER NOT NULL, + input_summary TEXT NOT NULL, + FOREIGN KEY (task_id) REFERENCES review_tasks(task_id) + ) + """.strip(), +) + +FILTER_DECISIONS_TABLE = TableSchema( + name="filter_decisions", + ddl=""" + CREATE TABLE IF NOT EXISTS filter_decisions ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + task_id TEXT NOT NULL, + decision TEXT NOT NULL, + reason_code TEXT NOT NULL, + reason_text TEXT NOT NULL, + target TEXT NOT NULL, + requires_human_review INTEGER NOT NULL, + FOREIGN KEY (task_id) REFERENCES review_tasks(task_id) + ) + """.strip(), +) + +SANDBOX_RUNS_TABLE = TableSchema( + name="sandbox_runs", + ddl=""" + CREATE TABLE IF NOT EXISTS sandbox_runs ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + task_id TEXT NOT NULL, + run_name TEXT NOT NULL, + command TEXT NOT NULL, + status TEXT NOT NULL, + runtime TEXT NOT NULL, + duration_ms INTEGER NOT NULL, + exit_code INTEGER, + stdout_summary TEXT NOT NULL, + stderr_summary TEXT NOT NULL, + timed_out INTEGER NOT NULL, + output_truncated INTEGER NOT NULL, + blocked_by_filter INTEGER NOT NULL, + FOREIGN KEY (task_id) REFERENCES review_tasks(task_id) + ) + """.strip(), +) + +FINDINGS_TABLE = TableSchema( + name="findings", + ddl=""" + CREATE TABLE IF NOT EXISTS findings ( + id INTEGER PRIMARY KEY AUTOINCREMENT, + task_id TEXT NOT NULL, + fingerprint TEXT, + severity TEXT NOT NULL, + category TEXT NOT NULL, + file TEXT NOT NULL, + line INTEGER, + title TEXT NOT NULL, + evidence TEXT NOT NULL, + recommendation TEXT NOT NULL, + confidence REAL NOT NULL, + source TEXT NOT NULL, + disposition TEXT NOT NULL, + FOREIGN KEY (task_id) REFERENCES review_tasks(task_id) + ) + """.strip(), +) + +REVIEW_REPORTS_TABLE = TableSchema( + name="review_reports", + ddl=""" + CREATE TABLE IF NOT EXISTS review_reports ( + task_id TEXT PRIMARY KEY, + final_verdict TEXT NOT NULL, + report_json TEXT NOT NULL, + report_markdown TEXT NOT NULL, + monitoring_summary TEXT NOT NULL, + FOREIGN KEY (task_id) REFERENCES review_tasks(task_id) + ) + """.strip(), +) + +ALL_TABLES = ( + REVIEW_TASKS_TABLE, + REVIEW_INPUTS_TABLE, + FILTER_DECISIONS_TABLE, + SANDBOX_RUNS_TABLE, + FINDINGS_TABLE, + REVIEW_REPORTS_TABLE, +) diff --git a/examples/skills_code_review_agent/src/storage/repository.py b/examples/skills_code_review_agent/src/storage/repository.py new file mode 100644 index 00000000..6cf46acc --- /dev/null +++ b/examples/skills_code_review_agent/src/storage/repository.py @@ -0,0 +1,256 @@ +"""Repository helpers built on top of SQLite for the review example.""" + +from __future__ import annotations + +import hashlib +import json +import sqlite3 +from pathlib import Path + +from ..review_types import ReviewReport, ReviewTask +from .init_db import initialize_database + + +class ReviewRepository: + """Small SQLite repository for review tasks and related records.""" + + def __init__(self, db_path: str | Path) -> None: + self.db_path = initialize_database(db_path) + + def save_review( + self, + *, + task: ReviewTask, + report: ReviewReport, + report_json: dict[str, object], + report_markdown: str, + runtime_type: str, + dry_run: bool, + fake_model: bool, + created_at: str, + finished_at: str, + total_duration_ms: int, + ) -> None: + """Persist a review task and all related structured records.""" + + connection = self._connect() + try: + connection.execute( + """ + INSERT OR REPLACE INTO review_tasks ( + task_id, status, input_type, runtime_type, dry_run, fake_model, + source, created_at, finished_at, total_duration_ms, error_message + ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + """, + ( + task.task_id, + task.status.value, + task.review_input.kind.value, + runtime_type, + int(dry_run), + int(fake_model), + task.review_input.source, + created_at, + finished_at, + total_duration_ms, + task.error_message, + ), + ) + connection.execute( + """ + INSERT OR REPLACE INTO review_inputs ( + task_id, diff_sha256, changed_files_count, hunk_count, + candidate_line_count, input_summary + ) VALUES (?, ?, ?, ?, ?, ?) + """, + ( + task.task_id, + hashlib.sha256(task.review_input.diff_text.encode("utf-8")).hexdigest(), + task.parsed_diff.changed_files_count if task.parsed_diff else 0, + _count_hunks(task), + _count_candidate_lines(task), + _build_input_summary(task), + ), + ) + + connection.execute("DELETE FROM filter_decisions WHERE task_id = ?", (task.task_id,)) + connection.executemany( + """ + INSERT INTO filter_decisions ( + task_id, decision, reason_code, reason_text, target, requires_human_review + ) VALUES (?, ?, ?, ?, ?, ?) + """, + [ + ( + task.task_id, + decision.decision.value, + decision.reason_code, + decision.reason, + decision.target, + int(decision.requires_human_review), + ) + for decision in task.filter_decisions + ], + ) + + connection.execute("DELETE FROM sandbox_runs WHERE task_id = ?", (task.task_id,)) + connection.executemany( + """ + INSERT INTO sandbox_runs ( + task_id, run_name, command, status, runtime, duration_ms, exit_code, + stdout_summary, stderr_summary, timed_out, output_truncated, blocked_by_filter + ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + """, + [ + ( + task.task_id, + sandbox_run.name, + json.dumps(sandbox_run.command), + sandbox_run.status.value, + sandbox_run.runtime, + sandbox_run.duration_ms, + sandbox_run.exit_code, + sandbox_run.stdout, + sandbox_run.stderr, + int(sandbox_run.timed_out), + int(sandbox_run.output_truncated), + int(sandbox_run.blocked_by_filter), + ) + for sandbox_run in task.sandbox_runs + ], + ) + + connection.execute("DELETE FROM findings WHERE task_id = ?", (task.task_id,)) + connection.executemany( + """ + INSERT INTO findings ( + task_id, fingerprint, severity, category, file, line, title, + evidence, recommendation, confidence, source, disposition + ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?) + """, + [ + ( + task.task_id, + finding.fingerprint, + finding.severity.value, + finding.category.value, + finding.file, + finding.line, + finding.title, + finding.evidence, + finding.recommendation, + finding.confidence, + finding.source.value, + finding.disposition.value, + ) + for finding in task.findings + ], + ) + + connection.execute( + """ + INSERT OR REPLACE INTO review_reports ( + task_id, final_verdict, report_json, report_markdown, monitoring_summary + ) VALUES (?, ?, ?, ?, ?) + """, + ( + task.task_id, + report.conclusion.value, + json.dumps(report_json, ensure_ascii=False), + report_markdown, + json.dumps(report.monitoring_summary, ensure_ascii=False), + ), + ) + connection.commit() + finally: + connection.close() + + def get_review_bundle(self, task_id: str) -> dict[str, object]: + """Fetch the full persisted record set for a task id.""" + + connection = self._connect() + try: + task_row = connection.execute( + "SELECT * FROM review_tasks WHERE task_id = ?", + (task_id,), + ).fetchone() + if task_row is None: + raise KeyError(f"task not found: {task_id}") + + return { + "task": dict(task_row), + "input": _fetch_one(connection, "review_inputs", task_id), + "filter_decisions": _fetch_many(connection, "filter_decisions", task_id), + "sandbox_runs": _fetch_many(connection, "sandbox_runs", task_id), + "findings": _fetch_many(connection, "findings", task_id), + "report": _fetch_one(connection, "review_reports", task_id), + } + finally: + connection.close() + + def _connect(self) -> sqlite3.Connection: + """Open a SQLite connection with row access by column name.""" + + connection = sqlite3.connect(self.db_path) + connection.row_factory = sqlite3.Row + return connection + + +def _fetch_one( + connection: sqlite3.Connection, + table_name: str, + task_id: str, +) -> dict[str, object] | None: + """Fetch a single row by task id.""" + + row = connection.execute( + f"SELECT * FROM {table_name} WHERE task_id = ?", + (task_id,), + ).fetchone() + return dict(row) if row is not None else None + + +def _fetch_many( + connection: sqlite3.Connection, + table_name: str, + task_id: str, +) -> list[dict[str, object]]: + """Fetch all rows by task id.""" + + rows = connection.execute( + f"SELECT * FROM {table_name} WHERE task_id = ?", + (task_id,), + ).fetchall() + return [dict(row) for row in rows] + + +def _count_hunks(task: ReviewTask) -> int: + """Count all parsed hunks for a task.""" + + if task.parsed_diff is None: + return 0 + return sum(len(changed_file.hunks) for changed_file in task.parsed_diff.files) + + +def _count_candidate_lines(task: ReviewTask) -> int: + """Count all candidate review lines across files.""" + + if task.parsed_diff is None: + return 0 + return sum( + len(changed_file.candidate_line_numbers()) + for changed_file in task.parsed_diff.files + ) + + +def _build_input_summary(task: ReviewTask) -> str: + """Build a short input summary suitable for storage and debugging.""" + + if task.parsed_diff is None: + return "No parsed diff available." + changed_paths = ", ".join(task.parsed_diff.changed_paths[:5]) + return ( + f"input_kind={task.review_input.kind.value}; " + f"changed_files={task.parsed_diff.changed_files_count}; " + f"paths={changed_paths}" + ) diff --git a/examples/skills_code_review_agent/src/telemetry.py b/examples/skills_code_review_agent/src/telemetry.py new file mode 100644 index 00000000..55ec8b15 --- /dev/null +++ b/examples/skills_code_review_agent/src/telemetry.py @@ -0,0 +1,54 @@ +"""Telemetry summary helpers for review metrics.""" + +from __future__ import annotations + +from collections import Counter + +from .review_types import FindingDisposition, ParsedDiff, ReviewFinding, ReviewTask + + +def build_monitoring_summary( + *, + task: ReviewTask, + parsed_diff: ParsedDiff, + total_duration_ms: int, +) -> dict[str, int | float | str]: + """Build the monitoring summary required by the review report.""" + + findings = task.findings + severity_distribution = Counter( + finding.severity.value + for finding in findings + if finding.disposition == FindingDisposition.FINDING + ) + category_distribution = Counter(finding.category.value for finding in findings) + exception_distribution = Counter() + if task.error_message: + exception_distribution["pipeline_error"] += 1 + + return { + "total_duration_ms": total_duration_ms, + "changed_files_count": parsed_diff.changed_files_count, + "added_lines_count": parsed_diff.added_lines_count, + "deleted_lines_count": parsed_diff.deleted_lines_count, + "sandbox_run_count": len(task.sandbox_runs), + "filter_decision_count": len(task.filter_decisions), + "finding_count": _count_by_disposition(findings, FindingDisposition.FINDING), + "needs_human_review_count": _count_by_disposition( + findings, + FindingDisposition.NEEDS_HUMAN_REVIEW, + ), + "warning_count": _count_by_disposition(findings, FindingDisposition.WARNING), + "severity_distribution": dict(severity_distribution), + "category_distribution": dict(category_distribution), + "exception_distribution": dict(exception_distribution), + } + + +def _count_by_disposition( + findings: list[ReviewFinding], + disposition: FindingDisposition, +) -> int: + """Count findings by output bucket.""" + + return sum(1 for finding in findings if finding.disposition == disposition) diff --git a/examples/skills_code_review_agent/tests/__init__.py b/examples/skills_code_review_agent/tests/__init__.py new file mode 100644 index 00000000..93d0a1e9 --- /dev/null +++ b/examples/skills_code_review_agent/tests/__init__.py @@ -0,0 +1 @@ +"""Tests for the skills code review example.""" diff --git a/examples/skills_code_review_agent/tests/fixtures/README.md b/examples/skills_code_review_agent/tests/fixtures/README.md new file mode 100644 index 00000000..1b8d3356 --- /dev/null +++ b/examples/skills_code_review_agent/tests/fixtures/README.md @@ -0,0 +1,18 @@ +# Fixture Index + +The final implementation should add at least these runnable fixtures: + +- `clean.diff` +- `security_issue.diff` +- `async_resource_leak.diff` +- `db_lifecycle_issue.diff` +- `missing_tests.diff` +- `duplicate_finding.diff` +- `sandbox_failure.diff` +- `secret_redaction.diff` + +Additional phase-two quality fixtures: + +- `safe_security_patterns.diff` +- `managed_lifecycle.diff` +- `placeholder_secrets.diff` diff --git a/examples/skills_code_review_agent/tests/fixtures/async_resource_leak.diff b/examples/skills_code_review_agent/tests/fixtures/async_resource_leak.diff new file mode 100644 index 00000000..e82c698e --- /dev/null +++ b/examples/skills_code_review_agent/tests/fixtures/async_resource_leak.diff @@ -0,0 +1,21 @@ +diff --git a/src/worker.py b/src/worker.py +index 1111111..2222222 100644 +--- a/src/worker.py ++++ b/src/worker.py +@@ -10,3 +10,10 @@ async def process_jobs(): + for job in load_jobs(): + await handle_job(job) + ++async def process_in_background(path: str): ++ asyncio.create_task(handle_job(path)) ++ handle = open(path, "r", encoding="utf-8") ++ return handle.read() +diff --git a/tests/test_worker.py b/tests/test_worker.py +index 3333333..4444444 100644 +--- a/tests/test_worker.py ++++ b/tests/test_worker.py +@@ -1,2 +1,4 @@ + def test_process_jobs_smoke(): + assert True ++ ++# Fixture keeps tests updated so missing_tests does not fire. diff --git a/examples/skills_code_review_agent/tests/fixtures/clean.diff b/examples/skills_code_review_agent/tests/fixtures/clean.diff new file mode 100644 index 00000000..034ea48c --- /dev/null +++ b/examples/skills_code_review_agent/tests/fixtures/clean.diff @@ -0,0 +1,12 @@ +diff --git a/tests/test_math_utils.py b/tests/test_math_utils.py +index 1111111..2222222 100644 +--- a/tests/test_math_utils.py ++++ b/tests/test_math_utils.py +@@ -1,3 +1,7 @@ + def test_add(): + assert add(1, 2) == 3 + ++def test_add_with_zero(): ++ assert add(0, 5) == 5 ++ ++# Safe test-only change with no production risk patterns. diff --git a/examples/skills_code_review_agent/tests/fixtures/db_lifecycle_issue.diff b/examples/skills_code_review_agent/tests/fixtures/db_lifecycle_issue.diff new file mode 100644 index 00000000..43806d6b --- /dev/null +++ b/examples/skills_code_review_agent/tests/fixtures/db_lifecycle_issue.diff @@ -0,0 +1,21 @@ +diff --git a/src/db_client.py b/src/db_client.py +index 1111111..2222222 100644 +--- a/src/db_client.py ++++ b/src/db_client.py +@@ -3,3 +3,8 @@ def fetch_user(user_id: int): + return None + ++def fetch_user(user_id: int): ++ conn = sqlite3.connect("app.db") ++ cursor = conn.cursor() ++ cursor.execute("SELECT * FROM users WHERE id = ?", (user_id,)) ++ return cursor.fetchone() +diff --git a/tests/test_db_client.py b/tests/test_db_client.py +index 3333333..4444444 100644 +--- a/tests/test_db_client.py ++++ b/tests/test_db_client.py +@@ -1,2 +1,4 @@ + def test_fetch_user_smoke(): + assert True ++ ++# Tests changed as well, so this fixture focuses on DB lifecycle only. diff --git a/examples/skills_code_review_agent/tests/fixtures/duplicate_finding.diff b/examples/skills_code_review_agent/tests/fixtures/duplicate_finding.diff new file mode 100644 index 00000000..34e6c732 --- /dev/null +++ b/examples/skills_code_review_agent/tests/fixtures/duplicate_finding.diff @@ -0,0 +1,10 @@ +diff --git a/src/settings.py b/src/settings.py +index 1111111..2222222 100644 +--- a/src/settings.py ++++ b/src/settings.py +@@ -1,2 +1,5 @@ + DEBUG = False ++API_TOKEN = "super-secret-token" ++PASSWORD = "super-secret-token" ++ ++# Both lines should be flagged, but duplicate matches on the same line should collapse. diff --git a/examples/skills_code_review_agent/tests/fixtures/managed_lifecycle.diff b/examples/skills_code_review_agent/tests/fixtures/managed_lifecycle.diff new file mode 100644 index 00000000..39965b14 --- /dev/null +++ b/examples/skills_code_review_agent/tests/fixtures/managed_lifecycle.diff @@ -0,0 +1,20 @@ +diff --git a/src/storage.py b/src/storage.py +index 1111111..2222222 100644 +--- a/src/storage.py ++++ b/src/storage.py +@@ -1,3 +1,13 @@ + def load_records(path: str) -> list[str]: + return [] + ++def load_records(path: str) -> list[str]: ++ with open(path, "r", encoding="utf-8") as handle: ++ data = handle.read().splitlines() ++ conn = sqlite3.connect("app.db") ++ try: ++ conn.execute("BEGIN") ++ conn.commit() ++ except Exception: ++ conn.rollback() ++ raise ++ conn.close() ++ return data diff --git a/examples/skills_code_review_agent/tests/fixtures/missing_tests.diff b/examples/skills_code_review_agent/tests/fixtures/missing_tests.diff new file mode 100644 index 00000000..4cee95f4 --- /dev/null +++ b/examples/skills_code_review_agent/tests/fixtures/missing_tests.diff @@ -0,0 +1,11 @@ +diff --git a/src/payment_service.py b/src/payment_service.py +index 1111111..2222222 100644 +--- a/src/payment_service.py ++++ b/src/payment_service.py +@@ -20,3 +20,8 @@ def create_order(amount: int) -> dict: + return {"amount": amount} + ++def create_order(amount: int) -> dict: ++ if amount <= 0: ++ raise ValueError("amount must be positive") ++ return {"amount": amount, "status": "pending"} diff --git a/examples/skills_code_review_agent/tests/fixtures/placeholder_secrets.diff b/examples/skills_code_review_agent/tests/fixtures/placeholder_secrets.diff new file mode 100644 index 00000000..abfc5896 --- /dev/null +++ b/examples/skills_code_review_agent/tests/fixtures/placeholder_secrets.diff @@ -0,0 +1,19 @@ +diff --git a/src/example_config.py b/src/example_config.py +index 1111111..2222222 100644 +--- a/src/example_config.py ++++ b/src/example_config.py +@@ -1,2 +1,5 @@ + DEBUG = False + TIMEOUT = 30 ++API_KEY = "example-key" ++PASSWORD = "changeme" ++AUTH_HEADER = "Bearer redacted-token" +diff --git a/tests/test_example_config.py b/tests/test_example_config.py +index 3333333..4444444 100644 +--- a/tests/test_example_config.py ++++ b/tests/test_example_config.py +@@ -1,2 +1,4 @@ + def test_example_config_documents_placeholders(): + assert True ++ ++# Placeholder values should not be treated as real secret leaks. diff --git a/examples/skills_code_review_agent/tests/fixtures/safe_security_patterns.diff b/examples/skills_code_review_agent/tests/fixtures/safe_security_patterns.diff new file mode 100644 index 00000000..64027981 --- /dev/null +++ b/examples/skills_code_review_agent/tests/fixtures/safe_security_patterns.diff @@ -0,0 +1,22 @@ +diff --git a/src/parser.py b/src/parser.py +index 1111111..2222222 100644 +--- a/src/parser.py ++++ b/src/parser.py +@@ -1,3 +1,10 @@ + def parse_payload(payload: str) -> dict: + return {} + ++def parse_payload(payload: str) -> dict: ++ # Never use eval(payload) here. ++ data = yaml.safe_load(payload) ++ result = subprocess.run(["python", "--version"], check=True, text=True) ++ return {"data": data, "result": result.stdout} +diff --git a/tests/test_parser.py b/tests/test_parser.py +index 3333333..4444444 100644 +--- a/tests/test_parser.py ++++ b/tests/test_parser.py +@@ -1,2 +1,4 @@ + def test_parse_payload_smoke(): + assert True ++ ++# Tests changed too, so missing_tests should stay silent. diff --git a/examples/skills_code_review_agent/tests/fixtures/sandbox_failure.diff b/examples/skills_code_review_agent/tests/fixtures/sandbox_failure.diff new file mode 100644 index 00000000..f75f189b --- /dev/null +++ b/examples/skills_code_review_agent/tests/fixtures/sandbox_failure.diff @@ -0,0 +1,11 @@ +diff --git a/src/failing_lint_case.py b/src/failing_lint_case.py +index 1111111..2222222 100644 +--- a/src/failing_lint_case.py ++++ b/src/failing_lint_case.py +@@ -1,2 +1,6 @@ + def run(): + return "ok" ++ ++# TODO_FAIL_SANDBOX ++def compute(expr: str): ++ return eval(expr) diff --git a/examples/skills_code_review_agent/tests/fixtures/secret_redaction.diff b/examples/skills_code_review_agent/tests/fixtures/secret_redaction.diff new file mode 100644 index 00000000..414baec1 --- /dev/null +++ b/examples/skills_code_review_agent/tests/fixtures/secret_redaction.diff @@ -0,0 +1,11 @@ +diff --git a/src/config.py b/src/config.py +index 1111111..2222222 100644 +--- a/src/config.py ++++ b/src/config.py +@@ -1,3 +1,6 @@ + DEBUG = False + TIMEOUT = 30 ++API_KEY = "sk-test-1234567890abcdef" ++AUTH_HEADER = "Bearer super-secret-token-value" ++ ++# Tests are intentionally omitted here; missing_tests may also appear. diff --git a/examples/skills_code_review_agent/tests/fixtures/security_issue.diff b/examples/skills_code_review_agent/tests/fixtures/security_issue.diff new file mode 100644 index 00000000..bfbed353 --- /dev/null +++ b/examples/skills_code_review_agent/tests/fixtures/security_issue.diff @@ -0,0 +1,22 @@ +diff --git a/src/calculator.py b/src/calculator.py +index 1111111..2222222 100644 +--- a/src/calculator.py ++++ b/src/calculator.py +@@ -8,4 +8,9 @@ def calculate(expression: str) -> int: + expression = expression.strip() + if not expression: + return 0 +- return parse_expression(expression) ++ return eval(expression) ++ ++def run_shell(command: str) -> str: ++ return subprocess.run(command, shell=True, check=True, text=True) +diff --git a/tests/test_calculator.py b/tests/test_calculator.py +index 3333333..4444444 100644 +--- a/tests/test_calculator.py ++++ b/tests/test_calculator.py +@@ -1,3 +1,5 @@ + def test_calculate_addition(): + assert calculate("1 + 2") == 3 ++ ++# Tests changed as well, so this fixture should not trigger missing_tests. diff --git a/examples/skills_code_review_agent/tests/test_code_review_agent.py b/examples/skills_code_review_agent/tests/test_code_review_agent.py new file mode 100644 index 00000000..98dd0663 --- /dev/null +++ b/examples/skills_code_review_agent/tests/test_code_review_agent.py @@ -0,0 +1,363 @@ +"""Foundation tests for the skills code review example.""" + +from __future__ import annotations + +from pathlib import Path + +import pytest + +from examples.skills_code_review_agent.agent.agent import run_review_task +from examples.skills_code_review_agent.agent.config import ReviewAgentConfig +from examples.skills_code_review_agent.run_agent import parse_args +from examples.skills_code_review_agent.src.deduper import dedupe_and_classify_findings +from examples.skills_code_review_agent.src.diff_parser import parse_unified_diff +from examples.skills_code_review_agent.src.input_loader import load_review_input +from examples.skills_code_review_agent.src.rule_engine import run_rule_engine +from examples.skills_code_review_agent.src.review_types import ( + DiffLineType, + FindingDisposition, + FindingSource, + ReviewCategory, + ReviewConclusion, + ReviewFinding, + ReviewInputKind, + ReviewSeverity, + ReviewStatus, +) + +FIXTURES_DIR = Path(__file__).parent / "fixtures" + + +def test_parse_unified_diff_extracts_files_hunks_and_candidates() -> None: + """The parser should extract stable structures from unified diff text.""" + + diff_text = """diff --git a/app.py b/app.py +index 1111111..2222222 100644 +--- a/app.py ++++ b/app.py +@@ -8,4 +8,5 @@ def run(): + client = make_client() +- return client.fetch() ++ result = client.fetch() ++ return result + + def done(): +""" + + parsed = parse_unified_diff(diff_text) + + assert parsed.changed_files_count == 1 + assert parsed.added_lines_count == 2 + assert parsed.deleted_lines_count == 1 + assert parsed.changed_paths == ["app.py"] + + changed_file = parsed.files[0] + assert changed_file.display_path == "app.py" + assert changed_file.added_line_numbers == [9, 10] + assert changed_file.deleted_line_numbers == [9] + assert changed_file.candidate_line_numbers(radius=1) == [8, 9, 10, 11] + + hunk = changed_file.hunks[0] + assert hunk.added_line_numbers == [9, 10] + assert hunk.deleted_line_numbers == [9] + assert hunk.lines[1].line_type == DiffLineType.DELETE + assert hunk.lines[2].line_type == DiffLineType.ADD + + +def test_parse_unified_diff_supports_patch_without_diff_git_header() -> None: + """Some patch sources omit the `diff --git` header and should still parse.""" + + diff_text = """--- a/config.py ++++ b/config.py +@@ -1,2 +1,3 @@ + DEBUG = False ++API_TOKEN = "masked" + TIMEOUT = 10 +""" + + parsed = parse_unified_diff(diff_text) + + assert parsed.changed_files_count == 1 + assert parsed.files[0].display_path == "config.py" + assert parsed.files[0].added_line_numbers == [2] + + +def test_load_review_input_reads_diff_file(tmp_path: Path) -> None: + """Loading a diff file should produce a normalized ReviewInput.""" + + diff_path = tmp_path / "sample.diff" + diff_path.write_text("diff --git a/a.py b/a.py\n", encoding="utf-8") + + review_input = load_review_input(diff_file=diff_path) + + assert review_input.kind == ReviewInputKind.DIFF_FILE + assert review_input.source == str(diff_path.resolve()) + assert review_input.diff_text == "diff --git a/a.py b/a.py\n" + assert review_input.repo_path is None + + +def test_load_review_input_requires_exactly_one_source(tmp_path: Path) -> None: + """The loader should reject ambiguous or empty inputs.""" + + diff_path = tmp_path / "sample.diff" + diff_path.write_text("content", encoding="utf-8") + + with pytest.raises(ValueError): + load_review_input() + + with pytest.raises(ValueError): + load_review_input(diff_file=diff_path, fixture_path=diff_path) + + +def test_parse_args_reads_diff_file_mode() -> None: + """The CLI parser should map diff mode arguments into a namespace.""" + + args = parse_args( + [ + "--diff-file", + "tests/example.diff", + "--output-dir", + "outputs", + "--db-path", + "review.db", + "--runtime", + "local", + "--dry-run", + "--fake-model", + ] + ) + + assert args.diff_file == "tests/example.diff" + assert args.repo_path is None + assert args.fixture is None + assert args.output_dir == "outputs" + assert args.db_path == "review.db" + assert args.runtime == "local" + assert args.dry_run is True + assert args.fake_model is True + + +def test_run_review_task_surfaces_missing_tests_for_code_only_change(tmp_path: Path) -> None: + """Main orchestration should surface a human-review item for missing tests.""" + + diff_path = tmp_path / "sample.diff" + diff_path.write_text( + """diff --git a/main.py b/main.py +--- a/main.py ++++ b/main.py +@@ -1 +1,2 @@ + print("hello") ++print("world") +""", + encoding="utf-8", + ) + + config = ReviewAgentConfig( + diff_file=str(diff_path), + output_dir=tmp_path / "outputs", + db_path=tmp_path / "review.db", + dry_run=True, + fake_model=True, + ) + + task, report = run_review_task(config) + + assert task.status == ReviewStatus.COMPLETED + assert task.parsed_diff is not None + assert task.parsed_diff.changed_paths == ["main.py"] + assert report.task_id == task.task_id + assert report.conclusion == ReviewConclusion.NEEDS_HUMAN_REVIEW + assert report.monitoring_summary["changed_files_count"] == 1 + assert report.monitoring_summary["needs_human_review_count"] == 1 + + +def test_rule_engine_returns_no_findings_for_clean_fixture() -> None: + """Clean fixture should not trigger deterministic findings.""" + + parsed = parse_unified_diff((FIXTURES_DIR / "clean.diff").read_text(encoding="utf-8")) + findings = dedupe_and_classify_findings(run_rule_engine(parsed)) + + assert findings == [] + + +def test_rule_engine_detects_security_patterns() -> None: + """Security fixture should trigger high-confidence security findings.""" + + parsed = parse_unified_diff((FIXTURES_DIR / "security_issue.diff").read_text(encoding="utf-8")) + findings = dedupe_and_classify_findings(run_rule_engine(parsed)) + + security_findings = [item for item in findings if item.category == ReviewCategory.SECURITY] + assert len(security_findings) >= 2 + assert all(item.disposition == FindingDisposition.FINDING for item in security_findings) + + +def test_rule_engine_detects_async_and_resource_leak_patterns() -> None: + """Async/resource fixture should emit lower-confidence review items.""" + + parsed = parse_unified_diff( + (FIXTURES_DIR / "async_resource_leak.diff").read_text(encoding="utf-8") + ) + findings = dedupe_and_classify_findings(run_rule_engine(parsed)) + + categories = {item.category for item in findings} + assert ReviewCategory.ASYNC in categories + assert ReviewCategory.RESOURCE_LEAK in categories + assert all( + item.disposition == FindingDisposition.NEEDS_HUMAN_REVIEW + for item in findings + if item.category in {ReviewCategory.ASYNC, ReviewCategory.RESOURCE_LEAK} + ) + + +def test_rule_engine_detects_db_lifecycle_pattern() -> None: + """DB lifecycle fixture should raise at least one DB lifecycle finding.""" + + parsed = parse_unified_diff( + (FIXTURES_DIR / "db_lifecycle_issue.diff").read_text(encoding="utf-8") + ) + findings = dedupe_and_classify_findings(run_rule_engine(parsed)) + + db_findings = [item for item in findings if item.category == ReviewCategory.DB_LIFECYCLE] + assert db_findings + assert any(item.disposition == FindingDisposition.FINDING for item in db_findings) + + +def test_rule_engine_detects_secret_patterns() -> None: + """Secret fixture should emit at least one high-confidence secret finding.""" + + parsed = parse_unified_diff( + (FIXTURES_DIR / "secret_redaction.diff").read_text(encoding="utf-8") + ) + findings = dedupe_and_classify_findings(run_rule_engine(parsed)) + + secret_findings = [item for item in findings if item.category == ReviewCategory.SECRET] + assert secret_findings + assert any(item.disposition == FindingDisposition.FINDING for item in secret_findings) + + +def test_rule_engine_detects_missing_tests_as_human_review_item() -> None: + """Changing production code without tests should request human review.""" + + parsed = parse_unified_diff((FIXTURES_DIR / "missing_tests.diff").read_text(encoding="utf-8")) + findings = dedupe_and_classify_findings(run_rule_engine(parsed)) + + assert len(findings) == 1 + assert findings[0].category == ReviewCategory.TEST_MISSING + assert findings[0].disposition == FindingDisposition.NEEDS_HUMAN_REVIEW + + +def test_rule_engine_ignores_safe_security_patterns_and_comments() -> None: + """Safe loader usage and commented examples should not raise security findings.""" + + parsed = parse_unified_diff( + (FIXTURES_DIR / "safe_security_patterns.diff").read_text(encoding="utf-8") + ) + findings = dedupe_and_classify_findings(run_rule_engine(parsed)) + + assert not any(item.category == ReviewCategory.SECURITY for item in findings) + + +def test_rule_engine_ignores_placeholder_secret_values() -> None: + """Example credentials should not be treated as real leaked secrets.""" + + parsed = parse_unified_diff( + (FIXTURES_DIR / "placeholder_secrets.diff").read_text(encoding="utf-8") + ) + findings = dedupe_and_classify_findings(run_rule_engine(parsed)) + + assert not any(item.category == ReviewCategory.SECRET for item in findings) + + +def test_rule_engine_ignores_managed_resource_and_db_lifecycle() -> None: + """Visible close/commit/rollback handling should suppress lifecycle findings.""" + + parsed = parse_unified_diff( + (FIXTURES_DIR / "managed_lifecycle.diff").read_text(encoding="utf-8") + ) + findings = dedupe_and_classify_findings(run_rule_engine(parsed)) + + categories = {item.category for item in findings} + assert ReviewCategory.RESOURCE_LEAK not in categories + assert ReviewCategory.DB_LIFECYCLE not in categories + + +def test_deduper_collapses_duplicate_matches_on_same_line() -> None: + """Duplicate rule hits for the same issue should collapse into one record.""" + + duplicate_one = ReviewFinding( + severity=ReviewSeverity.HIGH, + category=ReviewCategory.SECRET, + file="src/settings.py", + line=2, + title="Hard-coded secret detected", + evidence='+API_TOKEN = "super-secret-token"', + recommendation="Move the token to a secret manager.", + confidence=0.95, + source=FindingSource.RULE_ENGINE, + ) + duplicate_two = ReviewFinding( + severity=ReviewSeverity.HIGH, + category=ReviewCategory.SECRET, + file="src/settings.py", + line=2, + title="Hard-coded secret detected", + evidence='+API_TOKEN = "super-secret-token"', + recommendation="Move the token to a secret manager.", + confidence=0.90, + source=duplicate_one.source, + ) + + findings = dedupe_and_classify_findings([duplicate_one, duplicate_two]) + + assert len(findings) == 1 + assert findings[0].fingerprint is not None + + +def test_deduper_collapses_same_title_even_if_evidence_differs() -> None: + """Same issue title on the same line should dedupe even with different evidence slices.""" + + duplicate_one = ReviewFinding( + severity=ReviewSeverity.MEDIUM, + category=ReviewCategory.DB_LIFECYCLE, + file="src/db.py", + line=12, + title="Database connection is created without clear lifecycle management", + evidence='+conn = sqlite3.connect("app.db")', + recommendation="Use a context manager.", + confidence=0.86, + source=FindingSource.RULE_ENGINE, + ) + duplicate_two = ReviewFinding( + severity=ReviewSeverity.HIGH, + category=ReviewCategory.DB_LIFECYCLE, + file="src/db.py", + line=12, + title="Database connection is created without clear lifecycle management", + evidence='+conn = sqlite3.connect("app.db")\n+return conn.execute("select 1")', + recommendation="Use a context manager.", + confidence=0.74, + source=FindingSource.RULE_ENGINE, + ) + + findings = dedupe_and_classify_findings([duplicate_one, duplicate_two]) + + assert len(findings) == 1 + assert findings[0].severity == ReviewSeverity.MEDIUM + + +def test_run_review_task_with_security_fixture_returns_failure() -> None: + """Main orchestration should fail the review when high-confidence findings exist.""" + + config = ReviewAgentConfig( + fixture_path=str(FIXTURES_DIR / "security_issue.diff"), + output_dir=FIXTURES_DIR.parent / "outputs", + db_path=FIXTURES_DIR.parent / "review.db", + dry_run=True, + fake_model=True, + ) + + task, report = run_review_task(config) + + assert task.status == ReviewStatus.COMPLETED + assert report.conclusion == ReviewConclusion.FAIL + assert any(item.category == ReviewCategory.SECURITY for item in report.findings) diff --git a/examples/skills_code_review_agent/tests/test_phase6_quality_gate.py b/examples/skills_code_review_agent/tests/test_phase6_quality_gate.py new file mode 100644 index 00000000..bade8505 --- /dev/null +++ b/examples/skills_code_review_agent/tests/test_phase6_quality_gate.py @@ -0,0 +1,50 @@ +"""Phase 6 quality-gate tests for PR readiness.""" + +from __future__ import annotations + +import json +from pathlib import Path + +from examples.skills_code_review_agent.agent.agent import run_review_task +from examples.skills_code_review_agent.agent.config import ReviewAgentConfig + +FIXTURES_DIR = Path(__file__).parent / "fixtures" +PUBLIC_FIXTURES = [ + "clean.diff", + "security_issue.diff", + "async_resource_leak.diff", + "db_lifecycle_issue.diff", + "missing_tests.diff", + "duplicate_finding.diff", + "sandbox_failure.diff", + "secret_redaction.diff", +] + + +def test_all_public_fixtures_generate_reports(tmp_path: Path) -> None: + """All public fixtures should complete and generate report artifacts.""" + + fixture_paths = [FIXTURES_DIR / name for name in PUBLIC_FIXTURES] + assert all(path.exists() for path in fixture_paths) + + for fixture_path in fixture_paths: + out_dir = tmp_path / fixture_path.stem + db_path = out_dir / "review.db" + config = ReviewAgentConfig( + fixture_path=str(fixture_path), + output_dir=out_dir, + db_path=db_path, + dry_run=True, + fake_model=True, + ) + + task, report = run_review_task(config) + + assert task.status.value == "completed" + assert report.task_id == task.task_id + assert (out_dir / "review_report.json").exists() + assert (out_dir / "review_report.md").exists() + + payload = json.loads((out_dir / "review_report.json").read_text(encoding="utf-8")) + assert payload["task_id"] == task.task_id + assert "monitoring_summary" in payload diff --git a/examples/skills_code_review_agent/tests/test_skill_package.py b/examples/skills_code_review_agent/tests/test_skill_package.py new file mode 100644 index 00000000..6d926a47 --- /dev/null +++ b/examples/skills_code_review_agent/tests/test_skill_package.py @@ -0,0 +1,79 @@ +"""Tests for the formal code-review skill package.""" + +from __future__ import annotations + +from pathlib import Path + +from examples.skills_code_review_agent.agent.tools import build_skill_run_payload +from examples.skills_code_review_agent.agent.tools import build_skill_script_plan +from examples.skills_code_review_agent.agent.tools import create_skill_tool_set +from examples.skills_code_review_agent.agent.tools import resolve_code_review_skill_dir + +SKILL_DIR = ( + Path(__file__).resolve().parents[3] + / "skills" + / "code-review" +) + + +def test_skill_markdown_contains_expected_workflow_sections() -> None: + """The formal skill should document usage, outputs, and safety guidance.""" + + skill_md = (SKILL_DIR / "SKILL.md").read_text(encoding="utf-8") + + assert "skill_load(skill=\"code-review\")" in skill_md + assert "Available Docs" in skill_md + assert "Safety Guidance" in skill_md + assert "review_report.json" in skill_md + + +def test_build_skill_run_payload_points_to_skill_workspace_outputs(tmp_path: Path) -> None: + """Payload builder should produce a stable `skill_run` contract.""" + + diff_file = tmp_path / "sample.diff" + diff_file.write_text("diff --git a/a.py b/a.py\n", encoding="utf-8") + + payload = build_skill_run_payload( + diff_file=diff_file, + script_name="run_linters.py", + ) + + assert payload["skill"] == "code-review" + assert payload["cwd"] == "$SKILLS_DIR/code-review" + assert "python scripts/run_linters.py --diff-file" in payload["command"] + assert payload["output_files"] == ["out/run_linters.json"] + + +def test_resolve_code_review_skill_dir_prefers_repository_root() -> None: + """The example should prefer the repository-level skill directory.""" + + resolved = resolve_code_review_skill_dir() + + assert resolved == SKILL_DIR.resolve() + + +def test_build_skill_script_plan_uses_repository_root_skill_scripts(tmp_path: Path) -> None: + """Sandbox plans should point at the canonical root skill scripts.""" + + diff_file = tmp_path / "sample.diff" + diff_file.write_text("diff --git a/a.py b/a.py\n", encoding="utf-8") + + plan = build_skill_script_plan( + diff_file=diff_file, + project_root=Path(__file__).resolve().parents[3], + ) + + assert len(plan) == 3 + assert all(invocation.script_path.is_file() for invocation in plan) + assert all(SKILL_DIR.resolve() in invocation.script_path.resolve().parents for invocation in plan) + + +def test_skill_repository_indexes_root_code_review_skill() -> None: + """SkillToolSet should expose the canonical root code-review skill.""" + + _toolset, repository = create_skill_tool_set( + workspace_runtime_type="local", + use_cached_repository=False, + ) + + assert Path(repository.path("code-review")).resolve() == SKILL_DIR.resolve() diff --git a/examples/skills_code_review_agent/tests/test_storage_and_report.py b/examples/skills_code_review_agent/tests/test_storage_and_report.py new file mode 100644 index 00000000..c18b7701 --- /dev/null +++ b/examples/skills_code_review_agent/tests/test_storage_and_report.py @@ -0,0 +1,150 @@ +"""Storage and report integration tests for the code review example.""" + +from __future__ import annotations + +import json +from pathlib import Path + +from examples.skills_code_review_agent.agent.agent import run_review_task +from examples.skills_code_review_agent.agent.config import ReviewAgentConfig +from examples.skills_code_review_agent.src.storage.repository import ReviewRepository + +FIXTURES_DIR = Path(__file__).parent / "fixtures" + + +def test_run_review_task_writes_report_files_and_database(tmp_path: Path) -> None: + """Running the pipeline should persist records and write both report artifacts.""" + + output_dir = tmp_path / "outputs" + db_path = tmp_path / "review.db" + config = ReviewAgentConfig( + fixture_path=str(FIXTURES_DIR / "security_issue.diff"), + output_dir=output_dir, + db_path=db_path, + dry_run=True, + fake_model=True, + ) + + task, report = run_review_task(config) + + json_path = output_dir / "review_report.json" + markdown_path = output_dir / "review_report.md" + assert json_path.exists() + assert markdown_path.exists() + + payload = json.loads(json_path.read_text(encoding="utf-8")) + assert payload["task_id"] == task.task_id + assert payload["conclusion"] == "fail" + assert payload["findings"] + + markdown = markdown_path.read_text(encoding="utf-8") + assert "# Review Report" in markdown + assert "## Findings" in markdown + assert "## Monitoring" in markdown + + repository = ReviewRepository(db_path) + bundle = repository.get_review_bundle(task.task_id) + assert bundle["task"]["task_id"] == task.task_id + assert bundle["input"]["changed_files_count"] == 2 + assert len(bundle["findings"]) >= 2 + assert bundle["report"]["final_verdict"] == report.conclusion.value + + +def test_run_review_task_persists_human_review_state(tmp_path: Path) -> None: + """Missing-test scenarios should persist a needs-human-review conclusion.""" + + config = ReviewAgentConfig( + fixture_path=str(FIXTURES_DIR / "missing_tests.diff"), + output_dir=tmp_path / "outputs", + db_path=tmp_path / "review.db", + dry_run=True, + fake_model=True, + ) + + task, report = run_review_task(config) + + repository = ReviewRepository(tmp_path / "review.db") + bundle = repository.get_review_bundle(task.task_id) + assert report.conclusion.value == "needs_human_review" + assert bundle["report"]["final_verdict"] == "needs_human_review" + assert bundle["task"]["status"] == "completed" + + +def test_secret_values_are_redacted_in_reports_and_database(tmp_path: Path) -> None: + """Secrets must be redacted before report generation and persistence.""" + + config = ReviewAgentConfig( + fixture_path=str(FIXTURES_DIR / "secret_redaction.diff"), + output_dir=tmp_path / "outputs", + db_path=tmp_path / "review.db", + dry_run=True, + fake_model=True, + ) + + task, _report = run_review_task(config) + + json_payload = json.loads( + (tmp_path / "outputs" / "review_report.json").read_text(encoding="utf-8") + ) + markdown = (tmp_path / "outputs" / "review_report.md").read_text(encoding="utf-8") + bundle = ReviewRepository(tmp_path / "review.db").get_review_bundle(task.task_id) + + forbidden_fragments = [ + "sk-test-1234567890abcdef", + "Bearer super-secret-token-value", + "super-secret-token-value", + ] + joined_db_text = json.dumps(bundle, ensure_ascii=False) + for fragment in forbidden_fragments: + assert fragment not in json.dumps(json_payload, ensure_ascii=False) + assert fragment not in markdown + assert fragment not in joined_db_text + + +def test_filter_denies_forbidden_paths_and_skips_sandbox(tmp_path: Path) -> None: + """Forbidden paths should be denied before any sandbox script executes.""" + + diff_path = tmp_path / "forbidden.diff" + diff_path.write_text( + """diff --git a/.env b/.env +--- a/.env ++++ b/.env +@@ -0,0 +1 @@ ++API_KEY="sk-test-unsafe" +""", + encoding="utf-8", + ) + + config = ReviewAgentConfig( + diff_file=str(diff_path), + output_dir=tmp_path / "outputs", + db_path=tmp_path / "review.db", + dry_run=True, + fake_model=True, + ) + + task, _report = run_review_task(config) + + assert task.filter_decisions + assert all(decision.decision.value == "deny" for decision in task.filter_decisions) + assert task.sandbox_runs + assert all(run.status.value == "blocked" for run in task.sandbox_runs) + + +def test_sandbox_failure_is_recorded_without_crashing_task(tmp_path: Path) -> None: + """Sandbox failures should be recorded as findings while the review still completes.""" + + config = ReviewAgentConfig( + fixture_path=str(FIXTURES_DIR / "sandbox_failure.diff"), + output_dir=tmp_path / "outputs", + db_path=tmp_path / "review.db", + dry_run=True, + fake_model=True, + ) + + task, report = run_review_task(config) + + assert task.status.value == "completed" + assert any(run.status.value == "failed" for run in task.sandbox_runs) + assert any(finding.category.value == "sandbox" for finding in task.findings) + assert report.monitoring_summary["sandbox_run_count"] >= 1 diff --git a/skills/code-review/RULES.md b/skills/code-review/RULES.md new file mode 100644 index 00000000..3736bc22 --- /dev/null +++ b/skills/code-review/RULES.md @@ -0,0 +1,157 @@ +# Review Rules + +## Detection Categories + +The initial implementation covers these categories: + +- Security risk +- Async error +- Resource leak +- Missing tests +- Sensitive information leak +- Database transaction or connection lifecycle issue + +## Rule Design Principles + +- Prefer deterministic, high-signal rules for the first implementation. +- Run rules on structured diff data instead of raw text blobs whenever possible. +- Treat low-confidence heuristics as review aids, not final verdicts. +- Keep evidence small and local so reports stay readable. +- Avoid duplicate findings for the same issue location. + +## Category Notes + +### Security Risk + +High-confidence patterns: + +- `eval(...)` +- `exec(...)` +- `pickle.loads(...)` +- `yaml.load(...)` without a safe loader +- `subprocess.*(..., shell=True)` +- TLS verification disabled with `verify=False` + +Severity guidance: + +- `eval/exec/pickle.loads/shell=True`: high +- `verify=False`: medium to high depending on context + +Do not report when: + +- the match only appears inside comments or doc-style examples +- YAML parsing already uses `safe_load` or `SafeLoader` +- subprocess invocation does not use `shell=True` +- `verify=False` appears in clearly test-only or mock-only code: lower confidence and route to human review + +### Async Error + +Heuristic patterns: + +- Detached `asyncio.create_task(...)` without visible lifecycle tracking +- `except Exception: pass` style swallowing near async flows + +Routing guidance: + +- Most first-pass async findings should land in `needs_human_review` unless the pattern is obviously dangerous. + +### Resource Leak + +Heuristic patterns: + +- `open(...)` without `with open(...)` +- HTTP/session/client constructors without `with` / `async with` + +Routing guidance: + +- Resource-lifecycle heuristics are useful but often context-sensitive, so medium-confidence routing is preferred in the first version. + +Do not report when: + +- the handle is created with `with open(...)` +- a created handle or client is explicitly closed later in the added code + +### Missing Tests + +Diff-level pattern: + +- Production code changes are present but no test files are updated in the same diff. + +Routing guidance: + +- This category usually goes to `needs_human_review` unless the project later adds stronger ownership or coverage signals. + +### Sensitive Information Leak + +High-confidence patterns: + +- `api_key = "..."` +- `token = "..."` +- `password = "..."` +- `secret = "..."` +- `Bearer ...` +- `AKIA...` +- `sk-...` +- `ghp_...` +- private key headers such as `BEGIN PRIVATE KEY` + +Severity guidance: + +- Hard-coded credentials are generally high or critical. + +Do not report when: + +- the value is an obvious placeholder such as `example`, `changeme`, `dummy`, `masked`, or `redacted` +- the match only appears in comments or explanatory documentation + +### Database Lifecycle + +Heuristic patterns: + +- Database connection/session created without visible close handling +- Transaction opened without clear commit / rollback handling + +Routing guidance: + +- Emit high-confidence findings only when lifecycle handling is clearly absent. +- Otherwise route to `needs_human_review`. + +Do not report when: + +- the connection/session is visibly closed later in the added code +- transaction handling visibly includes both commit and rollback paths + +## Finding Contract + +Each finding should include at least: + +- `severity` +- `category` +- `file` +- `line` +- `title` +- `evidence` +- `recommendation` +- `confidence` +- `source` + +## Noise Control + +- Do not emit duplicate findings for the same category, file, and line +- Route low-confidence items into warnings or human-review buckets +- Redact secrets before persistence and reporting +- Prefer suppressing obvious placeholders and safe examples over emitting low-value findings + +## First-Pass Confidence Guidance + +- `confidence >= 0.8`: final finding +- `0.4 <= confidence < 0.8`: `needs_human_review` +- `confidence < 0.4`: warning + +## Phase-Two Quality Fixtures + +- Positive / safe examples: + - `safe_security_patterns.diff` + - `managed_lifecycle.diff` +- Boundary / negative examples: + - `placeholder_secrets.diff` diff --git a/skills/code-review/SCRIPT_CONTRACTS.md b/skills/code-review/SCRIPT_CONTRACTS.md new file mode 100644 index 00000000..50fdcd2f --- /dev/null +++ b/skills/code-review/SCRIPT_CONTRACTS.md @@ -0,0 +1,74 @@ +# Script Contracts + +## `scripts/parse_diff.py` + +Purpose: + +- summarizes diff size and whether high-risk tokens appear + +CLI: + +```text +python scripts/parse_diff.py --diff-file +``` + +Output: + +- JSON to stdout +- fields: + - `diff_file` + - `line_count` + - `file_count` + - `has_security_keywords` + +## `scripts/run_linters.py` + +Purpose: + +- runs deterministic lint-style checks over the diff content + +CLI: + +```text +python scripts/run_linters.py --diff-file +``` + +Output: + +- JSON to stdout +- fields: + - `diff_file` + - `warning_count` + - `warnings` + +Failure behavior: + +- exits non-zero when the diff contains the `TODO_FAIL_SANDBOX` marker +- intended for sandbox failure testing and pipeline resilience checks + +## `scripts/run_tests.py` + +Purpose: + +- reports whether test files are updated in the diff + +CLI: + +```text +python scripts/run_tests.py --diff-file +``` + +Output: + +- JSON to stdout +- fields: + - `diff_file` + - `changed_test_files` + - `test_update_present` + +## Shared Expectations + +- scripts should stay deterministic +- scripts should not require network access +- scripts should be safe to run in dry-run and fake-model modes +- stdout/stderr may be truncated by sandbox policy diff --git a/skills/code-review/SKILL.md b/skills/code-review/SKILL.md new file mode 100644 index 00000000..4c8d6b17 --- /dev/null +++ b/skills/code-review/SKILL.md @@ -0,0 +1,122 @@ +--- +name: code-review +description: Reviews diffs with reusable rules and sandboxed scripts. Invoke when a task needs structured code review, risk detection, or validation runs. +--- + +# Code Review Skill + +Overview + +Use this skill when a task needs structured code review over a unified diff, +local repository changes, or deterministic fixtures. It packages reusable review +rules, sandboxable scripts, and output conventions for the +`skills_code_review_agent` example. + +This skill is designed for: + +- deterministic first-pass code review +- security and secret scanning on changed lines +- review validation in dry-run or fake-model mode +- controlled script execution through `skill_run` + +Primary workflow + +1. Load the skill: + + ```text + skill_load(skill="code-review") + ``` + +2. If more detail is needed, load docs such as `USAGE.md` or `SCRIPT_CONTRACTS.md`. + +3. Run one or more skill scripts through `skill_run`: + + - `scripts/parse_diff.py` + - `scripts/run_linters.py` + - `scripts/run_tests.py` + +4. Convert results into structured review findings with the required fields: + + - `severity` + - `category` + - `file` + - `line` + - `title` + - `evidence` + - `recommendation` + - `confidence` + - `source` + +Inputs + +- unified diff files +- repository paths with local modifications +- prepared fixtures for deterministic tests + +Outputs + +- structured findings +- sandbox execution summaries +- `review_report.json` +- `review_report.md` + +Available Docs + +- `RULES.md`: review categories and rule semantics +- `USAGE.md`: when and how to invoke the skill +- `SCRIPT_CONTRACTS.md`: each script's CLI and output contract + +Scripts + +- `scripts/parse_diff.py` +- `scripts/run_linters.py` +- `scripts/run_tests.py` + +Safety Guidance + +- Always pass review inputs as files rather than inlining huge diffs in prompts. +- Keep execution deterministic in dry-run and fake-model mode. +- Do not allow high-risk scripts to bypass filter checks. +- Redact secrets before persisting or displaying evidence. + +Example `skill_run` commands + +1. Parse a diff: + + ```text + skill_run( + skill="code-review", + cwd="$SKILLS_DIR/code-review", + command="python scripts/parse_diff.py --diff-file $WORK_DIR/sample.diff > out/parse_summary.json", + output_files=["out/parse_summary.json"] + ) + ``` + +2. Run deterministic lint checks: + + ```text + skill_run( + skill="code-review", + cwd="$SKILLS_DIR/code-review", + command="python scripts/run_linters.py --diff-file $WORK_DIR/sample.diff > out/lint_summary.json", + output_files=["out/lint_summary.json"] + ) + ``` + +3. Run deterministic test-presence checks: + + ```text + skill_run( + skill="code-review", + cwd="$SKILLS_DIR/code-review", + command="python scripts/run_tests.py --diff-file $WORK_DIR/sample.diff > out/test_summary.json", + output_files=["out/test_summary.json"] + ) + ``` + +Notes + +- The first implementation keeps the core logic deterministic so dry-run mode can + exercise the full pipeline without a real model API key. +- The skill complements the main agent orchestrator rather than replacing it: + the agent owns task lifecycle, persistence, reporting, and governance. diff --git a/skills/code-review/USAGE.md b/skills/code-review/USAGE.md new file mode 100644 index 00000000..983f7982 --- /dev/null +++ b/skills/code-review/USAGE.md @@ -0,0 +1,43 @@ +# Code Review Skill Usage + +## When To Invoke + +Invoke `code-review` when you need: + +- structured review over a unified diff +- deterministic risk detection in dry-run or fake-model mode +- a controlled script-based review step before writing a final report +- reusable review logic that should stay outside the main agent prompt + +## When Not To Invoke + +Do not invoke this skill for: + +- trivial file reads or one-line code questions +- full repository security auditing outside the changed diff scope +- governance decisions that belong to `Filter` +- persistence, report writing, or task lifecycle management + +## Recommended Pattern + +1. Normalize the input diff or repo change set in the main agent. +2. Load the `code-review` skill. +3. Run one or more scripts through `skill_run`. +4. Merge script output with deterministic rule-engine findings. +5. Apply redaction before persistence and reporting. + +## Integration Boundary + +This skill owns: + +- reusable review instructions +- rule documentation +- script-level deterministic checks + +The main agent still owns: + +- task orchestration +- filter decisions +- sandbox policy +- storage +- final report generation diff --git a/skills/code-review/scripts/parse_diff.py b/skills/code-review/scripts/parse_diff.py new file mode 100644 index 00000000..d2e239eb --- /dev/null +++ b/skills/code-review/scripts/parse_diff.py @@ -0,0 +1,44 @@ +"""Parse diffs for the code review skill.""" + +from __future__ import annotations + +import argparse +import json +from pathlib import Path + + +def build_parser() -> argparse.ArgumentParser: + """Build CLI arguments for the diff parser script.""" + + parser = argparse.ArgumentParser(description="Parse a diff file for review skill diagnostics.") + parser.add_argument("--diff-file", required=True, help="Path to the diff file.") + return parser + + +def parse_args(argv: list[str] | None = None) -> argparse.Namespace: + """Parse command-line arguments.""" + + return build_parser().parse_args(argv) + + +def main(argv: list[str] | None = None) -> int: + """Summarize diff shape for sandboxed skill diagnostics.""" + + args = parse_args(argv) + diff_path = Path(args.diff_file).expanduser().resolve() + diff_text = diff_path.read_text(encoding="utf-8") + payload = { + "diff_file": str(diff_path), + "line_count": len(diff_text.splitlines()), + "file_count": diff_text.count("diff --git "), + "has_security_keywords": any( + token in diff_text + for token in ("eval(", "shell=True", "pickle.loads(", "yaml.load(") + ), + } + print(json.dumps(payload, indent=2)) + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/skills/code-review/scripts/run_linters.py b/skills/code-review/scripts/run_linters.py new file mode 100644 index 00000000..77e33792 --- /dev/null +++ b/skills/code-review/scripts/run_linters.py @@ -0,0 +1,54 @@ +"""Run sandboxed lint or static checks for the code review skill.""" + +from __future__ import annotations + +import argparse +import json +import sys +from pathlib import Path + + +def build_parser() -> argparse.ArgumentParser: + """Build CLI arguments for the linter simulation script.""" + + parser = argparse.ArgumentParser(description="Run deterministic lint checks on a diff file.") + parser.add_argument("--diff-file", required=True, help="Path to the diff file.") + return parser + + +def parse_args(argv: list[str] | None = None) -> argparse.Namespace: + """Parse command-line arguments.""" + + return build_parser().parse_args(argv) + + +def main(argv: list[str] | None = None) -> int: + """Run deterministic lint-style checks over the diff content.""" + + args = parse_args(argv) + diff_path = Path(args.diff_file).expanduser().resolve() + diff_text = diff_path.read_text(encoding="utf-8") + + if "TODO_FAIL_SANDBOX" in diff_text: + print("Simulated linter failure requested by fixture marker.", file=sys.stderr) + return 2 + + warnings: list[str] = [] + if "eval(" in diff_text: + warnings.append("Security-sensitive call detected: eval") + if "shell=True" in diff_text: + warnings.append("Shell execution enabled in subprocess call") + if "verify=False" in diff_text: + warnings.append("TLS verification disabled") + + payload = { + "diff_file": str(diff_path), + "warning_count": len(warnings), + "warnings": warnings, + } + print(json.dumps(payload, indent=2)) + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/skills/code-review/scripts/run_tests.py b/skills/code-review/scripts/run_tests.py new file mode 100644 index 00000000..7537d288 --- /dev/null +++ b/skills/code-review/scripts/run_tests.py @@ -0,0 +1,45 @@ +"""Run sandboxed tests for the code review skill.""" + +from __future__ import annotations + +import argparse +import json +from pathlib import Path + + +def build_parser() -> argparse.ArgumentParser: + """Build CLI arguments for the test simulation script.""" + + parser = argparse.ArgumentParser(description="Simulate test execution for a diff file.") + parser.add_argument("--diff-file", required=True, help="Path to the diff file.") + return parser + + +def parse_args(argv: list[str] | None = None) -> argparse.Namespace: + """Parse command-line arguments.""" + + return build_parser().parse_args(argv) + + +def main(argv: list[str] | None = None) -> int: + """Emit a deterministic test summary based on changed paths.""" + + args = parse_args(argv) + diff_path = Path(args.diff_file).expanduser().resolve() + diff_text = diff_path.read_text(encoding="utf-8") + changed_test_files = [ + line.split(" b/", maxsplit=1)[1] + for line in diff_text.splitlines() + if line.startswith("diff --git ") and "/tests/" in line + ] + payload = { + "diff_file": str(diff_path), + "changed_test_files": changed_test_files, + "test_update_present": bool(changed_test_files), + } + print(json.dumps(payload, indent=2)) + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/tests/sessions/replay_acceptance_design.md b/tests/sessions/replay_acceptance_design.md new file mode 100644 index 00000000..fbff01fc --- /dev/null +++ b/tests/sessions/replay_acceptance_design.md @@ -0,0 +1,32 @@ +# Replay Consistency Design Note + +## 设计说明 +本框架以统一 `ReplayCase` 协议驱动 `InMemory`、文件型 `SQLite` 与可选 `Redis` 后端,先生成 `session/state/memory/summary` 四类快照,再做归一化比较。归一化只移除非业务噪声,不掩盖事件顺序、state 最终值、memory 检索结果及 summary 归属与覆盖关系;其中普通时间戳做精度收敛,`summary_timestamp` 使用 case 级确定性时钟参与比较。summary 比较分为两层:一层比较摘要文本与压缩后的事件窗口,另一层比较 `summary_id/version/replaces/session_id` 等 lineage 元数据。为验证保存/读取语义,summary 元数据会随 summary event 一起持久化,`SQLite/Redis` 适配器在抓取快照前执行一次关服务后重开读回,并支持在 case 中显式插入中途重启步骤验证“重启后继续写”。协议支持用 `session_alias` 在同一 case 内驱动多 session / 多 user,并在最终快照中同时保留 `active session` 与 `sessions_by_alias` 视图,避免非活跃 session 损坏被漏检。memory 检索按 step 记录为独立 observation,不会在重启后被重算覆盖;同名 query 可跨 session 重复使用。负例分为 snapshot mutation 与 runtime fault 两类,二者都支持 alias 级注入,前者验证比较器精度,后者验证重复写入、中途失败、非活跃 session 污染和运行时 summary 破坏。轻量模式默认运行 `InMemory + SQLite`,集成模式通过环境变量启用 `Redis`。 + +## 官方 10 条验收 Case +| Case ID | 场景 | 验收点 | +| --- | --- | --- | +| `single_turn_text` | 单轮普通对话 | 基础事件一致性 | +| `multi_turn_dialogue` | 多轮对话 | 事件顺序与多轮回放 | +| `tool_call_and_response` | 工具调用对话 | `function_call/response` 一致性 | +| `state_and_memory_roundtrip` | 多次 state 更新覆盖 + memory 检索 | state 覆盖语义与 memory 一致性 | +| `summary_compaction_with_history` | summary 生成与历史事件压缩 | summary 与 historical events | +| `summary_version_rolls_forward` | summary 更新 | version / replaces 递增 | +| `summary_binding_mismatch_injection` | summary 归属错误 | `summary.session_id` 检出 | +| `summary_missing_injection` | summary 丢失 | `summary` 缺失检出 | +| `runtime_summary_overwrite_fault` | summary 覆盖错误 | `summary.replaces` 检出 | +| `partial_failure_event_loss_fault` | 写入中途失败 | 事件窗口异常检出 | + +## 扩展 Case +- `state_corruption_injection`:补充 state 字段级污染检测。 +- `summary_lineage_corruption_injection`:补充 snapshot 层 lineage 篡改。 +- `duplicate_event_runtime_fault`:补充重复写入异常。 +- `runtime_state_corruption_fault`:补充运行时 state 污染。 +- `runtime_summary_loss_fault`:补充运行时 summary 丢失。 +- `non_active_session_summary_loss_fault`:补充非活跃 session 的 summary 损坏检测。 +- `cross_session_memory_aggregation`:补充同一 app/user 下跨 session 的 memory 聚合语义。 +- `restart_mid_replay_after_summary`:补充 summary 持久化后中途重启再续写的恢复语义。 +- `state_namespace_roundtrip`:补充 `app:/user:/temp:` 状态命名空间在跨 session 和重启后的可见性语义。 +- `cross_user_memory_isolation`:补充同一 app 下不同 user 的长期记忆隔离语义。 +- `duplicate_memory_query_name_across_sessions`:用定向测试覆盖跨 alias 的同名 memory query 不应互相覆盖。 +- `memory_query_observation_survives_restart`:用定向测试覆盖重启后 memory query 观测不得被后续结果回填。 diff --git a/tests/sessions/replay_cases.py b/tests/sessions/replay_cases.py new file mode 100644 index 00000000..16e2673e --- /dev/null +++ b/tests/sessions/replay_cases.py @@ -0,0 +1,746 @@ +"""Replay cases for acceptance and extended consistency tests.""" + +from __future__ import annotations + +from .replay_models import EventSpec +from .replay_models import FunctionCallSpec +from .replay_models import FunctionResponseSpec +from .replay_models import ReplayCase +from .replay_models import ReplayStep +from .replay_models import RuntimeFault +from .replay_models import RuntimeFaultOperation +from .replay_models import SnapshotMutation +from .replay_models import SnapshotMutationOperation + + +_PERSISTENT_BACKEND = "persistent" + + +_BASELINE_CASES: tuple[ReplayCase, ...] = ( + ReplayCase( + case_id="single_turn_text", + description="One user turn followed by one assistant text response.", + session_id="replay-single-turn", + steps=( + ReplayStep.create_session(), + ReplayStep.append_event( + EventSpec(author="user", role="user", text="Hello, what can you do?"), + ), + ReplayStep.append_event( + EventSpec(author="assistant", role="model", text="I can help answer questions."), + ), + ), + ), + ReplayCase( + case_id="multi_turn_dialogue", + description="Multiple user and assistant turns should preserve event ordering across backends.", + session_id="replay-multi-turn", + steps=( + ReplayStep.create_session(), + ReplayStep.append_event( + EventSpec(author="user", role="user", text="Hello assistant."), + ), + ReplayStep.append_event( + EventSpec(author="assistant", role="model", text="Hello, how can I help?"), + ), + ReplayStep.append_event( + EventSpec(author="user", role="user", text="Please remember my travel plan."), + ), + ReplayStep.append_event( + EventSpec(author="assistant", role="model", text="I will remember your travel plan."), + ), + ), + ), + ReplayCase( + case_id="tool_call_and_response", + description="Assistant tool call followed by tool response.", + session_id="replay-tool-call", + steps=( + ReplayStep.create_session(), + ReplayStep.append_event( + EventSpec(author="user", role="user", text="Check the Beijing weather."), + ), + ReplayStep.append_event( + EventSpec( + author="assistant", + role="model", + function_calls=( + FunctionCallSpec( + name="get_weather", + args={"city": "Beijing"}, + call_id="call-weather-1", + ), + ), + ), + ), + ReplayStep.append_event( + EventSpec( + author="assistant", + role="user", + function_responses=( + FunctionResponseSpec( + name="get_weather", + response={"temperature": "25C", "condition": "Sunny"}, + call_id="call-weather-1", + ), + ), + ), + ), + ), + ), + ReplayCase( + case_id="state_and_memory_roundtrip", + description="Repeated session state updates should preserve overwrite semantics before memory persistence.", + session_id="replay-memory-state", + steps=( + ReplayStep.create_session(initial_state={"user_name": "alice"}), + ReplayStep.append_event( + EventSpec(author="user", role="user", text="Please remember that I prefer tea."), + ), + ReplayStep.append_event( + EventSpec( + author="assistant", + role="model", + text="Noted. You prefer tea over coffee.", + state_delta={"preference": "tea"}, + ), + ), + ReplayStep.append_event( + EventSpec(author="user", role="user", text="Actually update that to green tea."), + ), + ReplayStep.append_event( + EventSpec( + author="assistant", + role="model", + text="Updated. You now prefer green tea.", + state_delta={"preference": "green tea", "drink_temperature": "hot"}, + ), + ), + ReplayStep.store_memory(), + ReplayStep.search_memory(name="preference_search", query="green tea", limit=5), + ), + ), + ReplayCase( + case_id="summary_compaction_with_history", + description="Deterministic summary compaction keeps recent events and stores historical events.", + session_id="replay-summary-history", + enable_summary=True, + summary_keep_recent_count=2, + store_historical_events=True, + steps=( + ReplayStep.create_session(), + ReplayStep.append_event( + EventSpec(author="user", role="user", text="I am planning a weekend trip."), + ), + ReplayStep.append_event( + EventSpec(author="assistant", role="model", text="Great, where would you like to go?"), + ), + ReplayStep.append_event( + EventSpec(author="user", role="user", text="I want to visit Hangzhou."), + ), + ReplayStep.append_event( + EventSpec(author="assistant", role="model", text="Hangzhou is known for West Lake."), + ), + ReplayStep.create_summary(force=True), + ), + ), + ReplayCase( + case_id="summary_version_rolls_forward", + description="A second summary should replace the first one with incremented lineage metadata.", + session_id="replay-summary-version", + enable_summary=True, + summary_keep_recent_count=2, + store_historical_events=True, + steps=( + ReplayStep.create_session(), + ReplayStep.append_event( + EventSpec(author="user", role="user", text="Remember my project is called Atlas."), + ), + ReplayStep.append_event( + EventSpec(author="assistant", role="model", text="Got it, your project is Atlas."), + ), + ReplayStep.append_event( + EventSpec(author="user", role="user", text="It uses Redis and SQL backends."), + ), + ReplayStep.append_event( + EventSpec(author="assistant", role="model", text="Atlas uses Redis and SQL backends."), + ), + ReplayStep.create_summary(force=True), + ReplayStep.append_event( + EventSpec(author="user", role="user", text="Also note that replay consistency is critical."), + ), + ReplayStep.append_event( + EventSpec(author="assistant", role="model", text="I will keep replay consistency in mind."), + ), + ReplayStep.create_summary(force=True), + ), + ), +) + + +_NEGATIVE_CASES: tuple[ReplayCase, ...] = ( + ReplayCase( + case_id="summary_binding_mismatch_injection", + description="Injected summary ownership mismatch must be reported at the exact summary field path.", + session_id="replay-negative-summary-binding", + enable_summary=True, + summary_keep_recent_count=2, + store_historical_events=True, + expected_diff_paths=("summary.session_id",), + snapshot_mutations=( + SnapshotMutation( + backend_name=_PERSISTENT_BACKEND, + path="summary.session_id", + value="wrong-session-id", + ), + ), + steps=( + ReplayStep.create_session(), + ReplayStep.append_event( + EventSpec(author="user", role="user", text="Please summarize my travel plan."), + ), + ReplayStep.append_event( + EventSpec(author="assistant", role="model", text="Sure, tell me the route."), + ), + ReplayStep.append_event( + EventSpec(author="user", role="user", text="Shanghai to Hangzhou by train."), + ), + ReplayStep.append_event( + EventSpec(author="assistant", role="model", text="That route is short and convenient."), + ), + ReplayStep.create_summary(force=True), + ), + ), + ReplayCase( + case_id="summary_missing_injection", + description="Injected summary loss must be detected as a summary-level mismatch.", + session_id="replay-negative-summary-missing", + enable_summary=True, + summary_keep_recent_count=2, + store_historical_events=True, + expected_diff_paths=("summary",), + snapshot_mutations=( + SnapshotMutation( + backend_name=_PERSISTENT_BACKEND, + path="summary", + value=None, + ), + ), + steps=( + ReplayStep.create_session(), + ReplayStep.append_event( + EventSpec(author="user", role="user", text="Track my preferences for black coffee."), + ), + ReplayStep.append_event( + EventSpec(author="assistant", role="model", text="I noted your coffee preference."), + ), + ReplayStep.append_event( + EventSpec(author="user", role="user", text="Also remember I dislike sugary drinks."), + ), + ReplayStep.append_event( + EventSpec(author="assistant", role="model", text="I will avoid sugary drink suggestions."), + ), + ReplayStep.create_summary(force=True), + ), + ), + ReplayCase( + case_id="state_corruption_injection", + description="Injected state corruption must surface at the exact state field path.", + session_id="replay-negative-state-corruption", + expected_diff_paths=("state.preference",), + snapshot_mutations=( + SnapshotMutation( + backend_name=_PERSISTENT_BACKEND, + path="state.preference", + value="coffee", + ), + ), + steps=( + ReplayStep.create_session(initial_state={"user_name": "alice"}), + ReplayStep.append_event( + EventSpec(author="user", role="user", text="Please remember that I prefer tea."), + ), + ReplayStep.append_event( + EventSpec( + author="assistant", + role="model", + text="Noted. You prefer tea over coffee.", + state_delta={"preference": "tea"}, + ), + ), + ), + ), + ReplayCase( + case_id="summary_lineage_corruption_injection", + description="Injected summary lineage corruption must be detected via the replaces field.", + session_id="replay-negative-summary-lineage", + enable_summary=True, + summary_keep_recent_count=2, + store_historical_events=True, + expected_diff_paths=("summary.replaces",), + snapshot_mutations=( + SnapshotMutation( + backend_name=_PERSISTENT_BACKEND, + path="summary.replaces", + value="wrong-summary-id", + ), + ), + steps=( + ReplayStep.create_session(), + ReplayStep.append_event( + EventSpec(author="user", role="user", text="Remember my project codename is Northstar."), + ), + ReplayStep.append_event( + EventSpec(author="assistant", role="model", text="The codename is Northstar."), + ), + ReplayStep.append_event( + EventSpec(author="user", role="user", text="It runs on both SQLite and Redis."), + ), + ReplayStep.append_event( + EventSpec(author="assistant", role="model", text="Northstar runs on SQLite and Redis."), + ), + ReplayStep.create_summary(force=True), + ReplayStep.append_event( + EventSpec(author="user", role="user", text="Replay consistency matters a lot."), + ), + ReplayStep.append_event( + EventSpec(author="assistant", role="model", text="I will keep replay consistency as a priority."), + ), + ReplayStep.create_summary(force=True), + ), + ), + ReplayCase( + case_id="duplicate_event_runtime_fault", + description="A duplicated event injected during replay must be detected as an event-count mismatch.", + session_id="replay-negative-duplicate-event", + expected_diff_paths=("session.events.length",), + runtime_faults=( + RuntimeFault( + backend_name=_PERSISTENT_BACKEND, + after_step=2, + operation=RuntimeFaultOperation.DUPLICATE_LAST_EVENT, + ), + ), + steps=( + ReplayStep.create_session(), + ReplayStep.append_event( + EventSpec(author="user", role="user", text="Please store this reminder."), + ), + ReplayStep.append_event( + EventSpec(author="assistant", role="model", text="I stored the reminder."), + ), + ), + ), + ReplayCase( + case_id="runtime_state_corruption_fault", + description="A runtime state corruption must be detected on the precise state path.", + session_id="replay-negative-runtime-state", + expected_diff_paths=("state.preference",), + runtime_faults=( + RuntimeFault( + backend_name=_PERSISTENT_BACKEND, + after_step=2, + operation=RuntimeFaultOperation.SET_SESSION_VALUE, + path="state.preference", + value="coffee", + ), + ), + steps=( + ReplayStep.create_session(initial_state={"user_name": "alice"}), + ReplayStep.append_event( + EventSpec(author="user", role="user", text="Please remember that I prefer tea."), + ), + ReplayStep.append_event( + EventSpec( + author="assistant", + role="model", + text="Noted. You prefer tea over coffee.", + state_delta={"preference": "tea"}, + ), + ), + ), + ), + ReplayCase( + case_id="runtime_summary_loss_fault", + description="A runtime summary deletion must be detected as a missing summary.", + session_id="replay-negative-runtime-summary-loss", + enable_summary=True, + summary_keep_recent_count=2, + store_historical_events=True, + expected_diff_paths=("summary",), + runtime_faults=( + RuntimeFault( + backend_name=_PERSISTENT_BACKEND, + after_step=5, + operation=RuntimeFaultOperation.DELETE_SUMMARY, + ), + ), + steps=( + ReplayStep.create_session(), + ReplayStep.append_event( + EventSpec(author="user", role="user", text="Please summarize my sprint notes."), + ), + ReplayStep.append_event( + EventSpec(author="assistant", role="model", text="Sure, continue."), + ), + ReplayStep.append_event( + EventSpec(author="user", role="user", text="We fixed replay ordering bugs."), + ), + ReplayStep.append_event( + EventSpec(author="assistant", role="model", text="The replay ordering bugs were fixed."), + ), + ReplayStep.create_summary(force=True), + ), + ), + ReplayCase( + case_id="runtime_summary_overwrite_fault", + description="A runtime summary overwrite must be detected through lineage replacement fields.", + session_id="replay-negative-runtime-summary-overwrite", + enable_summary=True, + summary_keep_recent_count=2, + store_historical_events=True, + expected_diff_paths=( + "summary.replaces", + "summary.metadata.replaces", + ), + runtime_faults=( + RuntimeFault( + backend_name=_PERSISTENT_BACKEND, + after_step=8, + operation=RuntimeFaultOperation.SET_SUMMARY_VALUE, + path="metadata.replaces", + value="wrong-summary-id", + ), + ), + steps=( + ReplayStep.create_session(), + ReplayStep.append_event( + EventSpec(author="user", role="user", text="Remember the codename is Aurora."), + ), + ReplayStep.append_event( + EventSpec(author="assistant", role="model", text="The codename is Aurora."), + ), + ReplayStep.append_event( + EventSpec(author="user", role="user", text="Aurora runs on SQL and Redis."), + ), + ReplayStep.append_event( + EventSpec(author="assistant", role="model", text="Aurora runs on SQL and Redis."), + ), + ReplayStep.create_summary(force=True), + ReplayStep.append_event( + EventSpec(author="user", role="user", text="Replay correctness matters a lot."), + ), + ReplayStep.append_event( + EventSpec(author="assistant", role="model", text="I will prioritize replay correctness."), + ), + ReplayStep.create_summary(force=True), + ), + ), + ReplayCase( + case_id="partial_failure_event_loss_fault", + description="A partial failure that loses the final event but keeps state must be detected as an event-window mismatch.", + session_id="replay-negative-partial-failure", + expected_diff_paths=("session.events.length",), + runtime_faults=( + RuntimeFault( + backend_name=_PERSISTENT_BACKEND, + after_step=2, + operation=RuntimeFaultOperation.DROP_LAST_EVENT_KEEP_STATE, + ), + ), + steps=( + ReplayStep.create_session(initial_state={"user_name": "alice"}), + ReplayStep.append_event( + EventSpec(author="user", role="user", text="Please remember that I prefer tea."), + ), + ReplayStep.append_event( + EventSpec( + author="assistant", + role="model", + text="Noted. You prefer tea over coffee.", + state_delta={"preference": "tea"}, + ), + ), + ), + ), + ReplayCase( + case_id="non_active_session_summary_loss_fault", + description="A runtime summary deletion on a non-active session alias must still be detected through alias-scoped snapshots.", + session_id="replay-negative-target-session", + enable_summary=True, + summary_keep_recent_count=2, + store_historical_events=True, + expected_diff_paths=("sessions_by_alias.source.summary",), + runtime_faults=( + RuntimeFault( + backend_name=_PERSISTENT_BACKEND, + after_step=6, + operation=RuntimeFaultOperation.DELETE_SUMMARY, + session_alias="source", + ), + ), + steps=( + ReplayStep.create_session(session_alias="source", session_id="replay-negative-source-session"), + ReplayStep.append_event( + EventSpec(author="user", role="user", text="Please summarize my architecture notes."), + session_alias="source", + ), + ReplayStep.append_event( + EventSpec(author="assistant", role="model", text="Sure, continue with the details."), + session_alias="source", + ), + ReplayStep.append_event( + EventSpec(author="user", role="user", text="We run SQLite for local replay and Redis in production."), + session_alias="source", + ), + ReplayStep.append_event( + EventSpec(author="assistant", role="model", text="I will summarize the backend setup."), + session_alias="source", + ), + ReplayStep.create_summary(force=True, session_alias="source"), + ReplayStep.create_session(session_alias="default", session_id="replay-negative-target-session"), + ReplayStep.append_event( + EventSpec(author="user", role="user", text="Switch focus to the current session."), + ), + ), + ), +) + + +_ROBUSTNESS_CASES: tuple[ReplayCase, ...] = ( + ReplayCase( + case_id="cross_session_memory_aggregation", + description="Memory written by one session should remain searchable from another session under the same app/user scope.", + session_id="replay-memory-target", + steps=( + ReplayStep.create_session(session_alias="source", session_id="replay-memory-source"), + ReplayStep.append_event( + EventSpec(author="user", role="user", text="Please remember that I prefer oolong tea."), + session_alias="source", + ), + ReplayStep.append_event( + EventSpec(author="assistant", role="model", text="Noted. You prefer oolong tea."), + session_alias="source", + ), + ReplayStep.store_memory(session_alias="source"), + ReplayStep.create_session(session_alias="default", session_id="replay-memory-target"), + ReplayStep.append_event( + EventSpec(author="user", role="user", text="What drink did I say I prefer?"), + ), + ReplayStep.search_memory( + name="cross_session_preference_search", + query="oolong", + limit=5, + ), + ), + ), + ReplayCase( + case_id="restart_mid_replay_after_summary", + description="Persistent backends should restore summary state correctly after a restart and continue replaying later turns.", + session_id="replay-restart-summary", + enable_summary=True, + summary_keep_recent_count=2, + store_historical_events=True, + steps=( + ReplayStep.create_session(), + ReplayStep.append_event( + EventSpec(author="user", role="user", text="I am planning a Hangzhou trip."), + ), + ReplayStep.append_event( + EventSpec(author="assistant", role="model", text="Great, what should I remember?"), + ), + ReplayStep.append_event( + EventSpec(author="user", role="user", text="Please remember I need a hotel near West Lake."), + ), + ReplayStep.append_event( + EventSpec(author="assistant", role="model", text="I will remember the hotel preference."), + ), + ReplayStep.create_summary(force=True), + ReplayStep.restart_services(), + ReplayStep.append_event( + EventSpec(author="user", role="user", text="Also note that I will arrive next Friday."), + ), + ReplayStep.append_event( + EventSpec(author="assistant", role="model", text="Arrival next Friday is recorded."), + ), + ReplayStep.restart_services(), + ReplayStep.append_event( + EventSpec(author="user", role="user", text="I prefer morning check-in if available."), + ), + ReplayStep.append_event( + EventSpec(author="assistant", role="model", text="I will keep the morning check-in preference."), + ), + ReplayStep.create_summary(force=True), + ), + ), + ReplayCase( + case_id="state_namespace_roundtrip", + description="App, user, session, and temp state should preserve their intended visibility across sessions and restarts.", + session_id="replay-state-namespace-b", + steps=( + ReplayStep.create_session( + session_alias="writer", + session_id="replay-state-namespace-a", + initial_state={ + "app:locale": "zh-CN", + "user:timezone": "Asia/Shanghai", + "temp:request_id": "req-1", + "draft": "first-session", + }, + ), + ReplayStep.append_event( + EventSpec( + author="assistant", + role="model", + text="Updating shared and session state.", + state_delta={ + "app:release": "2026.07", + "user:tone": "concise", + "temp:trace_id": "trace-1", + "draft": "writer-updated", + }, + ), + session_alias="writer", + ), + ReplayStep.restart_services(), + ReplayStep.create_session( + session_alias="default", + session_id="replay-state-namespace-b", + initial_state={ + "temp:request_id": "req-2", + "draft": "second-session", + }, + ), + ReplayStep.append_event( + EventSpec(author="assistant", role="model", text="Second session should inherit shared state only."), + ), + ), + ), + ReplayCase( + case_id="duplicate_memory_query_name_across_sessions", + description="Two sessions may reuse the same query name without overwriting earlier memory observations.", + session_id="replay-duplicate-query-target", + steps=( + ReplayStep.create_session(session_alias="source", session_id="replay-duplicate-query-source"), + ReplayStep.append_event( + EventSpec(author="user", role="user", text="Please remember that I prefer dragon well tea."), + session_alias="source", + ), + ReplayStep.append_event( + EventSpec(author="assistant", role="model", text="Dragon well tea preference recorded."), + session_alias="source", + ), + ReplayStep.store_memory(session_alias="source"), + ReplayStep.search_memory( + name="shared_preference_search", + query="dragon well", + limit=5, + session_alias="source", + ), + ReplayStep.restart_services(), + ReplayStep.create_session(session_alias="default", session_id="replay-duplicate-query-target"), + ReplayStep.search_memory( + name="shared_preference_search", + query="dragon well", + limit=5, + ), + ), + ), + ReplayCase( + case_id="memory_query_observation_survives_restart", + description="Memory query observations should retain their original step-time results across restarts and later writes.", + session_id="replay-memory-observation", + steps=( + ReplayStep.create_session(), + ReplayStep.append_event( + EventSpec(author="user", role="user", text="Please remember that my favorite tea is oolong."), + ), + ReplayStep.append_event( + EventSpec(author="assistant", role="model", text="I will remember your oolong preference."), + ), + ReplayStep.store_memory(), + ReplayStep.search_memory(name="tea_preference", query="oolong", limit=5), + ReplayStep.restart_services(), + ReplayStep.append_event( + EventSpec(author="user", role="user", text="Also remember that I enjoy jasmine tea."), + ), + ReplayStep.append_event( + EventSpec(author="assistant", role="model", text="I will remember the jasmine preference too."), + ), + ReplayStep.store_memory(), + ReplayStep.search_memory(name="tea_preference", query="jasmine", limit=5), + ), + ), + ReplayCase( + case_id="cross_user_memory_isolation", + description="Memory saved for one user must not become visible to another user in the same application.", + session_id="replay-cross-user-target", + steps=( + ReplayStep.create_session( + session_alias="source", + session_id="replay-cross-user-source", + user_id="replay-user-a", + ), + ReplayStep.append_event( + EventSpec(author="user", role="user", text="Please remember that I prefer matcha desserts."), + session_alias="source", + ), + ReplayStep.append_event( + EventSpec(author="assistant", role="model", text="Matcha dessert preference recorded."), + session_alias="source", + ), + ReplayStep.store_memory(session_alias="source"), + ReplayStep.create_session( + session_alias="default", + session_id="replay-cross-user-target", + user_id="replay-user-b", + ), + ReplayStep.search_memory( + name="cross_user_matcha_search", + query="matcha", + limit=5, + ), + ), + ), +) + + +# The acceptance suite is the public, fixed set of 10 replay cases used to +# demonstrate baseline correctness and inconsistency detection. +REPLAY_ACCEPTANCE_CASES: tuple[ReplayCase, ...] = ( + _BASELINE_CASES[0], # single_turn_text + _BASELINE_CASES[1], # multi_turn_dialogue + _BASELINE_CASES[2], # tool_call_and_response + _BASELINE_CASES[3], # state_and_memory_roundtrip + _BASELINE_CASES[4], # summary_compaction_with_history + _BASELINE_CASES[5], # summary_version_rolls_forward + _NEGATIVE_CASES[0], # summary_binding_mismatch_injection + _NEGATIVE_CASES[1], # summary_missing_injection + _NEGATIVE_CASES[7], # runtime_summary_overwrite_fault + _NEGATIVE_CASES[8], # partial_failure_event_loss_fault +) + + +# Extra cases extend coverage beyond the fixed acceptance set while reusing the +# same harness and reporting pipeline. +REPLAY_EXTRA_CASES: tuple[ReplayCase, ...] = ( + _NEGATIVE_CASES[2], # state_corruption_injection + _NEGATIVE_CASES[3], # summary_lineage_corruption_injection + _NEGATIVE_CASES[4], # duplicate_event_runtime_fault + _NEGATIVE_CASES[5], # runtime_state_corruption_fault + _NEGATIVE_CASES[6], # runtime_summary_loss_fault + _NEGATIVE_CASES[9], # non_active_session_summary_loss_fault + _ROBUSTNESS_CASES[0], # cross_session_memory_aggregation + _ROBUSTNESS_CASES[1], # restart_mid_replay_after_summary + _ROBUSTNESS_CASES[2], # state_namespace_roundtrip + _ROBUSTNESS_CASES[5], # cross_user_memory_isolation +) + + +REPLAY_TARGETED_CASES: tuple[ReplayCase, ...] = ( + _ROBUSTNESS_CASES[3], # duplicate_memory_query_name_across_sessions + _ROBUSTNESS_CASES[4], # memory_query_observation_survives_restart +) + + +REPLAY_ALL_CASES: tuple[ReplayCase, ...] = REPLAY_ACCEPTANCE_CASES + REPLAY_EXTRA_CASES diff --git a/tests/sessions/replay_harness.py b/tests/sessions/replay_harness.py new file mode 100644 index 00000000..5f4bb547 --- /dev/null +++ b/tests/sessions/replay_harness.py @@ -0,0 +1,1425 @@ +"""Replay harness for session/memory consistency tests. + +This module provides: +- backend adapters for replaying the same case on multiple backends, +- snapshot extraction helpers, +- normalization helpers for backend-specific noise, +- structured diff generation, +- JSON report writing. + +The first iteration intentionally focuses on deterministic, LLM-free smoke +cases. More complex summary and failure-injection cases can be added on top of +the same protocol without changing the adapter boundary. +""" + +from __future__ import annotations + +from abc import ABC +from abc import abstractmethod +from contextlib import contextmanager +from dataclasses import replace +import hashlib +import os +from pathlib import Path +import json +import re +import tempfile +import time +import uuid +from typing import Any +from typing import Iterator +from typing import Optional +from urllib.parse import urlparse + +from google.genai.types import Content +from google.genai.types import FunctionCall +from google.genai.types import FunctionResponse +from google.genai.types import Part +from trpc_agent_sdk.abc import MemoryServiceConfig +from trpc_agent_sdk.events import Event +from trpc_agent_sdk.memory import BaseMemoryService +from trpc_agent_sdk.memory import InMemoryMemoryService +from trpc_agent_sdk.memory import RedisMemoryService +from trpc_agent_sdk.memory import SqlMemoryService +from trpc_agent_sdk.sessions import BaseSessionService +from trpc_agent_sdk.sessions import InMemorySessionService +from trpc_agent_sdk.sessions import RedisSessionService +from trpc_agent_sdk.sessions import Session +from trpc_agent_sdk.sessions import SessionServiceConfig +from trpc_agent_sdk.sessions import SqlSessionService + +from .replay_models import BackendSnapshot +from .replay_models import DiffEntry +from .replay_models import EventSpec +from .replay_models import FunctionCallSpec +from .replay_models import FunctionResponseSpec +from .replay_models import MemoryQuerySpec +from .replay_models import ReplayCase +from .replay_models import ReplayStep +from .replay_models import ReplayStepKind +from .replay_models import RuntimeFault +from .replay_models import RuntimeFaultOperation +from .replay_models import SessionSnapshot +from .replay_models import SnapshotMutation +from .replay_models import SnapshotMutationOperation +from .replay_models import SummarySnapshot +from .replay_summary import build_replay_summarizer_manager + + +DEFAULT_REPORT_PATH = Path(__file__).with_name("session_memory_summary_diff_report.json") +_EVENT_INDEX_RE = re.compile(r"\[(\d+)\]") +_SUMMARY_EVENT_METADATA_KEY = "session_summary" +_CASE_TIME_BASES: dict[str, float] = {} +_BASELINE_BACKEND_NAME = "inmemory" +_PERSISTENT_BACKEND_TARGETS = {"persistent", "secondary", "non_baseline"} +_BASELINE_BACKEND_TARGETS = {"baseline", "primary", "inmemory"} +_CASE_TIME_FUTURE_SKEW_SECONDS = 60.0 +_REPLAY_CLOCK_MODE_ENV = "TRPC_AGENT_REPLAY_CLOCK_MODE" +_REPLAY_FIXED_EPOCH_ENV = "TRPC_AGENT_REPLAY_FIXED_EPOCH" +_REPLAY_CLOCK_MODE_FRESHNESS_SAFE = "freshness_safe" +_REPLAY_CLOCK_MODE_FIXED_SAFE = "fixed_safe" +_DEFAULT_FIXED_SAFE_EPOCH = 4_102_444_800.0 + + +def _make_memory_config() -> MemoryServiceConfig: + config = MemoryServiceConfig(enabled=True) + config.clean_ttl_config() + return config + + +def _make_session_config() -> SessionServiceConfig: + config = SessionServiceConfig() + config.clean_ttl_config() + return config + + +class ReplayBackendAdapter(ABC): + """Shared backend replay logic. + + Concrete adapters only need to build the concrete session and memory + services. The replay protocol itself stays backend-agnostic. + """ + + name: str + + def __init__(self) -> None: + self._session_service: BaseSessionService | None = None + self._memory_service: BaseMemoryService | None = None + self._session: Session | None = None + self._sessions: dict[str, Session] = {} + self._session_identities: dict[str, dict[str, str]] = {} + self._active_session_alias = "default" + self._memory_results: dict[str, Any] = {} + self._case: ReplayCase | None = None + self._event_sequence = 0 + self._event_time_base = 0.0 + + @property + def session_service(self) -> BaseSessionService: + if self._session_service is None: + raise RuntimeError("Session service is not initialized.") + return self._session_service + + @property + def memory_service(self) -> BaseMemoryService: + if self._memory_service is None: + raise RuntimeError("Memory service is not initialized.") + return self._memory_service + + @property + def session(self) -> Session: + if self._session is None: + raise RuntimeError("Replay session has not been created.") + return self._session + + @property + def case(self) -> ReplayCase: + if self._case is None: + raise RuntimeError("Replay case is not initialized.") + return self._case + + async def setup(self, case: ReplayCase) -> None: + """Initialize backend services for one replay run.""" + + self._case = case + self._memory_results = {} + self._session = None + self._sessions = {} + self._session_identities = {} + self._active_session_alias = "default" + self._event_sequence = 0 + self._event_time_base = _deterministic_time_base(case.case_id) + self._session_service = await self._build_session_service() + self._memory_service = await self._build_memory_service() + + async def _close_services(self) -> None: + """Dispose backend services while preserving replay state.""" + if self._memory_service is not None: + await self._memory_service.close() + self._memory_service = None + if self._session_service is not None: + await self._session_service.close() + self._session_service = None + + async def close(self) -> None: + """Dispose backend services.""" + + await self._close_services() + self._session = None + self._sessions = {} + self._session_identities = {} + self._active_session_alias = "default" + self._memory_results = {} + self._case = None + self._event_sequence = 0 + self._event_time_base = 0.0 + + async def run_case(self, case: ReplayCase) -> BackendSnapshot: + """Replay all steps in a case and collect a snapshot.""" + + for step_index, step in enumerate(case.steps): + await self._run_step(case, step, step_index) + await self._apply_runtime_faults(step_index) + if self.should_restart_before_snapshot(): + await self._restart_services() + return await self.collect_snapshot(case) + + def should_restart_before_snapshot(self) -> bool: + """Return whether the adapter should validate persisted read-back.""" + + return False + + def get_runtime_metadata(self) -> dict[str, Any]: + return { + "backend_name": self.name, + "storage_kind": self.name, + } + + def get_report_metadata(self) -> dict[str, Any]: + return self.get_runtime_metadata() + + async def _restart_services(self) -> None: + """Restart backend services and rebuild the read snapshot from persistence.""" + + await self._close_services() + self._session_service = await self._build_session_service() + self._memory_service = await self._build_memory_service() + reopened_sessions: dict[str, Session] = {} + for session_alias, identity in self._session_identities.items(): + reopened_session = await self.session_service.get_session( + app_name=identity["app_name"], + user_id=identity["user_id"], + session_id=identity["session_id"], + ) + if reopened_session is not None: + reopened_sessions[session_alias] = reopened_session + self._sessions = reopened_sessions + self._session = self._sessions.get(self._active_session_alias) + if self._session is None and self._sessions: + self._active_session_alias = next(iter(self._sessions)) + self._session = self._sessions[self._active_session_alias] + if self._session is None: + return + + def should_restart_during_replay(self) -> bool: + """Return whether RESTART_SERVICES steps should perform a real restart.""" + + return False + + async def _run_step(self, case: ReplayCase, step: ReplayStep, step_index: int) -> None: + if step.kind == ReplayStepKind.CREATE_SESSION: + session_id = step.session_id or case.session_id + session = await self.session_service.create_session( + app_name=step.app_name or case.app_name, + user_id=step.user_id or case.user_id, + session_id=session_id, + state=step.initial_state, + ) + self._session = session + self._sessions[step.session_alias] = session + self._session_identities[step.session_alias] = self._make_session_identity(session) + self._active_session_alias = step.session_alias + return + + if step.kind in {ReplayStepKind.APPEND_EVENT, ReplayStepKind.APPEND_STATE}: + if step.event is None: + raise ValueError(f"Replay step {step.kind} requires an event.") + target_session = self._resolve_session(step.session_alias) + self._event_sequence += 1 + event = build_event( + step.event, + sequence=self._event_sequence, + timestamp=self._event_time_base + (self._event_sequence / 1000.0), + ) + await self.session_service.append_event(target_session, event) + self._session = target_session + return + + if step.kind == ReplayStepKind.STORE_MEMORY: + target_session = self._resolve_session(step.session_alias) + await self.memory_service.store_session(target_session) + self._session = target_session + return + + if step.kind == ReplayStepKind.SEARCH_MEMORY: + if step.memory_query is None: + raise ValueError("SEARCH_MEMORY step requires a query specification.") + target_session = self._resolve_session(step.session_alias) + entries = await self._search_memory_for_session(target_session, step.memory_query) + self._memory_results[self._memory_observation_key(step_index, step)] = { + "query_name": step.memory_query.name, + "session_alias": step.session_alias, + "app_name": target_session.app_name, + "user_id": target_session.user_id, + "session_id": target_session.id, + "step_index": step_index, + "entries": entries, + } + self._session = target_session + return + + if step.kind == ReplayStepKind.CREATE_SUMMARY: + target_session = self._resolve_session(step.session_alias) + self._event_sequence += 1 + summary_timestamp = self._event_time_base + (self._event_sequence / 1000.0) + manager = getattr(self.session_service, "summarizer_manager", None) + with _patched_time_time(summary_timestamp): + if manager is not None and step.force_summary: + await manager.create_session_summary(target_session, force=True) + else: + await self.session_service.create_session_summary(target_session) + self._session = target_session + return + + if step.kind == ReplayStepKind.RESTART_SERVICES: + if self.should_restart_during_replay(): + await self._restart_services() + return + + raise ValueError(f"Unsupported replay step kind: {step.kind}") + + async def _search_memory(self, query: MemoryQuerySpec) -> list[dict[str, Any]]: + return await self._search_memory_for_session(self.session, query) + + async def _search_memory_for_session( + self, + session: Session, + query: MemoryQuerySpec, + ) -> list[dict[str, Any]]: + response = await self.memory_service.search_memory( + key=session.save_key, + query=query.query, + limit=query.limit, + ) + return [_memory_entry_to_snapshot(entry) for entry in response.memories] + + async def collect_snapshot(self, case: ReplayCase) -> BackendSnapshot: + """Collect a backend snapshot after replay.""" + + if self._session is None: + raise RuntimeError(f"Replay case '{case.case_id}' did not produce an active session.") + sessions_by_alias: dict[str, SessionSnapshot] = {} + for session_alias, identity in self._session_identities.items(): + session = await self.session_service.get_session( + app_name=identity["app_name"], + user_id=identity["user_id"], + session_id=identity["session_id"], + ) + if session is None: + raise RuntimeError(f"Replay session '{identity['session_id']}' was not found.") + self._sessions[session_alias] = session + sessions_by_alias[session_alias] = await self._build_session_snapshot(session_alias, session) + + active_alias = self._active_session_alias + active_snapshot = sessions_by_alias.get(active_alias) + if active_snapshot is None and sessions_by_alias: + active_alias = next(iter(sessions_by_alias)) + active_snapshot = sessions_by_alias[active_alias] + self._active_session_alias = active_alias + self._session = self._sessions[active_alias] + if active_snapshot is None: + raise RuntimeError(f"Replay case '{case.case_id}' did not produce an active session.") + + return BackendSnapshot( + backend_name=self.name, + case_id=case.case_id, + app_name=active_snapshot.app_name, + user_id=active_snapshot.user_id, + session_id=active_snapshot.session_id, + active_session_alias=active_alias, + session=active_snapshot.session, + state=active_snapshot.state, + memory=dict(self._memory_results), + summary=active_snapshot.summary, + sessions_by_alias=sessions_by_alias, + ) + + def _resolve_session(self, session_alias: str) -> Session: + session = self._sessions.get(session_alias) + if session is None: + raise RuntimeError(f"Replay session alias '{session_alias}' has not been created.") + self._active_session_alias = session_alias + self._session = session + return session + + async def _get_summary_snapshot(self, session: Session) -> Optional[SummarySnapshot]: + manager = getattr(self.session_service, "summarizer_manager", None) + if manager is None: + return None + + summary = await manager.get_session_summary(session) + if summary is None: + return None + + return SummarySnapshot( + session_id=summary.session_id, + summary_text=summary.summary_text, + original_event_count=summary.original_event_count, + compressed_event_count=summary.compressed_event_count, + summary_id=(summary.metadata or {}).get("summary_id"), + version=(summary.metadata or {}).get("version"), + replaces=(summary.metadata or {}).get("replaces"), + summarized_event_count=(summary.metadata or {}).get("summarized_event_count"), + summary_timestamp=getattr(summary, "summary_timestamp", None), + metadata=dict(getattr(summary, "metadata", {}) or {}), + ) + + async def _apply_runtime_faults(self, step_index: int) -> None: + for fault in self.case.runtime_faults: + if not _backend_target_matches(fault.backend_name, self.name) or fault.after_step != step_index: + continue + await self._apply_runtime_fault(fault) + + async def _apply_runtime_fault(self, fault: RuntimeFault) -> None: + target_session = self._get_fault_session(fault) + if fault.operation == RuntimeFaultOperation.DUPLICATE_LAST_EVENT: + if not target_session.events: + raise RuntimeError("Cannot duplicate the last event of an empty session.") + duplicated_event = target_session.events[-1].model_copy(deep=True) + duplicated_event.id = f"{duplicated_event.id}-duplicate" + duplicated_event.invocation_id = f"{duplicated_event.invocation_id}-duplicate" + duplicated_event.timestamp += 0.0001 + target_session.events.append(duplicated_event) + await self.session_service.update_session(target_session) + return + + if fault.operation == RuntimeFaultOperation.DROP_LAST_EVENT_KEEP_STATE: + if not target_session.events: + raise RuntimeError("Cannot drop the last event of an empty session.") + target_session.events.pop() + await self.session_service.update_session(target_session) + return + + if fault.operation == RuntimeFaultOperation.SET_SESSION_VALUE: + if not fault.path: + raise ValueError("SET_SESSION_VALUE fault requires a target path.") + _set_path_value(target_session, fault.path, fault.value) + await self.session_service.update_session(target_session) + return + + manager = getattr(self.session_service, "summarizer_manager", None) + if manager is None: + raise RuntimeError("Runtime summary fault requires a summarizer manager.") + + if fault.operation == RuntimeFaultOperation.DELETE_SUMMARY: + summary_cache = getattr(manager, "_summarizer_cache", {}) + summary_cache.get(target_session.app_name, {}).get(target_session.user_id, {}).pop(target_session.id, None) + summary_event = _get_summary_event(target_session) + if summary_event is not None: + custom_metadata = dict(summary_event.custom_metadata or {}) + custom_metadata.pop(_SUMMARY_EVENT_METADATA_KEY, None) + summary_event.custom_metadata = custom_metadata or None + await self.session_service.update_session(target_session) + return + + if fault.operation == RuntimeFaultOperation.SET_SUMMARY_VALUE: + if not fault.path: + raise ValueError("SET_SUMMARY_VALUE fault requires a target path.") + summary = await manager.get_session_summary(target_session) + if summary is None: + raise RuntimeError("Cannot mutate summary value because no cached summary exists.") + _set_path_value(summary, fault.path, fault.value) + summary_event = _get_summary_event(target_session) + if summary_event is not None and summary_event.custom_metadata: + persisted_summary = summary_event.custom_metadata.get(_SUMMARY_EVENT_METADATA_KEY) + if isinstance(persisted_summary, dict): + _set_path_value(persisted_summary, fault.path, fault.value) + await self.session_service.update_session(target_session) + return + + raise ValueError(f"Unsupported runtime fault operation: {fault.operation}") + + def _make_session_identity(self, session: Session) -> dict[str, str]: + return { + "app_name": session.app_name, + "user_id": session.user_id, + "session_id": session.id, + } + + def _memory_observation_key(self, step_index: int, step: ReplayStep) -> str: + if step.memory_query is None: + raise ValueError("Memory observation key requires a query specification.") + return f"step_{step_index:03d}:{step.session_alias}:{step.memory_query.name}" + + def _get_fault_session(self, fault: RuntimeFault) -> Session: + session = self._sessions.get(fault.session_alias) + if session is None: + raise RuntimeError(f"Runtime fault session alias '{fault.session_alias}' has not been created.") + return session + + async def _build_session_snapshot(self, session_alias: str, session: Session) -> SessionSnapshot: + return SessionSnapshot( + session_alias=session_alias, + app_name=session.app_name, + user_id=session.user_id, + session_id=session.id, + session={ + "conversation_count": session.conversation_count, + "events": [_event_to_snapshot(event) for event in session.events], + "historical_events": [_event_to_snapshot(event) for event in session.historical_events], + }, + state=dict(session.state), + summary=await self._get_summary_snapshot(session), + ) + + @abstractmethod + async def _build_session_service(self) -> BaseSessionService: + """Create the session service used by this adapter.""" + + @abstractmethod + async def _build_memory_service(self) -> BaseMemoryService: + """Create the memory service used by this adapter.""" + + +class InMemoryReplayAdapter(ReplayBackendAdapter): + """Replay adapter backed by in-memory session and memory services.""" + + name = "inmemory" + + async def _build_session_service(self) -> BaseSessionService: + config = _make_session_config() + config.store_historical_events = self.case.store_historical_events + summarizer_manager = None + if self.case.enable_summary: + summarizer_manager = build_replay_summarizer_manager( + keep_recent_count=self.case.summary_keep_recent_count, + ) + return InMemorySessionService( + summarizer_manager=summarizer_manager, + session_config=config, + ) + + async def _build_memory_service(self) -> BaseMemoryService: + return InMemoryMemoryService(memory_service_config=_make_memory_config()) + + +class SqliteReplayAdapter(ReplayBackendAdapter): + """Replay adapter backed by SQLite-based session and memory services.""" + + name = "sqlite" + + def __init__(self) -> None: + super().__init__() + self._temp_dir = tempfile.TemporaryDirectory(prefix="trpc-replay-sqlite-") + self._session_db_url = self._sqlite_url("sessions.sqlite") + self._memory_db_url = self._sqlite_url("memory.sqlite") + + def _sqlite_url(self, filename: str) -> str: + return f"sqlite:///{(Path(self._temp_dir.name) / filename).as_posix()}" + + def should_restart_before_snapshot(self) -> bool: + return True + + def should_restart_during_replay(self) -> bool: + return True + + async def _build_session_service(self) -> BaseSessionService: + config = _make_session_config() + config.store_historical_events = self.case.store_historical_events + summarizer_manager = None + if self.case.enable_summary: + summarizer_manager = build_replay_summarizer_manager( + keep_recent_count=self.case.summary_keep_recent_count, + ) + service = SqlSessionService( + db_url=self._session_db_url, + summarizer_manager=summarizer_manager, + is_async=False, + session_config=config, + ) + await service._sql_storage.create_sql_engine() + return service + + async def _build_memory_service(self) -> BaseMemoryService: + service = SqlMemoryService( + db_url=self._memory_db_url, + is_async=False, + memory_service_config=_make_memory_config(), + ) + await service._sql_storage.create_sql_engine() + return service + + async def close(self) -> None: + await super().close() + self._temp_dir.cleanup() + + def get_runtime_metadata(self) -> dict[str, Any]: + return { + "backend_name": self.name, + "storage_kind": "sqlite", + "connection": { + "scheme": "sqlite", + "session_db": Path(urlparse(self._session_db_url).path).name, + "memory_db": Path(urlparse(self._memory_db_url).path).name, + }, + } + + +class RedisReplayAdapter(ReplayBackendAdapter): + """Replay adapter backed by Redis session and memory services.""" + + name = "redis" + + def __init__(self, redis_url: str) -> None: + super().__init__() + self._redis_url = redis_url + self._logical_case: ReplayCase | None = None + self._logical_session_identities: dict[str, dict[str, str]] = {} + self._storage_namespace = f"replay-{uuid.uuid4().hex[:8]}" + + @property + def logical_case(self) -> ReplayCase: + if self._logical_case is None: + raise RuntimeError("Logical replay case is not initialized.") + return self._logical_case + + async def setup(self, case: ReplayCase) -> None: + self._logical_case = case + self._logical_session_identities = _collect_logical_session_identities(case) + await super().setup(self._namespaced_case(case)) + + def should_restart_before_snapshot(self) -> bool: + return True + + def should_restart_during_replay(self) -> bool: + return True + + async def run_case(self, case: ReplayCase) -> BackendSnapshot: + snapshot = await super().run_case(self.case) + sessions_by_alias = { + alias: self._project_session_snapshot(alias, session_snapshot) + for alias, session_snapshot in snapshot.sessions_by_alias.items() + } + active_snapshot = sessions_by_alias[snapshot.active_session_alias] + memory = { + key: self._project_memory_observation(value) + for key, value in snapshot.memory.items() + } + summary = active_snapshot.summary + return replace( + snapshot, + app_name=active_snapshot.app_name, + user_id=active_snapshot.user_id, + session_id=active_snapshot.session_id, + session=active_snapshot.session, + state=active_snapshot.state, + memory=memory, + summary=summary, + sessions_by_alias=sessions_by_alias, + ) + + async def _build_session_service(self) -> BaseSessionService: + config = _make_session_config() + config.store_historical_events = self.case.store_historical_events + summarizer_manager = None + if self.case.enable_summary: + summarizer_manager = build_replay_summarizer_manager( + keep_recent_count=self.case.summary_keep_recent_count, + ) + return RedisSessionService( + db_url=self._redis_url, + summarizer_manager=summarizer_manager, + is_async=False, + session_config=config, + ) + + async def _build_memory_service(self) -> BaseMemoryService: + return RedisMemoryService( + db_url=self._redis_url, + enabled=True, + is_async=False, + memory_service_config=_make_memory_config(), + ) + + async def close(self) -> None: + await super().close() + self._logical_case = None + self._logical_session_identities = {} + + def _namespaced_case(self, case: ReplayCase) -> ReplayCase: + namespaced_steps = tuple( + replace( + step, + app_name=(f"{step.app_name}:{self._storage_namespace}" if step.app_name is not None else None), + user_id=(f"{step.user_id}:{self._storage_namespace}" if step.user_id is not None else None), + session_id=(f"{step.session_id}:{self._storage_namespace}" if step.session_id is not None else None), + ) + for step in case.steps + ) + return replace( + case, + app_name=f"{case.app_name}:{self._storage_namespace}", + user_id=f"{case.user_id}:{self._storage_namespace}", + session_id=f"{case.session_id}:{self._storage_namespace}", + steps=namespaced_steps, + ) + + def _project_identifier(self, value: Optional[str]) -> Optional[str]: + if value is None: + return None + return self._project_scalar(value) + + def _project_scalar(self, value: Any) -> Any: + if isinstance(value, dict): + return {key: self._project_scalar(item) for key, item in value.items()} + if isinstance(value, list): + return [self._project_scalar(item) for item in value] + if isinstance(value, tuple): + return [self._project_scalar(item) for item in value] + if isinstance(value, str): + for logical_identity in self._logical_session_identities.values(): + value = value.replace(f"{logical_identity['app_name']}:{self._storage_namespace}", logical_identity["app_name"]) + value = value.replace(f"{logical_identity['user_id']}:{self._storage_namespace}", logical_identity["user_id"]) + value = value.replace(f"{logical_identity['session_id']}:{self._storage_namespace}", logical_identity["session_id"]) + value = value.replace(self.case.app_name, self.logical_case.app_name) + value = value.replace(self.case.user_id, self.logical_case.user_id) + value = value.replace(self.case.session_id, self.logical_case.session_id) + return value + return value + + def _project_session_snapshot(self, session_alias: str, snapshot: SessionSnapshot) -> SessionSnapshot: + logical_identity = self._logical_session_identities.get(session_alias) + projected_summary = snapshot.summary + if projected_summary is not None: + projected_summary = replace( + projected_summary, + session_id=logical_identity["session_id"] if logical_identity is not None else self._project_scalar(projected_summary.session_id), + summary_id=self._project_identifier(projected_summary.summary_id), + replaces=self._project_identifier(projected_summary.replaces), + metadata=self._project_scalar(projected_summary.metadata), + ) + return replace( + snapshot, + app_name=logical_identity["app_name"] if logical_identity is not None else self._project_scalar(snapshot.app_name), + user_id=logical_identity["user_id"] if logical_identity is not None else self._project_scalar(snapshot.user_id), + session_id=logical_identity["session_id"] if logical_identity is not None else self._project_scalar(snapshot.session_id), + state=self._project_scalar(snapshot.state), + summary=projected_summary, + ) + + def _project_memory_observation(self, observation: Any) -> Any: + if not isinstance(observation, dict): + return self._project_scalar(observation) + projected = self._project_scalar(observation) + session_alias = projected.get("session_alias") + logical_identity = self._logical_session_identities.get(session_alias) + if logical_identity is not None: + projected["app_name"] = logical_identity["app_name"] + projected["user_id"] = logical_identity["user_id"] + projected["session_id"] = logical_identity["session_id"] + return projected + + def get_runtime_metadata(self) -> dict[str, Any]: + return { + "backend_name": self.name, + "storage_kind": "redis", + "connection": _summarize_connection_url(self._redis_url), + "storage_namespace": self._storage_namespace, + "storage_identity": { + "app_name": self.case.app_name, + "user_id": self.case.user_id, + "session_id": self.case.session_id, + }, + "logical_identity": { + "app_name": self.logical_case.app_name, + "user_id": self.logical_case.user_id, + "session_id": self.logical_case.session_id, + }, + } + + def get_report_metadata(self) -> dict[str, Any]: + return { + "backend_name": self.name, + "storage_kind": "redis", + "connection": _summarize_connection_url(self._redis_url), + "namespace_strategy": "per_case_random", + } + + +def build_event(spec: EventSpec, *, sequence: int, timestamp: float) -> Event: + """Materialize an Event from an EventSpec.""" + + parts: list[Part] = [] + if spec.text is not None: + parts.append(Part.from_text(text=spec.text)) + + for function_call in spec.function_calls: + parts.append(Part(function_call=_build_function_call(function_call))) + + for function_response in spec.function_responses: + parts.append(Part(function_response=_build_function_response(function_response))) + + content: Content | None = None + if parts: + content = Content(role=spec.role, parts=parts) + + event = Event( + id=spec.event_id or f"replay-event-{sequence}", + invocation_id=f"replay-invocation-{sequence}", + author=spec.author, + branch=spec.branch, + content=content, + visible=spec.visible, + partial=spec.partial, + timestamp=timestamp, + ) + if spec.state_delta: + event.actions.state_delta = dict(spec.state_delta) + if spec.is_summary_event: + event.set_summary_event(True) + return event + + +def _build_function_call(spec: FunctionCallSpec) -> FunctionCall: + return FunctionCall(name=spec.name, args=dict(spec.args), id=spec.call_id) + + +def _build_function_response(spec: FunctionResponseSpec) -> FunctionResponse: + return FunctionResponse(name=spec.name, response=dict(spec.response), id=spec.call_id) + + +def _event_to_snapshot(event: Event) -> dict[str, Any]: + return { + "author": event.author, + "branch": event.branch, + "visible": event.visible, + "partial": event.partial, + "error_code": event.error_code, + "error_message": event.error_message, + "text": event.get_text(), + "is_summary_event": event.is_summary_event(), + "model_visible": event.is_model_visible(), + "state_delta": dict(event.actions.state_delta or {}), + "function_calls": [ + { + "name": call.name, + "args": _normalize_scalar(call.args), + } + for call in event.get_function_calls() + ], + "function_responses": [ + { + "name": response.name, + "response": _normalize_scalar(response.response), + } + for response in event.get_function_responses() + ], + } + + +def _memory_entry_to_snapshot(entry: Any) -> dict[str, Any]: + parts = getattr(getattr(entry, "content", None), "parts", None) or [] + text = "".join(part.text for part in parts if getattr(part, "text", None)) + role = getattr(getattr(entry, "content", None), "role", None) + return { + "author": getattr(entry, "author", None), + "role": role, + "text": text, + "timestamp": getattr(entry, "timestamp", None), + } + + +def normalize_backend_snapshot(snapshot: BackendSnapshot) -> dict[str, Any]: + """Normalize backend noise before diffing.""" + + return { + "backend_name": snapshot.backend_name, + "case_id": snapshot.case_id, + "app_name": snapshot.app_name, + "user_id": snapshot.user_id, + "session_id": snapshot.session_id, + "active_session_alias": snapshot.active_session_alias, + "session": _normalize_scalar(snapshot.session), + "state": _normalize_scalar(snapshot.state), + "memory": _normalize_memory(snapshot.memory), + "summary": _normalize_summary(snapshot.summary), + "sessions_by_alias": _normalize_sessions_by_alias(snapshot.sessions_by_alias, snapshot.active_session_alias), + } + + +def _normalize_summary(summary: Optional[SummarySnapshot]) -> Optional[dict[str, Any]]: + if summary is None: + return None + return { + "session_id": summary.session_id, + "summary_text": summary.summary_text.strip(), + "original_event_count": summary.original_event_count, + "compressed_event_count": summary.compressed_event_count, + "summary_id": summary.summary_id, + "version": summary.version, + "replaces": summary.replaces, + "summarized_event_count": summary.summarized_event_count, + "summary_timestamp": _normalize_timestamp(summary.summary_timestamp), + "metadata": _normalize_scalar(summary.metadata), + } + + +def _normalize_memory(memory_results: dict[str, Any]) -> dict[str, Any]: + normalized: dict[str, Any] = {} + for observation_key, observation in memory_results.items(): + if isinstance(observation, dict): + entries = observation.get("entries", []) + normalized_observation = { + "query_name": observation.get("query_name"), + "session_alias": observation.get("session_alias"), + "app_name": observation.get("app_name"), + "user_id": observation.get("user_id"), + "session_id": observation.get("session_id"), + "step_index": observation.get("step_index"), + } + else: + entries = observation + normalized_observation = {} + normalized_entries = [] + for entry in entries: + normalized_entries.append( + { + "author": entry.get("author"), + "role": entry.get("role"), + "text": (entry.get("text") or "").strip(), + }) + normalized_observation["entries"] = sorted( + normalized_entries, + key=lambda item: (item["text"], item["author"] or "", item["role"] or ""), + ) + normalized[observation_key] = _normalize_scalar(normalized_observation) + return normalized + + +def _normalize_sessions_by_alias( + sessions_by_alias: dict[str, SessionSnapshot], + active_session_alias: str, +) -> dict[str, Any]: + normalized: dict[str, Any] = {} + for session_alias, snapshot in sessions_by_alias.items(): + if session_alias == active_session_alias: + continue + normalized[session_alias] = { + "session_alias": snapshot.session_alias, + "app_name": snapshot.app_name, + "user_id": snapshot.user_id, + "session_id": snapshot.session_id, + "session": _normalize_scalar(snapshot.session), + "state": _normalize_scalar(snapshot.state), + "summary": _normalize_summary(snapshot.summary), + } + return normalized + + +def _normalize_scalar(value: Any) -> Any: + if isinstance(value, dict): + return {key: _normalize_scalar(value[key]) for key in sorted(value)} + if isinstance(value, list): + return [_normalize_scalar(item) for item in value] + if isinstance(value, tuple): + return [_normalize_scalar(item) for item in value] + if isinstance(value, str): + return value.strip() + return value + + +def _normalize_timestamp(value: Optional[float]) -> Optional[float]: + if value is None: + return None + return round(float(value), 6) + + +def get_replay_clock_metadata() -> dict[str, Any]: + mode = (os.getenv(_REPLAY_CLOCK_MODE_ENV) or _REPLAY_CLOCK_MODE_FRESHNESS_SAFE).strip().lower() + if mode == _REPLAY_CLOCK_MODE_FIXED_SAFE: + configured_epoch = _parse_optional_float(os.getenv(_REPLAY_FIXED_EPOCH_ENV)) + effective_epoch = configured_epoch if configured_epoch is not None else _DEFAULT_FIXED_SAFE_EPOCH + return { + "mode": _REPLAY_CLOCK_MODE_FIXED_SAFE, + "future_skew_seconds": _CASE_TIME_FUTURE_SKEW_SECONDS, + "configured_fixed_epoch": configured_epoch, + "default_fixed_epoch": _DEFAULT_FIXED_SAFE_EPOCH, + "effective_fixed_epoch": effective_epoch, + "freshness_safe": True, + } + return { + "mode": _REPLAY_CLOCK_MODE_FRESHNESS_SAFE, + "future_skew_seconds": _CASE_TIME_FUTURE_SKEW_SECONDS, + "freshness_safe": True, + } + + +def _parse_optional_float(raw: Optional[str]) -> Optional[float]: + if raw is None or not raw.strip(): + return None + return float(raw.strip()) + + +def _summarize_connection_url(url: str) -> dict[str, Any]: + parsed = urlparse(url) + database = parsed.path.lstrip("/") or None + if database and database.isdigit(): + database = int(database) + return { + "scheme": parsed.scheme, + "host": parsed.hostname, + "port": parsed.port, + "database": database, + "has_auth": bool(parsed.username or parsed.password), + } + + +def _collect_logical_session_identities(case: ReplayCase) -> dict[str, dict[str, str]]: + identities: dict[str, dict[str, str]] = {} + for step in case.steps: + if step.kind != ReplayStepKind.CREATE_SESSION: + continue + identities[step.session_alias] = { + "app_name": step.app_name or case.app_name, + "user_id": step.user_id or case.user_id, + "session_id": step.session_id or case.session_id, + } + return identities + + +def _backend_target_matches(target_name: str, backend_name: str) -> bool: + normalized_target = target_name.strip().lower() + normalized_backend = backend_name.strip().lower() + if normalized_target == normalized_backend: + return True + if normalized_target in _PERSISTENT_BACKEND_TARGETS: + return normalized_backend != _BASELINE_BACKEND_NAME + if normalized_target in _BASELINE_BACKEND_TARGETS: + return normalized_backend == _BASELINE_BACKEND_NAME + return False + + +def expected_diff_paths_for_backend_pair( + case: ReplayCase, + *, + backend_a: str, + backend_b: str, +) -> tuple[str, ...]: + if not case.expected_diff_paths: + return () + + for target_name in [mutation.backend_name for mutation in case.snapshot_mutations] + [ + fault.backend_name for fault in case.runtime_faults + ]: + if _backend_target_matches(target_name, backend_a) or _backend_target_matches(target_name, backend_b): + return case.expected_diff_paths + return () + + +def diff_backend_snapshots( + *, + case: ReplayCase, + left: BackendSnapshot, + right: BackendSnapshot, +) -> list[DiffEntry]: + """Generate structured diffs for two snapshots.""" + + left_view = normalize_backend_snapshot(left) + right_view = normalize_backend_snapshot(right) + _apply_snapshot_mutations(left_view, case.snapshot_mutations, left.backend_name) + _apply_snapshot_mutations(right_view, case.snapshot_mutations, right.backend_name) + diffs: list[DiffEntry] = [] + + for scope in ("session", "state", "memory", "summary"): + scope_summary_id = _resolve_summary_id(left_view.get(scope), right_view.get(scope)) if scope == "summary" else None + _diff_values( + case=case, + backend_a=left.backend_name, + backend_b=right.backend_name, + scope=scope, + path=scope, + left=left_view[scope], + right=right_view[scope], + out=diffs, + session_id=_resolve_session_id(left_view, right_view, case.session_id), + summary_id=scope_summary_id, + ) + left_aliases = left_view.get("sessions_by_alias", {}) + right_aliases = right_view.get("sessions_by_alias", {}) + for session_alias in sorted(set(left_aliases) | set(right_aliases)): + left_alias_snapshot = left_aliases.get(session_alias) + right_alias_snapshot = right_aliases.get(session_alias) + _diff_values( + case=case, + backend_a=left.backend_name, + backend_b=right.backend_name, + scope="sessions_by_alias", + path=f"sessions_by_alias.{session_alias}", + left=left_alias_snapshot, + right=right_alias_snapshot, + out=diffs, + session_id=_resolve_value_session_id(left_alias_snapshot, right_alias_snapshot, case.session_id), + summary_id=_resolve_summary_id( + left_alias_snapshot.get("summary") if isinstance(left_alias_snapshot, dict) else None, + right_alias_snapshot.get("summary") if isinstance(right_alias_snapshot, dict) else None, + ), + ) + return diffs + + +def _apply_snapshot_mutations( + snapshot_view: dict[str, Any], + mutations: tuple[SnapshotMutation, ...], + backend_name: str, +) -> None: + for mutation in mutations: + if not _backend_target_matches(mutation.backend_name, backend_name): + continue + _apply_snapshot_mutation(snapshot_view, mutation) + + +def _apply_snapshot_mutation(snapshot_view: dict[str, Any], mutation: SnapshotMutation) -> None: + tokens = _parse_path_tokens(mutation.path) + if not tokens: + raise ValueError(f"Invalid mutation path: {mutation.path}") + + parent, last_token = _resolve_parent(snapshot_view, tokens) + if mutation.operation == SnapshotMutationOperation.SET: + _set_token(parent, last_token, mutation.value) + return + if mutation.operation == SnapshotMutationOperation.DELETE: + _delete_token(parent, last_token) + return + raise ValueError(f"Unsupported snapshot mutation operation: {mutation.operation}") + + +def _parse_path_tokens(path: str) -> list[Any]: + tokens: list[Any] = [] + for part in path.split("."): + if not part: + continue + cursor = part + while cursor: + match = re.match(r"^([^\[]+)(\[(\d+)\])?(.*)$", cursor) + if not match: + raise ValueError(f"Invalid path segment: {cursor}") + key, _, index, rest = match.groups() + if key: + tokens.append(key) + if index is not None: + tokens.append(int(index)) + cursor = rest + return tokens + + +def _resolve_parent(root: dict[str, Any], tokens: list[Any]) -> tuple[Any, Any]: + target = root + for token in tokens[:-1]: + target = _get_token(target, token) + return target, tokens[-1] + + +def _set_token(container: Any, token: Any, value: Any) -> None: + if isinstance(token, int): + container[token] = value + elif isinstance(container, dict): + container[token] = value + else: + setattr(container, token, value) + + +def _delete_token(container: Any, token: Any) -> None: + if isinstance(token, int): + del container[token] + elif isinstance(container, dict): + container.pop(token, None) + else: + delattr(container, token) + + +def _get_token(container: Any, token: Any) -> Any: + if isinstance(token, int): + return container[token] + if isinstance(container, dict): + return container[token] + return getattr(container, token) + + +def _set_path_value(root: Any, path: str, value: Any) -> None: + tokens = _parse_path_tokens(path) + if not tokens: + raise ValueError(f"Invalid mutation path: {path}") + parent, last_token = _resolve_parent(root, tokens) + _set_token(parent, last_token, value) + + +def _diff_values( + *, + case: ReplayCase, + backend_a: str, + backend_b: str, + scope: str, + path: str, + left: Any, + right: Any, + out: list[DiffEntry], + session_id: str, + summary_id: Optional[str], +) -> None: + if type(left) is not type(right): + out.append(_make_diff(case, backend_a, backend_b, scope, path, left, right, session_id, summary_id)) + return + + if isinstance(left, dict): + for key in sorted(set(left) | set(right)): + next_path = f"{path}.{key}" + if key not in left or key not in right: + out.append( + _make_diff( + case, + backend_a, + backend_b, + scope, + next_path, + left.get(key), + right.get(key), + session_id, + summary_id, + )) + continue + _diff_values( + case=case, + backend_a=backend_a, + backend_b=backend_b, + scope=scope, + path=next_path, + left=left[key], + right=right[key], + out=out, + session_id=session_id, + summary_id=summary_id, + ) + return + + if isinstance(left, list): + if len(left) != len(right): + out.append(_make_diff(case, backend_a, backend_b, scope, f"{path}.length", len(left), len(right), + session_id, summary_id)) + return + for index, (left_item, right_item) in enumerate(zip(left, right)): + _diff_values( + case=case, + backend_a=backend_a, + backend_b=backend_b, + scope=scope, + path=f"{path}[{index}]", + left=left_item, + right=right_item, + out=out, + session_id=session_id, + summary_id=summary_id, + ) + return + + if left != right: + out.append(_make_diff(case, backend_a, backend_b, scope, path, left, right, session_id, summary_id)) + + +def _make_diff( + case: ReplayCase, + backend_a: str, + backend_b: str, + scope: str, + path: str, + left: Any, + right: Any, + session_id: str, + summary_id: Optional[str], +) -> DiffEntry: + allowed = path in set(case.allowed_diff_paths) + event_index = _extract_event_index(path) + reason = "Allowed backend-specific difference." if allowed else None + return DiffEntry( + case_id=case.case_id, + backend_a=backend_a, + backend_b=backend_b, + scope=scope, + path=path, + left=left, + right=right, + allowed=allowed, + session_id=session_id, + event_index=event_index, + summary_id=summary_id, + reason=reason, + ) + + +def _resolve_summary_id(left: Any, right: Any) -> Optional[str]: + for value in (left, right): + if isinstance(value, dict): + summary_id = value.get("summary_id") + if summary_id is not None: + return str(summary_id) + return None + + +def _resolve_session_id(left: dict[str, Any], right: dict[str, Any], fallback: str) -> str: + left_session_id = left.get("session_id") + right_session_id = right.get("session_id") + if isinstance(left_session_id, str) and left_session_id == right_session_id: + return left_session_id + return fallback + + +def _resolve_value_session_id(left: Any, right: Any, fallback: str) -> str: + for value in (left, right): + if isinstance(value, dict): + session_id = value.get("session_id") + if isinstance(session_id, str): + return session_id + return fallback + + +def _deterministic_time_base(case_id: str) -> float: + if case_id not in _CASE_TIME_BASES: + digest = hashlib.sha1(case_id.encode("utf-8")).digest() + offset_millis = int.from_bytes(digest[:6], "big") % 1_000 + clock_metadata = get_replay_clock_metadata() + if clock_metadata["mode"] == _REPLAY_CLOCK_MODE_FIXED_SAFE: + base_epoch = float(clock_metadata["effective_fixed_epoch"]) + else: + base_epoch = round(time.time(), 3) + _CASE_TIME_FUTURE_SKEW_SECONDS + _CASE_TIME_BASES[case_id] = base_epoch + (offset_millis / 1000.0) + return _CASE_TIME_BASES[case_id] + + +@contextmanager +def _patched_time_time(timestamp: float) -> Iterator[None]: + original_time = time.time + time.time = lambda: timestamp + try: + yield + finally: + time.time = original_time + + +def _get_summary_event(session: Session) -> Optional[Event]: + for event in session.events: + if event.is_summary_event(): + return event + return None + + +def _extract_event_index(path: str) -> Optional[int]: + match = _EVENT_INDEX_RE.search(path) + if not match: + return None + return int(match.group(1)) + + +def format_diffs(diffs: list[DiffEntry]) -> str: + """Render human-readable diffs for assertion failures.""" + + if not diffs: + return "No differences detected." + lines = [] + for diff in diffs: + prefix = "ALLOWED" if diff.allowed else "DIFF" + lines.append(f"{prefix} {diff.path}: {diff.left!r} != {diff.right!r}") + return "\n".join(lines) + + +def build_case_report(case: ReplayCase, diffs: list[DiffEntry]) -> dict[str, Any]: + """Build a grouped report for one replay case.""" + + expected_paths = set(case.expected_diff_paths) + allowed_paths = set(case.allowed_diff_paths) + detected_paths = {diff.path for diff in diffs if not diff.allowed} + return { + "case_id": case.case_id, + "description": case.description, + "expects_diffs": bool(expected_paths), + "expected_diff_paths": sorted(expected_paths), + "allowed_diff_paths": sorted(allowed_paths), + "detected_diff_paths": sorted(detected_paths), + "missing_expected_paths": sorted(expected_paths - detected_paths), + "unexpected_diff_paths": sorted(detected_paths - expected_paths), + "diffs": [diff.to_dict() for diff in diffs], + } + + +def build_comparison_report( + case: ReplayCase, + *, + backend_a: str, + backend_b: str, + diffs: list[DiffEntry], + runtime_context: Optional[dict[str, Any]] = None, +) -> dict[str, Any]: + expected_paths = set(expected_diff_paths_for_backend_pair(case, backend_a=backend_a, backend_b=backend_b)) + allowed_paths = set(case.allowed_diff_paths) + detected_paths = {diff.path for diff in diffs if not diff.allowed} + return { + "backend_a": backend_a, + "backend_b": backend_b, + "expected_diff_paths": sorted(expected_paths), + "allowed_diff_paths": sorted(allowed_paths), + "detected_diff_paths": sorted(detected_paths), + "missing_expected_paths": sorted(expected_paths - detected_paths), + "unexpected_diff_paths": sorted(detected_paths - expected_paths), + "runtime_context": runtime_context or {}, + "diffs": [diff.to_dict() for diff in diffs], + } + + +def build_case_matrix_report(case: ReplayCase, comparisons: list[dict[str, Any]]) -> dict[str, Any]: + expected_paths = set(case.expected_diff_paths) + allowed_paths = set(case.allowed_diff_paths) + detected_paths = { + path + for comparison in comparisons + for path in comparison.get("detected_diff_paths", []) + } + all_diffs = [ + diff + for comparison in comparisons + for diff in comparison.get("diffs", []) + ] + return { + "case_id": case.case_id, + "description": case.description, + "expects_diffs": bool(expected_paths), + "expected_diff_paths": sorted(expected_paths), + "allowed_diff_paths": sorted(allowed_paths), + "detected_diff_paths": sorted(detected_paths), + "missing_expected_paths": sorted(expected_paths - detected_paths), + "unexpected_diff_paths": sorted(detected_paths - expected_paths), + "comparison_count": len(comparisons), + "comparisons": comparisons, + "diffs": all_diffs, + } + + +def write_diff_report( + report_path: Path, + case_reports: list[dict[str, Any]], + metadata: Optional[dict[str, Any]] = None, +) -> None: + """Write grouped replay reports to JSON.""" + + payload = { + "meta": metadata or {}, + "cases": case_reports, + } + report_path.write_text(json.dumps(payload, indent=2, ensure_ascii=False) + "\n", encoding="utf-8") diff --git a/tests/sessions/replay_models.py b/tests/sessions/replay_models.py new file mode 100644 index 00000000..f8e18a8c --- /dev/null +++ b/tests/sessions/replay_models.py @@ -0,0 +1,294 @@ +"""Typed models for session/memory replay consistency tests. + +The replay harness uses a small protocol instead of hard-coding backend calls +inside each test. This keeps the tests readable and makes it easy to add new +backends or new replay cases over time. +""" + +from __future__ import annotations + +from dataclasses import asdict +from dataclasses import dataclass +from dataclasses import field +from enum import Enum +from typing import Any +from typing import Optional + + +class ReplayStepKind(str, Enum): + """Supported operations in a replay case.""" + + CREATE_SESSION = "create_session" + APPEND_EVENT = "append_event" + APPEND_STATE = "append_state" + STORE_MEMORY = "store_memory" + SEARCH_MEMORY = "search_memory" + CREATE_SUMMARY = "create_summary" + RESTART_SERVICES = "restart_services" + + +class SnapshotMutationOperation(str, Enum): + """Supported snapshot mutation operations for negative cases.""" + + SET = "set" + DELETE = "delete" + + +class RuntimeFaultOperation(str, Enum): + """Supported runtime fault injections for negative replay cases.""" + + DUPLICATE_LAST_EVENT = "duplicate_last_event" + DROP_LAST_EVENT_KEEP_STATE = "drop_last_event_keep_state" + SET_SESSION_VALUE = "set_session_value" + DELETE_SUMMARY = "delete_summary" + SET_SUMMARY_VALUE = "set_summary_value" + + +@dataclass(frozen=True) +class FunctionCallSpec: + """Declarative function call content for an event.""" + + name: str + args: dict[str, Any] = field(default_factory=dict) + call_id: Optional[str] = None + + +@dataclass(frozen=True) +class FunctionResponseSpec: + """Declarative function response content for an event.""" + + name: str + response: dict[str, Any] = field(default_factory=dict) + call_id: Optional[str] = None + + +@dataclass(frozen=True) +class EventSpec: + """Business-level event description used by replay steps.""" + + author: str + text: Optional[str] = None + role: Optional[str] = None + state_delta: dict[str, Any] = field(default_factory=dict) + function_calls: tuple[FunctionCallSpec, ...] = field(default_factory=tuple) + function_responses: tuple[FunctionResponseSpec, ...] = field(default_factory=tuple) + branch: Optional[str] = None + visible: bool = True + partial: bool = False + is_summary_event: bool = False + event_id: Optional[str] = None + + +@dataclass(frozen=True) +class MemoryQuerySpec: + """Search query to run after replaying a case.""" + + name: str + query: str + limit: int = 10 + + +@dataclass(frozen=True) +class ReplayStep: + """One replay operation.""" + + kind: ReplayStepKind + event: Optional[EventSpec] = None + initial_state: dict[str, Any] = field(default_factory=dict) + memory_query: Optional[MemoryQuerySpec] = None + force_summary: bool = False + session_alias: str = "default" + session_id: Optional[str] = None + app_name: Optional[str] = None + user_id: Optional[str] = None + + @classmethod + def create_session( + cls, + *, + initial_state: Optional[dict[str, Any]] = None, + session_alias: str = "default", + session_id: Optional[str] = None, + app_name: Optional[str] = None, + user_id: Optional[str] = None, + ) -> "ReplayStep": + return cls( + kind=ReplayStepKind.CREATE_SESSION, + initial_state=initial_state or {}, + session_alias=session_alias, + session_id=session_id, + app_name=app_name, + user_id=user_id, + ) + + @classmethod + def append_event(cls, event: EventSpec, *, session_alias: str = "default") -> "ReplayStep": + return cls(kind=ReplayStepKind.APPEND_EVENT, event=event, session_alias=session_alias) + + @classmethod + def append_state( + cls, + *, + author: str, + state_delta: dict[str, Any], + session_alias: str = "default", + ) -> "ReplayStep": + return cls( + kind=ReplayStepKind.APPEND_STATE, + event=EventSpec(author=author, state_delta=state_delta), + session_alias=session_alias, + ) + + @classmethod + def store_memory(cls, *, session_alias: str = "default") -> "ReplayStep": + return cls(kind=ReplayStepKind.STORE_MEMORY, session_alias=session_alias) + + @classmethod + def search_memory( + cls, + *, + name: str, + query: str, + limit: int = 10, + session_alias: str = "default", + ) -> "ReplayStep": + return cls( + kind=ReplayStepKind.SEARCH_MEMORY, + memory_query=MemoryQuerySpec(name=name, query=query, limit=limit), + session_alias=session_alias, + ) + + @classmethod + def create_summary(cls, *, force: bool = False, session_alias: str = "default") -> "ReplayStep": + return cls(kind=ReplayStepKind.CREATE_SUMMARY, force_summary=force, session_alias=session_alias) + + @classmethod + def restart_services(cls) -> "ReplayStep": + return cls(kind=ReplayStepKind.RESTART_SERVICES) + + +@dataclass(frozen=True) +class ReplayCase: + """A deterministic replay scenario.""" + + case_id: str + description: str + app_name: str = "replay_app" + user_id: str = "replay_user" + session_id: str = "replay_session" + enable_summary: bool = False + summary_keep_recent_count: int = 2 + store_historical_events: bool = False + steps: tuple[ReplayStep, ...] = field(default_factory=tuple) + allowed_diff_paths: tuple[str, ...] = field(default_factory=tuple) + expected_diff_paths: tuple[str, ...] = field(default_factory=tuple) + snapshot_mutations: tuple["SnapshotMutation", ...] = field(default_factory=tuple) + runtime_faults: tuple["RuntimeFault", ...] = field(default_factory=tuple) + + +@dataclass(frozen=True) +class SnapshotMutation: + """A deterministic post-replay mutation applied to one backend snapshot.""" + + backend_name: str + path: str + operation: SnapshotMutationOperation = SnapshotMutationOperation.SET + value: Any = None + + +@dataclass(frozen=True) +class RuntimeFault: + """A deterministic fault injected during replay execution.""" + + backend_name: str + after_step: int + operation: RuntimeFaultOperation + session_alias: str = "default" + path: Optional[str] = None + value: Any = None + + +@dataclass(frozen=True) +class SummarySnapshot: + """Comparable summary view extracted from a backend.""" + + session_id: str + summary_text: str + original_event_count: int + compressed_event_count: int + summary_id: Optional[str] = None + version: Optional[int] = None + replaces: Optional[str] = None + summarized_event_count: Optional[int] = None + summary_timestamp: Optional[float] = None + metadata: dict[str, Any] = field(default_factory=dict) + + def to_dict(self) -> dict[str, Any]: + return asdict(self) + + +@dataclass(frozen=True) +class SessionSnapshot: + """Comparable view of one resolved session alias.""" + + session_alias: str + app_name: str + user_id: str + session_id: str + session: dict[str, Any] + state: dict[str, Any] + summary: Optional[SummarySnapshot] + + def to_dict(self) -> dict[str, Any]: + data = asdict(self) + if self.summary is not None: + data["summary"] = self.summary.to_dict() + return data + + +@dataclass(frozen=True) +class BackendSnapshot: + """Full business snapshot collected after replaying one case.""" + + backend_name: str + case_id: str + app_name: str + user_id: str + session_id: str + active_session_alias: str + session: dict[str, Any] + state: dict[str, Any] + memory: dict[str, Any] + summary: Optional[SummarySnapshot] + sessions_by_alias: dict[str, SessionSnapshot] = field(default_factory=dict) + + def to_dict(self) -> dict[str, Any]: + data = asdict(self) + if self.summary is not None: + data["summary"] = self.summary.to_dict() + data["sessions_by_alias"] = { + alias: snapshot.to_dict() + for alias, snapshot in self.sessions_by_alias.items() + } + return data + + +@dataclass(frozen=True) +class DiffEntry: + """One structured difference between two backend snapshots.""" + + case_id: str + backend_a: str + backend_b: str + scope: str + path: str + left: Any + right: Any + allowed: bool + session_id: Optional[str] = None + event_index: Optional[int] = None + summary_id: Optional[str] = None + reason: Optional[str] = None + + def to_dict(self) -> dict[str, Any]: + return asdict(self) diff --git a/tests/sessions/replay_summary.py b/tests/sessions/replay_summary.py new file mode 100644 index 00000000..950ec269 --- /dev/null +++ b/tests/sessions/replay_summary.py @@ -0,0 +1,55 @@ +"""Deterministic summary helpers for replay tests. + +The replay harness must not depend on a real LLM. This module provides a tiny +deterministic summarizer that reuses the framework's session compaction logic +while producing stable summary text and metadata. +""" + +from __future__ import annotations + +import re + +from trpc_agent_sdk.sessions import Session +from trpc_agent_sdk.sessions import SessionSummarizer +from trpc_agent_sdk.sessions import SummarizerSessionManager + + +_WHITESPACE_RE = re.compile(r"\s+") + + +class _ReplaySummaryModel: + """Minimal model-like object used only for summary metadata.""" + + name = "replay-summary-model" + + +class DeterministicSessionSummarizer(SessionSummarizer): + """A SessionSummarizer that produces deterministic text without LLM calls.""" + + def __init__(self, *, keep_recent_count: int = 2, start_by_user_turn: bool = True) -> None: + super().__init__( + model=_ReplaySummaryModel(), # type: ignore[arg-type] + keep_recent_count=keep_recent_count, + start_by_user_turn=start_by_user_turn, + ) + self._minimum_events = max(keep_recent_count + 1, 2) + + async def should_summarize(self, session: Session) -> bool: + return len(session.events) >= self._minimum_events + + async def _generate_summary(self, conversation_text: str, ctx=None) -> str: + normalized = _WHITESPACE_RE.sub(" ", conversation_text).strip() + if not normalized: + return "" + return f"deterministic-summary[{normalized}]" + + +def build_replay_summarizer_manager(*, keep_recent_count: int) -> SummarizerSessionManager: + """Create a deterministic summarizer manager for replay tests.""" + + summarizer = DeterministicSessionSummarizer(keep_recent_count=keep_recent_count) + return SummarizerSessionManager( + model=summarizer.model, + summarizer=summarizer, + auto_summarize=True, + ) diff --git a/tests/sessions/test_redis_session_service.py b/tests/sessions/test_redis_session_service.py index 8269b186..9aaace3c 100644 --- a/tests/sessions/test_redis_session_service.py +++ b/tests/sessions/test_redis_session_service.py @@ -83,9 +83,13 @@ async def execute_command(self, session, command): return [k for k in self._store.keys() if k.startswith(prefix)] elif method == 'hset': key = args[0] - pairs = args[1:] if key not in self._hash_store: self._hash_store[key] = {} + mapping = getattr(command, "kwargs", {}).get("mapping", {}) + if mapping: + self._hash_store[key].update(mapping) + return True + pairs = args[1:] for i in range(0, len(pairs), 2): self._hash_store[key][pairs[i]] = pairs[i + 1] return True diff --git a/tests/sessions/test_replay_consistency.py b/tests/sessions/test_replay_consistency.py new file mode 100644 index 00000000..d6b07e56 --- /dev/null +++ b/tests/sessions/test_replay_consistency.py @@ -0,0 +1,261 @@ +"""Replay consistency smoke tests across session and memory backends.""" + +from __future__ import annotations + +import asyncio +import os +from time import perf_counter +from typing import Callable +from typing import Type + +import pytest + +from .replay_cases import REPLAY_ACCEPTANCE_CASES +from .replay_cases import REPLAY_ALL_CASES +from .replay_cases import REPLAY_EXTRA_CASES +from .replay_cases import REPLAY_TARGETED_CASES +from .replay_harness import DEFAULT_REPORT_PATH +from .replay_harness import build_case_matrix_report +from .replay_harness import InMemoryReplayAdapter +from .replay_harness import RedisReplayAdapter +from .replay_harness import ReplayBackendAdapter +from .replay_harness import SqliteReplayAdapter +from .replay_harness import build_comparison_report +from .replay_harness import diff_backend_snapshots +from .replay_harness import expected_diff_paths_for_backend_pair +from .replay_harness import format_diffs +from .replay_harness import get_replay_clock_metadata +from .replay_harness import write_diff_report +from .replay_models import BackendSnapshot +from .replay_models import DiffEntry +from .replay_models import ReplayCase + + +ADAPTER_TYPES: tuple[Type[ReplayBackendAdapter], ...] = ( + InMemoryReplayAdapter, + SqliteReplayAdapter, +) +REDIS_REPLAY_URL_ENV = "TRPC_AGENT_REPLAY_REDIS_URL" +AdapterFactory = Callable[[], ReplayBackendAdapter] + + +def _find_case(case_id: str) -> ReplayCase: + for case in REPLAY_ALL_CASES + REPLAY_TARGETED_CASES: + if case.case_id == case_id: + return case + raise KeyError(f"Unknown replay case: {case_id}") + + +async def _run_case_on_backend( + adapter_factory: AdapterFactory, + case: ReplayCase, +) -> tuple[BackendSnapshot, dict[str, object], dict[str, object]]: + adapter = adapter_factory() + await adapter.setup(case) + try: + snapshot = await adapter.run_case(case) + return snapshot, adapter.get_runtime_metadata(), adapter.get_report_metadata() + finally: + await adapter.close() + + +async def _run_replay_cases( + adapter_factories: tuple[AdapterFactory, ...], + *, + mode_name: str, + write_report: bool = True, +) -> tuple[list[DiffEntry], list[dict[str, object]], float]: + all_diffs: list[DiffEntry] = [] + case_reports: list[dict[str, object]] = [] + backend_report_metadata: dict[str, dict[str, object]] = {} + start_time = perf_counter() + + for case in REPLAY_ALL_CASES: + backend_runs = [await _run_case_on_backend(adapter_factory, case) for adapter_factory in adapter_factories] + snapshots = [run[0] for run in backend_runs] + runtime_metadata = { + snapshot.backend_name: runtime_info + for snapshot, runtime_info, _ in backend_runs + } + for snapshot, _, report_metadata in backend_runs: + backend_report_metadata.setdefault(snapshot.backend_name, report_metadata) + baseline_snapshot = snapshots[0] + comparisons: list[dict[str, object]] = [] + for other_snapshot in snapshots[1:]: + diffs = diff_backend_snapshots(case=case, left=baseline_snapshot, right=other_snapshot) + all_diffs.extend(diffs) + comparisons.append( + build_comparison_report( + case, + backend_a=baseline_snapshot.backend_name, + backend_b=other_snapshot.backend_name, + diffs=diffs, + runtime_context={ + baseline_snapshot.backend_name: runtime_metadata[baseline_snapshot.backend_name], + other_snapshot.backend_name: runtime_metadata[other_snapshot.backend_name], + }, + )) + case_reports.append(build_case_matrix_report(case, comparisons)) + + elapsed_seconds = perf_counter() - start_time + if write_report: + write_diff_report( + DEFAULT_REPORT_PATH, + case_reports, + metadata={ + "mode": mode_name, + "elapsed_seconds": round(elapsed_seconds, 3), + "backend_names": [factory.name for factory in adapter_factories], + "baseline_backend": adapter_factories[0].name, + "comparison_mode": "baseline_vs_all", + "backend_summaries": [backend_report_metadata[name] for name in sorted(backend_report_metadata)], + "clock_strategy": get_replay_clock_metadata(), + "acceptance_case_count": len(REPLAY_ACCEPTANCE_CASES), + "all_case_count": len(REPLAY_ALL_CASES), + }, + ) + return all_diffs, case_reports, elapsed_seconds + + +def _make_redis_adapter_factory() -> AdapterFactory: + redis_url = os.getenv(REDIS_REPLAY_URL_ENV) + if not redis_url: + raise RuntimeError("Redis replay URL is not configured.") + + class _ConfiguredRedisReplayAdapter(RedisReplayAdapter): + name = "redis" + + def __init__(self) -> None: + super().__init__(redis_url=redis_url) + + return _ConfiguredRedisReplayAdapter + + +def _assert_case_expectations( + all_diffs: list[DiffEntry], + case_set: tuple[ReplayCase, ...], + case_reports: list[dict[str, object]], +) -> None: + case_map = {case.case_id: case for case in case_set} + report_map = {str(report["case_id"]): report for report in case_reports} + + for case_id, case in case_map.items(): + case_report = report_map[case_id] + comparisons = case_report.get("comparisons", []) + for comparison in comparisons: + backend_a = str(comparison["backend_a"]) + backend_b = str(comparison["backend_b"]) + expected_paths = set(expected_diff_paths_for_backend_pair(case, backend_a=backend_a, backend_b=backend_b)) + case_diffs = [ + diff + for diff in all_diffs + if diff.case_id == case_id + and diff.backend_a == backend_a + and diff.backend_b == backend_b + and not diff.allowed + ] + detected_paths = {diff.path for diff in case_diffs} + + if expected_paths: + missing_paths = sorted(expected_paths - detected_paths) + unexpected_paths = sorted(detected_paths - expected_paths) + assert not missing_paths, ( + f"{case_id} missing expected diffs for {backend_a} vs {backend_b}: {missing_paths}" + ) + assert not unexpected_paths, ( + f"{case_id} produced unexpected diffs for {backend_a} vs {backend_b}: {unexpected_paths}\n" + f"{format_diffs(case_diffs)}" + ) + continue + + assert not case_diffs, ( + f"{case_id} produced unexpected diffs for {backend_a} vs {backend_b}:\n" + f"{format_diffs(case_diffs)}" + ) + + +def test_replay_consistency_smoke_cases() -> None: + """Ensure acceptance and extended replay cases behave as expected.""" + + all_diffs, case_reports, elapsed_seconds = asyncio.run(_run_replay_cases(ADAPTER_TYPES, mode_name="lightweight")) + _assert_case_expectations(all_diffs, REPLAY_ALL_CASES, case_reports) + + assert elapsed_seconds <= 30.0, f"lightweight replay mode exceeded 30s: {elapsed_seconds:.3f}s" + + +def test_acceptance_case_count() -> None: + """Keep the public acceptance suite fixed at 10 cases.""" + + assert len(REPLAY_ACCEPTANCE_CASES) == 10 + assert len(REPLAY_ALL_CASES) >= len(REPLAY_ACCEPTANCE_CASES) + assert len(REPLAY_EXTRA_CASES) >= 1 + + +def test_replay_harness_collects_all_session_alias_snapshots() -> None: + """Non-active sessions should remain visible in the final snapshot.""" + + snapshot, _, _ = asyncio.run( + _run_case_on_backend(InMemoryReplayAdapter, _find_case("cross_session_memory_aggregation")) + ) + + assert snapshot.active_session_alias == "default" + assert set(snapshot.sessions_by_alias) == {"source", "default"} + assert snapshot.sessions_by_alias["source"].session_id == "replay-memory-source" + assert snapshot.sessions_by_alias["source"].session["events"][0]["text"] == "Please remember that I prefer oolong tea." + assert snapshot.sessions_by_alias["default"].session_id == "replay-memory-target" + + +def test_replay_harness_preserves_memory_query_observations_across_restart() -> None: + """Repeated query names should preserve separate observations before and after restart.""" + + snapshot, _, _ = asyncio.run( + _run_case_on_backend(SqliteReplayAdapter, _find_case("memory_query_observation_survives_restart")) + ) + + observations = sorted(snapshot.memory.values(), key=lambda item: item["step_index"]) + assert [item["query_name"] for item in observations] == ["tea_preference", "tea_preference"] + assert [item["session_alias"] for item in observations] == ["default", "default"] + assert len(observations) == 2 + first_texts = {entry["text"] for entry in observations[0]["entries"]} + second_texts = {entry["text"] for entry in observations[1]["entries"]} + assert "Please remember that my favorite tea is oolong." in first_texts + assert "I will remember your oolong preference." in first_texts + assert "Also remember that I enjoy jasmine tea." in second_texts + assert "I will remember the jasmine preference too." in second_texts + assert "Also remember that I enjoy jasmine tea." not in first_texts + + +def test_replay_harness_keeps_duplicate_query_names_per_session_alias() -> None: + """Query names may repeat across aliases without overwriting previous observations.""" + + snapshot, _, _ = asyncio.run( + _run_case_on_backend(SqliteReplayAdapter, _find_case("duplicate_memory_query_name_across_sessions")) + ) + + observations = sorted(snapshot.memory.values(), key=lambda item: item["step_index"]) + assert len(observations) == 2 + assert [item["query_name"] for item in observations] == ["shared_preference_search", "shared_preference_search"] + assert [item["session_alias"] for item in observations] == ["source", "default"] + assert observations[0]["step_index"] < observations[1]["step_index"] + assert observations[0]["entries"] != observations[1]["entries"] + first_texts = {entry["text"] for entry in observations[0]["entries"]} + second_texts = {entry["text"] for entry in observations[1]["entries"]} + assert any("dragon well" in text.lower() for text in first_texts) + assert any("dragon well" in text.lower() for text in second_texts) + + +def test_replay_consistency_redis_integration_mode() -> None: + """Run an optional Redis-backed integration comparison when configured.""" + + redis_url = os.getenv(REDIS_REPLAY_URL_ENV) + if not redis_url: + pytest.skip(f"{REDIS_REPLAY_URL_ENV} is not set") + + redis_adapter_factory = _make_redis_adapter_factory() + all_diffs, case_reports, _ = asyncio.run( + _run_replay_cases( + (InMemoryReplayAdapter, SqliteReplayAdapter, redis_adapter_factory), + mode_name="integration", + write_report=True, + )) + _assert_case_expectations(all_diffs, REPLAY_ALL_CASES, case_reports) diff --git a/trpc_agent_sdk/sessions/_in_memory_session_service.py b/trpc_agent_sdk/sessions/_in_memory_session_service.py index 567a52d1..154f056f 100644 --- a/trpc_agent_sdk/sessions/_in_memory_session_service.py +++ b/trpc_agent_sdk/sessions/_in_memory_session_service.py @@ -475,6 +475,10 @@ def _set_session(self, app_name: str, user_id: str, session_id: str, session: Se if user_id not in self._sessions[app_name]: self._sessions[app_name][user_id] = {} + # Storage should own its own session object. Reusing the caller's + # instance can create aliasing after update_session(), which makes + # later append_event() mutate the same object twice. + session = session.model_copy(deep=True) if not self._session_config.store_historical_events: session = session.model_copy(update={"historical_events": []}) diff --git a/trpc_agent_sdk/sessions/_redis_session_service.py b/trpc_agent_sdk/sessions/_redis_session_service.py index 8751a75f..63ecd1eb 100644 --- a/trpc_agent_sdk/sessions/_redis_session_service.py +++ b/trpc_agent_sdk/sessions/_redis_session_service.py @@ -36,6 +36,20 @@ from ._utils import user_state_key +def _decode_redis_hash(raw_state: Optional[dict[Any, Any]]) -> dict[str, Any]: + """Normalize Redis hash responses to plain Python strings when possible.""" + + if not raw_state: + return {} + + normalized: dict[str, Any] = {} + for raw_key, raw_value in raw_state.items(): + key = raw_key.decode("utf-8") if isinstance(raw_key, bytes) else raw_key + value = raw_value.decode("utf-8") if isinstance(raw_value, bytes) else raw_value + normalized[str(key)] = value + return normalized + + def _session_key_prefix(app_name: str, user_id: Optional[str] = None) -> str: """Generate a Redis key prefix for listing sessions. @@ -264,7 +278,7 @@ async def _update_app_state(self, redis_session: RedisSession, app_name: str, key = app_state_key(app_name) command = RedisCommand(method='hgetall', args=(key, )) - app_state: dict[str, Any] = await self._redis_storage.execute_command(redis_session, command) + app_state = _decode_redis_hash(await self._redis_storage.execute_command(redis_session, command)) if app_state: app_state.update(state_delta) else: @@ -277,13 +291,9 @@ async def _update_app_state(self, redis_session: RedisSession, app_name: str, await self._refresh_ttl(redis_session, key) return app_state - # Use HSET with TTL if TTL is configured, otherwise use HSET - args = [key] - for k, v in app_state.items(): - args.extend([k, v]) - command = RedisCommand(method='hset', - args=tuple(args), + args=(key, ), + kwargs={"mapping": app_state}, expire=RedisExpire(key=key, ttl=self._session_config.ttl)) await self._redis_storage.execute_command(redis_session, command) @@ -304,7 +314,7 @@ async def _update_user_state(self, redis_session: RedisSession, app_name: str, u key = user_state_key(app_name, user_id) command = RedisCommand(method='hgetall', args=(key, )) - user_state: dict[str, Any] = await self._redis_storage.execute_command(redis_session, command) + user_state = _decode_redis_hash(await self._redis_storage.execute_command(redis_session, command)) if user_state: user_state.update(state_delta) else: @@ -317,13 +327,9 @@ async def _update_user_state(self, redis_session: RedisSession, app_name: str, u await self._refresh_ttl(redis_session, key) return user_state - # Use HSET with TTL if TTL is configured, otherwise use HSET - args = [key] - for k, v in user_state.items(): - args.extend([k, v]) - command = RedisCommand(method='hset', - args=tuple(args), + args=(key, ), + kwargs={"mapping": user_state}, expire=RedisExpire(key=key, ttl=self._session_config.ttl)) await self._redis_storage.execute_command(redis_session, command) @@ -362,7 +368,7 @@ async def _get_app_state(self, redis_session: RedisSession, app_name: str) -> di """ key = app_state_key(app_name) command = RedisCommand(method='hgetall', args=(key, )) - app_state = await self._redis_storage.execute_command(redis_session, command) + app_state = _decode_redis_hash(await self._redis_storage.execute_command(redis_session, command)) if app_state: await self._refresh_ttl(redis_session, key) @@ -383,7 +389,7 @@ async def _get_user_state(self, redis_session: RedisSession, app_name: str, user """ key = user_state_key(app_name, user_id) command = RedisCommand(method='hgetall', args=(key, )) - user_state = await self._redis_storage.execute_command(redis_session, command) + user_state = _decode_redis_hash(await self._redis_storage.execute_command(redis_session, command)) if user_state: await self._refresh_ttl(redis_session, key) return user_state or {} diff --git a/trpc_agent_sdk/sessions/_sql_session_service.py b/trpc_agent_sdk/sessions/_sql_session_service.py index 4333ffeb..c2dc065c 100644 --- a/trpc_agent_sdk/sessions/_sql_session_service.py +++ b/trpc_agent_sdk/sessions/_sql_session_service.py @@ -105,6 +105,24 @@ def _events_from_storage(events: Optional[list[dict[str, Any]]]) -> list[Event]: return [Event.model_validate(event) for event in (events or [])] +def _normalize_active_event_order(events: list[Event]) -> list[Event]: + """Keep a summary anchor at the front of the active event window. + + Persistent backends reconstruct events from storage using timestamp order, + but a summary event is a logical anchor inserted at the beginning of the + active context window even though it is created later in time. Moving the + first summary anchor to the front keeps SQL behavior aligned with the + in-memory session semantics. + """ + + for index, event in enumerate(events): + if event.is_summary_event(): + if index == 0: + return events + return [event] + events[:index] + events[index + 1:] + return events + + class SessionStorageBase(DeclarativeBase): """Base class for SqlSessionService tables only. @@ -482,7 +500,7 @@ async def get_session( user_state_delta=user_state, session_state=storage_session.state)) - events = [e.to_event() for e in reversed(storage_events)] + events = _normalize_active_event_order([e.to_event() for e in reversed(storage_events)]) historical_events = (_events_from_storage(storage_session.historical_events) if self._session_config.store_historical_events else []) session = storage_session.to_session(state=merged_state, events=events, historical_events=historical_events) @@ -572,7 +590,7 @@ async def append_event(self, session: Session, event: Event) -> Event: session.last_update_time = storage_session.update_timestamp_tz session.state = storage_session.state session.conversation_count = storage_session.conversation_count - session.events = [e.to_event() for e in reversed(storage_events)] + session.events = _normalize_active_event_order([e.to_event() for e in reversed(storage_events)]) session.historical_events = (_events_from_storage(storage_session.historical_events) if self._session_config.store_historical_events else []) @@ -791,12 +809,15 @@ async def _cleanup_expired_async(self) -> None: async def _cleanup_loop(self) -> None: """Background task for periodic cleanup of expired sessions and states.""" logger.debug("Cleanup task started with interval: %ss", self._session_config.ttl.cleanup_interval_seconds) + if self.__cleanup_stop_event is None: + logger.debug("Cleanup loop exited because stop event was not initialized") + return try: - while not self.__cleanup_stop_event.is_set(): + stop_event = self.__cleanup_stop_event + while not stop_event.is_set(): try: - await asyncio.wait_for(self.__cleanup_stop_event.wait(), - timeout=self._session_config.ttl.cleanup_interval_seconds) + await asyncio.wait_for(stop_event.wait(), timeout=self._session_config.ttl.cleanup_interval_seconds) break except asyncio.TimeoutError: try: diff --git a/trpc_agent_sdk/sessions/_summarizer_manager.py b/trpc_agent_sdk/sessions/_summarizer_manager.py index 2a07f58c..07e68d84 100644 --- a/trpc_agent_sdk/sessions/_summarizer_manager.py +++ b/trpc_agent_sdk/sessions/_summarizer_manager.py @@ -41,6 +41,8 @@ from ._session_summarizer import SessionSummarizer from ._session_summarizer import SessionSummary +_SUMMARY_EVENT_METADATA_KEY = "session_summary" + class SummarizerSessionManager: """Session service with automatic summarization capabilities. @@ -71,6 +73,109 @@ def __init__( self._auto_summarize = auto_summarize self._summarizer_cache: Dict[str, Dict[str, Dict[str, SessionSummary]]] = {} + def _get_cached_summary(self, session: Session) -> Optional[SessionSummary]: + """Return cached summary for a session if available.""" + + app_name = session.app_name + user_id = session.user_id + cached_summary = self._summarizer_cache.get(app_name, {}).get(user_id, {}).get(session.id) + if cached_summary is not None: + return cached_summary + + restored_summary = self._restore_summary_from_session(session) + if restored_summary is not None: + self._cache_summary(session, restored_summary) + return restored_summary + + def _cache_summary(self, session: Session, summary: SessionSummary) -> None: + """Cache a session summary for fast same-process reads.""" + + app_name = session.app_name + user_id = session.user_id + if app_name not in self._summarizer_cache: + self._summarizer_cache[app_name] = {} + if user_id not in self._summarizer_cache[app_name]: + self._summarizer_cache[app_name][user_id] = {} + self._summarizer_cache[app_name][user_id][session.id] = summary + + def _get_summary_event(self, session: Session): + """Return the persisted summary anchor event if present.""" + + for event in session.events: + if event.is_summary_event(): + return event + return None + + def _serialize_summary(self, summary: SessionSummary) -> Dict[str, Any]: + """Serialize a summary into event metadata for persistence.""" + + return { + "session_id": summary.session_id, + "summary_text": summary.summary_text, + "original_event_count": summary.original_event_count, + "compressed_event_count": summary.compressed_event_count, + "summary_timestamp": summary.summary_timestamp, + "metadata": dict(summary.metadata or {}), + } + + def _persist_summary_to_session(self, session: Session, summary: SessionSummary) -> None: + """Attach summary metadata to the persisted summary event.""" + + summary_event = self._get_summary_event(session) + if summary_event is None: + return + custom_metadata = dict(summary_event.custom_metadata or {}) + custom_metadata[_SUMMARY_EVENT_METADATA_KEY] = self._serialize_summary(summary) + summary_event.custom_metadata = custom_metadata + summary_event.timestamp = summary.summary_timestamp + + def _restore_summary_from_session(self, session: Session) -> Optional[SessionSummary]: + """Rebuild a SessionSummary from persisted summary-event metadata.""" + + summary_event = self._get_summary_event(session) + if summary_event is None or not summary_event.custom_metadata: + return None + + persisted_summary = summary_event.custom_metadata.get(_SUMMARY_EVENT_METADATA_KEY) + if not isinstance(persisted_summary, dict): + return None + + try: + return SessionSummary( + session_id=str(persisted_summary["session_id"]), + summary_text=str(persisted_summary["summary_text"]), + original_event_count=int(persisted_summary["original_event_count"]), + compressed_event_count=int(persisted_summary["compressed_event_count"]), + summary_timestamp=float(persisted_summary["summary_timestamp"]), + metadata=dict(persisted_summary.get("metadata") or {}), + ) + except (KeyError, TypeError, ValueError) as ex: + logger.warning("Failed to restore persisted summary for session %s: %s", session.id, ex) + return None + + def _build_summary_metadata( + self, + *, + session: Session, + original_event_count: int, + compressed_event_count: int, + previous_summary: Optional[SessionSummary] = None, + ) -> Dict[str, Any]: + """Build stable lineage metadata for a session summary.""" + if previous_summary is None: + previous_summary = self._get_cached_summary(session) + previous_metadata = dict(previous_summary.metadata) if previous_summary and previous_summary.metadata else {} + previous_version = int(previous_metadata.get("version", 0) or 0) + version = previous_version + 1 + summary_id = f"{session.id}:summary:v{version}" + summarized_event_count = max(0, original_event_count - compressed_event_count + 1) + return { + "summary_id": summary_id, + "version": version, + "replaces": previous_metadata.get("summary_id"), + "summarized_event_count": summarized_event_count, + } + def set_session_service(self, session_service: SessionServiceABC, force: bool = False) -> None: """Set the session service to use. @@ -104,6 +209,7 @@ async def create_session_summary(self, # Check if session should be summarized if is_should_summarize: logger.debug("Summarizing session %s", session.id) + previous_summary = self._get_cached_summary(session) # Compress the session so the active events list contains only # model-visible summary/recent events. Raw events are retained only @@ -116,19 +222,21 @@ async def create_session_summary(self, summary_text = await self._summarizer.create_session_summary( session, ctx, store_historical_events=store_historical_events) if summary_text: - app_name = session.app_name - user_id = session.user_id - if app_name not in self._summarizer_cache: - self._summarizer_cache[app_name] = {} - if user_id not in self._summarizer_cache[app_name]: - self._summarizer_cache[app_name][user_id] = {} - self._summarizer_cache[app_name][user_id][session.id] = SessionSummary( + summary = SessionSummary( session_id=session.id, summary_text=summary_text, original_event_count=original_event_count, compressed_event_count=len(session.events), summary_timestamp=time.time(), + metadata=self._build_summary_metadata( + session=session, + original_event_count=original_event_count, + compressed_event_count=len(session.events), + previous_summary=previous_summary, + ), ) + self._cache_summary(session, summary) + self._persist_summary_to_session(session, summary) # Update the stored session if self._base_service: await self._base_service.update_session(session) @@ -142,16 +250,9 @@ async def get_session_summary(self, session: Session) -> Optional[SessionSummary Returns: SessionSummary if successful, None otherwise """ - if not self._summarizer or not self._summarizer_cache: - return None - app_name = session.app_name - user_id = session.user_id - - if app_name not in self._summarizer_cache or user_id not in self._summarizer_cache[ - app_name] or session.id not in self._summarizer_cache[app_name][user_id]: - return None - - return self._summarizer_cache[app_name][user_id][session.id] + if not self._summarizer: + return self._restore_summary_from_session(session) + return self._get_cached_summary(session) def get_summarizer_metadata(self) -> Dict[str, Any]: """Get metadata about the summarizer configuration.