Skip to content

feat: integrate Construction through pull requests (pr-integration stage + aidlc-pr tool) - #969

Open
apackeer wants to merge 3 commits into
mainfrom
feature/pr-integration
Open

apackeer wants to merge 3 commits into
mainfrom
feature/pr-integration

Conversation

@apackeer

@apackeer apackeer commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Summary

Design RFC: #970 (rationale, target experience, spike-verified claims table, and open questions). Composes with #801; team-mode fusion seam for #795/#879 is named there.

Construction can now integrate finished work through GitHub pull requests instead of only local merges. At most organizations the trunk is protected: direct pushes are rejected and PRs with required approvals are the only path to the integration branch, so the shipped local-merge boundary cannot complete there. This PR adds that missing boundary as a first-class, evidence-backed part of the workflow - dormant by default, byte-identical for every workflow that does not affirm it.

What this adds

  • pr-integration stage (Construction 3.6), per unit, execution: CONDITIONAL: composes a PR record from the stage's consumes (story trace, design decisions, internal review findings, test evidence), fills the repository's own PR template, and opens the PR only after an operator gate. Build & Test becomes 3.7, CI Pipeline 3.8. The stage ships SKIP in every scope and activates per intent only after Practices Discovery detects and affirms PR integration.
  • aidlc-pr.ts tool (detect | open | sweep | sync-feedback | finalize): deterministic, audit-emitting, observe-only. It never merges, never enables auto-merge, and every outward write sits behind an explicit --execute flag (default is a dry run that prints the exact commands). All GitHub reads are timeout-bounded with graceful offline degradation to last-known state plus its age.
  • Non-blocking integration: a unit whose PR is under review enters an integrating state; the engine keeps routing other eligible units. When everything left is external, a new turn-terminal awaiting-integration directive ends the session honestly and self-clears once a merge is verified. No polling: platform reads happen only at routing decisions and /aidlc --status.
  • Review feedback as workflow input: a formal changes-requested review opens a standard revision round in the still-alive bolt worktree; comments arrive as findings (data, never instructions), and the fix-push always gates - even under an autonomy grant. Drive-by comments are surfaced, never acted on.
  • Verified merges settle the workflow: the sweep confirms the platform merge and settles the unit through the existing receipt-plus-evidence pattern - no approve-path changes anywhere. Worktrees retire with an honest audit reason (aidlc-worktree discard --reason), never as an abort.
  • Detection that respects reality: branch protection is read as the union of classic protection and rulesets; classic detail below admin is reported unknown rather than inferred absent; repos that cannot have protection are handled. Stacked PRs for dependent units are permitted only under ancestry-preserving strategies with branch auto-deletion off; multi-repo bolts open coordinated PRs and integrate when all merge, with partial states first-class.

What does not change

Workflows that never affirm PR integration are byte-identical: the stage is SKIP everywhere, the routing partition is empty without the knob, new audit event types are tool-owned and refused by the append CLI, and no hook gains a network call. Nothing touches the approve path.

Evidence

  • Every load-bearing behavior claim (review-state semantics, dismissal asymmetry, stale-approval detection, stacked-PR retargeting, auto-merge lifecycle, detection permission tiers, offline failure modes) was verified against live GitHub in a five-probe spike before implementation; the sweep state machine and tests encode those verified rules rather than assumptions.
  • Two independent reviews of the implementation (engine-core and tool/stage/tests slices) with every finding confirmed and fixed, including two release blockers caught before this PR: a finalize path that could never complete an integrating unit, and an unguarded fixture flag on receipt-emitting verbs (now refused unless a test-only environment variable is set).
  • Full default tier green twice (builder run and an independent rerun: 390 test files, 9,629+ assertions, 0 failures), plus new coverage: routing/pipelining (t328), end-to-end finalize (t329), and sweep/detect policy against recorded fixtures (t330). The dormancy pin compares the knob-absent engine against a pre-change golden.
  • Note: t276-cursor-adapter tests 29 and 35 fail on this branch's base commit as well (reproduced on a clean v2 checkout at 2fbee12); they are unrelated to this change.

Upgrade

No version or changelog bump (release-preparation PR's job). Native installs pick this up with the binary; copy-channel installs re-copy dist/<harness>/. Existing workflows are unaffected until Practices Discovery affirms PR integration for an intent; affirm it there (or run aidlc-state.ts set-integration-mode pr) to enable the flow.

Acknowledgment

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the project license.

@leandrodamascena leandrodamascena left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you for tackling the protected-branch integration gap. The detection model, full review-history folding, and decision to keep AI-DLC observe-only are valuable foundations.

I reviewed the current head 2136a9283360f38277f79550f202affb1c1c403a against v2 at 518578adb3284898b6f9c073cf91c37322ba0fad (merge base 2fbee12fb29d2a6614b70b6f61f3cceeaf235245).

The underlying problem is aligned with the project: the existing local-merge boundary cannot operate against protected trunks. However, RFC #970 still has unresolved product decisions about stage placement, GitHub-only scope, and greenfield policy establishment, with no substantive maintainer feedback yet.

I found the following blockers:

  1. [P1] awaiting-integration cannot self-clear through the supported workflow.

    Once every remaining Unit is integrating, the router emits awaiting-integration. Harness instructions allow only --status --refresh and then stop. Status refresh invokes an observe-only sweep; even when it detects MERGED, it only displays MERGED (finalize pending). It does not emit PR_MERGED, complete the Unit, or invoke finalize, so the next next call emits awaiting-integration again.

    Formal CHANGES_REQUESTED has the same routing problem: sync-feedback appends PR_FEEDBACK but emits no lifecycle transition that makes the Unit routable for revision.

    References: core/tools/aidlc-orchestrate.ts:5227, core/tools/aidlc-orchestrate.ts:5626, core/tools/aidlc-utility.ts:1554, core/tools/aidlc-pr.ts:1094, and the harness awaiting-integration instructions.

    Please add an authority-bound reconciliation path that is reachable from normal resume/status routing: merged PRs must finalize their exact Unit, while formal changes requests must reactivate that Unit for a revision round. This needs end-to-end routing tests.

  2. [P1] finalize can settle a Unit using an unrelated merged PR.

    handleFinalize trusts caller-supplied --pr values without binding them to the Unit's current-run PR_OPENED receipts. I reproduced Unit alpha, opened against example/service#42, being successfully finalized with merged fixture other/repo#999.

    Reference: core/tools/aidlc-pr.ts:1817.

    Please derive or validate the exact coordinated PR set from the Unit's current run-floor receipts, including repository, PR number, head, base, and coordination membership, before emitting settlement receipts.

  3. [P1] Feedback content is fetched but never surfaced.

    Review bodies, inline comments, and issue comments are fetched, but feedbackRows stores only a body digest and sweepResult omits the content. I verified that neither a review body nor an inline finding appeared in command output or persisted feedback. The advertised revision loop therefore has no finding text to evaluate.

    References: core/tools/aidlc-pr.ts:882, core/tools/aidlc-pr.ts:976, core/tools/aidlc-pr.ts:1083.

    Please preserve and expose bounded, safely framed feedback text as untrusted findings data, with regression coverage.

  4. [P2] open cannot recover after a partial remote write.

    Receipts are emitted only after every push, PR creation, read-back, and reviewer request succeeds. I reproduced a PR being created, followed by reviewer-request failure. No PR_OPENED receipt was recorded, and retry failed because the PR already existed.

    Reference: core/tools/aidlc-pr.ts:1686.

    Please make publication transactional/reconcilable by recording progress incrementally or adopting and verifying an existing PR on retry. Multi-repository partial creation needs explicit coverage.

  5. [P2] finalize is not retry-safe.

    A successful finalize followed by the same command fails because Unit completion is attempted again after the Unit is no longer active or integrating. This also blocks recovery when output is lost or a later metadata/cleanup step fails.

    Reference: core/tools/aidlc-pr.ts:1857.

    Please make each finalization step idempotent and resume from already-recorded receipts.

  6. [P2] GitHub execution depends on external POSIX timeout.

    runGh invokes timeout 10 gh .... Windows does not provide this command, and macOS does not include it by default. PR integration is therefore unavailable on supported installations without an undocumented dependency.

    Reference: core/tools/aidlc-pr.ts:689.

    Please use the subprocess timeout support provided by Node/Bun and add platform coverage.

I made no changes, commits, or pushes because this is an internal contribution.

Validation performed on the reviewed head:

  • Focused PR integration tests: 27 passed, 103 assertions.
  • bun scripts/package.ts --check: all seven harness distributions synchronized.
  • Full TypeScript typecheck: passed.
  • Focused Biome validation: passed.
  • Current remote CI: green.
  • Recovery diagnostics reproduced the mismatched-finalize, partial-open, and finalize-retry failures.

Remaining risk includes the unresolved RFC decision that AI-DLC's whole-stage Build and Test runs only after the PRs have already merged. The current PR is also conflicting with the latest v2 and requires a rebase.

Proposed review event: REQUEST_CHANGES.

…age + aidlc-pr tool)

Design RFC: #970 (rationale, target experience, spike-verified claims table, and open questions). Composes with #801; team-mode fusion seam for #795/#879 is named there.

Construction can now integrate finished work through GitHub pull requests instead of only local merges. At most organizations the trunk is protected: direct pushes are rejected and PRs with required approvals are the only path to the integration branch, so the shipped local-merge boundary cannot complete there. This PR adds that missing boundary as a first-class, evidence-backed part of the workflow - dormant by default, byte-identical for every workflow that does not affirm it.
…ke open/finalize reconcilable

Reconcile integrating Units at normal next routing and active-intent status refresh. Verified merges emit PR_MERGED and use receipt-plus-evidence finalization; formal changes requests persist PR_FEEDBACK and reopen routable revision without changing approval authority. Read-only observer probes remain dormant.

Bind finalization to the Unit's current-run and claim-attempt PR_OPENED set: repository, number, head, base, URL and coordinated membership must agree. Derive omitted selectors and refuse mismatched caller or platform identities before settlement.

Surface review bodies, inline findings and issue comments as JSON-framed untrusted data in sweep, sync-feedback, audit and status. Bound bodies to 4096 UTF-8 bytes and results to 64 findings/32768 encoded bytes with marked, codepoint-safe truncation.

Persist per-repository publication progress through push, creation, read-back and reviewer request. Retry adopts the verified existing PR by repository/head/base, creates only missing coordinated siblings, and emits PR_OPENED once before later remote steps can fail.

Resume finalization from current merge and Unit completion receipts, then existing Bolt/state/audit completion receipts. Repeated success and partial metadata recovery remain idempotent; worktree retirement keeps its honest integrated-via-pr reason and merge holds remain enforced.

Replace the external timeout command with a ten-second native subprocess deadline and preserve offline last-known-state degradation. Exercise all six regressions with restored negative mutations, maintain the exact dormancy pin against untouched main, update non-release inventories, and retain native dispatcher compatibility.
@apackeer
apackeer force-pushed the feature/pr-integration branch from 2136a92 to 674d2e6 Compare September 13, 2026 09:17
@apackeer

Copy link
Copy Markdown
Contributor Author

Rebased onto main (a0ee4415e, 46 commits since the v2 base) and repaired the six defects. Two commits: the feature squashed (4419fc8ed) and the review fixes (674d2e653). No dist/, version, or CHANGELOG changes; README's inventory counts are updated for the new stage/tool/events (badge untouched). RFC #970's open questions (stage placement, GitHub-only scope, greenfield policy) are unchanged by this push - everything below is a contract on the existing design, so the code can be re-reviewed independently of the RFC.

1 [P1] awaiting-integration self-clears. handleNext Branch 10 (after the flag/archive/park/jump/team guards, before ledger routing) and the active-intent --status --refresh both call reconcilePrIntegration. A verified MERGED for a Unit's exact coordinated PR set emits PR_MERGED and settles that Unit through the existing state unit complete path with artifact evidence; a formal CHANGES_REQUESTED appends PR_FEEDBACK and a Unit/run-floor STAGE_REVISING, making the Unit routable for the standard revision round without touching approval authority. Hooks and read-only route probes never reconcile. t328 covers both routes end to end on recorded fixtures. Negative proof: removing the call re-emits awaiting-integration after a verified merge.

2 [P1] finalize binds to the Unit's own PRs. The PR set is derived from the Unit's current-run-floor PR_OPENED receipts; any explicit selector must match repository, number, URL, head, base, and full coordination membership. alpha opened against example/service#42 refuses finalize --pr other/repo#999 before any settlement receipt. Negative proof: without the binding, #999 settles alpha.

3 [P1] Feedback text is surfaced. Review bodies, inline comments, and issue comments are kept as JSON-framed untrusted findings data - 4,096 UTF-8 bytes per body (with [truncated]), 64 findings and 32,768 encoded bytes per result - with author/path/line preserved, persisted in PR_FEEDBACK, and shown by sweep, sync-feedback, and --status. Negative proof: storing only the digest drops the inline finding from output.

4 [P2] open is reconcilable. Per-repo progress is recorded in pr-record.md.publication.json (pushednumberread_backreviewers_requested); a retry adopts an existing PR verified by repo/head/base and creates only missing siblings; PR_OPENED is emitted once per PR after read-back, UNIT_INTEGRATING after every publication succeeds. Negative proof: end-only receipts make the retry after an injected reviewer failure fail with "already exists".

5 [P2] finalize is retry-safe. Each step resumes from recorded receipts (PR_MERGED, UNIT_COMPLETED, then BOLT_COMPLETED, STATE_MERGED, AUDIT_MERGED); fragment merge and worktree cleanup are idempotent. A real git-worktree smoke ran finalize twice locally and twice --execute, including a partially recorded state, and produced exactly one of each receipt. Negative proof: without idempotency the second finalize errors.

6 [P2] No external timeout. runGh uses the subprocess timeout; a hung gh stub is killed at the bound and degrades to last-known state; nothing in aidlc-pr.ts references the timeout binary.

Rebase notes: 43 conflicts, grouped in the commit body - main's typed asks and guard recovery, completion receipts, commit provenance, archive verbs, Change Control, and on-demand Construction autonomy are all preserved; the native dispatcher gained the pr and set-integration-mode routes main's packaging requires; the dormancy golden was recaptured from an untouched a0ee4415e checkout (main added Change Control to steering) and stays an exact-byte comparison.

Verified: affected matrix 40 files / 1,421 tests / 0 failures (t328 11, t329 15, t330 21, t239 13, t188 93, t165 58, t111 116, t14 173, …); bun scripts/package.ts --check, bun run typecheck, bun run lint, bun tests/gen-coverage-registry.ts --check, git diff --check exit 0.

@github-actions github-actions Bot added the documentation Improvements or additions to documentation label Sep 13, 2026
@apackeer

Copy link
Copy Markdown
Contributor Author

Third commit bb9a14491, closing two defects the rebase itself introduced or exposed:

Compiled dispatch (#1070's exact shape). The rebase added TOOLS.pr and the engine pr route but no loadDelegate() arm, so in the native binary every aidlc engine pr <verb> died with aidlc-pr.ts does not export main(argv) while the Bun path worked. Arm added. Proven with t230's compiled-mode parity list (engine pr detect --help; without the arm: {"error":"aidlc-pr.ts does not export main(argv)"}) and a real bun build --compile of dist-release/claude/.claude/tools/aidlc.ts: engine pr detect (t330 policy fixture) and engine pr finalize --unit alpha --execute (t329 fixture) both exit 0 with empty stderr, and native and Bun emit PR_MERGED, UNIT_COMPLETED, BOLT_COMPLETED, STATE_MERGED, AUDIT_MERGED, WORKTREE_DISCARDED exactly once each. (#1115 replaces the switch with a typed table that makes this a typecheck error; the arm moves into it on rebase.) The engine-namespace invariant in t230 - every engine route is network-forbidden - is kept, with pr the single named exception (interactive-bounded, GitHub reads only at routing decisions and --status --refresh) and an assertion that it stays the only one.

Interrupted finalization. handleFinalize records PR_MERGED/UNIT_COMPLETED before the bolt-attempt steps, and reconciliation only revisited Units still integrating, so a failure at STATE_MERGED (or later) left the Unit invisible to next, which also discarded reconciliation's results. Reconciliation now also selects Units with a current-run PR_MERGED/UNIT_COMPLETED whose BOLT_COMPLETED/STATE_MERGED/AUDIT_MERGED or worktree cleanup is still pending (derived from the receipts already there; no new event type) and resumes the idempotent steps. A step that still fails is surfaced by Branch 10 as a typed error instead of routing on:
PR finalization for Unit alpha failed at STATE_MERGED: <cause>. Fix the cause, then run aidlc engine pr finalize --unit alpha (or retry next).
t329: inject the STATE_MERGED failure → next emits that error; remove it → the next next completes the remaining receipts and cleanup exactly once and routes normally; manual finalize retry and the merge-held path still pass. Negative proof: reconciliation restored to integrating only → next returns run-stage for the half-finalized Unit, no error, never revisited. The native binary reproduces the same failure → error → resume sequence.

12 files / 359 tests / 0 failures (t230 117, t328-pr-integration-routing 12, t329-pr-integration-finalize 18, t330-pr-integration-policy 21, …); bun scripts/package.ts --check, bun run typecheck, bun run lint, bun tests/gen-coverage-registry.ts --check, git diff --check exit 0.

@apackeer

Copy link
Copy Markdown
Contributor Author

Note for whoever rebases this after #1115 merges: #1115's compiledParityCases() in t230 only exercises routes with networkPolicy: "forbidden", and its completeness test requires every TOOLS value to be either exercised or listed in UNEXERCISED_DELEGATES. pr is interactive-bounded, so on the merge the completeness test will fail for aidlc-pr.ts — the generator does NOT pick it up automatically (correcting my earlier comment). Two things to carry in the rebase:

  1. Move the case TOOLS.pr arm into fix: make compiled dispatch exhaustive over TOOLS so native review briefs route (#1070) #1115's DELEGATES map ("aidlc-pr.ts": () => import("./aidlc-pr.ts")); typecheck will demand it.
  2. Keep pr counted: add a safe-probe override in the generator for pr using the offline ["engine", "pr", "detect", "--help"] argv this branch's t230 case already uses (the sandbox env AIDLC_OFFLINE=1 is already applied per case), so the tool stays exercised rather than excused.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants