Skip to content

feat(verbs): IPostconditionVerifier shared abstraction (Move #3, absorbs #193) - #198

Merged
PolyphonyRequiem merged 2 commits into
mainfrom
sdlc/move-3-postcondition-verifier
May 8, 2026
Merged

feat(verbs): IPostconditionVerifier shared abstraction (Move #3, absorbs #193)#198
PolyphonyRequiem merged 2 commits into
mainfrom
sdlc/move-3-postcondition-verifier

Conversation

@PolyphonyRequiem

Copy link
Copy Markdown
Owner

Summary

Introduces IPostconditionVerifier — a shared, injectable check for "after this verb returns, origin must hold blob X at path Y". This is the Class B bug pattern that produced #192 (manifest commit-and-push silently no-oping when local HEAD already had the blob but origin didn't).

This PR absorbs and supersedes #193#193's fix to ManifestCommands.CommitAndPush becomes a one-line postconditions.VerifyAsync(...) call once the abstraction is in place, and the same fix is applied to PlanCommands.CommitAndPush (which had the identical Class B bug shape).

Closes #192.

What IPostconditionVerifier does

Task<PostconditionOutcome> VerifyAsync(
    string branch,
    IReadOnlyList<PostconditionExpectation> expectations,
    string remote = "origin",
    CancellationToken ct = default);

Returns a discriminated union with three cases:

Case Meaning Caller's typical action
Satisfied Origin holds expected blob at every path. Emit no-op success.
NeedsPush Origin missing one or more paths. Push HEAD.
Conflict Origin has different content for some paths. Push (let git reject as non-ff) or escalate.

Treats git fetch failure (most common: ref doesn't exist yet — first push of a fresh branch) as NeedsPush so the recovery path runs. Per-path read failures are treated the same way — push and let the real failure surface there if anything is actually broken.

Consumers refactored

Consumer Before After
ManifestCommands.CommitAndPush Inline git fetch + git show guard added in #193 postconditions.VerifyAsync(...) switch
PlanCommands.CommitAndPush Silent no-op when nothing staged (the #192 bug, just for plan files) Same verifier switch

Both verbs also now follow the Move #2 routing-style envelope: sentinel int/string defaults, in-body validation, exit 0 with error_code on every failure path — never ConfigError or RoutingFailure.

Discriminated union shape

PostconditionOutcome mirrors RebaseOutcome (in Infrastructure/Processes/): abstract record + sealed nested record cases. Consumers switch with a default arm that throws — the compiler can't prove exhaustiveness across the assembly boundary, and a future case must fail loudly.

Tests

  • PostconditionVerifierTests — full unit coverage of all three outcome cases, fetch-failure recovery, show-failure recovery, vacuous case (empty expectations → Satisfied without fetch), byte-exact comparison.
  • ManifestCommandsCommitAndPushTests — fully rewritten. Uses the real PostconditionVerifier(GitClient(runner)) to exercise the full fetch+show plumbing. Absorbs all 5 of fix(manifest): guard commit-and-push no-op against stale origin (#192) #193's test scenarios plus a Conflict-path test plus a sentinel-default test.
  • PlanCommandsCommitAndPushTests — fully rewritten. Uses FakePostconditionVerifier to drive switch arms without re-stubbing fetch+show (that's covered by the verifier's own tests). Adds Class-B-parity tests for the LocalCleanButOriginMissing and LocalCleanButOriginConflict cases.
  • CwdSerialCollection — new xUnit collection that serializes test classes that mutate Environment.CurrentDirectory (avoids races between Manifest and Plan commit-and-push test classes both wanting to chdir into a temp dir).
  • FakePostconditionVerifier — test fixture for verbs that don't need the real verifier wiring.

Total: 2880 unit tests pass, 101 Pester tests pass, jinja-resolver lint clean.

Skill doc

Adds a Post-condition verification — when origin must hold blob X section to .github/skills/polyphony-cli-developer/SKILL.md with a worked example showing the verifier-switch pattern.

After merge

Daniel Green and others added 2 commits May 8, 2026 08:33
…rbs #193)

Introduces IPostconditionVerifier — a shared, injectable check for `after
this verb returns, origin must hold blob X at path Y`. This is the Class B
bug pattern that produced #192 (manifest commit-and-push silently no-oping
when local HEAD already had the blob but origin didn't).

The verifier returns a discriminated union with three cases:

  - Satisfied: origin holds the expected blob at every path
  - NeedsPush: origin is missing one or more paths
  - Conflict:  origin has different content for some paths

Treats `git fetch` failure (most common: ref doesn't exist yet) as
NeedsPush so first-push-of-a-fresh-branch goes through the recovery path.

Applied to two consumers, killing the bug class everywhere it currently
lives:

  - ManifestCommands.CommitAndPush — absorbs #193's fix verbatim, plus
    the Conflict path
  - PlanCommands.CommitAndPush — same shape, same Class B bug shape

Both verbs now follow the Move #2 routing-style envelope (sentinel int/
string defaults, in-body validation, exit 0 with error_code on every
failure path — never ConfigError or RoutingFailure).

Adds:
  - PostconditionExpectation, PostconditionOutcome, IPostconditionVerifier,
    PostconditionVerifier (Polyphony.Postconditions namespace)
  - DI registration as singleton (uses IGitClient, no transport state)
  - PlanCommitAndPushResult.ErrorCode field for routing
  - FakePostconditionVerifier test fixture
  - CwdSerialCollection — xUnit collection that serializes test classes
    that mutate Environment.CurrentDirectory (avoids races between
    Manifest and Plan commit-and-push test classes)
  - Postconditions/PostconditionVerifierTests — full coverage of all
    three outcome cases plus fetch-failure and show-failure recovery
  - Skill doc: new `Post-condition verification` section in
    polyphony-cli-developer/SKILL.md with worked example

Closes #192. Supersedes and absorbs #193 (#193 will be closed).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
lint-type-agnostic flagged 'Bug #192 was this exact pattern' as a
type-name literal violation (P5). Rephrase to 'Issue #192' which is
both type-agnostic and accurate (the underlying twig work item is an
Issue, not a Bug).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@PolyphonyRequiem
PolyphonyRequiem merged commit d301052 into main May 8, 2026
1 check passed
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.

manifest commit-and-push idempotency hole: local-clean no-op even when origin lacks the manifest

1 participant