Fix #759: pr-search includes all PR states so merged PRs are found - #1331
Fix #759: pr-search includes all PR states so merged PRs are found#1331waleedkadous wants to merge 25 commits into
Conversation
…/AGENTS.md to the blog principles (G2) CLAUDE.md 5,815 -> 1,417 words; AGENTS.md byte-identical. ALWAYS_ON 34,231 -> 29,833. Acceptance is principle conformance, not size - the numbers are reported because M0c requires deletion and relocation to be distinguishable, not because anything passes or fails on them. DELETED vs RELOCATED (M0c): of 4,398 words removed from always-on, 1,129 were RELOCATED to skills and 3,269 were DELETED outright. Authored total fell only 4,294 (153,219 -> 148,925) because relocation writes to four trees - the honest picture, and what T15 exists to expose. Relocated (P3, P4): - Runnable Worktrees, entire section -> NEW runnable-worktrees skill (926w): config block, afx dev CLI, VSCode controls, URL/cleanup semantics, 7 recipes. Needed rarely, loaded always. - Local Build Testing + directory map + tokei -> codev skill (326 -> 529w). Includes the copy-skeleton/rebuild precondition found in Phase 0. Both written to ALL FOUR skill trees (.claude, .codex, and both skeleton copies). New T17 asserts parity, scoped per the plan-gate ruling to skills this project TOUCHES - untouched drift (afx, porch) stays exempt and belongs to the architect's separate issue. Deleted (P1, P7): 'Before Starting ANY Task', 'When Stuck After 15 Minutes', 'Understand Before Coding' - judgment a frontier model already exercises, and duplicated by hot-tier lessons. Duplicated scar blocks (worktree destruction x2, afx-from-root x2, git add -A x3) each survive exactly once. CLI reference link list, responsiveness examples, cmap walkthrough, porch command list, messaging examples - all addressable by skill name. Kept: all eight scar canonicals VERBATIM and unwrapped (a first draft reflowed them across lines, which broke exact-match - canonicals must stay single-line), the generated hot-context block byte-for-byte, repo dual nature, four-tier resolution, deliver-don't-fetch, gate semantics, area policy, consultation defaults including the load-bearing -sol suffix, commit formats, never-squash. M10: NO assertions retired. spec-1273-wait-discipline-docs, governance-sweep, framework-ref-audit and template-delivery all pass unmodified. The afx skill was deliberately NOT touched - relocating messaging into it would oblige resolving its pre-existing drift and propagating stale 'tick' references to adopters. The addressing CONTRACT stayed in CLAUDE.md; it is policy, not a how-to, so P4 does not apply. Judgment call flagged in the manifest, not made silently. Also fixed a design flaw in my own Phase 0 test: it pinned ALWAYS_ON to a literal, which this project changes every phase. A test edited every phase is a test edited carelessly (M10's argument, turned on my own suite). Replaced with arithmetic invariants that hold at any surface size, plus an immutable assertion that the FROZEN baseline artifact still records 34,231. Suite: 205 files, 4,083 tests, green (rebuilt first - skeleton edits are invisible to tests until copy-skeleton reruns).
…d (G6) 2,048 -> 761 words. ALWAYS_ON(architect) 8,599 -> 2,914. Deleted (P4): afx/porch/consult command blocks and the 14-row Quick Reference. Each CLI has a skill that is the single owner of its flags, and the role doc had become a stale second copy - it still advertised 'porch approve <id> spec-approval' without the --a-human-explicitly-approved-this flag the command now requires. A second owner of command syntax is a drift generator. Deleted (P1/P7): step-by-step 'Starting a New Feature' / 'Monitoring Progress' / 'Cleanup' walkthroughs (sequenced narration of three commands - the obligations survive as contract lines), and the Release Management state diagram (aspirational, no mechanism behind it in this repo). Kept: risk-triage table with highest-factor-wins precedence, the PRFT contract for presenting decisions to the human, UX verification, all boundaries, and the afx-from-root scar canonical VERBATIM. Verified rather than assumed, per the plan's open question: architect.md carries NOTHING load-bearing for multi-architect coordination (Specs 755/786/823) - grepped for architect:<name>, sibling/multi-architect language, spawnedByArchitect and whoami, zero matches. That contract lives in CLAUDE.md. Group-pure commit: G6 only.
…(G3) 1,837 -> 849 words. SPIR builder spawn 6,364 -> 5,371. Deleted (P1/P7): the numbered Core Loop, 'What You DON'T Do in Strict Mode', the 'Getting Started' 3-step list, and the duplicated protocol summary (the protocol is inlined into the spawn prompt - restating it here was a drift-prone second copy). ALL-CAPS repetitions collapsed so each prohibition appears exactly once. Kept: mode contract, gate semantics, deliverables, thread contract, notification triggers, wait discipline, worktree path discipline, scope, flaky-test rule, and the no-hand-edit-status canonical VERBATIM. M10 - NO assertions retired. spec-1273-wait-discipline-docs.test.ts (18 assertions across both role-doc copies) passes UNMODIFIED. Three initially failed against my rewrite: - '## Waiting on external work' heading - I had renamed the section - 'never chain foreground poll loops' - a LINE WRAP split the phrase - 'queues unread until your current turn ends' - I had dropped 'current' In all three the BEHAVIOUR survived; only the strings moved. I adjusted my prose rather than the assertions: those strings encode a wait-discipline incident, preserving them cost nothing in conformance, and editing a prior spec's protection to fit new prose is exactly the silent erosion M10 exists to prevent. Third occurrence of the wrap hazard (scar canonicals in Phase 1, test assertions here): reflowing prose breaks any exact-match string that spans a wrap. Verify after rewriting; keep canonicals on one line however long. Group-pure commit: G3 only.
roles/consultant.md inspected and left UNCHANGED (G5, no commit needed): already conformant - it states a contract, not a procedure. Under the acceptance model a conformant file passes as-is; shrinking it further would be size-chasing, which the charter amendment explicitly rejects. Suite: 205 files, 4,083 tests, green.
…relay-shaped (G6)
Architect inspection caught a cross-file contradiction my rewrite half-fixed.
builder.md correctly encodes the relay convention ('Approval reaches you as a
message from the architect. Then YOU run porch approve; the architect does not
run it for you') - but architect.md kept the OLD worked example showing the
ARCHITECT running (cd .builders/<id> && porch approve ...). The two roles
disagreed on who the approval actor is.
builder.md was the correct one: it matches the owner's standing convention -
the architect relays the human's word, the builder runs porch approve itself.
It is also what actually happened twice in this project's own gates, so the
architect.md example contradicted observed behaviour.
This is the SAME stale-second-owner class I had just caught on the porch-approve
flag syntax, one level up - and I introduced it by fixing one owner and leaving
the other. Worth recording: catching a class of defect is not the same as being
immune to it.
Gates section now: read the artifact, decide, relay via afx send; the builder
executes against its own porch state. Kept the --a-human-explicitly-approved-this
explanation because the WHY is load-bearing - a gate message is a notification TO
a human, never a token an agent may spend on its own authority.
afx-from-root canonical re-verified verbatim after the edit (the reflow hazard
applies to every rewrite, including fixes). Twins identical. 51 tests green
across the wait-discipline, skills-parity, governance-sweep and
framework-ref-audit suites.
Group-pure commit: G6 only.
… contradiction and its general form
…es from my own script
…builder/1280-rewrite # Conflicts: # packages/codev/src/__tests__/spec-1280-measurement-instrument.test.ts
# Conflicts: # packages/codev/src/__tests__/spec-1280-measurement-instrument.test.ts
… the P6 include mechanism (G3)
Ten decisions, both trees. ALWAYS_ON 28,844 -> 26,384; TOTAL_AUTHORED
144,465 -> 126,155. spir 3,699 -> 671 authored (1,239 served).
P6 MECHANISM: each protocol.md carries a fenced json block containing
{{> protocols/<p>/protocol.json}}. Verified rather than assumed:
resolveCodevIncludes is extension-agnostic, and spawn-roles.ts:127 passes
protocol.md through the same resolver before inlining it as
{{protocol_reference}} - so strict AND soft mode both get the structured source.
New T18 asserts delivery in both modes, which are NOT symmetric: strict-mode
builders also receive gates/checks as porch task JSON, but soft-mode builders
have ONLY this document. A silent expansion failure would leave them with a
protocol doc describing nothing.
RESOLVER MODEL CORRECTED, found by writing T18: tier 4 is getSkeletonDir() - the
INSTALLED NPM PACKAGE - not <root>/codev-skeleton/. The repo-local
codev-skeleton/ is a build source (copy-skeleton copies it into
packages/codev/skeleton); the resolver never reads it. My first fresh-install
test planted files in a temp codev-skeleton/ and 'passed' against the real
installed package. Rewritten to assert the actual adopter guarantee: skeleton is
in the npm files allowlist and every P6 protocol's protocol.json is in the built
skeleton.
release/protocol.md INSPECTED AND UNCHANGED: no protocol.json so P6 does not
apply, and 36% of it is exact commands (git add file lists, the version-anchor
pattern, the pre-release VSCode auto-skip, the backport path) where the sequence
IS the contract. A conformant file passes as-is.
CAPABILITY LOSS I INTRODUCED AND THE TESTS CAUGHT - 37 failures, all repaired,
zero assertions retired:
- #1279 (12): I replaced maintain/spike/experiment's TEMPLATE includes with the
protocol.json include instead of carrying both, orphaning maintenance-run.md,
findings.md and notes.md. Builders would have stopped receiving those artifact
structures. All three restored alongside the JSON.
- Spec 746 (24): Baked Decisions shortened in SPIR and dropped from ASPIR/AIR,
losing the category hints, the amend/rescind escape hatch, and 'absence is the
no-op default'. That last one invites a builder to invent constraints where the
architect deliberately left them open. Restored in full.
Suite: 206 files, 4,117 tests, green - verified after the repairs, not before.
…e premature-green correction
…uously TWO FIXES, one format decision and one root cause. FORMAT DECISION (mine to make, per the architect): the parser learns brace notation. The plan's inspection model is per DECISION, not per file — twins are byte-identical and T7 verifies the sync mechanically, so the architect reads ~66 decisions rather than 131 diffs, and the <=12 batch cap counts decisions. One manifest row naming both tree paths is therefore the correct semantics; the parser had to understand it or the skeleton twins read as uninspectable, which is exactly what it reported. Chose this over splitting rows because splitting would have broken the cap and silently abandoned the per-decision model. ROOT CAUSE of my false green: T16 diffed origin/main...HEAD, which sees only COMMITTED work. I ran the full suite BEFORE committing Phase 3, so the test found no changed prompt files and passed VACUOUSLY — I then reported '4,117 green' for a state that did not contain the rewrite. The test was correct both times; my run measured a tree that no longer existed by the time I claimed it. Now considers committed AND working-tree changes, so a pre-commit run cannot be vacuously green. A guard that passes because it looked at the wrong tree is worse than no guard: it manufactures false confidence precisely when the work is unreviewed. Mutation-verified rather than assumed: removing the spir row from the manifest makes it fail; restoring it makes it pass. After a vacuous pass I am not accepting a green tick as evidence that a guard bites.
… found gh pr list / glab mr list default to open-only; post-merge consult --type pr lookups returned nothing. Add --state all (github) / --all (gitlab), mirroring the pr-exists precedent (#568).
Reviewer Integration Review (CMAP-3)Three-lane consultation (Gemini, GPT-5.6 Sol via Codex, Claude Opus) plus an independent architect pass. Because GitHub is serving a stale diff for this PR (see below), the lanes were fed the true effective diff and all consumer call sites directly; every finding below was verified against the live repo, several by direct Verdict: REQUEST CHANGES — one blocker, everything else small. All three lanes returned REQUEST_CHANGES, converging independently on the same regression, which was also reproduced live against this repository before the lanes ran. The fix itself is correct and needed; it just has to bring one caller with it. First, the diff you see is not the diff you getThe PR page shows 44 files, +2812/−5759. That is a stale artifact: this branch was cut from the Spec 1280 working branch, whose commits have since merged to main via PR #1362 and are now shared history. The true effective diff, computed locally against the real merge-base, is 5 files, +101/−2: the two script one-liners, one regression test, and the thread/status artifacts. Recommend rebasing onto main before merge so the record reflects reality; the merge itself is safe either way (branch is MERGEABLE, and a merge-tree run confirms it). Premise verified empiricallyIssue #759 is real and still unfixed on main, reproduced against this repo with merged PR #1468 (branch The Blocking: the fix breaks
|
|
One clarification on the blocker above, because the framing matters: the latent defect is ours, not yours. Your change is correct in isolation and fixes #759 exactly as specified. What it exposes is an unstated open-only assumption in The underlying contract gap ( |
2 participants
Summary
The
pr-searchforge concept only matched open PRs, so once a PR merged, the post-mergeconsult --type prlookup failed withNo PR found for branch: .... Add an all-states flag so the search includes merged/closed PRs.Fixes #759
Root Cause
packages/codev/scripts/forge/github/pr-search.shrangh pr list --search "$CODEV_SEARCH_QUERY"with no--state.gh pr listdefaults to--state open(confirmed viagh pr list --help), so merged PRs were excluded and the lookup returned nothing after merge. The GitLab counterpart (glab mr list --search) has the identical latent defect (defaults to opened).Fix
github/pr-search.sh: add--state allto thegh pr listcall.gitlab/pr-search.sh: add--allto theglab mr listcall (the established GitLab all-states flag, pergitlab/pr-exists.sh).This mirrors the pr-exists precedent (bugfix #568), which added
--state allfor the same reason. No giteapr-search.shexists; these scripts are single-source underpackages/codev/scripts/forge/(no skeleton mirror).Test Plan
bugfix-759-pr-search-state-all.test.ts) — asserts each script fetches all states while preserving the search query; fails without the fix, passes with it