Add first-class forge issue and pull request tools - #471
Conversation
Automated PR ReviewCriticalNo critical findings. ImportantF1: Tool-time CLI errors discard the diagnostics needed to recover. F2: A generic nonzero mutation exit is treated as proof that no write occurred. F3: Valid lowercase GitHub origins break mutation identity verification. Mutation response URLs are compared byte-for-byte with URLs constructed from the origin's casing ( F4: Native GitHub relationship failures silently become authoritative empty results. F5: Cross-repository native sub-issues can assign an unrelated local issue. The relationship model preserves only number and URL, not repository identity ( F6: GitLab commit and pipeline reads cannot prove pagination completeness. F7: GitLab's unsupported review decision is indistinguishable from “no decision.” F8: GitLab draft creation changes the user-approved exact title. F9: The review command's new “complete diff” contract still hardcodes F10: Successful edit tools discard the verified fresh postimage promised by the plan. The implementation refetches and byte-verifies the updated artifact/comment, then returns only identity, target, and requested deltas ( SuggestionsS1: Give expanded forge reads a human view, and separately reduce the agent-facing serialization noise. Ctrl+O currently appends the persisted result verbatim ( S2: Make comment-title edits invalid in the provider schema, not only at execution. The edit schema permits S3: Do not suppress unexpected startup-probe defects with the expected transient failures. The detached startup probe ends in S4: Remove the obsolete newline-terminated issue-body requirement. Strengths
This is an automated review. Reviewed by GPT-5.6 Sol |
Independent Review AssessmentAssessing review comment: #471 (comment) ClassificationsF1 — Genuine issue — Tool-time CLI errors discard recovery diagnostics. F2 — Genuine issue — Generic nonzero mutation exits are treated as definite failure. F3 — Genuine issue — Lowercase GitHub origins break mutation identity verification. Response URLs are compared case-sensitively against F4 — Genuine issue — Native GitHub relationship failures become authoritative empty results. Every nonzero native relationship call falls back ( F5 — Genuine issue — Cross-repository native sub-issues can assign an unrelated local issue. Relationship data omits repository identity and keys children by number ( F6 — Genuine issue — GitLab commit and pipeline reads cannot prove pagination completeness. F7 — Genuine issue — GitLab's unsupported review decision is indistinguishable from no decision. GitLab hardcodes F8 — Genuine issue — GitLab draft creation changes the approved exact title. Orchestration and adapter code prefix an unprefixed approved title ( F9 — Genuine issue — The complete-diff contract hardcodes F10 — Genuine issue — Edit tools discard the verified fresh postimage. The refetched artifact/comment is verified but S1 — Genuine issue — Multiline forge reads can regress token efficiency and expose unreadable canonical bridges. A reproducible S2 — Nitpick — Comment-title edits remain provider-schema-valid. The schema permits the combination, but execution rejects it before repository or adapter work and tests cover that boundary ( S3 — Nitpick — The startup probe suppresses unexpected defects with expected silent outcomes. The terminal catch hides all detached failures ( S4 — Genuine issue — The final-newline issue-body requirement is obsolete. Summary
Staged Implementation Plan
No findings are deferred or classified as Regression. Assessed by GPT-5.6 Sol |
Review fixesOriginating review ID: Implemented the complete assessed repair batch:
Commit: Verification: Notable decisions: provider-schema guidance remains flat to preserve the public edit call shape while runtime validation stays authoritative; GitLab review decisions remain explicitly unsupported rather than guessed; mutation diagnostics classify only redacted authentication evidence as conclusively rejected. Reviewed by GPT-5.6 Sol |
Automated Executable PR ValidationExecutable validation of PR #471 against linked issue #468 retained two independently admitted new-contract defects. Both proofs are normalized in existing permanent suites and fail on the frozen PR head without setup, discovery, or environmental errors. ImportantF1 — Forge failure diagnostics expose terminal-control and bidirectional formatting characters
Expected behavior: Bounded forge stdout/stderr remains actionable, but raw C1 and bidirectional formatting controls are rendered as visible escape sequences. Observed behavior: Head / merge-base classification: PR head red / merge base inapplicable. The forge client subsystem does not exist at merge base Root cause and confidence: High confidence. The production path is Approved-plan scope defense: Issue #468 requires safely rendered actionable command diagnostics, and the approved plan plus Practical impact: A failing Minimal fix direction: Add a local diagnostic-display helper in Exact proof patch: diff --git a/packages/scramjet/tests/forge-client.test.ts b/packages/scramjet/tests/forge-client.test.ts
index 3cf77f03a93476ce7d1a6e556fae433b22d7b741..bf0a6133dba4b6b759a8e5dfbfe8571bda1e2c70 100644
--- a/packages/scramjet/tests/forge-client.test.ts
+++ b/packages/scramjet/tests/forge-client.test.ts
@@ -147,6 +147,21 @@ describe("runForgeCommand", () => {
expect(Buffer.byteLength(error.invocation.process?.stderr ?? "", "utf8")).toBeLessThan(4200);
});
+ it("escapes terminal control characters in exposed diagnostics", async () => {
+ const exec: ForgeExec = async () => result({ code: 1, stderr: "remote rejected \u202Espoof\u009B31m" });
+ let caught: unknown;
+ try {
+ await runForgeCommand(exec, invocation);
+ } catch (error) {
+ caught = error;
+ }
+
+ expect(caught).toBeInstanceOf(ForgeCommandError);
+ const message = (caught as ForgeCommandError).message;
+ expect(/[\u009B\u202E]/u.test(message)).toBe(false);
+ expect(message).toContain(String.raw`\u202Espoof\u009B31m`);
+ });
+
it("does not classify authentication from echoed mutation content", async () => {
const stdin = JSON.stringify({ body: "HTTP 401 Unauthorized" });
const exec: ForgeExec = async () =>F2 — The feature-completeness lens can treat a truncated forge range as complete authority
Expected behavior: Each feature-completeness instruction that reads a PR or linked issue continues every returned range with the unchanged snapshot before claiming the complete conversation, comments, acceptance criteria, or latest plan. Observed behavior: The changed agent asks Head / merge-base classification: PR head red / merge base inapplicable. The merge-base agent used shell-backed reads and had no first-class Root cause and confidence: High confidence. The omission is directly in the production agent prompt, and the proof isolates both required read instructions. No runtime or fixture ambiguity remains. Approved-plan scope defense: Issue #468 and its approved Stage 5/8 plan require bounded, losslessly continuable aggregate forge reads and migration of covered Mach 12 reads. The shipped forge contract requires unchanged-snapshot continuation. The proof enforces only that declared behavior. Practical impact: Any PR or issue over 2,000 XML lines or 50KB triggers truncation; linked issue #468 already exceeds the byte bound. The lens can miss late plan revisions, acceptance decisions, or review-fix progress and publish an incorrect completeness assessment. The lens itself is read-only, so it does not directly alter repository state, but its durable review artifact can misdirect downstream fixes and merge decisions. The trigger is realistic and the operational severity is Important. Minimal fix direction: Modify only the two Step 1 bullets in Exact proof patch: diff --git a/packages/scramjet/tests/mach12-wiring.test.ts b/packages/scramjet/tests/mach12-wiring.test.ts
index 33790ae05e355eab427937d9c20c03337a07e0d3..f96c1cb0632df24fa2e3f59aa484e86486e1f1b9 100644
--- a/packages/scramjet/tests/mach12-wiring.test.ts
+++ b/packages/scramjet/tests/mach12-wiring.test.ts
@@ -1408,6 +1408,19 @@ describe("mach12 wiring — bundled agent set (F18)", () => {
expect(tools).not.toContain("bash");
expect(content).toContain("Use the complete diff supplied by the parent review command");
});
+
+ it("requires snapshot continuation for feature-completeness forge reads", () => {
+ const content = readFileSync(join(MACH12_AGENTS_DIR, "mach12:feature-completeness-checker.md"), "utf-8");
+ const contextStep = content.slice(
+ content.indexOf("### Step 1: Gather Requirements Context"),
+ content.indexOf("### Step 2: Catalog the Actual Changes"),
+ );
+
+ for (const tool of ["read_pr", "read_issue"]) {
+ const instruction = contextStep.split("\n").find((line) => line.includes(`Use \`${tool}\``));
+ expect(instruction).toContain("continue every returned range with the unchanged snapshot");
+ }
+ });
});
describe("mach12 test designer contract", () => {Candidate dispositions
Non-finding coverage observations:
No narrowing allowance was consumed. No production boundary was left unreviewed: all 27 changed production/command files were assigned exactly once across six behavioral clusters. Test-only and documentation changes were retained as coverage and contract evidence. Consolidated red resultDisplay-only command: Result: two test files failed with exactly the two retained nodes; 169 unrelated nodes were skipped. There were no extra assertion, setup, discovery, unhandled, or environmental failures. Structured executable manifestRendered command strings above are display-only. A fresh session must reconstruct argv locally from this manifest. {
"cwd": ".",
"runner": "node",
"runnerArgvPrefix": ["./node_modules/vitest/vitest.mjs", "run"],
"runnerAuthority": ["package.json devDependencies.vitest", "packages/scramjet/package.json scripts.test", "packages/scramjet/vitest.config.ts"],
"nodes": [
{
"finding": "F1",
"path": "packages/scramjet/tests/forge-client.test.ts",
"nodeId": "runForgeCommand > escapes terminal control characters in exposed diagnostics",
"testNamePattern": "escapes terminal control characters in exposed diagnostics"
},
{
"finding": "F2",
"path": "packages/scramjet/tests/mach12-wiring.test.ts",
"nodeId": "mach12 wiring — bundled agent set (F18) > requires snapshot continuation for feature-completeness forge reads",
"testNamePattern": "requires snapshot continuation for feature-completeness forge reads"
}
],
"consolidatedArgs": [
"--no-file-parallelism",
"--maxWorkers=1",
"--testNamePattern",
"escapes terminal control characters in exposed diagnostics|requires snapshot continuation for feature-completeness forge reads",
"packages/scramjet/tests/forge-client.test.ts",
"packages/scramjet/tests/mach12-wiring.test.ts"
],
"validation": "Repository-relative paths were control-free, did not begin with an option prefix, and resolved inside the applicable worktree. Node IDs were locally authored control-free values."
}Merge-base comparison requires a fresh detached worktree at the frozen actual merge base. F1 is base-inapplicable because Proof-patch manifest
Exhaustive finding-to-ownership-group mapping: Frozen identities and publication guard
This is an automated executable review. Reviewed by GPT-5.6 Sol |
Executable validation repairCommitted and pushed
Verification passed: 293 affected tests, all 2,675 workspace tests, Recovery deviation: GitHub altered control-character evidence in the original validation publication, so its whole-comment authentication and the dependent remote assessment artifact were deliberately bypassed. Both findings were instead independently reassessed from current PR/issue authority, immutable Git evidence, and fresh executable reruns before repair. The underlying handoff defect is tracked in #472. Reviewed by GPT-5.6 Sol |
Automated PR ReviewCriticalNo critical findings. ImportantF1: Partial mutation-content echoes can be misclassified as conclusive authentication rejection. Diagnostic redaction records the complete stdin and complete decoded JSON string values, but not fragments of those values ( F2: GitLab PR title edits can silently change draft state. Artifact edits compute and send any exact replacement title ( F3: Deferred-finding issue publication lacks approval of the exact title and body. The assessment command asks the user only whether to create issues, then performs duplicate classification, constructs overlap notes, and instructs the agent to generate a summarizing title/body before calling F4: The explicit permission-separation acceptance criterion has no executable proof. The tools are independently registered and documented ( F5: Branch creation assumes an issue title is always in the first bounded range. SuggestionsS1: Make complete-conversation reads explicit in readiness commands. S2: Add negative comment-evidence and process-settlement regressions. Cover missing executable plus supplied stdin in S3: Validate supported GitHub review decisions as a closed set. S4: Remove duplicate canonical document storage. Strengths
This is an automated review. Reviewed by GPT-5.6 Sol |
Independent Review AssessmentAssessing review comment: #471 (comment) ClassificationsF1 — Genuine issue — Partial mutation-content echoes can be misclassified as authentication rejection. Redaction removes complete stdin/string values only ( F2 — Genuine issue — GitLab PR title edits can silently change draft state. GitLab sends edited PR titles directly ( F3 — Genuine issue — Deferred-finding issues can be published without exact-content approval. The batch choice authorizes issue creation generally, but final titles, bodies, and overlap notes are assembled afterward and passed directly to F4 — Genuine issue — Permission separation lacks an executable integration proof. The real-session test registers all forge tools and invokes a read without narrowing the active set ( F5 — Genuine issue — Branch creation assumes the issue title is in the initial bounded range. The branch flow relies on the initial result ( S1 — Nitpick — Readiness commands should repeat continuation requirements. Both commands request the complete conversation ( S2 — Nitpick — Additional negative evidence and settlement regressions would strengthen coverage. Existing tests cover missing executables, stdin errors, timeout/abort settlement ( S3 — Genuine issue — Supported GitHub review decisions accept arbitrary strings. S4 — Nitpick — Canonical document text is stored twice. Summary
Staged Implementation PlanStage 1 — Required: Harden forge mutation and readiness semantics
Stage 2 — Required: Restore exact deferred-issue publication approval
Stage 3 — Required: Make issue-title acquisition continuation-safe
Stage 4 — Required: Prove permission separation
Stage 5 — Optional: Strengthen prompts and negative-path coverage
Stage 6 — Optional: Remove redundant rendered-document storage
Assessed by GPT-5.6 Sol |
Review fixesOriginating review ID: Implemented the complete assessed batch:
Commit: Verification: Reviewed by GPT-5.6 Sol |
Forge Token and Recovery Optimization PlanGoal and constraintsReduce recurring and per-call model tokens while preserving the existing eight tool names and parameter schemas, permission separation, private read receipts, exact-edit behavior, one-write mutation protocol, and post-write verification. Failures must remain concise, diagnosable, and recoverable through deliberate read-only Primary token contractToken efficiency is measured against the most compact practical, semantically equivalent provider-CLI result that the model would otherwise need to read:
CI uses deterministic UTF-8 byte ceilings as secondary regression guards. Token comparisons use disposable Public behaviorSuccessful mutations return only a compact verified identity summary, for example: They no longer repeat submitted bodies, aggregate XML, edit deltas, or verified postimages. The exact request remains in the persisted tool call, and full verification still happens internally. Expected failures use four stable recovery classes:
Text-based authentication matching may provide login guidance, but it does not prove rejection after mutation dispatch. Stage 1 — Preserve structured details on returned tool errorsModify:
Add optional Test first: returned errors emit and persist Stage 2 — Remove redundant successful-call tokensModify:
Replace Shorten descriptions, snippets, guidelines, and schema descriptions without changing schema shape, constraints, or requiredness. Preserve continuation, exact approval, prior-read, exact-edit, and no-Git-state guidance. Test first: all six mutation tools still perform one write plus verification; large body/replacement sentinels appear nowhere in returned content/details; fixed mutation results remain within 512 UTF-8 bytes and match or beat filtered CLI identity output; all eight schemas reject public proof/receipt fields. Stage 3 — Add classified recovery diagnosticsModify:
Add model-hidden Use one internal failure marker rather than a generalized result framework. Expected failures return For invocations without stdin, retain bounded, redacted, control-safe stdout/stderr in details. For stdin-bearing mutations, retain only exit status, stream byte counts/hashes, stdin byte count/hash, process flags, and authentication guidance—never raw process output. Omit or fingerprint large GraphQL query arguments in model-visible text. Test first with tables covering all classes/phases, zero-write preflight behavior, at-most-one-write ambiguity, forbidden retry language, correct GitHub/GitLab read-only fallback guidance, no automatic fallback invocation, secret/control redaction, bounded output, and structured details surviving a real Stage 4 — Handle unknown provider values without favorable guessingModify:
Represent future actor and review-decision values explicitly as unknown; never as approved, clear, or absent. Map unfamiliar mergeability and check states to explicit unknown rather than favorable states. Unknown GitLab Identity, ownership, pagination, requested-section completeness, evidence, and mutation-response validation remain strict. Stage 5 — Gate representation compaction on CLI-equivalent measurementsMeasure fixed issue/PR corpora against semantically equivalent compact Adopt optional PR record compaction only if the candidate:
The candidate may use one-line read-only Stage 6 — Documentation and verificationUpdate:
Document concise mutation results, classified recovery, model-hidden diagnostics, read-only fallback rules, ambiguous-write handling, unknown safety values, and any optional XML change that passes Stage 5. Run focused agent/forge suites, Guardrails
This plan intentionally uses deletion first. The only runtime addition is an optional error bit on the existing result object so the existing details channel can retain truthful diagnostics; it rejects exhaustive adapter outcome unions, branded completeness types, provider frameworks, and agent-managed recovery state. Reviewed by GPT-5.6 Sol |
Add an optional AgentToolResult.isError to the Pi agent runtime so extension tools can return real error results while preserving their structured details, instead of the previous throw-only path that discarded them. Rewire the eight forge tools onto it: expected post-validation failures return classified scramjet:forge-failure@1 details (class/phase/write-certainty) with model-hidden diagnostics, distinguishing no-write paths from ambiguous writes that must never be retried. Reduce forge token cost to match or beat equivalent gh/glab JSON: mutation success returns only the canonical URL; reads use compact self-closing XML with a lossless expanded fallback; verbose schema and prompt prose removed. Normalize unexpected provider states to explicit non-favorable "unknown" values rather than throwing. Records a new SCRAMJET-DIVERGENCE (agent-loop.ts, types.ts) in UPSTREAM_DIVERGENCE.md and updates the affected docs. Refs #468, #471.
Progress: forge token efficiency + graceful failureCommitted and pushed Summary of changes (by plan stage)
Files: 20 changed (+1146 / −423) — agent runtime (3), forge production (6), forge tests (5), docs (6). Verification
Notable decisions
Committed by Claude Opus 4.8 |
…ge-artifact-tools # Conflicts: # CLAUDE.md # packages/scramjet/mach12/commands/mach12:gh-comment.md # packages/scramjet/mach12/commands/mach12:pr-validation-assessment.md # packages/scramjet/mach12/commands/mach12:pr-validation.md # packages/scramjet/mach12/commands/mach12:push.md # packages/scramjet/tests/mach12-pr-validation.test.ts
Forge model/TUI representation alignmentCommitted and pushed
TDD regressions cover model/TUI byte-for-byte parity, implementation-marker absence, reserved-escape chunk boundaries, control/code-unit rendering, full-range reconstruction, total result bounds, and Markdown-heavy efficiency. A disposable Verification passed: Reviewed by GPT-5.6 Sol |
1 participant
Summary
Test plan
npm run typecheck.npm run build.npm test(2,648 tests).npm run lint.Fixes #468