Skip to content

feat(issue-lifecycle): add pr_open phase and link_pr method#60

Merged
stack72 merged 1 commit intomainfrom
feature/issue-lifecycle-link-pr
Apr 8, 2026
Merged

feat(issue-lifecycle): add pr_open phase and link_pr method#60
stack72 merged 1 commit intomainfrom
feature/issue-lifecycle-link-pr

Conversation

@stack72
Copy link
Copy Markdown
Contributor

@stack72 stack72 commented Apr 8, 2026

Summary

Backport of systeminit/swamp#1143.

Restores the PR-linkage capability that was dropped in #57 (drop GitHub
integration) without re-introducing any git-host coupling. That refactor
correctly removed the gh CLI dependency, but it also removed the
concept of PR linkage — an audit-trail concept that isn't coupled to
GitHub. This PR brings back the minimal piece: a way to attach a PR URL
to the lifecycle record so future sessions reading
swamp model get issue-<N> can see where the fix lives.

Changes

New domain concepts

  • pr_open phase — the PR is linked and we're awaiting CI/review/merge.
    No new swamp-club status; maps to in_progress like implementing.
  • pullRequest-main resource — single-instance value object holding
    the opaque PR URL and a linkedAt timestamp. Git-host agnostic.
  • link_pr method — idempotent (valid from both implementing and
    pr_open); each call overwrites the resource so re-pushes, URL
    corrections, or replacement PRs don't need a new phase.

Backwards compatibility

complete now accepts both implementing and pr_open as valid source
phases. Existing records that didn't go through link_pr can still
reach done via the legacy path. New work should flow
implementing → link_pr → pr_open → complete → done.

Version bump: 2026.04.08.1 → 2026.04.08.2 in both the model and
manifest.yaml. The state machine diagram and methods list in the
manifest have been updated to reflect the new phase and method. Upgrade
entry added; no global-args migration needed.

What this deliberately does NOT do

  • No CI status recording, no check-run arrays, no review-comment loops.
    The agent/human observes CI passing and calls complete. The model
    doesn't need to know what "passing" means.
  • No fix method revival — post-merge follow-ups belong to new issues.
  • No re-introduction of the gh CLI or any git-host coupling. URLs
    are opaque strings.

Bonus: skill doc hardening

While editing implementation.md for the new link_pr step, also
scoped all hardcoded tmp filename examples in planning.md and
adversarial-review.md:

  • /tmp/plan.yaml/tmp/plan-issue-<N>.yaml
  • /tmp/findings.yaml/tmp/findings-issue-<N>.yaml
  • /tmp/resolutions.yaml/tmp/resolutions-issue-<N>.yaml

Generic filenames collide with stale content from previous lifecycle
sessions and can silently leak unrelated data into the current run.

Test Plan

Existing `schemas_test.ts` updated:

  • Happy-path walk now routes `implementing → link_pr → pr_open → complete`
  • New `legacy path` test ensures `complete` still accepts `implementing`
  • `completion gate` assertion expanded to `["implementing", "pr_open"]`

New tests added to `schemas_test.ts` (6):

  • `Phase: pr_open sits between implementing and done`
  • `TRANSITIONS: link_pr is idempotent from implementing and pr_open`
  • `TRANSITIONS: link_pr is rejected from earlier lifecycle phases`
  • `PullRequestSchema` positive, empty-rejection, required-field

New `issue_lifecycle_test.ts` (7):

  • `link_pr` happy path (writes `pullRequest-main`, transitions state)
  • `link_pr` idempotent (two calls, latest URL wins)
  • Zod schema argument rejection
  • Model registration smoke tests for the new resource/method/version

Full verification gate (in the `issue-lifecycle/` package):

  • `deno task check` — clean
  • `deno task lint` — clean
  • `deno task fmt:check` — clean
  • `deno task test` — 23 passed, 0 failed (16 schema + 7 method)
  • `deno install --frozen` — clean

🤖 Generated with Claude Code

Backport of systeminit/swamp#1143.

Restores the PR-linkage capability dropped in #57 (drop GitHub
integration) without re-introducing any git-host coupling. The refactor
correctly removed the `gh` CLI dependency, but it also removed the
concept of PR linkage — an audit-trail concept that isn't coupled to
GitHub. This brings back the minimal piece.

## New domain concepts

- `pr_open` phase — the PR is linked and we're awaiting CI/review/merge.
  No new swamp-club status; maps to `in_progress` like `implementing`.
- `pullRequest-main` resource — single-instance value object holding
  the opaque PR URL and a `linkedAt` timestamp. Git-host agnostic.
- `link_pr` method — idempotent (valid from both `implementing` and
  `pr_open`); each call overwrites the resource so re-pushes, URL
  corrections, or replacement PRs don't need a new phase.

## Backwards compatibility

`complete` now accepts both `implementing` and `pr_open` as source
phases. Existing records that didn't go through `link_pr` can still
reach `done` via the legacy path. New work should flow
`implementing → link_pr → pr_open → complete → done`.

## Version bump

manifest.yaml: `2026.04.08.1 → 2026.04.08.2`, with the state machine
diagram and methods list updated to reflect the new phase and method.
Model upgrade entry added; no global-args migration needed.

## Skill doc hardening

While updating `implementation.md` for the new `link_pr` step, also
scoped all hardcoded tmp filename examples in `planning.md` and
`adversarial-review.md`:

- `/tmp/plan.yaml` → `/tmp/plan-issue-<N>.yaml`
- `/tmp/findings.yaml` → `/tmp/findings-issue-<N>.yaml`
- `/tmp/resolutions.yaml` → `/tmp/resolutions-issue-<N>.yaml`

Generic filenames collide with stale content from previous lifecycle
sessions and can silently leak unrelated data into the current run.

## Test coverage

Existing `schemas_test.ts` updated to reflect the new state machine:

- Happy-path walk now routes `implementing → link_pr → pr_open → complete`.
- New `legacy path` test ensures `complete` still accepts `implementing`.
- `completion gate` assertion expanded to `["implementing", "pr_open"]`.

New tests added:

- `Phase: pr_open sits between implementing and done`
- `TRANSITIONS: link_pr is idempotent from implementing and pr_open`
- `TRANSITIONS: link_pr is rejected from earlier lifecycle phases`
- `PullRequestSchema` positive, empty-rejection, required-field tests

New `issue_lifecycle_test.ts` with 7 method-level tests covering the
`link_pr` happy path, state transition to `pr_open`, idempotency,
zod schema rejection, and model registration smoke tests.

Full verification gate (in the issue-lifecycle package):
- `deno task check` — clean
- `deno task lint` — clean
- `deno task fmt:check` — clean
- `deno task test` — **23 passed, 0 failed** (16 schema + 7 method)

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

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Blocking Issues

None.

Suggestions

  1. Minor: write ordering in link_pr — The method writes pullRequest before state. If the state write fails after the pullRequest write succeeds, the resource and state are momentarily out of sync. This is the same pattern used elsewhere in the model and the framework presumably handles it, but it's worth being aware of.

  2. Minor: linkedAt format not validatedPullRequestSchema accepts any string for linkedAt (no ISO-8601 enforcement). The model always generates it via new Date().toISOString(), so this can't go wrong in practice, but a z.string().datetime() would make the contract explicit.


Everything else looks solid:

  • State machine: pr_open sits correctly between implementing and done. link_pr is allowed from both implementing and pr_open (enabling the idempotent re-link pattern). complete accepts both source phases for backwards compatibility. All consistent with the TRANSITIONS map and tested.
  • Testing: Comprehensive — 6 new schema tests and 7 method tests. All tests using env var manipulation correctly call restore() in finally blocks. No live cloud services (credentials cleared, HOME redirected to a temp dir). No connection pools, so no sanitizeResources needed.
  • Security: The PR URL logged via logger.info is not a secret. No credential exposure.
  • CLAUDE.md compliance: No any types, named exports only, no model/ files touched, version bump applied consistently in both issue_lifecycle.ts and manifest.yaml.
  • Upgrade entry: Correct no-op upgrade (upgradeAttributes: (old) => old) since there are no schema migrations needed for existing records.
  • Manifest: State machine diagram, methods list, and data section all updated to reflect the new phase and resource.

Copy link
Copy Markdown
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Blocking Issues

None.

Suggestions

  1. Minor: write ordering in link_pr — The method writes pullRequest before state. If the state write fails after the pullRequest write succeeds, the resource and state would be momentarily out of sync. Same pattern used elsewhere in the model so likely fine, but worth being aware of.

  2. Minor: linkedAt format not validatedPullRequestSchema accepts any string for linkedAt (no ISO-8601 enforcement). The model always generates it via new Date().toISOString() so this can't go wrong in practice, but z.string().datetime() would make the contract explicit.


Everything else looks solid:

  • State machine: pr_open sits correctly between implementing and done. link_pr accepts both implementing and pr_open as source phases (enabling the idempotent re-link pattern). complete accepts both for backwards compatibility. All consistent with TRANSITIONS and fully tested.
  • Testing: 6 new schema tests + 7 method tests. All tests using env var manipulation call restore() in finally blocks. No live cloud services (credentials cleared, HOME redirected to temp dir). No connection pools so no sanitizeResources needed.
  • Security: The PR URL logged via logger.info is not a credential. No secret exposure.
  • CLAUDE.md compliance: No any types, named exports only, no model/ files touched, version bump applied consistently in both issue_lifecycle.ts and manifest.yaml.
  • Upgrade entry: Correct no-op upgrade (upgradeAttributes: (old) => old) — no schema migration needed for existing records.
  • Manifest: State machine diagram, methods list, and data section all updated correctly.

Copy link
Copy Markdown
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Blocking Issues

None.

Suggestions

  1. Minor: write ordering in link_pr - The method writes pullRequest before state. If the state write fails after the pullRequest write succeeds, the resource and state would be momentarily out of sync. Same pattern used elsewhere in the model so likely fine, but worth noting.

  2. Minor: linkedAt format not validated - PullRequestSchema accepts any string for linkedAt (no ISO-8601 enforcement). The model always generates it via new Date().toISOString() so this cannot go wrong in practice, but z.string().datetime() would make the contract explicit.

Everything else looks solid: state machine transitions are correct and fully tested, all env-mutating tests use finally+restore(), no live cloud services, no credential exposure, no any types, named exports only, no model/ files touched, version bump applied consistently in both issue_lifecycle.ts and manifest.yaml, upgrade entry is a correct no-op, and the manifest description is accurate.

Copy link
Copy Markdown
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Blocking Issues

None.

Suggestions

  1. Minor: write ordering in link_pr - The method writes pullRequest before state. If the state write fails after the pullRequest write succeeds, the resource and state would be momentarily out of sync. Same pattern used elsewhere in the model so likely fine, but worth noting.

  2. Minor: linkedAt format not validated - PullRequestSchema accepts any string for linkedAt (no ISO-8601 enforcement). The model always generates it via new Date().toISOString() so this cannot go wrong in practice, but z.string().datetime() would make the contract explicit.

Everything else looks solid: state machine transitions are correct and fully tested, all env-mutating tests use finally+restore(), no live cloud services, no credential exposure, no any types, named exports only, no model/ files touched, version bump applied consistently in both issue_lifecycle.ts and manifest.yaml, upgrade entry is a correct no-op, and the manifest description is accurate.

Copy link
Copy Markdown
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adversarial Review

Reviewed all non-model changed files: schemas.ts, schemas_test.ts, issue_lifecycle.ts, issue_lifecycle_test.ts, manifest.yaml, and the three skill reference docs.

Critical / High

None found.

Medium

  1. issue_lifecycle.ts:1105-1110link_pr description says "Transitions the phase to pr_open if currently implementing" but always writes phase: "pr_open" unconditionally.
    When called from pr_open (the idempotent re-link case), the description implies it might not transition, but it always does. Functionally harmless (writing pr_open when already in pr_open is a no-op state change with a timestamp refresh), but the description's "if currently implementing" qualifier could mislead a reader into thinking there's conditional logic. The implementation.md doc correctly describes the behavior. Consider aligning the method description: "Transitions the phase to pr_open" (drop the conditional).

Low

  1. schemas.ts:169-177PullRequestSchema.linkedAt uses z.string() for an ISO-8601 timestamp rather than z.string().datetime().
    Every other timestamp in this file (updatedAt, fetchedAt, classifiedAt, generatedAt, submittedAt, reviewedAt) also uses plain z.string(), so this is consistent with the codebase. And the value is always set internally by new Date().toISOString(), never by user input. Mentioning only because a schema-level datetime() constraint would catch future bugs if someone refactors the method to accept external timestamps.

  2. issue_lifecycle.ts:1134-1147 — Sequential writes without transactional guarantee: pullRequest-main could succeed while state-main fails. This would leave the PR URL recorded but the phase stuck at implementing. However, every other method in the model has the identical pattern (sequential writeResource calls without rollback), and a retry of link_pr from implementing would self-heal. Not unique to this PR; mentioning for completeness.

  3. issue_lifecycle_test.ts:37-87buildTestContext mutates global Deno.env. Tests use try/finally to restore, which is correct. The CLAUDE.md testing rules explicitly require "Restore all env vars in a finally block" — this is followed. No issue; just confirming the pattern holds.

Verdict

PASS — Clean, well-structured addition. The state machine extension is backwards compatible (complete still accepts implementing), the link_pr method follows established patterns for resource writes and swamp-club posting, test coverage is thorough (16 schema + 7 method tests), and the upgrade entry correctly maps old => old since no global args changed. The skill doc hardening (issue-scoped temp filenames) is a welcome defensive improvement.

@stack72 stack72 merged commit c464d97 into main Apr 8, 2026
24 checks passed
@stack72 stack72 deleted the feature/issue-lifecycle-link-pr branch April 8, 2026 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant