feat(model): add code conformance review to issue lifecycle #82

Merged

stack72 merged 2 commits from stack72/code-conformance-review into main 2026-06-29 10:59:04 +00:00

Conversation 4 Commits 2 Files changed 13 +1732 -14

stack72 commented 2026-06-29 10:25:43 +00:00

Owner

Copy link

Summary

• Add a mandatory adversarial code conformance review step that compares implemented code against the approved plan before a PR can be created
• New codeConformanceReview resource, code_conformance_review and justify_deviations methods, and code-conformance-clear check gating link_pr and complete
• Propagates identical changes from the swamp repo to swamp-extensions, including agent-constraints dimensions, skill references, and comprehensive tests (12 new tests, all passing)

Test plan

• deno fmt -- formatting passes
• deno check -- type checking passes
• deno lint -- linting passes
• deno test -- all 56 tests pass (9 schema + 47 lifecycle)

## Summary - Add a mandatory adversarial code conformance review step that compares implemented code against the approved plan before a PR can be created - New codeConformanceReview resource, code_conformance_review and justify_deviations methods, and code-conformance-clear check gating link_pr and complete - Propagates identical changes from the swamp repo to swamp-extensions, including agent-constraints dimensions, skill references, and comprehensive tests (12 new tests, all passing) ## Test plan - [x] deno fmt -- formatting passes - [x] deno check -- type checking passes - [x] deno lint -- linting passes - [x] deno test -- all 56 tests pass (9 schema + 47 lifecycle)

stack72 added 1 commit 2026-06-29 10:25:43 +00:00

feat(model): add code conformance review to issue lifecycle ... b396202692

Add a mandatory adversarial review step that compares implemented code
against the approved plan before a PR can be created. Deviations from
the plan are expected — they just need a documented justification.

- New `codeConformanceReview` resource storing per-step verification
- New `code_conformance_review` method to record the comparison
- New `justify_deviations` method to add justifications for deviations
- New `code-conformance-clear` check gating both `link_pr` and `complete`
- Both methods post structured lifecycle entries to swamp-club
- Review dimensions documented in agent-constraints/
- Skill references updated with the new mandatory step

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

stack72 commented 2026-06-29 10:37:14 +00:00

Author

Owner

Copy link

Code Review

Blocking Issues

None.

Suggestions

1. code-conformance-clear check gates complete from pr_open and releasing but code_conformance_review can't be called from those phases. The check appliesTo: ["link_pr", "complete"] fires for all three source phases of complete (implementing, pr_open, releasing). For new lifecycles this is fine — by the time you reach pr_open or releasing, the conformance review exists because link_pr already required it. But for any pre-existing lifecycle already in releasing when this version is deployed, calling complete will fail ("No code conformance review exists"), and there is no self-healing path: code_conformance_review is only valid from implementing or pr_failed (per TRANSITIONS), so you can't retroactively satisfy the check from releasing. Users would need to call ship instead of complete, but the existing documentation and SKILL.md both say complete still works from implementing, pr_open, or releasing. Consider narrowing the appliesTo to exclude complete from pr_open/releasing phases (i.e., only gate complete when the source is implementing), or add an operational note to the upgrade description.

2. All code-conformance-clear tests use methodName: "link_pr"; none exercise complete. The check logic doesn't branch on method name so behavior is identical, but having at least one test with methodName: "complete" would make the coverage match the declared appliesTo and provide executable documentation of the intent.

3. start: persists author in context-main test sends authorUsername in the API response body, but issue_lifecycle.ts writes author: issue.author. Whether fetchIssue() maps authorUsername → author depends on _lib/swamp_club.ts (not changed in this PR). If that mapping isn't there, the test passes for the wrong reason (the context write would record author: undefined rather than "external-user"). Worth verifying — though this is pre-existing behaviour, not introduced here.

## Code Review ### Blocking Issues None. ### Suggestions 1. **`code-conformance-clear` check gates `complete` from `pr_open` and `releasing` but `code_conformance_review` can't be called from those phases.** The check `appliesTo: ["link_pr", "complete"]` fires for all three source phases of `complete` (`implementing`, `pr_open`, `releasing`). For new lifecycles this is fine — by the time you reach `pr_open` or `releasing`, the conformance review exists because `link_pr` already required it. But for any **pre-existing lifecycle already in `releasing`** when this version is deployed, calling `complete` will fail ("No code conformance review exists"), and there is no self-healing path: `code_conformance_review` is only valid from `implementing` or `pr_failed` (per `TRANSITIONS`), so you can't retroactively satisfy the check from `releasing`. Users would need to call `ship` instead of `complete`, but the existing documentation and SKILL.md both say `complete still works from implementing, pr_open, or releasing`. Consider narrowing the `appliesTo` to exclude `complete` from `pr_open`/`releasing` phases (i.e., only gate `complete` when the source is `implementing`), or add an operational note to the upgrade description. 2. **All `code-conformance-clear` tests use `methodName: "link_pr"`; none exercise `complete`.** The check logic doesn't branch on method name so behavior is identical, but having at least one test with `methodName: "complete"` would make the coverage match the declared `appliesTo` and provide executable documentation of the intent. 3. **`start: persists author in context-main` test sends `authorUsername` in the API response body**, but `issue_lifecycle.ts` writes `author: issue.author`. Whether `fetchIssue()` maps `authorUsername → author` depends on `_lib/swamp_club.ts` (not changed in this PR). If that mapping isn't there, the test passes for the wrong reason (the context write would record `author: undefined` rather than `"external-user"`). Worth verifying — though this is pre-existing behaviour, not introduced here.

stack72 commented 2026-06-29 10:38:28 +00:00

Author

Owner

Copy link

Adversarial Review

Medium

1. issue-lifecycle/extensions/models/issue_lifecycle.ts:1373-1376 - justify_deviations silently drops empty-string justifications but reports them as justified

   The justify_deviations method uses a truthy check to decide whether to apply a justification to a step. An empty string is falsy in JavaScript, so justificationMap.get(s.order) returning an empty string causes the branch to be skipped and the step is left unchanged. However, the justified counter uses args.justifications.length, which counts the empty-string entry as successfully justified.

   Breaking input: justify_deviations with justifications containing order 2 and an empty-string justification.

   What happens:

   • Schema validation passes (z.string() allows empty strings)
   • justificationMap.get(2) returns empty string, if-check is false, step not updated
   • Log reports Justified 1 deviation(s), 1 unjustified deviation(s) remain
   • The Justified 1 is misleading - nothing was applied
   • The remaining count is correct (recalculated from actual steps), so the user sees a contradictory message

   Suggested fix: Add .min(1) to the justification field in the justify_deviations arguments schema (line 1331). This rejects empty strings at the schema boundary.

Low

1. issue-lifecycle/extensions/models/issue_lifecycle.ts:1373-1379 - justify_deviations silently ignores justifications for non-existent step orders

   If a justification targets an order number that does not appear in the existing review steps array, the justification is silently discarded. But justified = args.justifications.length still counts it, producing a misleading log.

   Breaking input: Review has steps with orders [1, 2]. Call justify_deviations with order 99.

   What happens: Log says Justified 1 deviation(s) but nothing changed. The remaining count is correct, so the inconsistency is visible but confusing.

   Suggested fix: Count how many justifications actually matched instead of using args.justifications.length. Optionally warn about unmatched orders.

   This is LOW because it only triggers on agent error (providing wrong order numbers) and the remaining count self-corrects.

2. issue-lifecycle/extensions/models/_lib/schemas.ts:209 - StepVerificationSchema.justification allows empty strings

   The justification field on StepVerificationSchema is z.string().optional() so when provided it can be empty. The code-conformance-clear check uses !s.justification to test for unjustified steps, so empty string is treated as unjustified (correct behavior). However, code_conformance_review would accept and persist an empty justification on a step. This is cosmetically inconsistent but functionally correct since the check handles it properly.

Verdict

PASS - The code is well-structured, follows existing patterns in the codebase (resource/check/method lifecycle), and has solid test coverage across happy paths, error cases, and the check gate. The medium finding (empty-string justification) is unlikely in practice and the check still blocks correctly - it is a UX/logging issue, not a data integrity one. No critical or high severity issues found.

## Adversarial Review ### Medium 1. **issue-lifecycle/extensions/models/issue_lifecycle.ts:1373-1376 - justify_deviations silently drops empty-string justifications but reports them as justified** The justify_deviations method uses a truthy check to decide whether to apply a justification to a step. An empty string is falsy in JavaScript, so justificationMap.get(s.order) returning an empty string causes the branch to be skipped and the step is left unchanged. However, the justified counter uses args.justifications.length, which counts the empty-string entry as successfully justified. **Breaking input:** justify_deviations with justifications containing order 2 and an empty-string justification. **What happens:** - Schema validation passes (z.string() allows empty strings) - justificationMap.get(2) returns empty string, if-check is false, step not updated - Log reports Justified 1 deviation(s), 1 unjustified deviation(s) remain - The Justified 1 is misleading - nothing was applied - The remaining count is correct (recalculated from actual steps), so the user sees a contradictory message **Suggested fix:** Add .min(1) to the justification field in the justify_deviations arguments schema (line 1331). This rejects empty strings at the schema boundary. ### Low 1. **issue-lifecycle/extensions/models/issue_lifecycle.ts:1373-1379 - justify_deviations silently ignores justifications for non-existent step orders** If a justification targets an order number that does not appear in the existing review steps array, the justification is silently discarded. But justified = args.justifications.length still counts it, producing a misleading log. **Breaking input:** Review has steps with orders [1, 2]. Call justify_deviations with order 99. **What happens:** Log says Justified 1 deviation(s) but nothing changed. The remaining count is correct, so the inconsistency is visible but confusing. **Suggested fix:** Count how many justifications actually matched instead of using args.justifications.length. Optionally warn about unmatched orders. This is LOW because it only triggers on agent error (providing wrong order numbers) and the remaining count self-corrects. 2. **issue-lifecycle/extensions/models/_lib/schemas.ts:209 - StepVerificationSchema.justification allows empty strings** The justification field on StepVerificationSchema is z.string().optional() so when provided it can be empty. The code-conformance-clear check uses !s.justification to test for unjustified steps, so empty string is treated as unjustified (correct behavior). However, code_conformance_review would accept and persist an empty justification on a step. This is cosmetically inconsistent but functionally correct since the check handles it properly. ### Verdict **PASS** - The code is well-structured, follows existing patterns in the codebase (resource/check/method lifecycle), and has solid test coverage across happy paths, error cases, and the check gate. The medium finding (empty-string justification) is unlikely in practice and the check still blocks correctly - it is a UX/logging issue, not a data integrity one. No critical or high severity issues found.

stack72 added 1 commit 2026-06-29 10:45:08 +00:00

fix: sync root extensions/models with issue-lifecycle copy ... 579626dd57

The root extensions/models/ directory was missed in the initial commit.
Copies the updated schemas, model, and tests from issue-lifecycle/.

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

stack72 commented 2026-06-29 10:54:59 +00:00

Author

Owner

Copy link

Code Review

Blocking Issues

None.

Suggestions

1. Upgrade description understates the scope of code-conformance-clear (extensions/models/issue_lifecycle.ts:149–157): The version upgrade description says "Gates link_pr" but the check also gates complete (via appliesTo: ["link_pr", "complete"]). This means calling complete directly from implementing now requires a conformance review where it previously didn't. The design is correct and intentional, but the upgrade note is misleading — worth clarifying as "Gates link_pr and complete" to set accurate expectations for operators applying the upgrade.

2. Missing test case: code-conformance-clear when no plan exists (extensions/models/issue_lifecycle_test.ts): The check has a "No plan exists." error path (issue_lifecycle.ts:398–400) but no test covers it. The four existing test cases (passes, no review, stale version, unjustified deviations) are good, but this fifth path goes untested. Low priority since both link_pr and complete require an earlier plan call in the normal flow, but it's a reachable code path.

3. buildTestContext return type is pinned to start context (issue_lifecycle_test.ts:42): context is typed as Parameters<typeof model.methods.pr_merged.execute>[1], which works for tests but may confuse readers since the helper is used for many other methods. A union or a hand-written interface would be clearer. No functional impact.

## Code Review ### Blocking Issues None. ### Suggestions 1. **Upgrade description understates the scope of `code-conformance-clear`** (`extensions/models/issue_lifecycle.ts:149–157`): The version upgrade description says "Gates link_pr" but the check also gates `complete` (via `appliesTo: ["link_pr", "complete"]`). This means calling `complete` directly from `implementing` now requires a conformance review where it previously didn't. The design is correct and intentional, but the upgrade note is misleading — worth clarifying as "Gates link_pr and complete" to set accurate expectations for operators applying the upgrade. 2. **Missing test case: `code-conformance-clear` when no plan exists** (`extensions/models/issue_lifecycle_test.ts`): The check has a `"No plan exists."` error path (`issue_lifecycle.ts:398–400`) but no test covers it. The four existing test cases (passes, no review, stale version, unjustified deviations) are good, but this fifth path goes untested. Low priority since both `link_pr` and `complete` require an earlier `plan` call in the normal flow, but it's a reachable code path. 3. **`buildTestContext` return type is pinned to `start` context** (`issue_lifecycle_test.ts:42`): `context` is typed as `Parameters<typeof model.methods.pr_merged.execute>[1]`, which works for tests but may confuse readers since the helper is used for many other methods. A union or a hand-written interface would be clearer. No functional impact.

stack72 commented 2026-06-29 10:56:05 +00:00

Author

Owner

Copy link

Adversarial Review

Critical / High

None found.

Medium

1. justify_deviations reports misleading count for non-existent step orders
   File: extensions/models/issue_lifecycle.ts:1393-1395

   The justified count comes from args.justifications.length (inputs
   provided), not from the number of justifications actually applied. If a
   caller passes an order that does not exist in the review (e.g., order 999
   when only steps 1 and 2 exist), the map lookup silently misses, no step is
   updated, but the log says "Justified 1 deviation(s)". The remaining count
   is correct (computed from updatedSteps), so the code-conformance-clear
   gate still works -- but the feedback to the caller is wrong.

   Breaking example: Call justify_deviations with order 999 and a
   justification when the review has steps 1 and 2. Log says "Justified 1
   deviation(s), 1 unjustified remain" but nothing was actually justified.

   Suggested fix: Count applied justifications instead of input length. Also
   use justification !== undefined instead of the truthy check, which would
   silently drop empty-string justifications. (The analogous pattern exists in
   resolve_findings for finding IDs, but that is pre-existing code.)

Low

1. Duplicate order values in justifications array silently lose data
   File: extensions/models/issue_lifecycle.ts:1369-1371

   The justificationMap is built with new Map(...). If two entries share
   the same order, only the last one wins -- the earlier justification is
   silently discarded. The Zod schema does not enforce unique orders.

   Impact: Unlikely in practice since the LLM agent generates these
   programmatically, but the API contract does not prevent it.

2. Documentation gap: complete as quick close-out now requires review
   File: implementation.md:135-136

   The code-conformance-clear check applies to both link_pr and
   complete. The documentation says complete still works from
   implementing, pr_open, or releasing for quick close-out, but does not
   mention that it now requires a conformance review first. Callers following
   the documented quick close-out path will hit an unexpected gate failure.

   Impact: Confusing error for agents following the documented path, not a
   correctness bug. The gate is arguably the right behavior, but the docs
   should say so.

Verdict

PASS -- The core logic is sound. The new code_conformance_review method,
justify_deviations method, code-conformance-clear check, StepVerificationSchema,
and CodeConformanceReviewSchema are correctly wired into the state machine. The
gate check properly validates plan version alignment and unjustified deviations.
Tests cover the key paths (happy path, missing plan, missing review, stale
version, unjustified deviations). Root and issue-lifecycle/ copies are verified
identical. The medium finding about misleading justification counts is a logging
issue, not a correctness bug -- the actual gate enforcement works correctly.

## Adversarial Review ### Critical / High None found. ### Medium 1. **justify_deviations reports misleading count for non-existent step orders** File: `extensions/models/issue_lifecycle.ts:1393-1395` The `justified` count comes from `args.justifications.length` (inputs provided), not from the number of justifications actually applied. If a caller passes an order that does not exist in the review (e.g., order 999 when only steps 1 and 2 exist), the map lookup silently misses, no step is updated, but the log says "Justified 1 deviation(s)". The `remaining` count is correct (computed from `updatedSteps`), so the `code-conformance-clear` gate still works -- but the feedback to the caller is wrong. Breaking example: Call `justify_deviations` with order 999 and a justification when the review has steps 1 and 2. Log says "Justified 1 deviation(s), 1 unjustified remain" but nothing was actually justified. Suggested fix: Count applied justifications instead of input length. Also use `justification !== undefined` instead of the truthy check, which would silently drop empty-string justifications. (The analogous pattern exists in `resolve_findings` for finding IDs, but that is pre-existing code.) ### Low 1. **Duplicate order values in justifications array silently lose data** File: `extensions/models/issue_lifecycle.ts:1369-1371` The `justificationMap` is built with `new Map(...)`. If two entries share the same `order`, only the last one wins -- the earlier justification is silently discarded. The Zod schema does not enforce unique orders. Impact: Unlikely in practice since the LLM agent generates these programmatically, but the API contract does not prevent it. 2. **Documentation gap: complete as quick close-out now requires review** File: `implementation.md:135-136` The `code-conformance-clear` check applies to both `link_pr` and `complete`. The documentation says `complete` still works from implementing, pr_open, or releasing for quick close-out, but does not mention that it now requires a conformance review first. Callers following the documented quick close-out path will hit an unexpected gate failure. Impact: Confusing error for agents following the documented path, not a correctness bug. The gate is arguably the right behavior, but the docs should say so. ### Verdict PASS -- The core logic is sound. The new code_conformance_review method, justify_deviations method, code-conformance-clear check, StepVerificationSchema, and CodeConformanceReviewSchema are correctly wired into the state machine. The gate check properly validates plan version alignment and unjustified deviations. Tests cover the key paths (happy path, missing plan, missing review, stale version, unjustified deviations). Root and issue-lifecycle/ copies are verified identical. The medium finding about misleading justification counts is a logging issue, not a correctness bug -- the actual gate enforcement works correctly.

stack72 merged commit 1aa1278649 into main 2026-06-29 10:59:04 +00:00

stack72 deleted branch stack72/code-conformance-review 2026-06-29 10:59:04 +00:00

stack72 referenced this pull request from a commit 2026-06-29 10:59:06 +00:00

feat(model): add code conformance review to issue lifecycle (#82)

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

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!82

No description provided.