feat(issue-lifecycle): add summarizing phase, code conformance review, and adversarial regression verification #156

Merged

stack72 merged 1 commit from feat/issue-lifecycle-2026.07.30.1 into main 2026-07-30 18:56:50 +00:00

Conversation 2 Commits 1 Files changed 14 +662 -56

stack72 commented 2026-07-30 18:15:04 +00:00

Owner

Copy link

Bump issue-lifecycle model to 2026.07.30.1. Adds the summarizing phase
between notify and done, code conformance review resource/methods/check,
adversarial regression evidence fields and triage validation, and the
summarize method. Updates skill documentation with Phase 6, resume table
entry, and adversarial regression triage instructions.

Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com

Bump issue-lifecycle model to 2026.07.30.1. Adds the summarizing phase between notify and done, code conformance review resource/methods/check, adversarial regression evidence fields and triage validation, and the summarize method. Updates skill documentation with Phase 6, resume table entry, and adversarial regression triage instructions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

stack72 added 1 commit 2026-07-30 18:15:04 +00:00

feat(issue-lifecycle): add summarizing phase, code conformance review, and adversarial regression verification ... f41a0ce42f

Bump issue-lifecycle model to 2026.07.30.1. Adds the summarizing phase
between notify and done, code conformance review resource/methods/check,
adversarial regression evidence fields and triage validation, and the
summarize method. Updates skill documentation with Phase 6, resume table
entry, and adversarial regression triage instructions.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

stack72 commented 2026-07-30 18:27:56 +00:00

Author

Owner

Copy link

Code Review

Blocking Issues

1. Missing tests for triage regression verification logic — The triage method (extensions/models/issue_lifecycle.ts:731-751) now throws when isRegression=true without all four required fields (regressionEvidence, regressionCounterEvidence, regressionVerdict, regressionVerdictReasoning), and automatically clears isRegression when regressionVerdict === "downgraded". This is the primary new behavioral feature in the PR ("Adversarial regression verification"), but neither extensions/models/issue_lifecycle_test.ts nor issue-lifecycle/extensions/models/issue_lifecycle_test.ts contains any test covering this code path. Specifically untested:
   • triage throws when isRegression=true and evidence fields are absent
   • triage sets effectiveIsRegression = false (and logs the downgrade) when verdict is downgraded
   • triage succeeds normally when all four regression fields are supplied

Suggestions

1. SummarySchema doesn't export its inferred type — Every other persisted schema in _lib/schemas.ts exports a named type alias (PlanData, AdversarialReviewData, CodeConformanceReviewData, PullRequestData, StateData), but SummarySchema is missing export type SummaryData = z.infer<typeof SummarySchema>. Not a bug since the schema itself is exported, but it breaks the established pattern.

2. schemas_test.ts doesn't assert summarizing in TRANSITIONS.start — The test "TRANSITIONS: start (resume) includes pr_open, pr_failed, releasing, and notify" (line 48) was not updated to also assert that summarizing is in the allowed phases for start. Since summarizing was added to TRANSITIONS.start in this PR, a corresponding assertion would make the test consistent with the change.

3. Regression validation done in execute rather than Zod — The cross-field constraint (isRegression=true requires four other fields) is enforced by a manual if check in the execute function rather than z.superRefine(). This means the Zod schema for triage's arguments accepts invalid inputs that the execute body then rejects. Not a bug, but keeping schema and validation co-located in Zod would make the constraint self-documenting and easier to test via parseAsync.

## Code Review ### Blocking Issues 1. **Missing tests for `triage` regression verification logic** — The `triage` method (`extensions/models/issue_lifecycle.ts:731-751`) now throws when `isRegression=true` without all four required fields (`regressionEvidence`, `regressionCounterEvidence`, `regressionVerdict`, `regressionVerdictReasoning`), and automatically clears `isRegression` when `regressionVerdict === "downgraded"`. This is the primary new behavioral feature in the PR ("Adversarial regression verification"), but **neither** `extensions/models/issue_lifecycle_test.ts` nor `issue-lifecycle/extensions/models/issue_lifecycle_test.ts` contains any test covering this code path. Specifically untested: - `triage` throws when `isRegression=true` and evidence fields are absent - `triage` sets `effectiveIsRegression = false` (and logs the downgrade) when verdict is `downgraded` - `triage` succeeds normally when all four regression fields are supplied ### Suggestions 1. **`SummarySchema` doesn't export its inferred type** — Every other persisted schema in `_lib/schemas.ts` exports a named type alias (`PlanData`, `AdversarialReviewData`, `CodeConformanceReviewData`, `PullRequestData`, `StateData`), but `SummarySchema` is missing `export type SummaryData = z.infer<typeof SummarySchema>`. Not a bug since the schema itself is exported, but it breaks the established pattern. 2. **`schemas_test.ts` doesn't assert `summarizing` in `TRANSITIONS.start`** — The test `"TRANSITIONS: start (resume) includes pr_open, pr_failed, releasing, and notify"` (line 48) was not updated to also assert that `summarizing` is in the allowed phases for `start`. Since `summarizing` was added to `TRANSITIONS.start` in this PR, a corresponding assertion would make the test consistent with the change. 3. **Regression validation done in `execute` rather than Zod** — The cross-field constraint (`isRegression=true` requires four other fields) is enforced by a manual `if` check in the `execute` function rather than `z.superRefine()`. This means the Zod schema for `triage`'s arguments accepts invalid inputs that the execute body then rejects. Not a bug, but keeping schema and validation co-located in Zod would make the constraint self-documenting and easier to test via `parseAsync`.

stack72 commented 2026-07-30 18:30:32 +00:00

Author

Owner

Copy link

Adversarial Review

Critical / High

No critical or high severity issues found.

Medium

1. No test coverage for adversarial regression validation logic (extensions/models/issue_lifecycle.ts:732-751, issue-lifecycle/extensions/models/issue_lifecycle.ts:732-751)

   The triage method's regression validation is entirely untested. The test file has zero references to "regression" or "isRegression". Two distinct code paths are uncovered:

   • Error path (line 739): When isRegression=true but any of the four evidence fields is missing, the method throws. No test verifies this throw happens, nor that the error message is correct.
   • Downgrade path (line 745): When regressionVerdict === "downgraded", effectiveIsRegression is set to false, which changes what gets written to the classification resource and the lifecycle entry. No test verifies the downgraded value propagates to writeResource or postLifecycleEntry.

   Breaking example: Someone refactors the if condition at line 732 to if (args.isRegression === true) (a common "strictness" refactor) but accidentally moves the closing brace, and the downgrade block silently stops executing. Nothing catches it.

   Suggested fix: Add at least two tests — one asserting execute throws when isRegression=true with missing evidence fields, and one asserting that regressionVerdict: "downgraded" writes isRegression: false to the classification resource.

2. summarize unconditionally closes the lifecycle even when outcomeMet: false (extensions/models/issue_lifecycle.ts:2150-2182, issue-lifecycle/extensions/models/issue_lifecycle.ts:2150-2182)

   The SKILL.md and implementation.md describe the summary as verifying "the work addressed the issue before transitioning to done." But the code transitions to done and posts targetStatus: "shipped" identically for both outcomeMet: true and outcomeMet: false. The only difference is the summary string prefix ("Outcome met" vs "Outcome NOT met").

   Breaking example: An agent calls summarize with outcomeMet: false because the fix didn't address the reported issue. The lifecycle completes as done/shipped anyway. There's no mechanism to re-open, re-plan, or flag for human review. The verification step becomes a passive annotation rather than a gate.

   Suggested fix: If this is intentional (the summary is a record, not a gate), update the docs to say "records whether the outcome was met" rather than "verifies the work addressed the issue." If it should be a gate, have outcomeMet: false transition to a different phase (e.g. back to implementing or a new needs_rework phase) instead of done.

Low

1. Whitespace-only strings pass regression evidence validation (extensions/models/issue_lifecycle.ts:733-737)

   The falsy check !args.regressionEvidence catches undefined and empty string, but a whitespace-only string like " " passes validation. The Zod schema uses z.string() without .min(1) or .trim(), so regressionEvidence: " " satisfies both schema and runtime validation while providing no meaningful evidence.

   Suggested fix: Use z.string().min(1) in the argument schema, or add .trim().length > 0 to the runtime check.

2. Regression evidence fields silently accepted without isRegression (extensions/models/issue_lifecycle.ts:730-752)

   If a caller passes regressionEvidence, regressionCounterEvidence, regressionVerdict, and regressionVerdictReasoning but sets isRegression: false (or omits it), all evidence fields are stored in the classification resource alongside isRegression: false. No warning is logged about the inconsistency. This could create confusing records where evidence exists but the flag is off.

   Suggested fix: Log a warning when regression evidence fields are present but isRegression is not true, or strip the fields when isRegression is falsy.

Verdict

PASS — The code is structurally sound: phase transitions are correctly wired, the TRANSITIONS table matches the new phase ordering, upgrade entries are properly sequenced, tests cover the happy paths for summarize/notify/skip_notify, and the schema additions are consistent. The two medium findings (untested regression validation logic and the outcomeMet: false semantic gap) are real gaps but neither causes data loss or security issues. Both can be addressed in a follow-up.

## Adversarial Review ### Critical / High No critical or high severity issues found. ### Medium 1. **No test coverage for adversarial regression validation logic** (`extensions/models/issue_lifecycle.ts:732-751`, `issue-lifecycle/extensions/models/issue_lifecycle.ts:732-751`) The triage method's regression validation is entirely untested. The test file has zero references to "regression" or "isRegression". Two distinct code paths are uncovered: - **Error path** (line 739): When `isRegression=true` but any of the four evidence fields is missing, the method throws. No test verifies this throw happens, nor that the error message is correct. - **Downgrade path** (line 745): When `regressionVerdict === "downgraded"`, `effectiveIsRegression` is set to `false`, which changes what gets written to the classification resource and the lifecycle entry. No test verifies the downgraded value propagates to `writeResource` or `postLifecycleEntry`. **Breaking example**: Someone refactors the `if` condition at line 732 to `if (args.isRegression === true)` (a common "strictness" refactor) but accidentally moves the closing brace, and the downgrade block silently stops executing. Nothing catches it. **Suggested fix**: Add at least two tests — one asserting `execute` throws when `isRegression=true` with missing evidence fields, and one asserting that `regressionVerdict: "downgraded"` writes `isRegression: false` to the classification resource. 2. **`summarize` unconditionally closes the lifecycle even when `outcomeMet: false`** (`extensions/models/issue_lifecycle.ts:2150-2182`, `issue-lifecycle/extensions/models/issue_lifecycle.ts:2150-2182`) The SKILL.md and implementation.md describe the summary as verifying "the work addressed the issue before transitioning to done." But the code transitions to `done` and posts `targetStatus: "shipped"` identically for both `outcomeMet: true` and `outcomeMet: false`. The only difference is the summary string prefix ("Outcome met" vs "Outcome NOT met"). **Breaking example**: An agent calls `summarize` with `outcomeMet: false` because the fix didn't address the reported issue. The lifecycle completes as `done`/`shipped` anyway. There's no mechanism to re-open, re-plan, or flag for human review. The verification step becomes a passive annotation rather than a gate. **Suggested fix**: If this is intentional (the summary is a record, not a gate), update the docs to say "records whether the outcome was met" rather than "verifies the work addressed the issue." If it should be a gate, have `outcomeMet: false` transition to a different phase (e.g. back to `implementing` or a new `needs_rework` phase) instead of `done`. ### Low 1. **Whitespace-only strings pass regression evidence validation** (`extensions/models/issue_lifecycle.ts:733-737`) The falsy check `!args.regressionEvidence` catches `undefined` and empty string, but a whitespace-only string like `" "` passes validation. The Zod schema uses `z.string()` without `.min(1)` or `.trim()`, so `regressionEvidence: " "` satisfies both schema and runtime validation while providing no meaningful evidence. **Suggested fix**: Use `z.string().min(1)` in the argument schema, or add `.trim().length > 0` to the runtime check. 2. **Regression evidence fields silently accepted without `isRegression`** (`extensions/models/issue_lifecycle.ts:730-752`) If a caller passes `regressionEvidence`, `regressionCounterEvidence`, `regressionVerdict`, and `regressionVerdictReasoning` but sets `isRegression: false` (or omits it), all evidence fields are stored in the classification resource alongside `isRegression: false`. No warning is logged about the inconsistency. This could create confusing records where evidence exists but the flag is off. **Suggested fix**: Log a warning when regression evidence fields are present but `isRegression` is not `true`, or strip the fields when `isRegression` is falsy. ### Verdict **PASS** — The code is structurally sound: phase transitions are correctly wired, the TRANSITIONS table matches the new phase ordering, upgrade entries are properly sequenced, tests cover the happy paths for `summarize`/`notify`/`skip_notify`, and the schema additions are consistent. The two medium findings (untested regression validation logic and the `outcomeMet: false` semantic gap) are real gaps but neither causes data loss or security issues. Both can be addressed in a follow-up.

stack72 merged commit c141d6caee into main 2026-07-30 18:56:50 +00:00

stack72 deleted branch feat/issue-lifecycle-2026.07.30.1 2026-07-30 18:56:50 +00:00

stack72 referenced this pull request from a commit 2026-07-30 18:56:51 +00:00

feat(issue-lifecycle): add summarizing phase, code conformance review, and adversarial regression verification (#156)

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

swamp-club/swamp-extensions!156

No description provided.