From 21821a30bbca69e76f52baa30749d4f482776a3d Mon Sep 17 00:00:00 2001 From: Mansur Azatbek Date: Sun, 26 Jul 2026 23:41:29 +0500 Subject: [PATCH 1/7] test(guard): pin the stage/step tool matrix and its dynamic transitions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Before narrowing the active tool list per stage, the allowed set has to be a thing that cannot change by accident. Today it can: a tool dropped from a step's list produces no error a model can act on — the call is refused and the step has no way forward, so the only symptom is a run that stops. Writes the whole matrix out as a literal (65 assertions, one per stage/step plus the state transitions) rather than recomputing it from the table under test. Order is pinned too, not just membership: if the active list is ever narrowed to this set, the matrix becomes the head of the prompt, and a reordering re-reads the whole prefix on any backend that caches one. Also pins what appears and disappears without the step changing: the four planner_debug_* wrappers while a stuck debug session is open, the recovery set while broken or awaiting a user decision, planner_elenchus_check at the repair decision only, and the read-only pair while a compact boundary is pending. Two findings, recorded as assertions rather than fixed here: - planner_create_plan is reachable from no step at all. It is offered by the preflight decision before a plan exists (STATUS_ONLY_TOOLS), so a naive "every wrapper must appear in some step" check would call it dead. - planner_report_stuck is allowed on 7 execution steps but the debug wrappers are listed on only 6. At contract_check the model can report stuck — which opens a debug session — and then cannot call a single planner_debug_* tool, including the cleanup that closes it. Nothing gates on the leftover session, so the run is stranded rather than deadlocked. Co-Authored-By: Claude Opus 5 --- src/guard/tool-matrix.test.ts | 626 ++++++++++++++++++++++++++++++++++ 1 file changed, 626 insertions(+) create mode 100644 src/guard/tool-matrix.test.ts diff --git a/src/guard/tool-matrix.test.ts b/src/guard/tool-matrix.test.ts new file mode 100644 index 0000000..74b6912 --- /dev/null +++ b/src/guard/tool-matrix.test.ts @@ -0,0 +1,626 @@ +import { describe, expect, it } from "vitest"; +import { PLANNER_STAGE_STEPS } from "../storage/schema"; +import { + ALL_PLANNER_TOOL_NAMES, + getAllowedPlannerWrapperTools, + PLANNER_LIFECYCLE_TRANSITION_TOOLS, + PLANNER_WRAPPER_TOOLS, + type PlannerWrapperTool, +} from "./tool-policy"; + +/** + * The tool matrix, pinned. + * + * `getAllowedPlannerWrapperTools` is the call-time gate: it decides which + * semantic wrapper tools the model may use at a given planner position. Two + * separate things depend on it being exactly right, and both fail quietly: + * + * - **Reachability.** A tool dropped from a step's list is not an error the + * model can report — it just gets refused and has no way forward. The step + * deadlocks and the only symptom is a stuck run. + * - **Prompt shape.** If the active tool list is ever narrowed to this set (so + * the schemas leave the request), then this matrix *is* the head of the + * prompt, and a change here re-reads the whole prefix on every backend that + * caches one. Order is part of the bytes, so the expectations below pin the + * order too, not just the membership. + * + * So the matrix is written out in full rather than recomputed: a test that + * derives the answer from the same table it checks proves nothing. Regenerate + * by hand when a step's tools genuinely change, and read the diff — every line + * that moves is a tool the model gains or loses. + * + * The *visibility* layer (plan active, contract gate, recovery-report unlock) + * is a different mechanism and is covered in `index.tool-visibility.test.ts`. + */ + +const STAGE_STEP_MATRIX: Record< + string, + Record +> = { + init: { + check_project: [ + "planner_status", + "planner_artifact_read", + "planner_git_inspect", + ], + check_git: [ + "planner_status", + "planner_artifact_read", + "planner_git_inspect", + "planner_git_init", + ], + prepare_storage: ["planner_status", "planner_artifact_read"], + choose_worktree_location: ["planner_status", "planner_artifact_read"], + create_plan_record: ["planner_status", "planner_artifact_read"], + create_plan_worktree: [ + "planner_status", + "planner_artifact_read", + "planner_git_inspect", + ], + enter_intake: [ + "planner_status", + "planner_artifact_read", + "planner_git_inspect", + ], + }, + intake: { + draft_goal: [ + "planner_status", + "planner_artifact_read", + "planner_goal_submit", + ], + await_goal_approval: [ + "planner_status", + "planner_artifact_read", + "planner_goal_decide", + ], + }, + discovery: { + scan_project_structure: [ + "planner_status", + "planner_artifact_read", + "planner_git_inspect", + "planner_discovery_submit", + "planner_contract_scan", + "planner_contract_route", + "planner_contract_read", + "planner_contract_upsert", + "planner_elenchus_check", + "planner_reason", + "planner_git_commit", + "planner_exec", + ], + write_questions: [ + "planner_status", + "planner_artifact_read", + "planner_questions_submit", + "planner_questions_resolve", + "planner_contract_scan", + "planner_contract_route", + "planner_contract_read", + "planner_exec", + ], + enter_planning: ["planner_status", "planner_artifact_read"], + }, + spec: { + draft_requirements: [ + "planner_status", + "planner_artifact_read", + "planner_spec_submit", + "planner_contract_route", + "planner_contract_read", + ], + elicit_gaps: [ + "planner_status", + "planner_artifact_read", + "planner_questions_submit", + "planner_questions_resolve", + "planner_spec_submit", + ], + verify_spec: [ + "planner_status", + "planner_artifact_read", + "planner_gate_check", + "planner_spec_submit", + ], + finish_spec: ["planner_status", "planner_artifact_read"], + }, + planning: { + read_context: [ + "planner_status", + "planner_artifact_read", + "planner_contract_route", + "planner_contract_read", + ], + draft_plan: [ + "planner_status", + "planner_artifact_read", + "planner_plan_submit", + "planner_contract_route", + "planner_contract_read", + ], + split_tasks: [ + "planner_status", + "planner_artifact_read", + "planner_contract_route", + "planner_contract_read", + ], + write_task_files: [ + "planner_status", + "planner_artifact_read", + "planner_task_upsert", + "planner_contract_route", + "planner_contract_read", + ], + verify_plan: ["planner_status", "planner_artifact_read"], + consistency_check: [ + "planner_status", + "planner_artifact_read", + "planner_gate_check", + "planner_elenchus_check", + "planner_reason", + "planner_task_upsert", + "planner_contract_route", + "planner_contract_read", + ], + enter_execution: ["planner_status", "planner_artifact_read"], + }, + execution: { + prepare_task: [ + "planner_status", + "planner_artifact_read", + "planner_git_inspect", + "planner_git_create_task_branch", + ], + write_tdd_plan: [ + "planner_status", + "planner_artifact_read", + "planner_git_inspect", + "planner_tdd_submit", + "planner_behavior_upsert", + "planner_gate_check", + "planner_report_stuck", + "planner_skill_create", + "planner_skill_update", + "planner_elenchus_check", + "planner_reason", + "planner_exec", + ], + write_tests: [ + "planner_status", + "planner_artifact_read", + "planner_git_inspect", + "planner_tdd_submit", + "planner_behavior_upsert", + "planner_gate_check", + "planner_git_commit", + "planner_report_stuck", + "planner_skill_create", + "planner_skill_update", + "planner_exec", + ], + run_failing_tests: [ + "planner_status", + "planner_artifact_read", + "planner_git_inspect", + "planner_tdd_submit", + "planner_behavior_upsert", + "planner_gate_check", + "planner_report_stuck", + "planner_skill_create", + "planner_skill_update", + "planner_exec", + ], + implement_task: [ + "planner_status", + "planner_artifact_read", + "planner_git_inspect", + "planner_tdd_submit", + "planner_git_commit", + "planner_contract_route", + "planner_contract_read", + "planner_report_stuck", + "planner_skill_create", + "planner_skill_update", + "planner_exec", + ], + contract_check: [ + "planner_status", + "planner_artifact_read", + "planner_git_inspect", + "planner_tdd_submit", + "planner_git_commit", + "planner_contract_route", + "planner_contract_read", + "planner_contract_check", + "planner_contract_upsert", + "planner_elenchus_check", + "planner_reason", + "planner_report_stuck", + "planner_skill_create", + "planner_skill_update", + "planner_exec", + ], + refactor_task: [ + "planner_status", + "planner_artifact_read", + "planner_git_inspect", + "planner_tdd_submit", + "planner_refactor_review", + "planner_git_commit", + "planner_contract_route", + "planner_contract_read", + "planner_contract_check", + "planner_contract_upsert", + "planner_git_create_refactor_branch", + "planner_git_merge_refactor_to_task", + "planner_report_stuck", + "planner_skill_create", + "planner_skill_update", + "planner_exec", + ], + run_final_tests: [ + "planner_status", + "planner_artifact_read", + "planner_git_inspect", + "planner_tdd_submit", + "planner_behavior_upsert", + "planner_gate_check", + "planner_git_commit", + "planner_report_stuck", + "planner_skill_create", + "planner_skill_update", + "planner_exec", + ], + capture_skill: [ + "planner_status", + "planner_artifact_read", + "planner_git_inspect", + "planner_skill_create", + "planner_skill_update", + "planner_git_discard_changes", + ], + merge_task_to_plan: [ + "planner_status", + "planner_artifact_read", + "planner_git_inspect", + "planner_tdd_submit", + "planner_git_merge_task_to_plan", + ], + select_next_task: [ + "planner_status", + "planner_artifact_read", + "planner_git_inspect", + ], + }, + finalize: { + verify_plan_branch: [ + "planner_status", + "planner_artifact_read", + "planner_git_inspect", + "planner_contract_route", + "planner_contract_read", + "planner_git_discard_changes", + ], + compact_before_doubt: ["planner_status", "planner_artifact_read"], + doubt_review: [ + "planner_status", + "planner_artifact_read", + "planner_git_inspect", + "planner_doubt_review", + "planner_elenchus_check", + "planner_reason", + "planner_skill_create", + "planner_skill_update", + "planner_contract_route", + "planner_contract_read", + "planner_contract_check", + "planner_contract_upsert", + "planner_git_discard_changes", + ], + write_final_summary: [ + "planner_status", + "planner_artifact_read", + "planner_summary_submit", + "planner_skill_create", + "planner_skill_update", + "planner_contract_route", + "planner_contract_read", + "planner_contract_check", + "planner_contract_upsert", + "planner_git_discard_changes", + ], + enter_done: ["planner_status", "planner_artifact_read"], + }, + done: { + present_result: [ + "planner_status", + "planner_artifact_read", + "planner_skill_create", + "planner_skill_update", + "planner_contract_decide", + "planner_git_discard_changes", + ], + await_user_acceptance: [ + "planner_status", + "planner_artifact_read", + "planner_contract_decide", + "planner_git_discard_changes", + ], + handle_change_request: [ + "planner_status", + "planner_artifact_read", + "planner_plan_submit", + "planner_discovery_submit", + ], + prepare_output_branch: ["planner_status", "planner_artifact_read"], + merge_or_export_result: ["planner_status", "planner_artifact_read"], + cleanup_worktree: ["planner_status", "planner_artifact_read"], + mark_done: ["planner_status", "planner_artifact_read"], + cleanup_plan_files: ["planner_status", "planner_artifact_read"], + }, + recovery: { + read_state: [ + "planner_status", + "planner_artifact_read", + "planner_recovery_inspect", + ], + inspect_git: [ + "planner_status", + "planner_artifact_read", + "planner_git_inspect", + "planner_recovery_inspect", + ], + compare_expected_actual: [ + "planner_status", + "planner_artifact_read", + "planner_recovery_inspect", + ], + classify_recovery: [ + "planner_status", + "planner_artifact_read", + "planner_recovery_inspect", + ], + ask_user_if_destructive: [ + "planner_status", + "planner_artifact_read", + "planner_recovery_inspect", + ], + repair_or_resume: [ + "planner_status", + "planner_artifact_read", + "planner_recovery_inspect", + "planner_recovery_resume", + "planner_git_inspect", + "planner_elenchus_check", + "planner_reason", + ], + }, +}; + +/** A running, healthy state at the given position — the matrix baseline. */ +function runningAt(stage: string, step: string) { + return { + stage, + step, + broken: false, + requiresUserDecision: false, + requiresCompact: false, + debugArtifactsDir: null, + } as never; +} + +const DEBUG_TOOLS = [ + "planner_debug_strategy", + "planner_debug_probe", + "planner_debug_result", + "planner_debug_cleanup", +] as const; + +// Read-only and cross-stage: refusing these can never protect the state machine, +// and losing them blinds the model everywhere, including in recovery. +const ALWAYS = ["planner_status", "planner_artifact_read"] as const; + +describe("stage/step tool matrix", () => { + it("covers every stage and step the state machine can be in", () => { + expect(Object.keys(STAGE_STEP_MATRIX).sort()).toEqual( + Object.keys(PLANNER_STAGE_STEPS).sort(), + ); + for (const [stage, steps] of Object.entries(PLANNER_STAGE_STEPS)) { + expect(Object.keys(STAGE_STEP_MATRIX[stage])).toEqual([...steps]); + } + }); + + for (const [stage, steps] of Object.entries(STAGE_STEP_MATRIX)) { + for (const [step, expected] of Object.entries(steps)) { + it(`${stage}/${step} offers exactly ${expected.length} tools, in order`, () => { + expect(getAllowedPlannerWrapperTools(runningAt(stage, step))).toEqual( + expected, + ); + }); + } + } + + it("offers the read-only pair at every single position", () => { + for (const [stage, steps] of Object.entries(STAGE_STEP_MATRIX)) { + for (const step of Object.keys(steps)) { + const tools = getAllowedPlannerWrapperTools(runningAt(stage, step)); + expect(tools.slice(0, ALWAYS.length)).toEqual([...ALWAYS]); + } + } + }); +}); + +describe("tool reachability", () => { + it("leaves no wrapper tool unreachable from a normal run", () => { + const reachable = new Set(); + for (const [stage, steps] of Object.entries(STAGE_STEP_MATRIX)) { + for (const step of Object.keys(steps)) { + for (const tool of getAllowedPlannerWrapperTools( + runningAt(stage, step), + )) { + reachable.add(tool); + } + } + } + // Opened only by planner_report_stuck, so they are absent from the baseline. + for (const tool of DEBUG_TOOLS) reachable.add(tool); + // Reached through the preflight decision before a plan exists + // (STATUS_ONLY_TOOLS in runtime/planner-runtime.ts), never through a step. + reachable.add("planner_create_plan"); + // Offered only while the run is broken / awaiting a user decision. + reachable.add("planner_recovery_inspect"); + reachable.add("planner_recovery_resume"); + + expect(PLANNER_WRAPPER_TOOLS.filter((t) => !reachable.has(t))).toEqual([]); + }); + + it("keeps lifecycle transitions out of the wrapper gate entirely", () => { + // They are gated by their own exit conditions, not by this allowlist; if one + // ever appeared here it could be refused and the machine could not advance. + for (const tool of PLANNER_LIFECYCLE_TRANSITION_TOOLS) { + expect(PLANNER_WRAPPER_TOOLS as readonly string[]).not.toContain(tool); + expect(ALL_PLANNER_TOOL_NAMES as readonly string[]).toContain(tool); + } + }); +}); + +describe("tools that appear and disappear with run state", () => { + // planner_report_stuck opens a debug session (sets debugArtifactsDir); the four + // debug wrappers appear then, and planner_debug_cleanup closes it again. This is + // the one place where the offered set changes without the step changing. + it("adds the debug wrappers when a stuck debug session is open", () => { + const closed = getAllowedPlannerWrapperTools( + runningAt("execution", "implement_task"), + ); + const open = getAllowedPlannerWrapperTools({ + ...runningAt("execution", "implement_task"), + debugArtifactsDir: "/w/.pi/pi-code-planner/debug/task/sess", + } as never); + + expect(closed).not.toContain("planner_debug_probe"); + expect( + open.filter((t) => (DEBUG_TOOLS as readonly string[]).includes(t)), + ).toEqual([...DEBUG_TOOLS]); + // Opening a debug session only adds; nothing the model had is taken away. + expect( + open.filter((t) => !(DEBUG_TOOLS as readonly string[]).includes(t)), + ).toEqual([...closed]); + }); + + it("removes them again once the debug session is cleaned up", () => { + const after = getAllowedPlannerWrapperTools( + runningAt("execution", "implement_task"), + ); + for (const tool of DEBUG_TOOLS) expect(after).not.toContain(tool); + }); + + // `filterDebugToolsForState` only *removes* debug wrappers from a step whose + // list already contains them — it never adds one. So "a debug session is open" + // is necessary but not sufficient: the step must list them too. These two + // expectations pin which steps do, and they do NOT match the steps that allow + // planner_report_stuck. contract_check can report stuck (opening a session) + // yet cannot call a single planner_debug_* tool, including the cleanup that + // closes it. Nothing gates on the leftover session, so this strands the debug + // loop rather than deadlocking the run — but it is a gap, not a design. + const STUCK_STEPS = [ + "write_tdd_plan", + "write_tests", + "run_failing_tests", + "implement_task", + "contract_check", + "refactor_task", + "run_final_tests", + ] as const; + const DEBUG_CAPABLE_STEPS = [ + "write_tdd_plan", + "write_tests", + "run_failing_tests", + "implement_task", + "refactor_task", + "run_final_tests", + ] as const; + + it("allows planner_report_stuck on exactly these execution steps", () => { + const actual = Object.entries(STAGE_STEP_MATRIX.execution) + .filter(([, tools]) => tools.includes("planner_report_stuck")) + .map(([step]) => step); + expect(actual).toEqual([...STUCK_STEPS]); + }); + + it("drives the debug loop on exactly these execution steps", () => { + const withDebug = (step: string) => + getAllowedPlannerWrapperTools({ + ...runningAt("execution", step), + debugArtifactsDir: "/w/dbg", + } as never); + const actual = Object.keys(STAGE_STEP_MATRIX.execution).filter((step) => + withDebug(step).includes("planner_debug_probe"), + ); + expect(actual).toEqual([...DEBUG_CAPABLE_STEPS]); + // The gap, stated as a fact so it cannot widen unnoticed. + expect(STUCK_STEPS.filter((s) => !DEBUG_CAPABLE_STEPS.includes(s))).toEqual( + ["contract_check"], + ); + }); + + it("collapses to recovery tools while broken", () => { + expect( + getAllowedPlannerWrapperTools({ + ...runningAt("execution", "implement_task"), + broken: true, + } as never), + ).toEqual([ + "planner_status", + "planner_artifact_read", + "planner_git_inspect", + "planner_recovery_inspect", + "planner_recovery_resume", + ]); + }); + + it("collapses to the same set while awaiting a user decision", () => { + expect( + getAllowedPlannerWrapperTools({ + ...runningAt("done", "await_user_acceptance"), + requiresUserDecision: true, + } as never), + ).toEqual([ + "planner_status", + "planner_artifact_read", + "planner_git_inspect", + "planner_recovery_inspect", + "planner_recovery_resume", + ]); + }); + + it("adds the consistency checker at the repair decision, and only there", () => { + const repair = getAllowedPlannerWrapperTools({ + ...runningAt("recovery", "repair_or_resume"), + broken: true, + } as never); + expect(repair).toEqual([ + "planner_status", + "planner_artifact_read", + "planner_git_inspect", + "planner_recovery_inspect", + "planner_recovery_resume", + "planner_elenchus_check", + "planner_reason", + ]); + const otherRecoveryStep = getAllowedPlannerWrapperTools({ + ...runningAt("recovery", "classify_recovery"), + broken: true, + } as never); + expect(otherRecoveryStep).not.toContain("planner_elenchus_check"); + }); + + it("collapses to the read-only pair while a compact boundary is pending", () => { + expect( + getAllowedPlannerWrapperTools({ + ...runningAt("finalize", "compact_before_doubt"), + requiresCompact: true, + } as never), + ).toEqual([...ALWAYS]); + }); +}); From 18d3bb78e4690bde190c368c03be16e12895a39c Mon Sep 17 00:00:00 2001 From: Mansur Azatbek Date: Sun, 26 Jul 2026 23:51:03 +0500 Subject: [PATCH 2/7] fix(guard): derive the debug wrappers from the stuck flow instead of listing them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit planner_report_stuck unconditionally opens a debug session (initializePlannerDebugSession), so the stuck report and the four planner_debug_* wrappers are two halves of one flow. They were maintained as separate hand-written lists in two places, and both had drifted at the same step. At execution/contract_check the model could report stuck — opening a session and writing a README into the artifacts directory — and then call none of the debug wrappers. That is not merely a missing feature: the open directory makes assertNoPlannerDebugArtifactsBeforeCommit block planner_git_commit, and its error message tells the model to run planner_debug_cleanup, which the step refuses. The run escapes only via finish_step (commitPolicy there is allowed_if_dirty, not required), i.e. by ignoring the instruction it was just given, while already stuck. The guard now derives them — offered exactly when a session is open AND the step allows planner_report_stuck — so the two halves cannot drift again. The stage-behavior expectedTools list is still per-step, but its drift is already caught by tool-gating-invariant.test.ts, which failed on this change and is what surfaced the second list. Co-Authored-By: Claude Opus 5 --- src/guard/tool-matrix.test.ts | 50 +++++++++++++++------------- src/guard/tool-policy.ts | 62 ++++++++++++++--------------------- src/runtime/stage-behavior.ts | 9 +++++ 3 files changed, 62 insertions(+), 59 deletions(-) diff --git a/src/guard/tool-matrix.test.ts b/src/guard/tool-matrix.test.ts index 74b6912..7ca5a3f 100644 --- a/src/guard/tool-matrix.test.ts +++ b/src/guard/tool-matrix.test.ts @@ -515,14 +515,11 @@ describe("tools that appear and disappear with run state", () => { for (const tool of DEBUG_TOOLS) expect(after).not.toContain(tool); }); - // `filterDebugToolsForState` only *removes* debug wrappers from a step whose - // list already contains them — it never adds one. So "a debug session is open" - // is necessary but not sufficient: the step must list them too. These two - // expectations pin which steps do, and they do NOT match the steps that allow - // planner_report_stuck. contract_check can report stuck (opening a session) - // yet cannot call a single planner_debug_* tool, including the cleanup that - // closes it. Nothing gates on the leftover session, so this strands the debug - // loop rather than deadlocking the run — but it is a gap, not a design. + // The debug wrappers are derived, not listed per step: they are offered exactly + // when a session is open AND the step could have opened one (it allows + // planner_report_stuck). Pinned as an equality over the whole stage so the two + // halves of the stuck flow cannot drift apart again — they did once, and + // contract_check ended up able to open a session it could not close. const STUCK_STEPS = [ "write_tdd_plan", "write_tests", @@ -532,14 +529,6 @@ describe("tools that appear and disappear with run state", () => { "refactor_task", "run_final_tests", ] as const; - const DEBUG_CAPABLE_STEPS = [ - "write_tdd_plan", - "write_tests", - "run_failing_tests", - "implement_task", - "refactor_task", - "run_final_tests", - ] as const; it("allows planner_report_stuck on exactly these execution steps", () => { const actual = Object.entries(STAGE_STEP_MATRIX.execution) @@ -548,7 +537,7 @@ describe("tools that appear and disappear with run state", () => { expect(actual).toEqual([...STUCK_STEPS]); }); - it("drives the debug loop on exactly these execution steps", () => { + it("drives the debug loop on exactly the steps that can report stuck", () => { const withDebug = (step: string) => getAllowedPlannerWrapperTools({ ...runningAt("execution", step), @@ -557,11 +546,28 @@ describe("tools that appear and disappear with run state", () => { const actual = Object.keys(STAGE_STEP_MATRIX.execution).filter((step) => withDebug(step).includes("planner_debug_probe"), ); - expect(actual).toEqual([...DEBUG_CAPABLE_STEPS]); - // The gap, stated as a fact so it cannot widen unnoticed. - expect(STUCK_STEPS.filter((s) => !DEBUG_CAPABLE_STEPS.includes(s))).toEqual( - ["contract_check"], - ); + expect(actual).toEqual([...STUCK_STEPS]); + }); + + it("can always close a session it was able to open", () => { + // The failure this replaces: report stuck at contract_check, then find + // planner_git_commit blocked by the leftover artifacts and planner_debug_cleanup + // refused by the step. + for (const step of STUCK_STEPS) { + const open = getAllowedPlannerWrapperTools({ + ...runningAt("execution", step), + debugArtifactsDir: "/w/dbg", + } as never); + expect(open).toContain("planner_debug_cleanup"); + } + }); + + it("offers no debug wrapper on a step that cannot report stuck", () => { + const open = getAllowedPlannerWrapperTools({ + ...runningAt("execution", "prepare_task"), + debugArtifactsDir: "/w/dbg", + } as never); + for (const tool of DEBUG_TOOLS) expect(open).not.toContain(tool); }); it("collapses to recovery tools while broken", () => { diff --git a/src/guard/tool-policy.ts b/src/guard/tool-policy.ts index ec031f8..3fb47c7 100644 --- a/src/guard/tool-policy.ts +++ b/src/guard/tool-policy.ts @@ -206,10 +206,6 @@ const STEP_ALLOWED_TOOLS = { "planner_skill_update", "planner_elenchus_check", "planner_reason", - "planner_debug_strategy", - "planner_debug_probe", - "planner_debug_result", - "planner_debug_cleanup", "planner_exec", ], write_tests: [ @@ -221,10 +217,6 @@ const STEP_ALLOWED_TOOLS = { "planner_report_stuck", "planner_skill_create", "planner_skill_update", - "planner_debug_strategy", - "planner_debug_probe", - "planner_debug_result", - "planner_debug_cleanup", "planner_exec", ], run_failing_tests: [ @@ -235,10 +227,6 @@ const STEP_ALLOWED_TOOLS = { "planner_report_stuck", "planner_skill_create", "planner_skill_update", - "planner_debug_strategy", - "planner_debug_probe", - "planner_debug_result", - "planner_debug_cleanup", "planner_exec", ], implement_task: [ @@ -250,10 +238,6 @@ const STEP_ALLOWED_TOOLS = { "planner_report_stuck", "planner_skill_create", "planner_skill_update", - "planner_debug_strategy", - "planner_debug_probe", - "planner_debug_result", - "planner_debug_cleanup", "planner_exec", ], contract_check: [ @@ -285,10 +269,6 @@ const STEP_ALLOWED_TOOLS = { "planner_report_stuck", "planner_skill_create", "planner_skill_update", - "planner_debug_strategy", - "planner_debug_probe", - "planner_debug_result", - "planner_debug_cleanup", "planner_exec", ], run_final_tests: [ @@ -300,10 +280,6 @@ const STEP_ALLOWED_TOOLS = { "planner_report_stuck", "planner_skill_create", "planner_skill_update", - "planner_debug_strategy", - "planner_debug_probe", - "planner_debug_result", - "planner_debug_cleanup", "planner_exec", ], capture_skill: [ @@ -422,7 +398,10 @@ export function getAllowedPlannerWrapperTools( Record > = STEP_ALLOWED_TOOLS[state.stage]; const stepRules = stageRules[state.step] ?? []; - return withAlwaysAllowed(filterDebugToolsForState(stepRules, state)); + return withAlwaysAllowed([ + ...stepRules, + ...debugToolsForState(stepRules, state), + ]); } /** @@ -547,18 +526,27 @@ function withAlwaysAllowed( ); } -// Debug tools stay listed in STEP_ALLOWED_TOOLS for execution steps but are -// hidden unless a debug session is actually open (debugArtifactsDir set), so -// the model isn't offered debug wrappers it can't yet call. -function filterDebugToolsForState( - tools: readonly PlannerWrapperTool[], +/** + * The debug wrappers a step may use right now. + * + * They are DERIVED, not listed per step, because they are the second half of one + * flow: `planner_report_stuck` unconditionally opens a debug session + * (`initializePlannerDebugSession` in `runtime/debug-tools.ts`), and that session + * can only be driven — and closed — with these four. Maintaining the two lists by + * hand let them drift: `contract_check` allowed the stuck report but not the + * wrappers, so a model that reported stuck there opened a session it could not + * touch, and `planner_git_commit` stayed blocked by + * `assertNoPlannerDebugArtifactsBeforeCommit` while telling it to run a + * `planner_debug_cleanup` the step would refuse. + * + * So the rule is the invariant, and the lists cannot drift again: offer them + * exactly when a session is open AND this step could have opened one. + */ +function debugToolsForState( + stepTools: readonly PlannerWrapperTool[], state: Pick, ): readonly PlannerWrapperTool[] { - if (state.debugArtifactsDir) { - return tools; - } - return tools.filter( - (tool) => - !(DEBUG_WRAPPER_TOOLS as readonly PlannerWrapperTool[]).includes(tool), - ); + if (!state.debugArtifactsDir) return []; + if (!stepTools.includes("planner_report_stuck")) return []; + return DEBUG_WRAPPER_TOOLS; } diff --git a/src/runtime/stage-behavior.ts b/src/runtime/stage-behavior.ts index 5ad8347..a83e70a 100644 --- a/src/runtime/stage-behavior.ts +++ b/src/runtime/stage-behavior.ts @@ -486,6 +486,15 @@ export const PLANNER_STAGE_BEHAVIOR = { "planner_reason", "planner_git_commit", "planner_report_stuck", + // Listed wherever planner_report_stuck is: the stuck report opens a debug + // session unconditionally, and without these the step can open one it + // cannot drive or close — which also leaves planner_git_commit blocked by + // assertNoPlannerDebugArtifactsBeforeCommit. Kept in sync by + // runtime/tool-gating-invariant.test.ts. + "planner_debug_strategy", + "planner_debug_probe", + "planner_debug_result", + "planner_debug_cleanup", "planner_skill_create", "planner_skill_update", "planner_exec", From 9c7fbfe399ced376c50493a8bbc30326c7872205 Mon Sep 17 00:00:00 2001 From: Mansur Azatbek Date: Sun, 26 Jul 2026 23:53:30 +0500 Subject: [PATCH 3/7] fix(tools): state only our own tool invariants instead of declaring the world MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit updateToolVisibility rebuilt the active list from getAllTools() on every before_provider_request. setActiveTools is a global setter with no notion of whose tools are whose, so that resurrected every tool another extension had just hidden — and at least one other extension in the same session does the same thing, from the other side. Two extensions each declaring the whole world, on every request. The cost lands where it is hardest to see. Tool schemas sit at the head of the prompt, and a prefix cache reuses exactly one thing: bytes it has already read. A head that changes between two calls of one turn is a whole prompt re-read; measured elsewhere on this setup at prefill 24 302 -> 11 653 with the cache going 24 348 -> 0. So the list is now a subtraction from what is already active: next = (active now u what we claim) \ what we hide Nobody else's decision is overwritten, and it converges — our claim/hide sets are a pure function of planner state, so recomputing over the output yields the same list. A stable set of tools is a stable head. Order comes from walking the registry, since order is part of those bytes. The contract gate stays a sandbox rather than a subtraction: while it is up the model must not reach project reads at all. That needs the world remembered on the way in, because the live list IS the sandbox while it is up — subtracting from it on the way out would leave every other extension's tools hidden for good. Both new properties were checked against the old behaviour: reverting the subtraction fails the resurrection test, and dropping the remembered world fails both sandbox-exit tests. Co-Authored-By: Claude Opus 5 --- src/index.tool-visibility.test.ts | 157 ++++++++++++++++++++++++++++++ src/index.tool-visibility.ts | 102 ++++++++++++++++--- 2 files changed, 243 insertions(+), 16 deletions(-) diff --git a/src/index.tool-visibility.test.ts b/src/index.tool-visibility.test.ts index 53846be..e27d2d9 100644 --- a/src/index.tool-visibility.test.ts +++ b/src/index.tool-visibility.test.ts @@ -6,6 +6,7 @@ import { } from "./guard/tool-policy"; import { activatePlannerToolVisibility, + computePlannerActiveTools, filterPlannerTools, persistPlannerToolVisibilityActive, persistPlannerToolVisibilityActiveToSession, @@ -170,6 +171,7 @@ describe("updateToolVisibility", () => { const activeTools: string[][] = []; const mockPi = { getAllTools: () => mockTools, + getActiveTools: () => mockTools.map((t) => t.name), setActiveTools: (names: string[]) => { activeTools.push(names); }, @@ -184,6 +186,7 @@ describe("updateToolVisibility", () => { const activeTools: string[][] = []; const mockPi = { getAllTools: () => mockTools, + getActiveTools: () => mockTools.map((t) => t.name), setActiveTools: (names: string[]) => { activeTools.push(names); }, @@ -201,6 +204,7 @@ describe("updateToolVisibility", () => { const handlers: Record Promise> = {}; const mockPi = { getAllTools: () => mockTools, + getActiveTools: () => mockTools.map((t) => t.name), setActiveTools: (names: string[]) => { activeTools.push(names); }, @@ -228,6 +232,7 @@ describe("updateToolVisibility", () => { > = {}; const mockPi = { getAllTools: () => mockTools, + getActiveTools: () => mockTools.map((t) => t.name), setActiveTools: (names: string[]) => { activeTools.push(names); }, @@ -308,6 +313,7 @@ describe("contract gate tool visibility", () => { let captured: string[] = []; const mockPi = { getAllTools: () => tools, + getActiveTools: () => tools.map((t) => t.name), setActiveTools: (names: string[]) => { captured = names; }, @@ -363,6 +369,7 @@ describe("recovery report tool visibility", () => { let captured: string[] = []; const mockPi = { getAllTools: () => tools, + getActiveTools: () => tools.map((t) => t.name), setActiveTools: (names: string[]) => { captured = names; }, @@ -401,3 +408,153 @@ describe("recovery report tool visibility", () => { expect(setRecoveryReportUnlocked(true)).toBe(false); }); }); + +describe("computePlannerActiveTools", () => { + // Registry order, and it does not move — the result is built by walking it, so + // a stable decision produces byte-identical tool schemas at the head of the + // prompt. `manager_reply` stands in for another extension's tool. + const ALL = [ + "bash", + "read", + "manager_reply", + "planner_status", + "planner_contract_scan", + "planner_git_commit", + "planner_recovery_report", + "planner_finish_step", + ]; + + const compute = ( + over: Partial[0]>, + ) => + computePlannerActiveTools({ + allToolNames: ALL, + activeNow: ALL, + planActive: true, + contractGate: false, + recoveryReportUnlocked: false, + ...over, + }); + + it("leaves another extension's hidden tool hidden", () => { + // The bug this replaces: we rebuilt the list from getAllTools(), which + // resurrected every tool anyone else had just hidden — and changed the head + // of the prompt between two calls of one turn. + const activeNow = ALL.filter((name) => name !== "manager_reply"); + expect(compute({ activeNow })).not.toContain("manager_reply"); + }); + + it("hides every planner tool when no plan is active, and touches nothing else", () => { + expect(compute({ planActive: false })).toEqual([ + "bash", + "read", + "manager_reply", + ]); + }); + + it("claims planner tools back even when they are not currently active", () => { + // Ours must be reachable whatever anyone else decided the world should be. + const activeNow = ["bash"]; + const result = compute({ activeNow }); + expect(result).toContain("planner_status"); + expect(result).toContain("planner_finish_step"); + }); + + it("keeps the recovery report hidden until stuck-detection unlocks it", () => { + expect(compute({})).not.toContain("planner_recovery_report"); + expect(compute({ recoveryReportUnlocked: true })).toContain( + "planner_recovery_report", + ); + }); + + it("is a fixed point: recomputing over its own output changes nothing", () => { + // A stable set of tools is a stable head, which is a prompt the backend does + // not have to read twice. Convergence is the property that buys that. + const once = compute({}); + expect(compute({ activeNow: once })).toEqual(once); + }); + + it("preserves registry order, since order is part of the head's bytes", () => { + const result = compute({ activeNow: [...ALL].reverse() }); + expect(result).toEqual(ALL.filter((n) => n !== "planner_recovery_report")); + }); + + it("closes the world down to the allowlist while the contract gate is up", () => { + // A sandbox, not a subtraction: project reads must be unreachable, so other + // extensions' tools go too. + expect(compute({ contractGate: true })).toEqual([ + "planner_status", + "planner_contract_scan", + "planner_finish_step", + ]); + }); +}); + +describe("contract-gate sandbox entry and exit", () => { + const ALL = [ + "bash", + "manager_reply", + "planner_status", + "planner_contract_scan", + "planner_git_commit", + "planner_finish_step", + ]; + + function driver() { + let world = ALL.filter((name) => name !== "manager_reply"); + const mockPi = { + getAllTools: () => ALL.map((name) => ({ name })), + getActiveTools: () => world, + setActiveTools: (names: string[]) => { + world = names; + }, + } as unknown as ExtensionAPI; + return { + run: () => { + updateToolVisibility(mockPi); + return world; + }, + }; + } + + beforeEach(() => { + setPlanActive(true); + setContractGateActive(false); + setRecoveryReportUnlocked(false); + }); + + afterEach(() => { + setContractGateActive(false); + setPlanActive(false); + }); + + it("restores the pre-sandbox world on the way out, not the sandbox", () => { + // Subtracting from the live list on the way out would leave everyone else's + // tools hidden for good: while the gate is up the live list IS the sandbox, + // so there is nothing left to subtract from. + const { run } = driver(); + run(); + setContractGateActive(true); + expect(run()).toEqual([ + "planner_status", + "planner_contract_scan", + "planner_finish_step", + ]); + setContractGateActive(false); + const after = run(); + expect(after).toContain("bash"); + expect(after).toContain("planner_git_commit"); + // Still respected: another extension hid this before the gate went up. + expect(after).not.toContain("manager_reply"); + }); + + it("remembers the world once, not on every refresh inside the gate", () => { + const { run } = driver(); + run(); + setContractGateActive(true); + run(); + run(); + setContractGateActive(false); + expect(run()).toContain("bash"); + }); +}); diff --git a/src/index.tool-visibility.ts b/src/index.tool-visibility.ts index 04c4192..4e23783 100644 --- a/src/index.tool-visibility.ts +++ b/src/index.tool-visibility.ts @@ -161,25 +161,95 @@ export function resetPlanActiveCache(pi: ExtensionAPI): void { updateToolVisibility(pi); } +/** + * The active tool list to apply, as a SUBTRACTION from what is already active + * rather than a fresh statement of the whole world. + * + * `setActiveTools` is a global setter with no notion of whose tools are whose: + * whoever writes last wins the entire list. We used to rebuild it from + * `getAllTools()` on every provider request — and so does at least one other + * extension in the same session. Two extensions each declaring the world, on + * every request, resurrect each other's hidden tools and change the head of the + * prompt between two calls of one turn. The head is where the tool schemas live, + * and a prefix cache reuses exactly one thing: bytes it has already read. Change + * byte zero and the whole prompt is re-read. + * + * So we state only our own invariants and leave everyone else's decisions alone: + * + * next = (active now ∪ what we claim) \ what we hide + * + * This converges: our claim/hide sets are a pure function of planner state, so + * repeating the computation yields the same list — a stable set of tools, which + * is a stable head. Order is part of those bytes, so the result is built by + * walking the registry, whose order does not move. + * + * The contract gate is the exception, and deliberately so: it is a SANDBOX, not + * a subtraction. While it is up the model must not reach project reads at all, + * so the list is exactly the allowlist and other extensions' tools go with it. + */ +export function computePlannerActiveTools(input: { + allToolNames: readonly string[]; + activeNow: readonly string[]; + planActive: boolean; + contractGate: boolean; + recoveryReportUnlocked: boolean; +}): string[] { + if (input.planActive && input.contractGate) { + return input.allToolNames.filter((name) => CONTRACT_GATE_ALLOWED.has(name)); + } + + const claimed = new Set(); + const hidden = new Set(); + if (!input.planActive) { + // No plan: none of our tools belong in the prompt. + for (const name of plannerNames) hidden.add(name); + } else { + for (const name of plannerNames) claimed.add(name); + if (!input.recoveryReportUnlocked) { + claimed.delete(PLANNER_RECOVERY_REPORT_TOOL_NAME); + hidden.add(PLANNER_RECOVERY_REPORT_TOOL_NAME); + } + } + + const active = new Set(input.activeNow); + return input.allToolNames.filter( + (name) => (active.has(name) || claimed.has(name)) && !hidden.has(name), + ); +} + +/** + * The world as it was before the contract-gate sandbox went up. + * + * While the sandbox is up the live list IS the sandbox, so "subtract from what is + * active" would have almost nothing to subtract from: come out of the gate + * reading the live list and every other extension's tools would stay hidden for + * good. So the world is remembered on the way in and restored on the way out. + */ +let worldBeforeContractGate: string[] | null = null; + /** Update the list of active tools in the Pi extension API. */ export function updateToolVisibility(pi: ExtensionAPI): void { - const allTools = pi.getAllTools(); - let toolNames: string[]; - if (!planActiveCache) { - toolNames = filterPlannerTools(allTools).map((t) => t.name); - } else if (contractGateActive) { - toolNames = allTools - .map((t) => t.name) - .filter((name) => CONTRACT_GATE_ALLOWED.has(name)); - } else { - toolNames = allTools - .map((t) => t.name) - .filter( - (name) => - name !== PLANNER_RECOVERY_REPORT_TOOL_NAME || recoveryReportUnlocked, - ); + const allToolNames = pi.getAllTools().map((tool) => tool.name); + const sandboxed = planActiveCache && contractGateActive; + if (sandboxed) { + worldBeforeContractGate ??= pi.getActiveTools(); + } + const activeNow = sandboxed + ? [] + : (worldBeforeContractGate ?? pi.getActiveTools()); + if (!sandboxed) { + worldBeforeContractGate = null; } - pi.setActiveTools(toolNames); + + pi.setActiveTools( + computePlannerActiveTools({ + allToolNames, + activeNow, + planActive: planActiveCache, + contractGate: contractGateActive, + recoveryReportUnlocked, + }), + ); } function restorePlannerToolVisibilityFromSession(ctx?: ExtensionContext): void { From 41754e4e163847a861bd9b75d49b684e464e37d8 Mon Sep 17 00:00:00 2001 From: Mansur Azatbek Date: Sun, 26 Jul 2026 23:58:04 +0500 Subject: [PATCH 4/7] feat(runtime): watch the head of the prompt for churn nobody chose MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Narrowing the active tool list per stage would save real tokens — 54 planner schemas are ~13k of a fixed ~24k head on this project's own session, ~18% of a 131072 window that no compaction can reclaim. But it is a deliberate head change, and every serving backend reuses exactly one thing: a prefix of bytes it has already read. Deciding that without measuring is precisely how the compaction floor removed in 1b60f0d was built. So: measure first. `runtime/prefix-watch.ts` reduces each provider payload to what a prefix cache cares about (system prompt + serialized tool schemas, their fingerprint, the tools in the order offered) and reports when the head moved. The report separates two different questions. WHEN: between runs a changed tool set is expected; between two calls of one run nothing about the model's situation changed and the prefix was thrown away for nothing. WHO: setActiveTools is a global setter and this extension is not the only writer, so record() takes the list we last set. Our own mid-run changes are a cost chosen with open eyes, not news — an alarm that cannot recognise its owner's footsteps is one nobody keeps armed. Only mid-run AND foreign is a defect, surfaced as one ctx.ui.notify per run, locally, with nothing sent anywhere. Ported from pi-telegram-manager's core/payload-probe.ts, which is where this class of bug was first named — and where this extension was the one named. Co-Authored-By: Claude Opus 5 --- src/AGENTS.md | 2 +- src/index.tool-visibility.ts | 34 +++- src/index.ts | 41 +++++ src/runtime/AGENTS.md | 1 + src/runtime/prefix-watch.test.ts | 207 +++++++++++++++++++++++ src/runtime/prefix-watch.ts | 273 +++++++++++++++++++++++++++++++ 6 files changed, 548 insertions(+), 10 deletions(-) create mode 100644 src/runtime/prefix-watch.test.ts create mode 100644 src/runtime/prefix-watch.ts diff --git a/src/AGENTS.md b/src/AGENTS.md index 7575324..3debb99 100644 --- a/src/AGENTS.md +++ b/src/AGENTS.md @@ -36,7 +36,7 @@ Source domain for the Pi extension implementation. Route to narrower AGENTS.md f ### Domain Details - `index.ts` → Pi extension entry point; registers commands, tools, and events, and wires `guard/tool-policy.ts` in front of every tool call before it reaches `runtime/`. -- `index.tool-visibility.ts` → computes which tools are visible/hidden to the model for the current state (separate from whether a tool call is *allowed*, which `guard/tool-policy.ts` decides); read by `index.ts` when building the tool list for a turn. +- `index.tool-visibility.ts` → computes which tools are visible/hidden to the model for the current state (separate from whether a tool call is *allowed*, which `guard/tool-policy.ts` decides); read by `index.ts` when building the tool list for a turn. **It states only the planner's own invariants — never the whole world.** `pi.setActiveTools` is a global setter with no notion of whose tools are whose, and other extensions write it too; rebuilding the list from `getAllTools()` resurrected everything they had just hidden and rewrote the head of the prompt mid-turn, discarding the prefix cache. `computePlannerActiveTools` is therefore a subtraction — `(active now ∪ what we claim) \ what we hide`, walked in registry order because order is part of those bytes — and it converges, so a stable decision means a stable head. The contract gate is the one exception and is a *sandbox*: project reads must be unreachable, so the world is remembered on the way in and restored on the way out. `plannerLastSetToolNames()` exposes our own footprint for `runtime/prefix-watch.ts`. - `constants.ts` → two package-wide constants, `EXTENSION_NAME` and `SCHEMA_VERSION`; bump `SCHEMA_VERSION` only alongside a real persisted-schema migration in `storage/schema.ts`. - `public-api.ts` → barrel re-export of the full public surface (types + functions) across every domain — git, guard, instructions, project-local, runtime, session, settings, storage, worktree. Adding an export here without a corresponding domain change is a smell; every new domain export usually needs an entry added here too. - Most extension work starts in `src/index.ts`, then moves to a runtime/storage helper once behavior needs tests. diff --git a/src/index.tool-visibility.ts b/src/index.tool-visibility.ts index 4e23783..484127a 100644 --- a/src/index.tool-visibility.ts +++ b/src/index.tool-visibility.ts @@ -227,6 +227,22 @@ export function computePlannerActiveTools(input: { */ let worldBeforeContractGate: string[] | null = null; +/** + * The exact list we last wrote, or `null` before the first refresh — our own + * footprint on a global setter. + * + * `setActiveTools` has no notion of whose tools are whose, so a tool list + * arriving at the provider is not self-evidently ours: another extension may have + * written after us. Only the writer can answer "is this what WE decided?", and + * `runtime/prefix-watch.ts` needs the answer to tell a head we rewrote from a + * head somebody else did. + */ +let lastSetToolNames: string[] | null = null; + +export function plannerLastSetToolNames(): readonly string[] | null { + return lastSetToolNames; +} + /** Update the list of active tools in the Pi extension API. */ export function updateToolVisibility(pi: ExtensionAPI): void { const allToolNames = pi.getAllTools().map((tool) => tool.name); @@ -241,15 +257,15 @@ export function updateToolVisibility(pi: ExtensionAPI): void { worldBeforeContractGate = null; } - pi.setActiveTools( - computePlannerActiveTools({ - allToolNames, - activeNow, - planActive: planActiveCache, - contractGate: contractGateActive, - recoveryReportUnlocked, - }), - ); + const next = computePlannerActiveTools({ + allToolNames, + activeNow, + planActive: planActiveCache, + contractGate: contractGateActive, + recoveryReportUnlocked, + }); + lastSetToolNames = next; + pi.setActiveTools(next); } function restorePlannerToolVisibilityFromSession(ctx?: ExtensionContext): void { diff --git a/src/index.ts b/src/index.ts index b752032..8744dee 100644 --- a/src/index.ts +++ b/src/index.ts @@ -27,6 +27,7 @@ import { isPlanActive, markPlannerToolVisibilityActive, persistPlannerToolVisibilityActiveToSession, + plannerLastSetToolNames, registerPlannerToolVisibility, resetPlanActiveCache, setContractGateActive, @@ -158,6 +159,7 @@ import { PLANNER_PLAN_TOOL_NAMES, type PlannerPlanToolName, } from "./runtime/plan-tools"; +import { formatHeadChurnWarning, PrefixWatch } from "./runtime/prefix-watch"; import { runPlannerPreflight } from "./runtime/preflight"; import { executePlannerQuestionTool, @@ -1426,6 +1428,45 @@ export default function piCodePlannerExtension(pi: ExtensionAPI): void { registerPlannerSkillResources(pi); registerInstructionDefaultsSync(pi); registerPlannerToolVisibility(pi); + registerPlannerPrefixWatch(pi); +} + +/** + * Watch the head of every provider request and name it when something rewrites it + * mid-run. + * + * The tool schemas sit at the front of the prompt, and a prefix cache reuses one + * thing: bytes it has already read. Between runs a changed head is expected; a + * head rewritten between two calls of ONE run means the backend threw away + * everything it had read for nothing. This is measurement first — narrowing the + * active tool list per stage is a deliberate head change, and deciding that + * without numbers is how the previous context mechanism here was built and then + * had to be removed. + * + * Local only: `ctx.ui.notify`, nothing is ever sent anywhere. One notice per run, + * so an alarm firing on every call cannot drown the session it is reporting on. + */ +/** One watcher per extension load; a reload starts a fresh history. */ +const plannerPrefixWatch = new PrefixWatch(); + +function registerPlannerPrefixWatch(pi: ExtensionAPI): void { + let notifiedThisRun = false; + pi.on("agent_start", async () => { + plannerPrefixWatch.runStarted(); + notifiedThisRun = false; + }); + pi.on("before_provider_request", async (event, ctx) => { + const churn = plannerPrefixWatch.record( + event.payload, + plannerLastSetToolNames(), + ); + // Only a mid-run change we did not make is a defect: our own deliberate + // moves are a cost chosen with open eyes, and an alarm that cannot + // recognise its owner's footsteps is one nobody keeps armed. + if (!churn?.midRun || !churn.foreign || notifiedThisRun) return; + notifiedThisRun = true; + ctx.ui.notify(formatHeadChurnWarning(churn), "warning"); + }); } type RegisterToolFn = ExtensionAPI["registerTool"]; diff --git a/src/runtime/AGENTS.md b/src/runtime/AGENTS.md index ae9e005..59f59df 100644 --- a/src/runtime/AGENTS.md +++ b/src/runtime/AGENTS.md @@ -108,6 +108,7 @@ Runtime domain for planner stages, model-facing status, tool wrappers, timers, r **Misc** - `compact.ts` → builds the system-instructions bundle injected after a context compact (`PLANNER_COMPACT_MARKER`/`PLANNER_SYSTEM_INSTRUCTIONS_HEADER`), pulling section content from `instructions/manager.ts`. - **Window management belongs to Pi, not to the planner.** The extension no longer decides *when* to compact for context relief: the window-management compact steps (`compact_discovery`/`spec`/`planning`/`task` and `compact_finalize`) were removed from `PLANNER_STAGE_STEPS`, and the proactive `turn_end` monitor that replaced them (an output-aware token floor) has now been removed too. Reason: the floor was derived from `model.maxTokens` because an extension cannot read Pi's `reserveTokens`, so the two thresholds land wherever the user's settings happen to put them. On a real session (window 131072, `reserveTokens` 24576) our floor sat at 106823 against Pi's 106496 — 0.25% of the window apart, i.e. a guaranteed race instead of the intended separation, and a second compaction started right after Pi's leaves nothing to summarize (`prepareCompaction` measures only what accumulated since the previous cut point, so it throws "Nothing to compact (session too small)" while the window is 96% full). Pi's own auto-compaction sees `reserveTokens` and cannot race itself, so it is the only trigger now. The one surviving compact step is `compact_before_doubt` — a deliberate confidence reset before the doubt audit, not window relief — so `isPlannerCompactEnabled` returns true for it unconditionally and `planner_request_compact` runs it with no token gate at all. Everything that reacts *after* a compaction is untouched and works for Pi's compactions exactly as it did for ours: the `session_before_compact` indicator, the `session_compact` handler, and the post-compact `[SYSTEM_INSTRUCTIONS]` bundle. A legacy `state.json` parked at a removed step is remapped forward by `normalizePlanState` (`storage/state-store.ts`). The `compactBoundaries` (`compact.stage`/`compact.task`) setting and the `state.compactBoundaries` field were removed with the boundaries they gated — a legacy `settings.json` with a `compact` block gets an "unknown key" note, and a legacy `state.json` with a `compactBoundaries` field just ignores it. +- `prefix-watch.ts` → pure probe over the actual provider payload (`before_provider_request`). Tool schemas render INTO the system message, so they sit at the very front of the prompt, and every serving backend reuses exactly one thing: a prefix of bytes it already read. `describePayload` reduces a payload to `headChars`/`toolNames`/`toolChars`/`headKey`; `comparePayloads` reports what the head gained and lost; `PrefixWatch` remembers churn and separates *when* (between runs is expected; between two calls of ONE run the prefix was discarded for nothing) from *who* (`setActiveTools` is a global setter and this extension is not the only writer, so `record()` takes the list WE last set — `plannerLastSetToolNames()`). Only mid-run AND foreign is a defect; wired in `index.ts` `registerPlannerPrefixWatch`, one `ctx.ui.notify` per run, nothing sent anywhere. It exists as measurement BEFORE any per-stage narrowing of the active tool list: that is a deliberate head change, and the previous context mechanism here was built without numbers and had to be removed. Ported from pi-telegram-manager's `core/payload-probe.ts`. - `compact-eta.ts` → pure empirical ETA for the compaction indicator. `estimateCompactionDuration` fits `T(x)=a+b·x` (weighted least squares, recency- and model-weighted; falls back to a through-origin rate model) over `storage/compact-timing-store.ts` history and reports a point ETA + band + variance (`cv` → `stable`/`noisy`/`single`/`none`). `compactionProgressFraction` drives an honest asymptotic bar that fills to a confidence-scaled target at the ETA and never reaches 100% before the real completion event. Wired in `index.ts` `registerPlannerCompactEvents`, gated by `isPlanActive`; the SDK does not stream summary generation, so this learned model is the only real progress signal. - `skill-library.ts` → `planner_skill_create`/`planner_skill_update`: persists reusable skill markdown + a JSON index (`storage/json.ts`) keyed by source kind (`stuck`/`debug`/`doubt_review`/…); `status.ts` lists active skills from here. - `question-tools.ts` → `planner_questions_submit`/`planner_questions_resolve`, the user-clarification loop; writes state via `storage/state-store.ts`. diff --git a/src/runtime/prefix-watch.test.ts b/src/runtime/prefix-watch.test.ts new file mode 100644 index 0000000..69492ea --- /dev/null +++ b/src/runtime/prefix-watch.test.ts @@ -0,0 +1,207 @@ +import { describe, expect, it } from "vitest"; +import { + comparePayloads, + describePayload, + fingerprintHead, + formatHeadChurnWarning, + PrefixWatch, +} from "./prefix-watch"; + +// An openai-completions payload, which is what a llama.cpp server speaks: the +// system prompt is the leading message and the tool schemas ride alongside. +function payload(input: { tools?: string[]; system?: string; turns?: number }) { + return { + messages: [ + { role: "system", content: input.system ?? "you are a planner" }, + ...Array.from({ length: input.turns ?? 1 }, (_, i) => ({ + role: "user", + content: `turn ${i}`, + })), + ], + tools: (input.tools ?? ["planner_status"]).map((name) => ({ + type: "function", + function: { name, parameters: {} }, + })), + }; +} + +describe("describePayload", () => { + it("counts the system prompt and the tool schemas as the head", () => { + const shape = describePayload(payload({ tools: ["a", "b"] })); + expect(shape.toolNames).toEqual(["a", "b"]); + expect(shape.toolChars).toBeGreaterThan(0); + // The head is the serialized system message plus the serialized schemas — + // what the backend actually reads, not just the prose inside them. + const systemChars = JSON.stringify({ + role: "system", + content: "you are a planner", + }).length; + expect(shape.headChars).toBe(systemChars + shape.toolChars); + // The conversation is not the head — it comes after it. + expect(shape.messages).toBe(1); + expect(shape.messageChars).toBeGreaterThan(0); + }); + + it("reads an Anthropic-style system field as well as a leading message", () => { + const shape = describePayload({ + system: "abc", + messages: [{ role: "user", content: "hi" }], + tools: [], + }); + expect(shape.headChars).toBe(3); + expect(shape.messages).toBe(1); + }); + + it("charges nothing for an empty tool list", () => { + // Faithful to the payload, which omits the key, and to the prompt, where an + // empty list renders to nothing. + expect(describePayload({ messages: [], tools: [] }).toolChars).toBe(0); + }); + + it("survives a payload that is not an object", () => { + expect(describePayload(null).headChars).toBe(0); + expect(describePayload("nonsense").toolNames).toEqual([]); + }); + + it("names tools in both provider shapes", () => { + const shape = describePayload({ + messages: [], + tools: [{ function: { name: "wrapped" } }, { name: "bare" }, 42], + }); + expect(shape.toolNames).toEqual(["wrapped", "bare", "?"]); + }); +}); + +describe("head fingerprint", () => { + it("is equal for equal bytes and different for a one-character change", () => { + expect(fingerprintHead("abc")).toBe(fingerprintHead("abc")); + expect(fingerprintHead("abc")).not.toBe(fingerprintHead("abd")); + }); + + it("separates the same tools offered in a different order", () => { + // Order is part of the bytes: a reordered tool list is a different head, and + // a backend re-reads the whole prompt for it. + const a = describePayload(payload({ tools: ["x", "y"] })); + const b = describePayload(payload({ tools: ["y", "x"] })); + expect(a.headKey).not.toBe(b.headKey); + expect(comparePayloads(a, b).headStable).toBe(false); + }); + + it("is unmoved by a growing conversation", () => { + // The whole point: messages accumulate, the head does not, and the prefix + // stays reusable. + const a = describePayload(payload({ turns: 1 })); + const b = describePayload(payload({ turns: 9 })); + expect(comparePayloads(a, b).headStable).toBe(true); + }); +}); + +describe("comparePayloads", () => { + it("reports which tools were gained and lost, and by how much", () => { + const a = describePayload(payload({ tools: ["keep", "drop"] })); + const b = describePayload(payload({ tools: ["keep", "add"] })); + const delta = comparePayloads(a, b); + expect(delta.headStable).toBe(false); + expect(delta.toolsAdded).toEqual(["add"]); + expect(delta.toolsRemoved).toEqual(["drop"]); + expect(delta.headCharsDelta).not.toBe(Number.NaN); + }); +}); + +describe("PrefixWatch", () => { + const OURS = ["planner_status"]; + + it("says nothing about the first request of a session", () => { + const watch = new PrefixWatch(); + expect(watch.record(payload({}), OURS)).toBeNull(); + }); + + it("says nothing while the head holds still", () => { + const watch = new PrefixWatch(); + watch.record(payload({ turns: 1 }), OURS); + expect(watch.record(payload({ turns: 2 }), OURS)).toBeNull(); + expect(watch.history()).toEqual([]); + }); + + it("marks a change within one run as mid-run", () => { + const watch = new PrefixWatch(); + watch.runStarted(); + watch.record(payload({ tools: ["a"] }), ["a"]); + const churn = watch.record(payload({ tools: ["a", "b"] }), ["a", "b"]); + expect(churn?.midRun).toBe(true); + }); + + it("does not blame the first request after a run boundary", () => { + // Between runs the tool set may legitimately differ; that is not the defect. + const watch = new PrefixWatch(); + watch.runStarted(); + watch.record(payload({ tools: ["a"] }), ["a"]); + watch.runStarted(); + const churn = watch.record(payload({ tools: ["a", "b"] }), ["a", "b"]); + expect(churn?.midRun).toBe(false); + expect(watch.defects()).toEqual([]); + }); + + it("recognises its own footsteps and does not report them as a defect", () => { + const watch = new PrefixWatch(); + watch.runStarted(); + watch.record(payload({ tools: ["a"] }), ["a"]); + const churn = watch.record(payload({ tools: ["a", "b"] }), ["a", "b"]); + expect(churn?.foreign).toBe(false); + expect(watch.defects()).toEqual([]); + }); + + it("reports a mid-run head somebody else wrote", () => { + // The bug this exists to name: another extension rebuilt the whole active + // tool list on a request of ours, and the backend re-read the prompt. + const watch = new PrefixWatch(); + watch.runStarted(); + watch.record(payload({ tools: ["a"] }), ["a"]); + const churn = watch.record(payload({ tools: ["a", "stranger"] }), ["a"]); + expect(churn?.foreign).toBe(true); + expect(watch.defects()).toHaveLength(1); + }); + + it("accuses nobody when it does not know what it last set", () => { + const watch = new PrefixWatch(); + watch.runStarted(); + watch.record(payload({ tools: ["a"] }), null); + const churn = watch.record(payload({ tools: ["b"] }), null); + expect(churn?.foreign).toBe(false); + }); + + it("keeps only the newest churns", () => { + const watch = new PrefixWatch(() => 0, 2); + watch.runStarted(); + for (const tools of [["a"], ["b"], ["c"], ["d"]]) { + watch.record(payload({ tools }), tools); + } + expect(watch.history()).toHaveLength(2); + expect(watch.history()[0]?.shape.toolNames).toEqual(["d"]); + }); + + it("remembers the last shape it saw", () => { + const watch = new PrefixWatch(); + expect(watch.current()).toBeNull(); + watch.record(payload({ tools: ["a"] }), ["a"]); + expect(watch.current()?.toolNames).toEqual(["a"]); + }); +}); + +describe("formatHeadChurnWarning", () => { + it("says what moved, by how much, and that nothing left the machine", () => { + const watch = new PrefixWatch(); + watch.runStarted(); + watch.record(payload({ tools: ["a"] }), ["a"]); + const churn = watch.record(payload({ tools: ["a", "stranger"] }), ["a"]); + const text = formatHeadChurnWarning( + churn ?? + (() => { + throw new Error("expected a churn"); + })(), + ); + expect(text).toContain("mid-run"); + expect(text).toContain("stranger"); + expect(text).toContain("nothing was sent anywhere"); + }); +}); diff --git a/src/runtime/prefix-watch.ts b/src/runtime/prefix-watch.ts new file mode 100644 index 0000000..50fe167 --- /dev/null +++ b/src/runtime/prefix-watch.ts @@ -0,0 +1,273 @@ +/** + * What actually went to the provider — and whether its HEAD changed since last time. + * + * The planner can measure the messages it hands Pi. It cannot see the other half of a + * prompt: the system prompt Pi builds and the tool schemas it sends. On a local backend + * those are not a footnote — the chat template renders the tool list INTO the system + * message, so the tools sit at the very front of the prompt, ahead of every word anyone + * has said. Measured on this project's own session: 68 tool schemas ≈ 15.8k tokens plus a + * ~7.9k system prompt, i.e. ~18% of a 131072 window that no compaction can ever reclaim. + * + * Which makes the head the one thing that must not change without a reason. Every serving + * backend (llama.cpp, vLLM, SGLang, Anthropic's cache) reuses exactly one thing: a prefix + * of bytes it has already read. Change byte zero and the whole prompt is re-read — the + * tokens are the same, the seconds are not. + * + * This exists because narrowing the active tool list per stage is under consideration, and + * that is a deliberate head change. Deciding it without measuring is how the last + * context-management mechanism in this extension was built, and it had to be removed. So: + * measure first. A head change between RUNS is expected — the tool set legitimately + * differs. Between two calls of ONE run, nothing about the model's situation changed and + * the backend threw away everything it had read; that is the defect. + * + * Ported from pi-telegram-manager's `core/payload-probe.ts`, which found the class of bug + * this extension was on the wrong side of: two extensions each rebuilding the whole active + * tool list on every request, resurrecting each other's hidden tools and rewriting the + * head mid-turn. + * + * Structural typing, no SDK import: a payload is whatever the provider was handed. Every + * function here is pure, so the whole probe is testable with plain objects. + */ + +/** The parts of a provider payload we can compare. Provider-agnostic by shape. */ +interface PayloadLike { + messages?: unknown; + tools?: unknown; + system?: unknown; +} + +/** One request, reduced to what decides whether a backend can reuse it. */ +export interface PayloadShape { + /** Characters in the head: the system prompt plus the serialized tool schemas. */ + headChars: number; + /** The tools the model was offered, in the order it was offered them. */ + toolNames: string[]; + /** Characters in the tool schemas alone — usually most of the head. */ + toolChars: number; + /** Characters in the conversation. */ + messageChars: number; + messages: number; + /** A cheap fingerprint of the head. Equal heads → equal fingerprints. */ + headKey: string; +} + +/** How this request compares with the one before it. */ +export interface PayloadDelta { + /** The head is byte-identical to the previous request's. This is the good case. */ + headStable: boolean; + /** Tools the model gained since the previous request. */ + toolsAdded: string[]; + /** Tools it lost. */ + toolsRemoved: string[]; + /** Characters the head grew (negative: shrank). */ + headCharsDelta: number; +} + +/** A head that changed, and everything needed to say who changed it. */ +export interface HeadChurn { + at: number; + /** True when the head changed BETWEEN CALLS OF ONE RUN. */ + midRun: boolean; + /** + * The tools the model was offered are NOT the ones we last set: the list was + * rewritten by something outside this extension. + */ + foreign: boolean; + delta: PayloadDelta; + shape: PayloadShape; +} + +function textOf(value: unknown): string { + if (typeof value === "string") return value; + if (value === undefined || value === null) return ""; + return JSON.stringify(value); +} + +/** + * The name of a tool as the provider was given it. OpenAI-style wraps it + * (`{type:"function", function:{name}}`); Anthropic-style does not (`{name}`). + */ +function toolName(tool: unknown): string { + if (typeof tool !== "object" || tool === null) return "?"; + const record = tool as { name?: unknown; function?: { name?: unknown } }; + const name = record.function?.name ?? record.name; + return typeof name === "string" ? name : "?"; +} + +/** + * Whether a message is the system prompt. OpenAI-completions carries it as the + * first message (`role: "system"`); Anthropic carries it in a `system` field of + * its own, which {@link describePayload} reads separately. + */ +function isSystemMessage(message: unknown): boolean { + if (typeof message !== "object" || message === null) return false; + const role = (message as { role?: unknown }).role; + return role === "system" || role === "developer"; +} + +/** + * A stable fingerprint of a string. Not cryptographic and does not need to be: it + * answers one question — "are these the same bytes?" — for a value we already hold + * in full. + */ +export function fingerprintHead(text: string): string { + let h1 = 0x811c9dc5; + let h2 = 0x01000193; + for (let i = 0; i < text.length; i += 1) { + const c = text.charCodeAt(i); + h1 = Math.imul(h1 ^ c, 0x01000193); + h2 = Math.imul(h2 + c, 0x85ebca6b) ^ (h2 >>> 13); + } + const a = (h1 >>> 0).toString(16).padStart(8, "0"); + const b = (h2 >>> 0).toString(16).padStart(8, "0"); + return `${a}${b}:${text.length}`; +} + +/** Reduce a provider payload to the shape a prefix cache cares about. */ +export function describePayload(payload: unknown): PayloadShape { + const body = ( + typeof payload === "object" && payload !== null ? payload : {} + ) as PayloadLike; + const messages = Array.isArray(body.messages) ? body.messages : []; + const tools = Array.isArray(body.tools) ? body.tools : []; + + const leading = messages.filter(isSystemMessage); + const systemText = textOf(body.system) + leading.map(textOf).join(""); + // No tools → no tool bytes. Faithful to the payload, which omits the key + // entirely when the list is empty, and to the prompt, where it renders to + // nothing. + const toolsText = tools.length > 0 ? JSON.stringify(tools) : ""; + const conversation = messages.filter((message) => !isSystemMessage(message)); + const messageChars = conversation.reduce( + (sum, message) => sum + textOf(message).length, + 0, + ); + + return { + headChars: systemText.length + toolsText.length, + toolNames: tools.map(toolName), + toolChars: toolsText.length, + messageChars, + messages: conversation.length, + headKey: fingerprintHead(`${systemText} ${toolsText}`), + }; +} + +/** Compare a request with the one before it. */ +export function comparePayloads( + previous: PayloadShape, + next: PayloadShape, +): PayloadDelta { + const before = new Set(previous.toolNames); + const after = new Set(next.toolNames); + return { + headStable: previous.headKey === next.headKey, + toolsAdded: next.toolNames.filter((name) => !before.has(name)), + toolsRemoved: previous.toolNames.filter((name) => !after.has(name)), + headCharsDelta: next.headChars - previous.headChars, + }; +} + +/** Whether two tool lists hold the same names (order is not the question here). */ +function sameTools(a: readonly string[], b: readonly string[]): boolean { + if (a.length !== b.length) return false; + const seen = new Set(a); + return b.every((name) => seen.has(name)); +} + +/** + * Watches every provider request and remembers when the head changed. + * + * Two questions decide whether a head change is worth reporting, and they are + * different questions: + * + * - **When?** Between runs is expected. Between two calls of ONE run, nothing + * about the model's situation changed and the prefix was thrown away. + * - **Who?** `setActiveTools` is a global setter with no notion of whose tools + * are whose, and this extension is not the only one writing it. A mid-run + * change may be a stranger's — the thing worth naming — or it may be ours, + * which is a cost we chose with our eyes open, not news. + * + * Only a change we did not make counts as a defect. An alarm that cannot + * recognise its owner's footsteps is one nobody keeps armed. Hence + * {@link record} takes the list WE set, and {@link defects} counts only what is + * both mid-run and foreign. + */ +export class PrefixWatch { + private last: PayloadShape | null = null; + private churns: HeadChurn[] = []; + /** Requests seen since the current run began — 0 means the next one opens a run. */ + private requestsThisRun = 0; + + constructor( + private readonly now: () => number = Date.now, + /** How many churn events to keep. The newest are the ones anybody reads. */ + private readonly keep = 8, + ) {} + + /** A run started: the next request is its first, so a change there is not mid-run. */ + runStarted(): void { + this.requestsThisRun = 0; + } + + /** + * Record one provider request. Returns the churn it caused, if any. + * + * `ours` is the tool list we last wrote, or `null` when we have not written one + * — the only way to tell a head we rewrote from a head somebody else did. + * Unknown ownership is not an accusation: we cannot claim it and we will not + * blame anyone for it. + */ + record(payload: unknown, ours?: readonly string[] | null): HeadChurn | null { + const shape = describePayload(payload); + const previous = this.last; + this.last = shape; + const midRun = this.requestsThisRun > 0; + this.requestsThisRun += 1; + if (!previous) return null; + const delta = comparePayloads(previous, shape); + if (delta.headStable) return null; + const foreign = ours != null && !sameTools(shape.toolNames, ours); + const churn: HeadChurn = { at: this.now(), midRun, foreign, delta, shape }; + this.churns = [churn, ...this.churns].slice(0, this.keep); + return churn; + } + + /** The last request's shape, or null before the first one. */ + current(): PayloadShape | null { + return this.last; + } + + /** Head changes seen this session, newest first — ours and everyone else's. */ + history(): readonly HeadChurn[] { + return this.churns; + } + + /** A head rewritten mid-run by something outside this extension. */ + defects(): readonly HeadChurn[] { + return this.churns.filter((churn) => churn.midRun && churn.foreign); + } +} + +/** + * The one-line report for a mid-run foreign churn. Local only — the caller passes + * it to `ctx.ui.notify`; nothing is ever sent anywhere. + */ +export function formatHeadChurnWarning(churn: HeadChurn): string { + const moved = + churn.delta.headCharsDelta > 0 + ? `grew ${churn.delta.headCharsDelta}` + : `shrank ${-churn.delta.headCharsDelta}`; + const parts = [ + `Prompt head rewritten mid-run: ${moved} chars (~${Math.round(Math.abs(churn.delta.headCharsDelta) / 4)} tokens).`, + "The prefix cache was discarded and the whole prompt re-read.", + ]; + if (churn.delta.toolsAdded.length > 0) { + parts.push(`Tools added: ${churn.delta.toolsAdded.join(", ")}.`); + } + if (churn.delta.toolsRemoved.length > 0) { + parts.push(`Tools removed: ${churn.delta.toolsRemoved.join(", ")}.`); + } + parts.push("This is a local check — nothing was sent anywhere."); + return parts.join(" "); +} From 900bed55021d6630f7632ad08014c12733d900ba Mon Sep 17 00:00:00 2001 From: Mansur Azatbek Date: Mon, 27 Jul 2026 13:37:59 +0500 Subject: [PATCH 5/7] fix(contracts): page the contract-check coverage map instead of dumping it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The listing is one line per file in the HEAD commit, and nothing bounded it. A repository that committed its build output turned that into a single tool result of 79 802 characters; across one measured session planner_contract_check spent 215 211 characters in eight calls — 31.5% of everything the model read from tools. A result that size cannot be re-read, cannot be summarized away, and dominates the next compaction's cut point. So the map is paged rather than silently truncated: eight directories and ten files each, with the count of what is missing and the exact call that shows it. Paging is the model's decision, because a listing this large is usually noise and the one directory it needs is reachable directly — coveragePath takes a directory, or a file whose directory is shown. A page turn re-sends the required fields, which makes it indistinguishable from a second check except by what is already stored. So the stored check decides: matching task, action and summary means the tdd.md append already happened. Co-Authored-By: Claude Opus 5 --- src/index.ts | 10 ++ src/runtime/AGENTS.md | 2 +- src/runtime/contracts.test.ts | 113 +++++++++++++++ src/runtime/contracts.ts | 255 ++++++++++++++++++++++++++++------ 4 files changed, 339 insertions(+), 41 deletions(-) diff --git a/src/index.ts b/src/index.ts index 8744dee..0af23d9 100644 --- a/src/index.ts +++ b/src/index.ts @@ -551,6 +551,16 @@ const CONTRACT_CHECK_TOOL_PARAMETERS = { description: "Nearest meaningful AGENTS.md path when action is upsert_existing or create_new.", }, + coverageCursor: { + type: "number", + description: + "First directory of the coverage listing to show, 0-based. The result names the next cursor when more remain. Re-send the same fields; paging does not record a second check.", + }, + coveragePath: { + type: "string", + description: + "Show only one place in the coverage listing: a directory, or a file (the directory holding it is shown). Use instead of paging when only one domain matters.", + }, }, required: [ "action", diff --git a/src/runtime/AGENTS.md b/src/runtime/AGENTS.md index 59f59df..7d6a642 100644 --- a/src/runtime/AGENTS.md +++ b/src/runtime/AGENTS.md @@ -42,7 +42,7 @@ Runtime domain for planner stages, model-facing status, tool wrappers, timers, r - `planner-runtime.ts` → `evaluatePlannerRuntimeReality`: decides `allow_stage_machine` vs `require_recovery`/`require_user_decision`/`require_compact`/`no_active_plan` from `ActivePlanContextStatus` + `PlannerGitReality`. Feeds `lifecycle.ts`. - `preflight.ts` → `runPlannerPreflight`/`checkPlannerPreflightToolAllowed`: assembles `PlannerPreflightResult` (active plan context, git reality, instruction routing, runtime decision) — the single read pass every tool depends on before deciding anything. -**Contracts / DOX** (`contracts.ts`) — implements the AGENTS.md contract flow: scans AGENTS.md/context files → routes chains → reads → upserts → validates (`parsePlannerContractMarkdown`, `formatPlannerContractBlock`, `validateContractReferences`). Contract state (summaries, chains, touchedFiles) lives in `PlannerContractsState` inside `state.json`. `workflow-tools.ts` calls `validateContractCheckCompleted`/`validateDiscoveryContractRouting` from here to gate step completion, and `status.ts` calls `formatPlannerContractsStatus` to surface it to the model. +**Contracts / DOX** (`contracts.ts`) — implements the AGENTS.md contract flow: scans AGENTS.md/context files → routes chains → reads → upserts → validates (`parsePlannerContractMarkdown`, `formatPlannerContractBlock`, `validateContractReferences`). Contract state (summaries, chains, touchedFiles) lives in `PlannerContractsState` inside `state.json`. `workflow-tools.ts` calls `validateContractCheckCompleted`/`validateDiscoveryContractRouting` from here to gate step completion, and `status.ts` calls `formatPlannerContractsStatus` to surface it to the model. The coverage listing `planner_contract_check` returns is **paged** (`formatCoverageMapPage`), not whole: it is one line per file in the HEAD commit, so a repository that committed its build output produced a single 79 802-character tool result — 31.5% of all tool output in one measured session. The page names its own `coverageCursor` and offers `coveragePath` for one directory or the directory holding one file, so the model chooses how much of it to read. Because a page turn re-sends the same required fields, `contractCheck` compares them against `state.contracts.lastCheck` and skips the tdd.md append when they match — otherwise paging would write the same check twice. **Plan & task lifecycle tools** — model-facing tool handlers that read/write `PlanRecord`/`TaskRecord`/`PlanStateRecord`. - `plan-tools.ts` → create/improve a plan: syncs bundled instruction defaults, creates the plan branch (`git/branches.ts`), initializes plan files/state (`storage/plan-store.ts`, `storage/schema.ts` `createInitialPlanState`). diff --git a/src/runtime/contracts.test.ts b/src/runtime/contracts.test.ts index be6a681..b12a25d 100644 --- a/src/runtime/contracts.test.ts +++ b/src/runtime/contracts.test.ts @@ -25,7 +25,9 @@ import { initializePlanState } from "../storage/state-store"; import { MockPlannerFs } from "../test/mock-fs"; import { buildDirCoverageMap, + type DirectoryCoverageEntry, executePlannerContractTool, + formatCoverageMapPage, formatPlannerContractBlock, formatPlannerContractsStatus, initialWritableContractRequired, @@ -98,6 +100,117 @@ describe("buildDirCoverageMap gitignore filtering", () => { }); }); +describe("coverage map paging", () => { + const root = "/repo/app"; + const entry = ( + dir: string, + fileCount: number, + level: DirectoryCoverageEntry["agentsMdLevel"] = "none", + ): DirectoryCoverageEntry => ({ + dir: join(root, dir), + files: Array.from({ length: fileCount }, (_, i) => + join(root, dir, `f${i}.rs`), + ), + agentsMdPath: level === "none" ? null : join(root, dir, "AGENTS.md"), + agentsMdLevel: level, + }); + const many = (count: number) => + Array.from({ length: count }, (_, i) => entry(`d${i}`, 1)); + + it("says so when the commit touched nothing", () => { + const view = formatCoverageMapPage({ entries: [], root }); + expect(view.lines).toEqual(["No committed files detected in HEAD."]); + expect(view.page.nextCursor).toBeNull(); + }); + + it("shows everything and offers no next page when the map is small", () => { + const view = formatCoverageMapPage({ entries: many(3), root }); + expect(view.page).toMatchObject({ shown: 3, total: 3, nextCursor: null }); + expect(view.lines.join("\n")).not.toContain("coverageCursor"); + }); + + it("caps a large map and names the cursor that continues it", () => { + const view = formatCoverageMapPage({ entries: many(39), root }); + expect(view.page).toMatchObject({ cursor: 0, shown: 8, nextCursor: 8 }); + const text = view.lines.join("\n"); + expect(text).toContain("39 directories, 39 files"); + expect(text).toContain("Showing directories 1-8 of 39"); + expect(text).toContain("coverageCursor: 8"); + expect(text).toContain("31 more directories are not shown"); + }); + + it("keeps the whole listing far below one uncapped result", () => { + // The defect this exists for: a committed build directory produced a single + // 79 802-character result. Same shape, paged. + const huge = Array.from({ length: 39 }, (_, i) => entry(`d${i}`, 15)); + const uncapped = huge.reduce((sum, e) => sum + e.files.length, 0); + const view = formatCoverageMapPage({ entries: huge, root }); + expect(uncapped).toBe(585); + expect(view.lines.join("\n").length).toBeLessThan(4000); + }); + + it("continues from a cursor and ends without offering another page", () => { + const view = formatCoverageMapPage({ entries: many(10), root, cursor: 8 }); + expect(view.page).toMatchObject({ cursor: 8, shown: 2, nextCursor: null }); + expect(view.lines.join("\n")).toContain("Showing directories 9-10 of 10"); + }); + + it("truncates the files inside one directory and says how to see them", () => { + const view = formatCoverageMapPage({ entries: [entry("src", 25)], root }); + const text = view.lines.join("\n"); + expect(text).toContain("… 15 more files here"); + expect(text).toContain('coveragePath "src/"'); + }); + + it("selects a single directory by path", () => { + const entries = [...many(20), entry("src", 25, "exact")]; + const view = formatCoverageMapPage({ + entries, + root, + selectPath: "src", + }); + expect(view.page).toMatchObject({ + shown: 1, + selected: "src", + nextCursor: null, + }); + const text = view.lines.join("\n"); + expect(text).toContain("[AGENTS.md ✓]"); + // The selected directory earns the larger budget, so nothing is hidden. + expect(text).not.toContain("more files here"); + }); + + it("selects the directory holding a named file", () => { + const entries = [...many(20), entry("src", 3)]; + const view = formatCoverageMapPage({ + entries, + root, + selectPath: "src/f1.rs", + }); + expect(view.page.selected).toBe("src"); + }); + + it("accepts an absolute path as well as a root-relative one", () => { + const entries = [...many(20), entry("src", 3)]; + const view = formatCoverageMapPage({ + entries, + root, + selectPath: join(root, "src"), + }); + expect(view.page.selected).toBe("src"); + }); + + it("falls back to the first page when the path matches nothing", () => { + const view = formatCoverageMapPage({ + entries: many(39), + root, + selectPath: "nowhere/", + }); + expect(view.page).toMatchObject({ selected: null, cursor: 0, shown: 8 }); + expect(view.lines[0]).toContain("No touched directory matches"); + }); +}); + describe("planner local contracts parser", () => { const root = "/repo/app"; const path = "/repo/app/src/AGENTS.md"; diff --git a/src/runtime/contracts.ts b/src/runtime/contracts.ts index 57d13e3..702f8dd 100644 --- a/src/runtime/contracts.ts +++ b/src/runtime/contracts.ts @@ -464,7 +464,7 @@ async function contractRead( ); } -interface DirectoryCoverageEntry { +export interface DirectoryCoverageEntry { dir: string; files: string[]; agentsMdPath: string | null; @@ -542,33 +542,183 @@ export async function buildDirCoverageMap(input: { return entries; } -function formatCoverageMap( - entries: DirectoryCoverageEntry[], +/** + * How much of the coverage map one tool result may carry. + * + * The listing is unbounded by nature — it is one line per file in the HEAD + * commit — and a repository that committed its build output turns that into a + * single tool result larger than everything else the model reads. One measured + * session: 8 calls, 215 211 characters, 31.5% of all tool output, the worst of + * them 79 802 characters (39 directories, 580 files, nearly all of them + * `target/`). A result of that size cannot be re-read, cannot be summarized + * away, and dominates the next compaction's cut point. + * + * So the map is paged instead of truncated silently: the model is told what it + * is not seeing and given the exact call that shows it. Paging is the model's + * decision — a listing this large is usually noise, and the one directory it + * actually needs is reachable directly through `coveragePath`. + */ +const COVERAGE_PAGE_DIRS = 8; +const COVERAGE_DIR_FILES = 10; +/** A single selected directory earns a larger budget, but not an unbounded one. */ +const COVERAGE_SELECTED_DIR_FILES = 200; + +export interface CoverageMapPage { + /** Index of the first directory listed, 0-based. */ + cursor: number; + /** Cursor for the next page, or null when the listing ends here. */ + nextCursor: number | null; + /** Directories listed in this page. */ + shown: number; + /** Directories in the whole map. */ + total: number; + /** The directory the caller selected, relative to root, or null. */ + selected: string | null; +} + +export interface CoverageMapView { + lines: string[]; + page: CoverageMapPage; +} + +function coverageBadge(entry: DirectoryCoverageEntry, root: string): string { + if (entry.agentsMdLevel === "exact") return "[AGENTS.md ✓]"; + if (entry.agentsMdLevel === "parent") { + const parent = relative(root, dirname(entry.agentsMdPath ?? "")) || "."; + return `[AGENTS.md at parent: ${parent}]`; + } + return "[NO AGENTS.md]"; +} + +function coverageRule(entry: DirectoryCoverageEntry): string { + if (entry.agentsMdLevel === "exact") { + return "→ default: upsert_existing (prove no_update if nothing durable changed)"; + } + if (entry.agentsMdLevel === "parent") { + return "→ default: upsert_existing or create_new at this dir (prove no_update if subdomain is trivial)"; + } + return "→ default: create_new (prove no_update if change is fully self-contained)"; +} + +/** + * Find the entry a caller's `coveragePath` points at. A directory selects + * itself; a file selects the directory holding it, because that is the unit the + * map is keyed by and the unit an AGENTS.md covers. Absolute and root-relative + * paths both work — the model has seen both shapes in earlier results. + */ +function selectCoverageEntry( + entries: readonly DirectoryCoverageEntry[], root: string, -): string[] { - if (entries.length === 0) return ["No committed files detected in HEAD."]; - const lines: string[] = ["Touched directories (from git HEAD commit):"]; - for (const entry of entries) { + path: string, +): DirectoryCoverageEntry | null { + const absolute = isAbsolute(path) ? normalize(path) : join(root, path); + const trimmed = absolute.replace(/[/\\]+$/, ""); + const byDir = entries.find( + (entry) => entry.dir.replace(/[/\\]+$/, "") === trimmed, + ); + if (byDir) return byDir; + return ( + entries.find((entry) => entry.files.some((file) => file === trimmed)) ?? + null + ); +} + +/** + * The coverage map as one page of lines plus the cursor that reaches the rest. + * + * Pure: it takes the whole map and returns the slice, so the paging rules are + * testable without git, a filesystem, or a plan. + */ +export function formatCoverageMapPage(input: { + entries: readonly DirectoryCoverageEntry[]; + root: string; + cursor?: number; + selectPath?: string | null; +}): CoverageMapView { + const { entries, root } = input; + const total = entries.length; + if (total === 0) { + return { + lines: ["No committed files detected in HEAD."], + page: { cursor: 0, nextCursor: null, shown: 0, total: 0, selected: null }, + }; + } + + const totalFiles = entries.reduce( + (sum, entry) => sum + entry.files.length, + 0, + ); + const selected = input.selectPath + ? selectCoverageEntry(entries, root, input.selectPath) + : null; + const missSelection = Boolean(input.selectPath) && selected === null; + + const cursor = missSelection ? 0 : Math.min(input.cursor ?? 0, total); + const page = selected + ? [selected] + : entries.slice(cursor, cursor + COVERAGE_PAGE_DIRS); + const nextCursor = + selected || cursor + page.length >= total ? null : cursor + page.length; + const fileBudget = selected + ? COVERAGE_SELECTED_DIR_FILES + : COVERAGE_DIR_FILES; + + const lines: string[] = []; + if (missSelection) { + lines.push( + `No touched directory matches coveragePath "${input.selectPath}" — showing the first page instead.`, + ); + } + lines.push( + `Touched directories (from git HEAD commit): ${total} directories, ${totalFiles} files.`, + ); + lines.push( + selected + ? `Showing the directory selected by coveragePath.` + : `Showing directories ${cursor + 1}-${cursor + page.length} of ${total}.`, + ); + + for (const entry of page) { const relDir = relative(root, entry.dir) || "."; - const badge = - entry.agentsMdLevel === "exact" - ? "[AGENTS.md ✓]" - : entry.agentsMdLevel === "parent" - ? `[AGENTS.md at parent: ${relative(root, dirname(entry.agentsMdPath ?? ""))}]` - : "[NO AGENTS.md]"; - lines.push(` ${relDir}/ ${badge}`); - for (const f of entry.files) { - lines.push(` - ${relative(root, f)}`); + lines.push(` ${relDir}/ ${coverageBadge(entry, root)}`); + for (const file of entry.files.slice(0, fileBudget)) { + lines.push(` - ${relative(root, file)}`); + } + const hidden = entry.files.length - fileBudget; + if (hidden > 0) { + lines.push( + ` … ${hidden} more files here — pass coveragePath "${relDir}/" to list this directory alone.`, + ); } - const rule = - entry.agentsMdLevel === "exact" - ? "→ default: upsert_existing (prove no_update if nothing durable changed)" - : entry.agentsMdLevel === "parent" - ? "→ default: upsert_existing or create_new at this dir (prove no_update if subdomain is trivial)" - : "→ default: create_new (prove no_update if change is fully self-contained)"; - lines.push(` ${rule}`); + lines.push(` ${coverageRule(entry)}`); } - return lines; + + if (nextCursor !== null) { + lines.push(""); + lines.push( + `${total - (cursor + page.length)} more directories are not shown. This listing is paged so one commit cannot fill the context.`, + ); + lines.push( + `- Next page: call planner_contract_check again with the same fields plus coverageCursor: ${nextCursor}.`, + ); + lines.push( + `- One place instead: pass coveragePath with a directory ("src/") or a file ("src/main.rs" — the directory holding it is shown).`, + ); + lines.push( + "- Paging does not record a second check: the recorded action is the one already stored.", + ); + } + + return { + lines, + page: { + cursor, + nextCursor, + shown: page.length, + total, + selected: selected ? relative(root, selected.dir) || "." : null, + }, + }; } async function contractCheck(input: { @@ -599,6 +749,22 @@ async function contractCheck(input: { const domainImpact = requiredString(params, "domainImpact"); const evidence = stringArray(params.evidence, "evidence", true); const recommendedPath = optionalString(params.recommendedPath); + const coverageCursor = positiveIntegerOrUndefined( + params.coverageCursor, + "coverageCursor", + ); + const coveragePath = optionalString(params.coveragePath); + // A page turn re-sends the same fields, because the guard requires them and + // the model already holds them. That makes it indistinguishable from a real + // second check except by what is already stored, so the stored check decides: + // identical task, action and summary means this call is a page of the check + // that was already recorded, and appending it to tdd.md again would write the + // same evidence twice under the same heading. + const alreadyRecorded = + state.contracts.lastCheck !== null && + state.contracts.lastCheck.taskId === taskId && + state.contracts.lastCheck.action === action && + state.contracts.lastCheck.summary === outcomeSummary; const initialContractReason = initialWritableContractRequired({ state, action, @@ -631,17 +797,19 @@ async function contractCheck(input: { reason: outcomeSummary, taskId, }; - await appendContractCheckToTask({ - fs: input.fs, - planPaths, - taskId, - action, - outcomeSummary, - domainImpact, - changedFiles, - evidence, - recommendedPath: pendingUpsert?.path ?? null, - }); + if (!alreadyRecorded) { + await appendContractCheckToTask({ + fs: input.fs, + planPaths, + taskId, + action, + outcomeSummary, + domainImpact, + changedFiles, + evidence, + recommendedPath: pendingUpsert?.path ?? null, + }); + } const root = requireWorktreePath( state, "Planner contract tools require state.worktreePath.", @@ -667,21 +835,28 @@ async function contractCheck(input: { }, }, })); - const coverageLines = formatCoverageMap(coverageEntries, root); + const coverage = formatCoverageMapPage({ + entries: coverageEntries, + root, + cursor: coverageCursor, + selectPath: coveragePath, + }); return applied( input.toolName, [ - `Planner contract check recorded for task: ${taskId ?? "(none)"}.`, + alreadyRecorded + ? `Planner contract check already recorded for task: ${taskId ?? "(none)"} — this call only pages the coverage map.` + : `Planner contract check recorded for task: ${taskId ?? "(none)"}.`, `Action: ${action}`, pendingUpsert ? `Contract update required: call planner_contract_upsert for ${pendingUpsert.path}.` : "No contract update is required for this task.", "", - ...coverageLines, + ...coverage.lines, "", "Call planner_status before choosing the next planner action.", ].join("\n"), - { taskId, action, pendingUpsert, coverageEntries }, + { taskId, action, pendingUpsert, coverage: coverage.page }, ); } @@ -2145,7 +2320,7 @@ function positiveIntegerOrUndefined( key: string, ): number | undefined { if (value === undefined) return undefined; - const minimum = key === "cursor" ? 0 : 1; + const minimum = key.toLowerCase().endsWith("cursor") ? 0 : 1; if ( typeof value !== "number" || !Number.isInteger(value) || From 1e16229f6c1abe400e2f7c35b5bed89730654de1 Mon Sep 17 00:00:00 2001 From: Mansur Azatbek Date: Mon, 27 Jul 2026 13:39:49 +0500 Subject: [PATCH 6/7] refactor(runtime): keep the head alarm, drop the reporting surface behind it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PrefixWatch shipped a churn history, a defect filter and a ring buffer for a panel that does not exist. A grep for a reader outside the module finds none: index.ts calls runStarted() and record(), reads the returned churn once, and notifies. Everything else was a surface built for a future caller. What stays is the alarm, and it stays for a reason that stopped being hypothetical: setActiveTools is a global setter, and this session runs pi-telegram-manager, confirmed by four compactions it answered through session_before_compact. A mid-run head somebody else wrote is still worth naming, and the watch now holds exactly what naming it requires — the previous shape. Co-Authored-By: Claude Opus 5 --- src/runtime/AGENTS.md | 2 +- src/runtime/prefix-watch.test.ts | 29 +++++---------- src/runtime/prefix-watch.ts | 63 +++++++++++--------------------- 3 files changed, 33 insertions(+), 61 deletions(-) diff --git a/src/runtime/AGENTS.md b/src/runtime/AGENTS.md index 7d6a642..00af7a8 100644 --- a/src/runtime/AGENTS.md +++ b/src/runtime/AGENTS.md @@ -108,7 +108,7 @@ Runtime domain for planner stages, model-facing status, tool wrappers, timers, r **Misc** - `compact.ts` → builds the system-instructions bundle injected after a context compact (`PLANNER_COMPACT_MARKER`/`PLANNER_SYSTEM_INSTRUCTIONS_HEADER`), pulling section content from `instructions/manager.ts`. - **Window management belongs to Pi, not to the planner.** The extension no longer decides *when* to compact for context relief: the window-management compact steps (`compact_discovery`/`spec`/`planning`/`task` and `compact_finalize`) were removed from `PLANNER_STAGE_STEPS`, and the proactive `turn_end` monitor that replaced them (an output-aware token floor) has now been removed too. Reason: the floor was derived from `model.maxTokens` because an extension cannot read Pi's `reserveTokens`, so the two thresholds land wherever the user's settings happen to put them. On a real session (window 131072, `reserveTokens` 24576) our floor sat at 106823 against Pi's 106496 — 0.25% of the window apart, i.e. a guaranteed race instead of the intended separation, and a second compaction started right after Pi's leaves nothing to summarize (`prepareCompaction` measures only what accumulated since the previous cut point, so it throws "Nothing to compact (session too small)" while the window is 96% full). Pi's own auto-compaction sees `reserveTokens` and cannot race itself, so it is the only trigger now. The one surviving compact step is `compact_before_doubt` — a deliberate confidence reset before the doubt audit, not window relief — so `isPlannerCompactEnabled` returns true for it unconditionally and `planner_request_compact` runs it with no token gate at all. Everything that reacts *after* a compaction is untouched and works for Pi's compactions exactly as it did for ours: the `session_before_compact` indicator, the `session_compact` handler, and the post-compact `[SYSTEM_INSTRUCTIONS]` bundle. A legacy `state.json` parked at a removed step is remapped forward by `normalizePlanState` (`storage/state-store.ts`). The `compactBoundaries` (`compact.stage`/`compact.task`) setting and the `state.compactBoundaries` field were removed with the boundaries they gated — a legacy `settings.json` with a `compact` block gets an "unknown key" note, and a legacy `state.json` with a `compactBoundaries` field just ignores it. -- `prefix-watch.ts` → pure probe over the actual provider payload (`before_provider_request`). Tool schemas render INTO the system message, so they sit at the very front of the prompt, and every serving backend reuses exactly one thing: a prefix of bytes it already read. `describePayload` reduces a payload to `headChars`/`toolNames`/`toolChars`/`headKey`; `comparePayloads` reports what the head gained and lost; `PrefixWatch` remembers churn and separates *when* (between runs is expected; between two calls of ONE run the prefix was discarded for nothing) from *who* (`setActiveTools` is a global setter and this extension is not the only writer, so `record()` takes the list WE last set — `plannerLastSetToolNames()`). Only mid-run AND foreign is a defect; wired in `index.ts` `registerPlannerPrefixWatch`, one `ctx.ui.notify` per run, nothing sent anywhere. It exists as measurement BEFORE any per-stage narrowing of the active tool list: that is a deliberate head change, and the previous context mechanism here was built without numbers and had to be removed. Ported from pi-telegram-manager's `core/payload-probe.ts`. +- `prefix-watch.ts` → pure probe over the actual provider payload (`before_provider_request`). Tool schemas render INTO the system message, so they sit at the very front of the prompt, and every serving backend reuses exactly one thing: a prefix of bytes it already read. `describePayload` reduces a payload to `headChars`/`toolNames`/`toolChars`/`headKey`; `comparePayloads` reports what the head gained and lost; `PrefixWatch` holds ONE previous shape — no history, no defect log, nothing to report from later — and separates *when* (between runs is expected; between two calls of ONE run the prefix was discarded for nothing) from *who* (`setActiveTools` is a global setter and this extension is not the only writer, so `record()` takes the list WE last set — `plannerLastSetToolNames()`). Only mid-run AND foreign earns an alarm; wired in `index.ts` `registerPlannerPrefixWatch`, one `ctx.ui.notify` per run, nothing sent anywhere. It stays armed because the foreign writer is real, not hypothetical: the same session runs pi-telegram-manager, which answers `session_before_compact` with its own compaction. Ported from that extension's `core/payload-probe.ts`. - `compact-eta.ts` → pure empirical ETA for the compaction indicator. `estimateCompactionDuration` fits `T(x)=a+b·x` (weighted least squares, recency- and model-weighted; falls back to a through-origin rate model) over `storage/compact-timing-store.ts` history and reports a point ETA + band + variance (`cv` → `stable`/`noisy`/`single`/`none`). `compactionProgressFraction` drives an honest asymptotic bar that fills to a confidence-scaled target at the ETA and never reaches 100% before the real completion event. Wired in `index.ts` `registerPlannerCompactEvents`, gated by `isPlanActive`; the SDK does not stream summary generation, so this learned model is the only real progress signal. - `skill-library.ts` → `planner_skill_create`/`planner_skill_update`: persists reusable skill markdown + a JSON index (`storage/json.ts`) keyed by source kind (`stuck`/`debug`/`doubt_review`/…); `status.ts` lists active skills from here. - `question-tools.ts` → `planner_questions_submit`/`planner_questions_resolve`, the user-clarification loop; writes state via `storage/state-store.ts`. diff --git a/src/runtime/prefix-watch.test.ts b/src/runtime/prefix-watch.test.ts index 69492ea..8a10083 100644 --- a/src/runtime/prefix-watch.test.ts +++ b/src/runtime/prefix-watch.test.ts @@ -120,7 +120,6 @@ describe("PrefixWatch", () => { const watch = new PrefixWatch(); watch.record(payload({ turns: 1 }), OURS); expect(watch.record(payload({ turns: 2 }), OURS)).toBeNull(); - expect(watch.history()).toEqual([]); }); it("marks a change within one run as mid-run", () => { @@ -139,16 +138,14 @@ describe("PrefixWatch", () => { watch.runStarted(); const churn = watch.record(payload({ tools: ["a", "b"] }), ["a", "b"]); expect(churn?.midRun).toBe(false); - expect(watch.defects()).toEqual([]); }); - it("recognises its own footsteps and does not report them as a defect", () => { + it("recognises its own footsteps and does not report them as foreign", () => { const watch = new PrefixWatch(); watch.runStarted(); watch.record(payload({ tools: ["a"] }), ["a"]); const churn = watch.record(payload({ tools: ["a", "b"] }), ["a", "b"]); expect(churn?.foreign).toBe(false); - expect(watch.defects()).toEqual([]); }); it("reports a mid-run head somebody else wrote", () => { @@ -158,8 +155,7 @@ describe("PrefixWatch", () => { watch.runStarted(); watch.record(payload({ tools: ["a"] }), ["a"]); const churn = watch.record(payload({ tools: ["a", "stranger"] }), ["a"]); - expect(churn?.foreign).toBe(true); - expect(watch.defects()).toHaveLength(1); + expect(churn).toMatchObject({ midRun: true, foreign: true }); }); it("accuses nobody when it does not know what it last set", () => { @@ -170,21 +166,16 @@ describe("PrefixWatch", () => { expect(churn?.foreign).toBe(false); }); - it("keeps only the newest churns", () => { - const watch = new PrefixWatch(() => 0, 2); + it("compares each request against the one before it, not against the first", () => { + // Nothing accumulates: the watch holds one shape, so a churn is always the + // step just taken and never a replay of an older one. + const watch = new PrefixWatch(() => 0); watch.runStarted(); - for (const tools of [["a"], ["b"], ["c"], ["d"]]) { - watch.record(payload({ tools }), tools); - } - expect(watch.history()).toHaveLength(2); - expect(watch.history()[0]?.shape.toolNames).toEqual(["d"]); - }); - - it("remembers the last shape it saw", () => { - const watch = new PrefixWatch(); - expect(watch.current()).toBeNull(); watch.record(payload({ tools: ["a"] }), ["a"]); - expect(watch.current()?.toolNames).toEqual(["a"]); + watch.record(payload({ tools: ["b"] }), ["b"]); + const churn = watch.record(payload({ tools: ["c"] }), ["c"]); + expect(churn?.delta.toolsRemoved).toEqual(["b"]); + expect(churn?.delta.toolsAdded).toEqual(["c"]); }); }); diff --git a/src/runtime/prefix-watch.ts b/src/runtime/prefix-watch.ts index 50fe167..e743541 100644 --- a/src/runtime/prefix-watch.ts +++ b/src/runtime/prefix-watch.ts @@ -5,25 +5,25 @@ * prompt: the system prompt Pi builds and the tool schemas it sends. On a local backend * those are not a footnote — the chat template renders the tool list INTO the system * message, so the tools sit at the very front of the prompt, ahead of every word anyone - * has said. Measured on this project's own session: 68 tool schemas ≈ 15.8k tokens plus a - * ~7.9k system prompt, i.e. ~18% of a 131072 window that no compaction can ever reclaim. + * has said. Measured on a real planner session: a 32 197-character system prompt plus + * 61 023 characters of schemas for 65 tools, 52 015 of them the planner's own 54 — about + * 23 300 tokens, ~18% of a 131072 window that no compaction can ever reclaim. * * Which makes the head the one thing that must not change without a reason. Every serving * backend (llama.cpp, vLLM, SGLang, Anthropic's cache) reuses exactly one thing: a prefix * of bytes it has already read. Change byte zero and the whole prompt is re-read — the * tokens are the same, the seconds are not. * - * This exists because narrowing the active tool list per stage is under consideration, and - * that is a deliberate head change. Deciding it without measuring is how the last - * context-management mechanism in this extension was built, and it had to be removed. So: - * measure first. A head change between RUNS is expected — the tool set legitimately - * differs. Between two calls of ONE run, nothing about the model's situation changed and - * the backend threw away everything it had read; that is the defect. + * A head change between RUNS is expected: the tool set legitimately differs. Between two + * calls of ONE run, nothing about the model's situation changed and the backend threw away + * everything it had read; that is the defect this names. * - * Ported from pi-telegram-manager's `core/payload-probe.ts`, which found the class of bug - * this extension was on the wrong side of: two extensions each rebuilding the whole active - * tool list on every request, resurrecting each other's hidden tools and rewriting the - * head mid-turn. + * It stays armed because `setActiveTools` is a global setter and this extension is not the + * only one writing it — the same session runs pi-telegram-manager, confirmed by four + * compactions it answered through `session_before_compact`. Ported from that extension's + * `core/payload-probe.ts`, which found the class of bug this one was on the wrong side of: + * two extensions each rebuilding the whole active tool list on every request, resurrecting + * each other's hidden tools and rewriting the head mid-turn. * * Structural typing, no SDK import: a payload is whatever the provider was handed. Every * function here is pure, so the whole probe is testable with plain objects. @@ -176,7 +176,7 @@ function sameTools(a: readonly string[], b: readonly string[]): boolean { } /** - * Watches every provider request and remembers when the head changed. + * Watches every provider request and reports the ones whose head changed. * * Two questions decide whether a head change is worth reporting, and they are * different questions: @@ -188,22 +188,20 @@ function sameTools(a: readonly string[], b: readonly string[]): boolean { * change may be a stranger's — the thing worth naming — or it may be ours, * which is a cost we chose with our eyes open, not news. * - * Only a change we did not make counts as a defect. An alarm that cannot - * recognise its owner's footsteps is one nobody keeps armed. Hence - * {@link record} takes the list WE set, and {@link defects} counts only what is - * both mid-run and foreign. + * Only a change we did not make is worth an alarm, and an alarm that cannot + * recognise its owner's footsteps is one nobody keeps armed — so {@link record} + * takes the list WE set and reports both answers to the caller, which decides. + * + * It keeps only the previous shape. Nothing here accumulates a history: a + * session-long log of head changes would be a reporting surface, and this + * module has exactly one caller, which reads each churn once and then notifies. */ export class PrefixWatch { private last: PayloadShape | null = null; - private churns: HeadChurn[] = []; /** Requests seen since the current run began — 0 means the next one opens a run. */ private requestsThisRun = 0; - constructor( - private readonly now: () => number = Date.now, - /** How many churn events to keep. The newest are the ones anybody reads. */ - private readonly keep = 8, - ) {} + constructor(private readonly now: () => number = Date.now) {} /** A run started: the next request is its first, so a change there is not mid-run. */ runStarted(): void { @@ -228,24 +226,7 @@ export class PrefixWatch { const delta = comparePayloads(previous, shape); if (delta.headStable) return null; const foreign = ours != null && !sameTools(shape.toolNames, ours); - const churn: HeadChurn = { at: this.now(), midRun, foreign, delta, shape }; - this.churns = [churn, ...this.churns].slice(0, this.keep); - return churn; - } - - /** The last request's shape, or null before the first one. */ - current(): PayloadShape | null { - return this.last; - } - - /** Head changes seen this session, newest first — ours and everyone else's. */ - history(): readonly HeadChurn[] { - return this.churns; - } - - /** A head rewritten mid-run by something outside this extension. */ - defects(): readonly HeadChurn[] { - return this.churns.filter((churn) => churn.midRun && churn.foreign); + return { at: this.now(), midRun, foreign, delta, shape }; } } From c216790e229e3546eb72da739bd78f1413632119 Mon Sep 17 00:00:00 2001 From: Mansur Azatbek Date: Mon, 27 Jul 2026 13:45:36 +0500 Subject: [PATCH 7/7] fix(contracts): strip trailing separators by scanning, not by pattern CodeQL is right: /[/\\]+$/ is a repeated character class anchored at the end, so a failed match backtracks from every position and a path that is mostly separators costs quadratic time. coveragePath arrives as a tool argument, which is exactly the input this must not be quadratic in. Co-Authored-By: Claude Opus 5 --- src/runtime/contracts.test.ts | 13 +++++++++++++ src/runtime/contracts.ts | 21 +++++++++++++++++++-- 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/src/runtime/contracts.test.ts b/src/runtime/contracts.test.ts index b12a25d..de0c838 100644 --- a/src/runtime/contracts.test.ts +++ b/src/runtime/contracts.test.ts @@ -200,6 +200,19 @@ describe("coverage map paging", () => { expect(view.page.selected).toBe("src"); }); + it("resolves a path buried in trailing separators", () => { + // The separators are stripped by scanning, not by /[/\\]+$/ — the pattern + // backtracks quadratically on exactly this shape, and the path comes in as a + // tool argument. + const entries = [entry("src", 3)]; + const view = formatCoverageMapPage({ + entries, + root, + selectPath: `src${"/".repeat(2000)}`, + }); + expect(view.page.selected).toBe("src"); + }); + it("falls back to the first page when the path matches nothing", () => { const view = formatCoverageMapPage({ entries: many(39), diff --git a/src/runtime/contracts.ts b/src/runtime/contracts.ts index 702f8dd..1c3780b 100644 --- a/src/runtime/contracts.ts +++ b/src/runtime/contracts.ts @@ -600,6 +600,23 @@ function coverageRule(entry: DirectoryCoverageEntry): string { return "→ default: create_new (prove no_update if change is fully self-contained)"; } +/** + * Drop trailing path separators, by scanning rather than by pattern. + * + * `/[/\\]+$/` says the same thing and is the obvious way to write it, but a + * repeated character class anchored at the end has to backtrack from every + * position when the match fails, so a path that is mostly separators costs + * quadratic time. `coveragePath` arrives from a tool argument, which is exactly + * the input this must not be quadratic in. + */ +function stripTrailingSeparators(value: string): string { + let end = value.length; + while (end > 0 && (value[end - 1] === "/" || value[end - 1] === "\\")) { + end -= 1; + } + return value.slice(0, end); +} + /** * Find the entry a caller's `coveragePath` points at. A directory selects * itself; a file selects the directory holding it, because that is the unit the @@ -612,9 +629,9 @@ function selectCoverageEntry( path: string, ): DirectoryCoverageEntry | null { const absolute = isAbsolute(path) ? normalize(path) : join(root, path); - const trimmed = absolute.replace(/[/\\]+$/, ""); + const trimmed = stripTrailingSeparators(absolute); const byDir = entries.find( - (entry) => entry.dir.replace(/[/\\]+$/, "") === trimmed, + (entry) => stripTrailingSeparators(entry.dir) === trimmed, ); if (byDir) return byDir; return (