feat(verification): add pre-PR verification workflows and issue-lifecycle support #243

Merged

stack72 merged 5 commits from feat/pre-pr-verification into main 2026-08-30 03:35:13 +00:00

Conversation 9 Commits 5 Files changed 12 +2392 -23

stack72 commented 2026-08-28 21:29:20 +00:00

Owner

Copy link

Summary

• Add two swamp verification workflows (verify-build and verify-reviews) adapted from swamp core's pre-PR verification system, with review prompts as single source of truth under verification/review-prompts/
• verify-build runs per-directory quality gates (check/lint/fmt/test/lockfile), codegen idempotency checks, and a model version/upgrade gate that catches missing upgrade entries (precedent: issue #554)
• verify-reviews runs code-review and adversarial on all source changes, ci-security guarded to .forgejo//.github//scripts/ changes
• Add verification-conventions.md agent constraints for the verify-fix-rerun loop
• Add four new issue-lifecycle methods (verify, verification_passed, verification_failed, post_attestation), verificationResult resource, and verification-clear check gating link_pr — no PR opens without passing verification
• Add postAttestation() to SwampClubClient for posting verification attestations to swamp-club
• Bump issue-lifecycle to 2026.08.28.1 with upgrade entry

Test plan

• deno check passes for issue-lifecycle
• deno lint passes for issue-lifecycle
• deno fmt --check passes for issue-lifecycle
• All 60 issue-lifecycle tests pass (including updated schema and version assertions)
• deno install --frozen passes (lockfile clean)
• Run SWAMP_WORKFLOWS_DIR=verification swamp workflow run verify-build locally to smoke-test the build workflow
• Run SWAMP_WORKFLOWS_DIR=verification swamp workflow run verify-reviews locally to smoke-test the reviews workflow
• Verify swamp extension source add accepts the new version and upgrade path works

🤖 Generated with Claude Code

## Summary - Add two swamp verification workflows (`verify-build` and `verify-reviews`) adapted from swamp core's pre-PR verification system, with review prompts as single source of truth under `verification/review-prompts/` - `verify-build` runs per-directory quality gates (check/lint/fmt/test/lockfile), codegen idempotency checks, and a model version/upgrade gate that catches missing upgrade entries (precedent: issue #554) - `verify-reviews` runs code-review and adversarial on all source changes, ci-security guarded to `.forgejo/`/`.github/`/`scripts/` changes - Add `verification-conventions.md` agent constraints for the verify-fix-rerun loop - Add four new issue-lifecycle methods (`verify`, `verification_passed`, `verification_failed`, `post_attestation`), `verificationResult` resource, and `verification-clear` check gating `link_pr` — no PR opens without passing verification - Add `postAttestation()` to `SwampClubClient` for posting verification attestations to swamp-club - Bump issue-lifecycle to `2026.08.28.1` with upgrade entry ## Test plan - [x] `deno check` passes for issue-lifecycle - [x] `deno lint` passes for issue-lifecycle - [x] `deno fmt --check` passes for issue-lifecycle - [x] All 60 issue-lifecycle tests pass (including updated schema and version assertions) - [x] `deno install --frozen` passes (lockfile clean) - [ ] Run `SWAMP_WORKFLOWS_DIR=verification swamp workflow run verify-build` locally to smoke-test the build workflow - [ ] Run `SWAMP_WORKFLOWS_DIR=verification swamp workflow run verify-reviews` locally to smoke-test the reviews workflow - [ ] Verify `swamp extension source add` accepts the new version and upgrade path works 🤖 Generated with [Claude Code](https://claude.com/claude-code)

stack72 added 1 commit 2026-08-28 21:29:20 +00:00

feat(verification): add pre-PR verification workflows and issue-lifecycle support ... 0f8f012c51

Add verification infrastructure adapted from swamp core's two-workflow
pre-PR verification system. Code-review and adversarial reviews run on
every source change; ci-security is guarded to .forgejo/.github/scripts.

The verify-build workflow runs per-directory quality gates, codegen
idempotency checks, and a model version/upgrade gate that catches
missing upgrade entries before they reach users.

Issue-lifecycle gains four new methods (verify, verification_passed,
verification_failed, post_attestation), a verificationResult resource,
and a verification-clear check gating link_pr — no PR opens without
passing verification.

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

stack72 commented 2026-08-28 21:37:24 +00:00

Author

Owner

Copy link

Code Review

Blocking Issues

1. Missing tests for all new verification methods and check (issue-lifecycle/extensions/models/issue_lifecycle.ts, issue_lifecycle_test.ts)

   The PR adds four new methods (verify, verification_passed, verification_failed, post_attestation) and one new check (verification-clear), plus the verificationResult resource — none of which have unit tests. Every other method in issue_lifecycle_test.ts has corresponding tests (including model-registration smoke tests). The new verification pathway is the most complex addition in this PR and is the gating mechanism before a PR can be opened; it needs test coverage.

   Missing tests:

   • verify: state transitions to verifying, lifecycle entry posted
   • verification_passed: verificationResult-main written with correct aggregated counts (stepsCompleted, stepsFailed, stepsSkipped, allPassed), state stays verifying
   • verification_failed: state transitions back to implementing
   • post_attestation: throws when swamp-club unreachable, throws on invalid JSON, posts attestation and lifecycle entry
   • verification-clear check: passes when allPassed=true, rejects when no result exists, rejects when allPassed=false
   • Smoke test: verificationResult in model.resources, verify/verification_passed/verification_failed/post_attestation in model.methods, verification-clear in model.checks

2. CI security review guard is inverted (verification/workflow-verify-reviews.yaml, line 143)

   guard: "${{ data.latest('repo', 'diff').attributes.files.filter(f, f.startsWith('.forgejo/') || f.startsWith('.github/') || f.startsWith('scripts/')).size() == 0 }}"
   

   The guard evaluates to true (step runs) when the filtered file count is zero — meaning the CI security review runs on every PR that does not touch .forgejo/, .github/, or scripts/, and is skipped on PRs that do modify those paths. This is the opposite of the intended behavior documented in agent-constraints/verification-conventions.md and confirmed by the codegen guard pattern in workflow-verify-build.yaml line 310 (which runs when the flag is present, i.e., guard=true → run).

   Fix: change == 0 to > 0.

3. Stale verificationResult satisfies verification-clear gate after re-verify (issue-lifecycle/extensions/models/issue_lifecycle.ts, lines 503–547)

   The verify method does not clear or invalidate the existing verificationResult-main. This creates a gate bypass: after a successful verification round (verify → verification_passed, allPassed=true), if the agent calls verification_failed (returns to implementing), makes code changes, and then calls verify again (re-entering verifying), the stale verificationResult from the previous round still satisfies verification-clear. The agent can then call link_pr without ever calling verification_passed for the current round.

   Fix: either (a) have verify delete or overwrite verificationResult-main with allPassed=false to invalidate the previous result, or (b) have verification-clear compare the verifiedAt timestamp against the state's updatedAt to reject stale results.

Suggestions

1. Stale test description (issue-lifecycle/extensions/models/_lib/schemas_test.ts, line 40)

   The test is named "TRANSITIONS: link_pr accepts implementing, pr_open, and pr_failed" but the assertion checks ["verifying", "pr_open", "pr_failed"] — the phase implementing was replaced by verifying in this PR. The description should say verifying instead of implementing.

2. manifest.yaml Data section is incomplete (issue-lifecycle/manifest.yaml, line 80–82)

   The Data section lists state, context, classification, plan, feedback, adversarialReview, pullRequest but omits codeConformanceReview, verificationResult, and summary, all of which are registered resources in model.resources.

3. manifest.yaml state machine diagram is stale (issue-lifecycle/manifest.yaml, lines 37–38)

   The diagram shows:

   notify ──[notify]──> done  (thank external contributor)
   notify ──[skip_notify]──> done  (skip notification)
   

   But the actual behavior (since the 2026.07.30.1 upgrade) transitions both methods to summarizing, which then transitions to done via summarize. The diagram needs summarizing added between notify and done.

## Code Review ### Blocking Issues 1. **Missing tests for all new verification methods and check** (`issue-lifecycle/extensions/models/issue_lifecycle.ts`, `issue_lifecycle_test.ts`) The PR adds four new methods (`verify`, `verification_passed`, `verification_failed`, `post_attestation`) and one new check (`verification-clear`), plus the `verificationResult` resource — none of which have unit tests. Every other method in `issue_lifecycle_test.ts` has corresponding tests (including model-registration smoke tests). The new verification pathway is the most complex addition in this PR and is the gating mechanism before a PR can be opened; it needs test coverage. Missing tests: - `verify`: state transitions to `verifying`, lifecycle entry posted - `verification_passed`: `verificationResult-main` written with correct aggregated counts (`stepsCompleted`, `stepsFailed`, `stepsSkipped`, `allPassed`), state stays `verifying` - `verification_failed`: state transitions back to `implementing` - `post_attestation`: throws when swamp-club unreachable, throws on invalid JSON, posts attestation and lifecycle entry - `verification-clear` check: passes when `allPassed=true`, rejects when no result exists, rejects when `allPassed=false` - Smoke test: `verificationResult` in `model.resources`, `verify`/`verification_passed`/`verification_failed`/`post_attestation` in `model.methods`, `verification-clear` in `model.checks` 2. **CI security review guard is inverted** (`verification/workflow-verify-reviews.yaml`, line 143) ```yaml guard: "${{ data.latest('repo', 'diff').attributes.files.filter(f, f.startsWith('.forgejo/') || f.startsWith('.github/') || f.startsWith('scripts/')).size() == 0 }}" ``` The guard evaluates to `true` (step runs) when the filtered file count is **zero** — meaning the CI security review runs on every PR that does **not** touch `.forgejo/`, `.github/`, or `scripts/`, and is **skipped** on PRs that **do** modify those paths. This is the opposite of the intended behavior documented in `agent-constraints/verification-conventions.md` and confirmed by the codegen guard pattern in `workflow-verify-build.yaml` line 310 (which runs when the flag is present, i.e., guard=true → run). Fix: change `== 0` to `> 0`. 3. **Stale `verificationResult` satisfies `verification-clear` gate after re-verify** (`issue-lifecycle/extensions/models/issue_lifecycle.ts`, lines 503–547) The `verify` method does not clear or invalidate the existing `verificationResult-main`. This creates a gate bypass: after a successful verification round (`verify` → `verification_passed`, `allPassed=true`), if the agent calls `verification_failed` (returns to `implementing`), makes code changes, and then calls `verify` again (re-entering `verifying`), the stale `verificationResult` from the previous round still satisfies `verification-clear`. The agent can then call `link_pr` without ever calling `verification_passed` for the current round. Fix: either (a) have `verify` delete or overwrite `verificationResult-main` with `allPassed=false` to invalidate the previous result, or (b) have `verification-clear` compare the `verifiedAt` timestamp against the state's `updatedAt` to reject stale results. ### Suggestions 1. **Stale test description** (`issue-lifecycle/extensions/models/_lib/schemas_test.ts`, line 40) The test is named `"TRANSITIONS: link_pr accepts implementing, pr_open, and pr_failed"` but the assertion checks `["verifying", "pr_open", "pr_failed"]` — the phase `implementing` was replaced by `verifying` in this PR. The description should say `verifying` instead of `implementing`. 2. **`manifest.yaml` Data section is incomplete** (`issue-lifecycle/manifest.yaml`, line 80–82) The Data section lists `state`, `context`, `classification`, `plan`, `feedback`, `adversarialReview`, `pullRequest` but omits `codeConformanceReview`, `verificationResult`, and `summary`, all of which are registered resources in `model.resources`. 3. **`manifest.yaml` state machine diagram is stale** (`issue-lifecycle/manifest.yaml`, lines 37–38) The diagram shows: ``` notify ──[notify]──> done (thank external contributor) notify ──[skip_notify]──> done (skip notification) ``` But the actual behavior (since the 2026.07.30.1 upgrade) transitions both methods to `summarizing`, which then transitions to `done` via `summarize`. The diagram needs `summarizing` added between `notify` and `done`.

stack72 commented 2026-08-28 21:38:33 +00:00

Author

Owner

Copy link

Adversarial Review

Critical / High (if any)

No critical or high severity findings.

Medium

1. verification/workflow-verify-reviews.yaml:143 — ci-security-review guard condition appears inconsistent with codegen guard

   The ci-security-review step guard is:

   guard: "${{ data.latest('repo', 'diff').attributes.files.filter(f, f.startsWith('.forgejo/') || f.startsWith('.github/') || f.startsWith('scripts/')).size() == 0 }}"
   

   This evaluates to true when no CI-related files changed. Compare with the codegen-verify job guard in workflow-verify-build.yaml:311:

   guard: "${{ data.latest('build-detect-' + run.id, 'log').attributes.stdout.contains('CHANGED_CODEGEN: true') }}"
   

   This evaluates to true when codegen is changed. If guard=true has the same meaning in both places (most likely: "condition to run"), then the ci-security guard is inverted — it would run the review when there are NO CI changes and skip it when CI files actually change, which is the exact opposite of the documented intent (agent-constraints/verification-conventions.md line 61 shows the guard should fire on .forgejo/, .github/, scripts/ changes).

   Breaking scenario: A PR modifies .forgejo/workflows/*.yaml with a security-sensitive change. The ci-security-review step is skipped because the guard evaluates to false (matching files exist, so size() != 0). The security review that was specifically designed for this scenario never runs.

   If the workflow engine uses different guard semantics at the job vs. step level (job=run-when-true, step=skip-when-true), then both guards are correct. But this ambiguity should be verified against the workflow engine's documentation.

   Suggested fix: If guards have uniform semantics, change to size() > 0 (or size() != 0). Add a comment documenting the intended semantics.

2. issue-lifecycle/extensions/models/issue_lifecycle_test.ts — No tests for the 4 new methods or the verification-clear check

   This PR adds 4 new methods (verify, verification_passed, verification_failed, post_attestation) and a new check (verification-clear), but the test file diff only updates the version assertion and schemas test. Every other method in the model (link_pr, pr_merged, pr_failed, ship, notify, skip_notify, summarize, code_conformance_review, justify_deviations) has dedicated test cases. The new methods have zero test coverage.

   Breaking scenario: The verification_passed method computes allPassed: failed === 0 (line 1799). If this logic were accidentally changed to allPassed: failed > 0, there would be no test to catch the inversion — the verification gate would block all valid PRs and pass all invalid ones.

   Suggested fix: Add tests for at minimum: verify transitions to verifying, verification_passed writes the verificationResult with correct allPassed/counts, verification_failed transitions back to implementing, post_attestation throws on invalid JSON, and the verification-clear check rejects when allPassed is false and passes when true. Follow the existing test patterns (use buildTestContext).

3. issue-lifecycle/extensions/models/issue_lifecycle.ts:505 — verification-clear only gates link_pr, not complete

   The verification-clear check has appliesTo: ["link_pr"], but the code-conformance-clear check (line 435) applies to ["link_pr", "complete"]. This inconsistency means calling complete from the implementing phase bypasses verification entirely while still requiring code conformance review.

   This may be intentional — complete is a fallback for non-PR flows where pre-PR verification doesn't apply. But the asymmetry with code-conformance-clear (which does gate complete) suggests this was an oversight rather than a design choice.

   Breaking scenario: An agent calls complete from implementing without running verification. The code conformance check passes, but no build/test/review verification was performed. The lifecycle closes without any build validation.

   Suggested fix: Either add "complete" to the appliesTo array, or document why complete intentionally bypasses verification.

Low

1. verification/workflow-verify-build.yaml:502-503 — Fragile grep for model type extraction

   MODEL_TYPE=$(grep "type:" "$dir/extensions/models/"*.ts 2>/dev/null | grep -o '"[^"]*"' | head -1 | tr -d '"' || true)
   

   This matches the first quoted string on any line containing type: in any .ts file. It could match Zod schema fields (type: IssueType), interface properties, or import statements before hitting the actual model type declaration. If a model file has a type: reference in an import or schema before the model definition, the wrong string gets extracted.

   Suggested fix: Use a more specific pattern like grep "type:" "$dir/extensions/models/"*.ts | grep -v "import\|z\.\|schema\|Schema" or search for the model export pattern directly.

2. verification/workflow-verify-reviews.yaml:97-107 — Temp files not cleaned up on pipeline failure

   Each review step creates three temp files (DIFF_FILE, PROMPT_FILE, RESULT_FILE) but only cleans some of them conditionally. If set -o pipefail causes an early exit (e.g., claude -p fails), remaining temp files leak. This is harmless (they're in /tmp) but untidy.

   Suggested fix: Add a trap 'rm -f "$DIFF_FILE" "$PROMPT_FILE" "$RESULT_FILE"' EXIT after creating them.

Verdict

PASS — The code is structurally sound. The new verification phase integrates cleanly into the existing state machine, the transition constraints are correct, the upgrade entry matches the version bump, and the schema additions are consistent. The ci-security guard ambiguity (Medium #1) depends on workflow engine semantics that I can't fully verify. The missing tests (Medium #2) are a real gap but don't indicate code incorrectly. No security vulnerabilities, data integrity issues, or correctness bugs were found in the production code paths.

## Adversarial Review ### Critical / High (if any) No critical or high severity findings. ### Medium 1. **`verification/workflow-verify-reviews.yaml:143` — ci-security-review guard condition appears inconsistent with codegen guard** The ci-security-review step guard is: ```yaml guard: "${{ data.latest('repo', 'diff').attributes.files.filter(f, f.startsWith('.forgejo/') || f.startsWith('.github/') || f.startsWith('scripts/')).size() == 0 }}" ``` This evaluates to `true` when **no** CI-related files changed. Compare with the codegen-verify **job** guard in `workflow-verify-build.yaml:311`: ```yaml guard: "${{ data.latest('build-detect-' + run.id, 'log').attributes.stdout.contains('CHANGED_CODEGEN: true') }}" ``` This evaluates to `true` when codegen **is** changed. If `guard=true` has the same meaning in both places (most likely: "condition to run"), then the ci-security guard is inverted — it would run the review when there are NO CI changes and **skip it when CI files actually change**, which is the exact opposite of the documented intent (`agent-constraints/verification-conventions.md` line 61 shows the guard should fire on `.forgejo/`, `.github/`, `scripts/` changes). **Breaking scenario**: A PR modifies `.forgejo/workflows/*.yaml` with a security-sensitive change. The ci-security-review step is skipped because the guard evaluates to `false` (matching files exist, so `size() != 0`). The security review that was specifically designed for this scenario never runs. **If** the workflow engine uses different guard semantics at the job vs. step level (job=run-when-true, step=skip-when-true), then both guards are correct. But this ambiguity should be verified against the workflow engine's documentation. **Suggested fix**: If guards have uniform semantics, change to `size() > 0` (or `size() != 0`). Add a comment documenting the intended semantics. 2. **`issue-lifecycle/extensions/models/issue_lifecycle_test.ts` — No tests for the 4 new methods or the verification-clear check** This PR adds 4 new methods (`verify`, `verification_passed`, `verification_failed`, `post_attestation`) and a new check (`verification-clear`), but the test file diff only updates the version assertion and schemas test. Every other method in the model (`link_pr`, `pr_merged`, `pr_failed`, `ship`, `notify`, `skip_notify`, `summarize`, `code_conformance_review`, `justify_deviations`) has dedicated test cases. The new methods have zero test coverage. **Breaking scenario**: The `verification_passed` method computes `allPassed: failed === 0` (line 1799). If this logic were accidentally changed to `allPassed: failed > 0`, there would be no test to catch the inversion — the verification gate would block all valid PRs and pass all invalid ones. **Suggested fix**: Add tests for at minimum: `verify` transitions to verifying, `verification_passed` writes the verificationResult with correct allPassed/counts, `verification_failed` transitions back to implementing, `post_attestation` throws on invalid JSON, and the `verification-clear` check rejects when allPassed is false and passes when true. Follow the existing test patterns (use `buildTestContext`). 3. **`issue-lifecycle/extensions/models/issue_lifecycle.ts:505` — `verification-clear` only gates `link_pr`, not `complete`** The `verification-clear` check has `appliesTo: ["link_pr"]`, but the `code-conformance-clear` check (line 435) applies to `["link_pr", "complete"]`. This inconsistency means calling `complete` from the `implementing` phase bypasses verification entirely while still requiring code conformance review. This may be intentional — `complete` is a fallback for non-PR flows where pre-PR verification doesn't apply. But the asymmetry with `code-conformance-clear` (which does gate `complete`) suggests this was an oversight rather than a design choice. **Breaking scenario**: An agent calls `complete` from `implementing` without running verification. The code conformance check passes, but no build/test/review verification was performed. The lifecycle closes without any build validation. **Suggested fix**: Either add `"complete"` to the `appliesTo` array, or document why `complete` intentionally bypasses verification. ### Low 1. **`verification/workflow-verify-build.yaml:502-503` — Fragile grep for model type extraction** ```bash MODEL_TYPE=$(grep "type:" "$dir/extensions/models/"*.ts 2>/dev/null | grep -o '"[^"]*"' | head -1 | tr -d '"' || true) ``` This matches the first quoted string on any line containing `type:` in any `.ts` file. It could match Zod schema fields (`type: IssueType`), interface properties, or import statements before hitting the actual model type declaration. If a model file has a `type:` reference in an import or schema before the model definition, the wrong string gets extracted. **Suggested fix**: Use a more specific pattern like `grep "type:" "$dir/extensions/models/"*.ts | grep -v "import\|z\.\|schema\|Schema"` or search for the model export pattern directly. 2. **`verification/workflow-verify-reviews.yaml:97-107` — Temp files not cleaned up on pipeline failure** Each review step creates three temp files (`DIFF_FILE`, `PROMPT_FILE`, `RESULT_FILE`) but only cleans some of them conditionally. If `set -o pipefail` causes an early exit (e.g., `claude -p` fails), remaining temp files leak. This is harmless (they're in `/tmp`) but untidy. **Suggested fix**: Add a `trap 'rm -f "$DIFF_FILE" "$PROMPT_FILE" "$RESULT_FILE"' EXIT` after creating them. ### Verdict **PASS** — The code is structurally sound. The new verification phase integrates cleanly into the existing state machine, the transition constraints are correct, the upgrade entry matches the version bump, and the schema additions are consistent. The ci-security guard ambiguity (Medium #1) depends on workflow engine semantics that I can't fully verify. The missing tests (Medium #2) are a real gap but don't indicate code incorrectly. No security vulnerabilities, data integrity issues, or correctness bugs were found in the production code paths.

stack72 added 1 commit 2026-08-28 21:45:41 +00:00

fix: address review feedback on verification PR ... 01e2da50a3

Blockers fixed:
- Add 15 tests for verify, verification_passed, verification_failed,
  post_attestation methods and verification-clear check (75 total now)
- Fix inverted ci-security guard (== 0 → > 0) so the review runs
  when CI files changed, not when they didn't
- Fix stale verificationResult bypass: verify() now invalidates the
  previous result with allPassed=false so a stale passing result
  cannot satisfy the verification-clear gate

Suggestions fixed:
- Fix stale test description (implementing → verifying)
- Add codeConformanceReview, verificationResult, summary to manifest
  Data section
- Fix stale state machine diagram (notify → summarizing → done)
- Add "complete" to verification-clear appliesTo for consistency
  with code-conformance-clear
- Use more specific grep pattern for model type extraction
- Add trap-based temp file cleanup to review workflow steps

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

stack72 commented 2026-08-28 21:54:55 +00:00

Author

Owner

Copy link

Code Review

Blocking Issues

1. post_attestation is described as a "hard gate" but no client-side check enforces it (issue_lifecycle.ts:1930–1935).

   The method description says: "The PR must not open without a stored attestation — this is a hard gate." However, the model's checks object has no attestation-clear (or equivalent) entry with appliesTo: ["link_pr"]. The existing verification-clear check (line 503) only verifies allPassed, not that post_attestation was ever called. An agent can legally call link_pr from the verifying phase without ever calling post_attestation, satisfying every check that runs before link_pr.

   The description and the implementation directly contradict each other. One of these must change:

   • Option A (preferred): Add an attestation-clear check that reads an attestation-main resource written by post_attestation, gated on appliesTo: ["link_pr"].
   • Option B: Update the description to say the gate is procedural (agent convention + server-side enforcement), not a client-side technical gate, and remove the word "hard gate" to avoid setting false expectations.

   If Option B is correct, the conventions doc and the method description should also be updated to explain where the enforcement actually lives.

Suggestions

1. Stale method descriptions in manifest.yaml (lines 73–76): Both notify and skip_notify are described as transitioning "directly to done," but they actually transition to summarizing (the state machine diagram in the same manifest is correct). Since the manifest is changed in this PR, this is a good opportunity to fix:

   • notify → "transition to summarizing"
   • skip_notify → "transition directly to summarizing"

2. Missing success-path test for post_attestation (issue_lifecycle_test.ts): Only two error-path tests exist (unreachable swamp-club, invalid JSON). There is no test for the happy path — successful postAttestation call, response parsing (id, postedBy, postedAt), or the lifecycle entry that follows. Given the method is described as a critical gate, a local Deno.serve({ port: 0 }) stub for /healthz and /api/v1/admin/attestations would cover the success path without live dependencies.

3. verification_passed name vs. behavior: The method accepts a steps array that may include "failed" entries and computes allPassed: failed === 0 from the data. A caller can invoke it with all-failed steps and it will store allPassed: false — the name implies the caller has confirmed success, but the method does not enforce that. The description is clear, but the test "verification_passed: allPassed is false when any step failed" highlights the mismatch. Consider whether a runtime guard (throw if every non-skipped step failed) or a name change (record_verification_results) would better express the intent.

## Code Review ### Blocking Issues 1. **`post_attestation` is described as a "hard gate" but no client-side check enforces it** (`issue_lifecycle.ts:1930–1935`). The method description says: _"The PR must not open without a stored attestation — this is a hard gate."_ However, the model's `checks` object has no `attestation-clear` (or equivalent) entry with `appliesTo: ["link_pr"]`. The existing `verification-clear` check (line 503) only verifies `allPassed`, not that `post_attestation` was ever called. An agent can legally call `link_pr` from the `verifying` phase without ever calling `post_attestation`, satisfying every check that runs before `link_pr`. The description and the implementation directly contradict each other. One of these must change: - **Option A (preferred):** Add an `attestation-clear` check that reads an `attestation-main` resource written by `post_attestation`, gated on `appliesTo: ["link_pr"]`. - **Option B:** Update the description to say the gate is procedural (agent convention + server-side enforcement), not a client-side technical gate, and remove the word "hard gate" to avoid setting false expectations. If Option B is correct, the conventions doc and the method description should also be updated to explain _where_ the enforcement actually lives. ### Suggestions 1. **Stale method descriptions in `manifest.yaml` (lines 73–76):** Both `notify` and `skip_notify` are described as transitioning "directly to `done`," but they actually transition to `summarizing` (the state machine diagram in the same manifest is correct). Since the manifest is changed in this PR, this is a good opportunity to fix: - `notify` → "transition to `summarizing`" - `skip_notify` → "transition directly to `summarizing`" 2. **Missing success-path test for `post_attestation`** (`issue_lifecycle_test.ts`): Only two error-path tests exist (unreachable swamp-club, invalid JSON). There is no test for the happy path — successful `postAttestation` call, response parsing (`id`, `postedBy`, `postedAt`), or the lifecycle entry that follows. Given the method is described as a critical gate, a local `Deno.serve({ port: 0 })` stub for `/healthz` and `/api/v1/admin/attestations` would cover the success path without live dependencies. 3. **`verification_passed` name vs. behavior**: The method accepts a `steps` array that may include `"failed"` entries and computes `allPassed: failed === 0` from the data. A caller can invoke it with all-failed steps and it will store `allPassed: false` — the name implies the caller has confirmed success, but the method does not enforce that. The description is clear, but the test `"verification_passed: allPassed is false when any step failed"` highlights the mismatch. Consider whether a runtime guard (throw if every non-skipped step failed) or a name change (`record_verification_results`) would better express the intent.

stack72 added 1 commit 2026-08-28 22:41:26 +00:00

fix: add attestation-clear gate and address second review round ... 587757fa5f

Blocker: post_attestation claimed "hard gate" but had no check
enforcing it. Added:
- attestation resource written by post_attestation
- attestation-clear check gating link_pr (rejects missing or
  invalidated attestations)
- verify() now invalidates both verificationResult AND attestation
  so stale results from a previous round cannot bypass either gate

Tests added (81 total now):
- post_attestation success path (mock server, attestation resource)
- attestation-clear: passes/rejects-missing/rejects-invalidated
- Model smoke tests for attestation resource and check
- Updated verify test to assert attestation invalidation

Suggestions:
- Fixed stale notify/skip_notify descriptions in manifest
- Added summarize method to manifest method list
- Added attestation to manifest Data section

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

stack72 commented 2026-08-28 22:50:41 +00:00

Author

Owner

Copy link

Adversarial Review

Critical / High

1. schemas.ts:62-74 — start transition missing verifying phase, breaking the resume invariant

   The start transition lists every phase except done and verifying as valid source phases. The manifest documents: "start can resume from any phase except done." Every other non-done phase is included — verifying is the sole omission.

   Breaking example: An agent enters the verifying phase (by calling verify), then crashes or loses its session. The user tries to resume with start. The valid-transition check fires, sees phase is verifying, and rejects with: Method start cannot run in phase verifying. Allowed phases: created, triaging, etc. There is no other method that can be called from verifying to escape to a start-resumable phase without completing the full verification loop (verification_failed goes to implementing, but that requires knowing to call it — and the resume flow is specifically designed so start handles recovery from any stuck phase).

   The existing schemas_test.ts test at line 50 only spot-checks a few phases and does not assert the complete set, so it misses this gap.

   Suggested fix: Add verifying to the start transition array in schemas.ts between implementing and pr_open. And add a corresponding assertion in schemas_test.ts.

Medium

1. issue_lifecycle.ts:1850-1856 — verification_passed accepts an empty steps array, producing allPassed true with zero verification work

   The steps argument uses z.array with no .min(1) constraint. If called with an empty steps array, the aggregation at lines 1887-1890 produces succeeded=0, failed=0, so allPassed is true (failed === 0). This writes a verification result that passes the verification-clear gate (line 552) despite no verification steps having executed.

   Breaking example: An agent (buggy or adversarial) calls verification_passed with workflowRunId run-x, commit abc, branch b, and an empty steps array. The verification-clear gate passes. Combined with a posted attestation, link_pr succeeds with zero actual verification.

   Suggested fix: Add .min(1) to the steps array in the verification_passed arguments schema, or add a guard in the execute body that throws when args.steps.length is 0.

2. issue_lifecycle.ts:518-563 — verification-clear check applies to complete but the implementing shortcut path cannot satisfy it

   The complete transition accepts implementing, pr_open, releasing (line 92). The verification-clear check has appliesTo link_pr and complete (line 523). If an agent calls complete from implementing (the no-PR-needed shortcut path), the check demands a passing verification result. But the verify method (which enters verifying phase) invalidates prior results, and verification_passed stays in verifying, not implementing. So having allPassed true while in implementing phase is essentially impossible through the normal flow.

   This may be intentional (forcing verification even for the shortcut path), but if so the error message should say so explicitly rather than leaving the agent in a confusing dead-end. If unintentional, remove complete from the verification-clear appliesTo list.

Low

1. workflow-verify-reviews.yaml:94,121,148 — Review scripts use set -o pipefail but not set -e, masking upstream failures

   If git diff fails (e.g., corrupt repo, permissions), the diff file is empty. The script continues to invoke claude with an empty diff. Claude likely says there is nothing to review, and the verdict check reports review did not pass rather than the actual root cause. Not a correctness issue (the step still fails), but the error message is misleading.

2. issue_lifecycle.ts:2046-2049 — post_attestation accepts non-object JSON without validation

   JSON.parse could return a primitive (number, string, boolean) if the input is valid JSON but not an object. The cast to Record does not enforce object shape. postAttestation would then send a non-object body, which the API would reject with an unhelpful HTTP error. A typeof check would surface a clearer error.

Verdict

FAIL — The missing verifying phase in the start transition is a HIGH-severity bug that breaks the documented resume-from-any-phase invariant. An agent crash during verification leaves the lifecycle stuck with no standard recovery path.

## Adversarial Review ### Critical / High 1. **schemas.ts:62-74 — start transition missing verifying phase, breaking the resume invariant** The start transition lists every phase except done and verifying as valid source phases. The manifest documents: "start can resume from any phase except done." Every other non-done phase is included — verifying is the sole omission. Breaking example: An agent enters the verifying phase (by calling verify), then crashes or loses its session. The user tries to resume with start. The valid-transition check fires, sees phase is verifying, and rejects with: Method start cannot run in phase verifying. Allowed phases: created, triaging, etc. There is no other method that can be called from verifying to escape to a start-resumable phase without completing the full verification loop (verification_failed goes to implementing, but that requires knowing to call it — and the resume flow is specifically designed so start handles recovery from any stuck phase). The existing schemas_test.ts test at line 50 only spot-checks a few phases and does not assert the complete set, so it misses this gap. Suggested fix: Add verifying to the start transition array in schemas.ts between implementing and pr_open. And add a corresponding assertion in schemas_test.ts. ### Medium 1. **issue_lifecycle.ts:1850-1856 — verification_passed accepts an empty steps array, producing allPassed true with zero verification work** The steps argument uses z.array with no .min(1) constraint. If called with an empty steps array, the aggregation at lines 1887-1890 produces succeeded=0, failed=0, so allPassed is true (failed === 0). This writes a verification result that passes the verification-clear gate (line 552) despite no verification steps having executed. Breaking example: An agent (buggy or adversarial) calls verification_passed with workflowRunId run-x, commit abc, branch b, and an empty steps array. The verification-clear gate passes. Combined with a posted attestation, link_pr succeeds with zero actual verification. Suggested fix: Add .min(1) to the steps array in the verification_passed arguments schema, or add a guard in the execute body that throws when args.steps.length is 0. 2. **issue_lifecycle.ts:518-563 — verification-clear check applies to complete but the implementing shortcut path cannot satisfy it** The complete transition accepts implementing, pr_open, releasing (line 92). The verification-clear check has appliesTo link_pr and complete (line 523). If an agent calls complete from implementing (the no-PR-needed shortcut path), the check demands a passing verification result. But the verify method (which enters verifying phase) invalidates prior results, and verification_passed stays in verifying, not implementing. So having allPassed true while in implementing phase is essentially impossible through the normal flow. This may be intentional (forcing verification even for the shortcut path), but if so the error message should say so explicitly rather than leaving the agent in a confusing dead-end. If unintentional, remove complete from the verification-clear appliesTo list. ### Low 1. **workflow-verify-reviews.yaml:94,121,148 — Review scripts use set -o pipefail but not set -e, masking upstream failures** If git diff fails (e.g., corrupt repo, permissions), the diff file is empty. The script continues to invoke claude with an empty diff. Claude likely says there is nothing to review, and the verdict check reports review did not pass rather than the actual root cause. Not a correctness issue (the step still fails), but the error message is misleading. 2. **issue_lifecycle.ts:2046-2049 — post_attestation accepts non-object JSON without validation** JSON.parse could return a primitive (number, string, boolean) if the input is valid JSON but not an object. The cast to Record does not enforce object shape. postAttestation would then send a non-object body, which the API would reject with an unhelpful HTTP error. A typeof check would surface a clearer error. ### Verdict FAIL — The missing verifying phase in the start transition is a HIGH-severity bug that breaks the documented resume-from-any-phase invariant. An agent crash during verification leaves the lifecycle stuck with no standard recovery path.

stack72 commented 2026-08-28 22:52:01 +00:00

Author

Owner

Copy link

Code Review

Blocking Issues

None.

Suggestions

1. verifying missing from TRANSITIONS.start — If an agent process crashes while the lifecycle is in the verifying phase, it cannot call start to resume. Recovery requires calling verification_failed (which is allowed from verifying) to return to implementing, and then either calling start (which resets all the way to triaging) or continuing from implementing. The missing phase means crash-recovery from verifying has no clean "resume" path — only verification_failed → implementing, which discards the in-flight verification. Adding "verifying" to TRANSITIONS.start would make the recovery story consistent with all other mid-lifecycle phases already listed there.

2. verification-clear on complete from implementing is practically unsatisfiable via normal flows — The verification-clear check applies to both link_pr and complete. To satisfy it, a passing verificationResult must exist (written by verification_passed). But verification_passed requires verifying phase (via TRANSITIONS), and to reach implementing from verifying you must call verification_failed — which signals that verification failed. The only way to satisfy verification-clear for complete from implementing is to: run verification, pass it, then deliberately call verification_failed to return to implementing (semantically contradictory), and then call complete. The effective result is that complete from implementing is a dead path post-this-PR. If that's intentional (removing the shortcut), the upgrade description could say so explicitly so agents know to use the full verify → verification_passed → post_attestation → link_pr path instead.

3. workflow-verify-reviews.yaml guard uses data.latest('repo', 'diff') — The guard on ci-security-review (line 141) references the data type diff from model repo. Two preceding steps in detect-changes both call methodName: diff on repo — one without nameOnly and one with nameOnly: true. The guard depends on .attributes.files being a list of file paths, which comes from the nameOnly variant. The behavior of data.latest when two steps write to the same model/data-type slot is not obvious from the YAML; if the non-nameOnly output is returned instead, the guard expression would fail silently and ci-security-review would never trigger. Worth confirming (or renaming the data types in the two steps) that the guard reliably gets the name-only output.

4. verification_passed accepts failing steps without complaint — The method is named verification_passed but it accepts a steps array that may contain "failed" entries and computes allPassed: failed === 0. If an agent accidentally calls verification_passed with failed steps, allPassed will be false and the verification-clear gate will block, so there is no security consequence — but the method name is misleading. A Zod refinement rejecting the call when any step has status: "failed" would make the contract explicit and catch agent mistakes earlier.

## Code Review ### Blocking Issues None. ### Suggestions 1. **`verifying` missing from `TRANSITIONS.start`** — If an agent process crashes while the lifecycle is in the `verifying` phase, it cannot call `start` to resume. Recovery requires calling `verification_failed` (which is allowed from `verifying`) to return to `implementing`, and then either calling `start` (which resets all the way to `triaging`) or continuing from `implementing`. The missing phase means crash-recovery from `verifying` has no clean "resume" path — only `verification_failed` → `implementing`, which discards the in-flight verification. Adding `"verifying"` to `TRANSITIONS.start` would make the recovery story consistent with all other mid-lifecycle phases already listed there. 2. **`verification-clear` on `complete` from `implementing` is practically unsatisfiable via normal flows** — The `verification-clear` check applies to both `link_pr` and `complete`. To satisfy it, a passing `verificationResult` must exist (written by `verification_passed`). But `verification_passed` requires `verifying` phase (via TRANSITIONS), and to reach `implementing` from `verifying` you must call `verification_failed` — which signals that verification *failed*. The only way to satisfy `verification-clear` for `complete` from `implementing` is to: run verification, pass it, then deliberately call `verification_failed` to return to `implementing` (semantically contradictory), and then call `complete`. The effective result is that `complete` from `implementing` is a dead path post-this-PR. If that's intentional (removing the shortcut), the upgrade description could say so explicitly so agents know to use the full `verify → verification_passed → post_attestation → link_pr` path instead. 3. **`workflow-verify-reviews.yaml` guard uses `data.latest('repo', 'diff')`** — The guard on `ci-security-review` (line 141) references the data type `diff` from model `repo`. Two preceding steps in `detect-changes` both call `methodName: diff` on `repo` — one without `nameOnly` and one with `nameOnly: true`. The guard depends on `.attributes.files` being a list of file paths, which comes from the `nameOnly` variant. The behavior of `data.latest` when two steps write to the same model/data-type slot is not obvious from the YAML; if the non-nameOnly output is returned instead, the guard expression would fail silently and `ci-security-review` would never trigger. Worth confirming (or renaming the data types in the two steps) that the guard reliably gets the name-only output. 4. **`verification_passed` accepts failing steps without complaint** — The method is named `verification_passed` but it accepts a `steps` array that may contain `"failed"` entries and computes `allPassed: failed === 0`. If an agent accidentally calls `verification_passed` with failed steps, `allPassed` will be `false` and the `verification-clear` gate will block, so there is no security consequence — but the method name is misleading. A Zod refinement rejecting the call when any step has `status: "failed"` would make the contract explicit and catch agent mistakes earlier.

stack72 added 1 commit 2026-08-28 22:59:00 +00:00

fix: add verifying to start transition for session resume ... 8868aefe0e

An agent stuck in the verifying phase (crash, lost session) could not
resume with start because verifying was missing from the start
transition array. Added it between implementing and pr_open.

Updated the schemas test to assert the complete start transition set
(every phase except done) instead of spot-checking, so future phase
additions cannot silently break the resume invariant.

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

stack72 commented 2026-08-28 23:06:31 +00:00

Author

Owner

Copy link

Code Review

Blocking Issues (if any)

None.

Suggestions (if any)

1. Misleading error messages when complete fails verification-clear or code-conformance-clear
   (issue-lifecycle/extensions/models/issue_lifecycle.ts, checks at lines 518–563 and 446–516)

   Both verification-clear and code-conformance-clear apply to ["link_pr", "complete"], but their error messages refer only to "linking a PR":

   • "No verification result exists. Run 'verify' and then 'verification_passed' before linking a PR."
   • "No code conformance review exists. Run 'code_conformance_review' before linking a PR."

   An agent calling complete (e.g., from implementing or releasing) will receive an error telling them to do something "before linking a PR," which is confusing when they aren't trying to link a PR. Consider messages like "…before linking a PR or completing the lifecycle."

2. verification_passed method name implies unconditional success but records partial failures
   (issue-lifecycle/extensions/models/issue_lifecycle.ts, line 1841)

   The method accepts steps that include "failed" status entries and sets allPassed: failed === 0. Calling verification_passed with a failed step yields allPassed: false, which then blocks link_pr via verification-clear. The test "verification_passed: allPassed is false when any step failed" confirms this behaviour. The name verification_passed implies the caller has already confirmed all steps passed, but in practice it's more of a "record verification results" method. This is unlikely to cause a correctness bug in practice (the verification-clear gate catches it), but the semantics may surprise future maintainers.

3. ci-security-review guard does not cover verification/ workflow files
   (verification/workflow-verify-reviews.yaml, line 141)

   The guard fires only when .forgejo/, .github/, or scripts/ files change:

   guard: "${{ data.latest('repo', 'diff').attributes.files.filter(f, f.startsWith('.forgejo/') || f.startsWith('.github/') || f.startsWith('scripts/')).size() > 0 }}"
   

   The newly added verification/workflow-verify-build.yaml and verification/workflow-verify-reviews.yaml contain substantial shell scripts. Changes to those files will not trigger the dedicated CI security review (though they remain covered by code-review and adversarial-review). Consider adding verification/ to the guard if you want the stricter CI-focused review to apply to workflow changes.

4. verify method writes resources sequentially; minor efficiency loss
   (issue-lifecycle/extensions/models/issue_lifecycle.ts, lines 1781–1815)

   The three await context.writeResource(...) calls in verify are sequential. Since they are independent writes, they could be parallelised with Promise.all. This is purely a performance note and not a correctness issue.

## Code Review ### Blocking Issues (if any) None. ### Suggestions (if any) 1. **Misleading error messages when `complete` fails `verification-clear` or `code-conformance-clear`** (`issue-lifecycle/extensions/models/issue_lifecycle.ts`, checks at lines 518–563 and 446–516) Both `verification-clear` and `code-conformance-clear` apply to `["link_pr", "complete"]`, but their error messages refer only to "linking a PR": - `"No verification result exists. Run 'verify' and then 'verification_passed' before linking a PR."` - `"No code conformance review exists. Run 'code_conformance_review' before linking a PR."` An agent calling `complete` (e.g., from `implementing` or `releasing`) will receive an error telling them to do something "before linking a PR," which is confusing when they aren't trying to link a PR. Consider messages like "…before linking a PR or completing the lifecycle." 2. **`verification_passed` method name implies unconditional success but records partial failures** (`issue-lifecycle/extensions/models/issue_lifecycle.ts`, line 1841) The method accepts `steps` that include `"failed"` status entries and sets `allPassed: failed === 0`. Calling `verification_passed` with a failed step yields `allPassed: false`, which then blocks `link_pr` via `verification-clear`. The test "verification_passed: allPassed is false when any step failed" confirms this behaviour. The name `verification_passed` implies the caller has already confirmed all steps passed, but in practice it's more of a "record verification results" method. This is unlikely to cause a correctness bug in practice (the `verification-clear` gate catches it), but the semantics may surprise future maintainers. 3. **`ci-security-review` guard does not cover `verification/` workflow files** (`verification/workflow-verify-reviews.yaml`, line 141) The guard fires only when `.forgejo/`, `.github/`, or `scripts/` files change: ```yaml guard: "${{ data.latest('repo', 'diff').attributes.files.filter(f, f.startsWith('.forgejo/') || f.startsWith('.github/') || f.startsWith('scripts/')).size() > 0 }}" ``` The newly added `verification/workflow-verify-build.yaml` and `verification/workflow-verify-reviews.yaml` contain substantial shell scripts. Changes to those files will not trigger the dedicated CI security review (though they remain covered by code-review and adversarial-review). Consider adding `verification/` to the guard if you want the stricter CI-focused review to apply to workflow changes. 4. **`verify` method writes resources sequentially; minor efficiency loss** (`issue-lifecycle/extensions/models/issue_lifecycle.ts`, lines 1781–1815) The three `await context.writeResource(...)` calls in `verify` are sequential. Since they are independent writes, they could be parallelised with `Promise.all`. This is purely a performance note and not a correctness issue.

stack72 commented 2026-08-28 23:07:07 +00:00

Author

Owner

Copy link

Adversarial Review

Critical / High

None found.

Medium

1. post_attestation accesses parsed.subject and parsed.gate with unsafe casting, producing undefined silently if the attestation JSON has a different structure.

   File: issue-lifecycle/extensions/models/issue_lifecycle.ts:2062-2063
   What is wrong: The post_attestation method casts parsed.subject and parsed.gate to Record and accesses .commit and .allPassed without any validation. If the caller supplies a valid JSON object that lacks subject or gate keys, the commit and gatePassed fields in the stored attestation will be undefined.
   Breaking example: Calling post_attestation with attestation containing only a version field writes the attestation resource with commit: undefined and gatePassed: undefined. This does not crash, but the attestation record silently has no commit linkage.
   Suggested fix: Log a warning when the commit field is falsy, since a commitless attestation is operationally useless.

2. verification_passed method name is misleading when called with failed steps.

   File: issue-lifecycle/extensions/models/issue_lifecycle.ts:1897-1900
   What is wrong: The verification_passed method accepts steps with status failed and correctly sets allPassed: false. However, calling verification_passed with failing steps is semantically confusing.
   Breaking example: An agent calls verification_passed with a mix of succeeded and failed steps. The lifecycle entry says Verification passed with a checkmark emoji while the verification actually failed. The gate correctly blocks, but the lifecycle log on swamp-club will be contradictory.
   Suggested fix: Only post the passed lifecycle entry when allPassed is true.

3. codegen-verify guard uses expression that reads raw stdout, which may break if detect-changes output format changes.

   File: verification/workflow-verify-build.yaml:310
   What is wrong: The guard expression reads from the stdout of the detect-changes step and does a string contains check. This is a string match on raw stdout.
   Suggested fix: No immediate fix needed. Low-risk coupling to be aware of when modifying the detect-changes step.

Low

1. verify method does not validate commit SHA format.

   File: issue-lifecycle/extensions/models/issue_lifecycle.ts:1755-1756
   The commit argument accepts any string including empty strings.

2. Upgrade path test swallows all method execution failures.

   File: verification/workflow-verify-build.yaml:522-525
   By design per the inline comment. The subsequent typeVersion check is the real validation gate.

3. ci-security-review guard does not cover verification/ directory.

   File: verification/workflow-verify-reviews.yaml:141
   The guard filters on .forgejo/, .github/, and scripts/ but the verification/ workflow files would not trigger the CI security review.

Verdict

PASS - The code is well-structured. The state machine transitions are correctly updated with the new verifying phase. All new methods (verify, verification_passed, verification_failed, post_attestation) have corresponding tests. Checks are properly gated. The invalidation logic in verify() correctly prevents stale results from satisfying gates. No critical or high-severity issues found.

## Adversarial Review ### Critical / High None found. ### Medium 1. post_attestation accesses parsed.subject and parsed.gate with unsafe casting, producing undefined silently if the attestation JSON has a different structure. File: issue-lifecycle/extensions/models/issue_lifecycle.ts:2062-2063 What is wrong: The post_attestation method casts parsed.subject and parsed.gate to Record and accesses .commit and .allPassed without any validation. If the caller supplies a valid JSON object that lacks subject or gate keys, the commit and gatePassed fields in the stored attestation will be undefined. Breaking example: Calling post_attestation with attestation containing only a version field writes the attestation resource with commit: undefined and gatePassed: undefined. This does not crash, but the attestation record silently has no commit linkage. Suggested fix: Log a warning when the commit field is falsy, since a commitless attestation is operationally useless. 2. verification_passed method name is misleading when called with failed steps. File: issue-lifecycle/extensions/models/issue_lifecycle.ts:1897-1900 What is wrong: The verification_passed method accepts steps with status failed and correctly sets allPassed: false. However, calling verification_passed with failing steps is semantically confusing. Breaking example: An agent calls verification_passed with a mix of succeeded and failed steps. The lifecycle entry says Verification passed with a checkmark emoji while the verification actually failed. The gate correctly blocks, but the lifecycle log on swamp-club will be contradictory. Suggested fix: Only post the passed lifecycle entry when allPassed is true. 3. codegen-verify guard uses expression that reads raw stdout, which may break if detect-changes output format changes. File: verification/workflow-verify-build.yaml:310 What is wrong: The guard expression reads from the stdout of the detect-changes step and does a string contains check. This is a string match on raw stdout. Suggested fix: No immediate fix needed. Low-risk coupling to be aware of when modifying the detect-changes step. ### Low 1. verify method does not validate commit SHA format. File: issue-lifecycle/extensions/models/issue_lifecycle.ts:1755-1756 The commit argument accepts any string including empty strings. 2. Upgrade path test swallows all method execution failures. File: verification/workflow-verify-build.yaml:522-525 By design per the inline comment. The subsequent typeVersion check is the real validation gate. 3. ci-security-review guard does not cover verification/ directory. File: verification/workflow-verify-reviews.yaml:141 The guard filters on .forgejo/, .github/, and scripts/ but the verification/ workflow files would not trigger the CI security review. ### Verdict PASS - The code is well-structured. The state machine transitions are correctly updated with the new verifying phase. All new methods (verify, verification_passed, verification_failed, post_attestation) have corresponding tests. Checks are properly gated. The invalidation logic in verify() correctly prevents stale results from satisfying gates. No critical or high-severity issues found.

stack72 added 1 commit 2026-08-30 03:02:23 +00:00

fix: address third review round suggestions ... 11a5a8b579

- Fix misleading error messages: verification-clear and
  code-conformance-clear now say "before linking a PR or completing
  the lifecycle" since they gate both link_pr and complete
- Add verification/ to ci-security-review guard so workflow file
  changes get the stricter security review
- Fix verification_passed posting contradictory "passed" lifecycle
  entry when steps actually failed: now posts "results recorded"
  with a warning emoji when allPassed is false

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

stack72 commented 2026-08-30 03:10:03 +00:00

Author

Owner

Copy link

Code Review

Blocking Issues

None.

Suggestions

1. post_attestation tests with a real HTTP server may need sanitizeResources: false (issue_lifecycle_test.ts, lines 1616–1710): Two tests (throws on invalid JSON input and writes attestation resource on success) start a Deno.serve server and invoke createSwampClubClient, which makes fetch requests. Deno's fetch maintains an implicit keepalive connection pool. CLAUDE.md requires sanitizeResources: false (with an explanatory comment) for tests that create HTTP clients with connection pooling. The servers are properly aborted and awaited, so the tests likely pass in practice, but a future Deno version tightening resource tracking could cause spurious failures. Suggested fix: add sanitizeResources: false to both test options with a comment like // SwampClubClient uses fetch which maintains a connection pool.

2. verification_passed method is callable with failed steps (issue_lifecycle.ts, line 1841): The method name implies success, but the implementation accepts steps with status: "failed" and writes allPassed: false to the result resource. The actual gate is enforced by the verification-clear check. A brief inline comment on the method description noting that this method records results regardless of outcome (the gate is elsewhere) would prevent future confusion about when this vs. verification_failed should be called.

3. inputs.commit used unvalidated in shell commands (workflow-verify-build.yaml line 45, workflow-verify-reviews.yaml line 45): ${{ inputs.commit }} is interpolated directly into git worktree add --detach "$CHECKOUT_DIR" "${{ inputs.commit }}". While these workflows are agent-invoked rather than webhook-triggered (limiting practical exposure), validating that the commit value matches ^[0-9a-f]{40}$ before use would be a safe defensive practice consistent with the security posture of the review prompts.

## Code Review ### Blocking Issues None. ### Suggestions 1. **`post_attestation` tests with a real HTTP server may need `sanitizeResources: false`** (`issue_lifecycle_test.ts`, lines 1616–1710): Two tests (`throws on invalid JSON input` and `writes attestation resource on success`) start a `Deno.serve` server and invoke `createSwampClubClient`, which makes `fetch` requests. Deno's `fetch` maintains an implicit keepalive connection pool. CLAUDE.md requires `sanitizeResources: false` (with an explanatory comment) for tests that create HTTP clients with connection pooling. The servers are properly aborted and awaited, so the tests likely pass in practice, but a future Deno version tightening resource tracking could cause spurious failures. Suggested fix: add `sanitizeResources: false` to both test options with a comment like `// SwampClubClient uses fetch which maintains a connection pool`. 2. **`verification_passed` method is callable with failed steps** (`issue_lifecycle.ts`, line 1841): The method name implies success, but the implementation accepts steps with `status: "failed"` and writes `allPassed: false` to the result resource. The actual gate is enforced by the `verification-clear` check. A brief inline comment on the method description noting that this method records results regardless of outcome (the gate is elsewhere) would prevent future confusion about when this vs. `verification_failed` should be called. 3. **`inputs.commit` used unvalidated in shell commands** (`workflow-verify-build.yaml` line 45, `workflow-verify-reviews.yaml` line 45): `${{ inputs.commit }}` is interpolated directly into `git worktree add --detach "$CHECKOUT_DIR" "${{ inputs.commit }}"`. While these workflows are agent-invoked rather than webhook-triggered (limiting practical exposure), validating that the commit value matches `^[0-9a-f]{40}$` before use would be a safe defensive practice consistent with the security posture of the review prompts.

stack72 commented 2026-08-30 03:28:18 +00:00

Author

Owner

Copy link

Adversarial Review

Critical / High

No critical or high severity findings.

Medium

1. verification_passed accepts empty steps array, bypassing verification-clear gate

   • File: issue-lifecycle/extensions/models/issue_lifecycle.ts:1856 (arguments schema) and :1899 (allPassed calculation)
   • What is wrong: The steps argument in verification_passed is z.array with no .min(1) constraint. An empty array yields failed === 0, so allPassed is set to true. The verification-clear check (line 552) only inspects allPassed — it does not verify that any steps actually ran.
   • Breaking example: Agent calls verification_passed with workflowRunId "x", commit "abc", branch "main", and an empty steps array. This writes allPassed true, stepsCompleted 0, stepsTotal 0. The verification-clear gate passes. Combined with a valid attestation, link_pr proceeds with zero actual verification.
   • Suggested fix: Add .min(1) to the steps array in the verification_passed arguments schema. Alternatively, set allPassed to (failed === 0 AND succeeded > 0) so at least one step must succeed.

2. Same empty-steps issue: all-skipped steps also passes the gate

   • File: issue-lifecycle/extensions/models/issue_lifecycle.ts:1899
   • What is wrong: If every step has status skipped, then failed === 0 and allPassed is true, but stepsCompleted (succeeded count) is 0. The verification gate passes despite no step actually succeeding.
   • Breaking example: A single step with status skipped results in allPassed true and stepsCompleted 0.
   • Suggested fix: Change the allPassed calculation to (failed === 0 AND succeeded > 0) to require at least one successful step.

Low

1. post_attestation does no structural validation on the attestation JSON

   • File: issue-lifecycle/extensions/models/issue_lifecycle.ts:2021-2024
   • What is wrong: The attestation argument is z.string().min(1) — any non-empty JSON string passes. The describe documents required fields (version, subject, gate, configIntegrity, steps, timing), but none are enforced at the model layer. A malformed attestation (e.g., just an empty JSON object) would be posted to swamp-club.
   • Mitigating factor: The swamp-club API presumably validates structure server-side, so this is defense-in-depth only.
   • Suggested fix: Parse the attestation into a Zod schema that requires the documented fields, or at minimum assert required top-level keys exist before posting.

2. postAttestation response body is type-asserted without validation

   • File: issue-lifecycle/extensions/models/_lib/swamp_club.ts:209-213
   • What is wrong: The JSON response is cast to the expected shape via a type assertion. If the API returns a different shape (e.g., an empty object), result.id would be undefined, and the attestation resource would be written with attestationId as undefined. The attestation-clear check treats falsy attestationId as invalid, so the system is self-healing. But the error message (Attestation was invalidated) would be misleading.
   • Impact: Unlikely — the API would need to return 200 with a malformed body.

Verdict

PASS — The code is well-structured with thorough tests covering the new verification lifecycle phases. The state machine transitions are correctly constrained. The verify method properly invalidates stale results and attestations before starting a new verification cycle. The workflow YAML files correctly isolate the review agent to read-only tools (Read, Glob, Grep), preventing prompt injection from escalating to arbitrary command execution. The shell scripts in the build workflow properly double-quote template expressions, preventing injection through workflow inputs.

The empty-steps bypass (Medium 1 and 2) is a real gap in the verification gate but is mitigated by the attestation being posted to swamp-club (server-side validation) and the fact that the calling agent is expected to cooperate. Consider adding .min(1) or requiring at least one succeeded step as defense-in-depth.

## Adversarial Review ### Critical / High No critical or high severity findings. ### Medium 1. **verification_passed accepts empty steps array, bypassing verification-clear gate** - **File:** issue-lifecycle/extensions/models/issue_lifecycle.ts:1856 (arguments schema) and :1899 (allPassed calculation) - **What is wrong:** The steps argument in verification_passed is z.array with no .min(1) constraint. An empty array yields failed === 0, so allPassed is set to true. The verification-clear check (line 552) only inspects allPassed — it does not verify that any steps actually ran. - **Breaking example:** Agent calls verification_passed with workflowRunId "x", commit "abc", branch "main", and an empty steps array. This writes allPassed true, stepsCompleted 0, stepsTotal 0. The verification-clear gate passes. Combined with a valid attestation, link_pr proceeds with zero actual verification. - **Suggested fix:** Add .min(1) to the steps array in the verification_passed arguments schema. Alternatively, set allPassed to (failed === 0 AND succeeded > 0) so at least one step must succeed. 2. **Same empty-steps issue: all-skipped steps also passes the gate** - **File:** issue-lifecycle/extensions/models/issue_lifecycle.ts:1899 - **What is wrong:** If every step has status skipped, then failed === 0 and allPassed is true, but stepsCompleted (succeeded count) is 0. The verification gate passes despite no step actually succeeding. - **Breaking example:** A single step with status skipped results in allPassed true and stepsCompleted 0. - **Suggested fix:** Change the allPassed calculation to (failed === 0 AND succeeded > 0) to require at least one successful step. ### Low 1. **post_attestation does no structural validation on the attestation JSON** - **File:** issue-lifecycle/extensions/models/issue_lifecycle.ts:2021-2024 - **What is wrong:** The attestation argument is z.string().min(1) — any non-empty JSON string passes. The describe documents required fields (version, subject, gate, configIntegrity, steps, timing), but none are enforced at the model layer. A malformed attestation (e.g., just an empty JSON object) would be posted to swamp-club. - **Mitigating factor:** The swamp-club API presumably validates structure server-side, so this is defense-in-depth only. - **Suggested fix:** Parse the attestation into a Zod schema that requires the documented fields, or at minimum assert required top-level keys exist before posting. 2. **postAttestation response body is type-asserted without validation** - **File:** issue-lifecycle/extensions/models/_lib/swamp_club.ts:209-213 - **What is wrong:** The JSON response is cast to the expected shape via a type assertion. If the API returns a different shape (e.g., an empty object), result.id would be undefined, and the attestation resource would be written with attestationId as undefined. The attestation-clear check treats falsy attestationId as invalid, so the system is self-healing. But the error message (Attestation was invalidated) would be misleading. - **Impact:** Unlikely — the API would need to return 200 with a malformed body. ### Verdict PASS — The code is well-structured with thorough tests covering the new verification lifecycle phases. The state machine transitions are correctly constrained. The verify method properly invalidates stale results and attestations before starting a new verification cycle. The workflow YAML files correctly isolate the review agent to read-only tools (Read, Glob, Grep), preventing prompt injection from escalating to arbitrary command execution. The shell scripts in the build workflow properly double-quote template expressions, preventing injection through workflow inputs. The empty-steps bypass (Medium 1 and 2) is a real gap in the verification gate but is mitigated by the attestation being posted to swamp-club (server-side validation) and the fact that the calling agent is expected to cooperate. Consider adding .min(1) or requiring at least one succeeded step as defense-in-depth.

stack72 merged commit df42ce6563 into main 2026-08-30 03:35:13 +00:00

stack72 deleted branch feat/pre-pr-verification 2026-08-30 03:35:15 +00:00

stack72 referenced this pull request from a commit 2026-08-30 03:35:16 +00:00

feat(verification): add pre-PR verification workflows and issue-lifecycle support (#243)

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

No description provided.