fix: verdict parsing, README claims, example consistency
Fixes from adversarial code-vs-plan review (3 rounds): - Verdict format in prompts now matches parser (bare tokens) - Missing verdict treated as parse failure, not approval - README: softened backend swappability to "designed for extensibility" - Example: replaced incorrect FK scenario with valid transaction bug - Example: aligned fixes and round-2 summary with round-1 finding Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -32,10 +32,10 @@ This skill is a text instruction that any AI agent can interpret.
|
||||
It actively fixes issues based on reviewer feedback and resubmits
|
||||
for re-review. Up to 5 rounds until approved.
|
||||
|
||||
**Universal foundation** — the skill relies on basic agent capabilities:
|
||||
run a command, read a file, edit a file. The reviewer is a swappable
|
||||
component: today Codex, tomorrow Gemini CLI, next week a local model.
|
||||
Switching backends = changing one launch command; prompts and workflow stay the same.
|
||||
**Designed for extensibility** — the skill relies on basic agent capabilities:
|
||||
run a command, read a file, edit a file. The prompts and review workflow
|
||||
are model-agnostic. Currently uses OpenAI Codex as the reviewer;
|
||||
adding other backends (Gemini, local models) is on the roadmap.
|
||||
|
||||
## How it works
|
||||
|
||||
|
||||
@@ -169,8 +169,10 @@ For each finding:
|
||||
If no findings: "No actionable findings."
|
||||
|
||||
## Verdict
|
||||
VERDICT: APPROVED — no findings, or all findings are low severity
|
||||
VERDICT: REVISE — one or more high/critical findings
|
||||
Rules: approve if no findings or all low severity; revise if any high/critical.
|
||||
Choose exactly one. The LAST line of your response must be one of:
|
||||
VERDICT: APPROVED
|
||||
VERDICT: REVISE
|
||||
</output_format>
|
||||
```
|
||||
|
||||
@@ -245,8 +247,10 @@ For each finding:
|
||||
If no findings: "No actionable findings."
|
||||
|
||||
## Verdict
|
||||
VERDICT: APPROVED — no findings, or all findings are low severity
|
||||
VERDICT: REVISE — one or more high/critical findings
|
||||
Rules: approve if no findings or all low severity; revise if any high/critical.
|
||||
Choose exactly one. The LAST line of your response must be one of:
|
||||
VERDICT: APPROVED
|
||||
VERDICT: REVISE
|
||||
</output_format>
|
||||
```
|
||||
|
||||
@@ -327,7 +331,7 @@ timeout 600 codex exec \
|
||||
3. Проверить вердикт:
|
||||
- **VERDICT: APPROVED** → перейти к Шагу 8 (Готово)
|
||||
- **VERDICT: REVISE** → перейти к Шагу 6 (Правки)
|
||||
- Нет явного вердикта, но всё позитивно → считать одобренным
|
||||
- Нет явного вердикта → считать parse failure, запустить resume/fallback с просьбой дать чёткий вердикт
|
||||
- Достигнут максимум (5 раундов) → перейти к Шагу 8 с пометкой
|
||||
|
||||
### Шаг 6: Внести правки
|
||||
|
||||
+17
-16
@@ -15,21 +15,22 @@ data integrity and authorization.
|
||||
|
||||
## Findings
|
||||
|
||||
### [severity: critical] User deletion does not cascade to dependent records
|
||||
### [severity: critical] User deletion without transaction leaves partial state on failure
|
||||
|
||||
- **File:** src/api/users.py lines 45-52
|
||||
- **What can go wrong:** Deleting a user leaves orphaned records in `orders`,
|
||||
`sessions`, and `audit_log` tables. Foreign key constraints are set to
|
||||
`NO ACTION`, so the delete succeeds but dependent data becomes unreferenceable.
|
||||
Queries joining on `user_id` will silently drop rows.
|
||||
- **Why vulnerable:** The `delete_user()` function calls
|
||||
`db.users.delete(user_id)` directly without handling related tables.
|
||||
No cascade is configured at the database level.
|
||||
- **Impact:** Data integrity violation. Orphaned orders cannot be attributed
|
||||
to any user. Financial reconciliation reports will undercount.
|
||||
- **Recommendation:** Either add `ON DELETE CASCADE` to foreign keys in a
|
||||
migration, or delete dependent records explicitly in a transaction before
|
||||
deleting the user.
|
||||
- **What can go wrong:** The `delete_user()` function deletes from `sessions`,
|
||||
`orders`, and `users` in three separate queries without a transaction.
|
||||
If the process crashes or the DB connection drops after deleting sessions
|
||||
but before deleting the user, the user record persists with missing session
|
||||
history. There is no retry or cleanup mechanism.
|
||||
- **Why vulnerable:** The function calls `db.sessions.delete(user_id)`,
|
||||
`db.orders.delete(user_id)`, and `db.users.delete(user_id)` sequentially
|
||||
without wrapping them in `db.transaction()`.
|
||||
- **Impact:** Data loss. Partial deletion leaves the database in an
|
||||
inconsistent state that requires manual intervention to fix.
|
||||
- **Recommendation:** Wrap all three deletes in a single transaction:
|
||||
`with db.transaction(): ...`. If any step fails, the entire operation
|
||||
rolls back.
|
||||
|
||||
### [severity: high] Missing authorization check on delete endpoint
|
||||
|
||||
@@ -54,7 +55,7 @@ VERDICT: REVISE
|
||||
|
||||
### Fixes (Round 1)
|
||||
|
||||
- Added `ON DELETE CASCADE` migration for `orders.user_id` and `sessions.user_id`
|
||||
- Wrapped all three deletes (`sessions`, `orders`, `users`) in `db.transaction()`
|
||||
- Added `@require_role('admin')` to the delete endpoint
|
||||
- Added test for unauthorized deletion attempt (returns 403)
|
||||
|
||||
@@ -64,8 +65,8 @@ VERDICT: REVISE
|
||||
|
||||
## Summary
|
||||
|
||||
Both critical issues from round 1 are resolved. The cascade migration is correct
|
||||
and the authorization check is in place. No new issues found.
|
||||
Both issues from round 1 are resolved. The transaction wraps all deletes
|
||||
atomically and the authorization check is in place. No new issues found.
|
||||
|
||||
## Findings
|
||||
|
||||
|
||||
Reference in New Issue
Block a user