fix(session-flow): add compute-don't-assert rule to running-retro analysis prompts

[kyle-sexton]

Closes #1473

Summary

• running-retro's checkpoint analyzer had no rule requiring structural transcript claims (tool-call sequencing/batching/delegation, "emerging pattern" occurrence counts) to be computed rather than asserted from a narrative read — an independent fresh-context validation found the analyzer accurate on findings that harvest the session's own self-declared observations, but 0/2 on independently inferred structural claims. One asserted-and-wrong claim routed as a tracker issue a human would have filed for a non-problem.
• Adds a compute-don't-assert rule to both callers of the checkpoint method: checkpoint.md's Method section (new step 4) and its delegation prompt template's "Do:" summary, plus observer.py's _analysis_prompt (the detached observer has no delegation prompt to fall back on, so the rule is stated inline there too).
• A structural claim that can't be computed from the record must now be dropped rather than asserted uncomputed.
• Post-review hardening (two independent bot reviews — claude[bot] and Codex — found three real gaps across four follow-up commits, all fixed in this PR):
  • The headless prompt's "group by API message id" guidance now notes explicitly that the distilled observations may not carry that field at all (summarize_record() never preserves one) — an absent field is uncomputable, not license to assert from impression.
  • A correctly computed sequencing fact is not by itself proof of a missed batching opportunity (the calls may be genuinely dependent) — both prompts now require checking for a dependency before routing a computed sequential/unbatched claim as an Efficiency finding.
  • That dependency check was itself narrowed to data flow only (a later call's input consuming an earlier call's result) — broadened to also cover control, resource, and side-effect dependencies (e.g. a directory created before a file is written into it), which are just as real a reason two calls had to run in order.
• Scoped to recommendation 1 only of session-flow/running-retro analyzer: stenographer bias — accurate on self-declared findings, 0/2 on independent inference; structural claims asserted not computed #1473, per the triage comment. Recommendation 2 (raise the default analysis model tier) was split out as session-flow/running-retro: raise default analysis model tier above cheap/fast class (cost/quality decision) #1480; recommendation 3 (disclose partial-transcript coverage in the ledger header) is tracked under session-flow/running-retro observer: mtime-idle end detection fires mid-session (default 900s), analyzes a partial transcript, never resumes #1471 — neither is touched here. A deeper gap the reviews surfaced (the headless observer's distilled observations don't carry enough structure to ever compute a sequencing/batching/dependency finding, only ever drop it — covering both the message-id and the tool-inputs/results limitations of summarize_record()) is a schema change beyond this PR's scope — filed as session-flow/running-retro: detached observer's distilled observations never preserve enough structure to compute sequencing/batching/delegation/dependency claims #1485.
• session-flow version bumped 0.17.3 → 0.17.4 with a matching CHANGELOG entry.

Test plan

• Added an AnalysisPrompt test class to test_observer.py asserting the compute-don't-assert rule text (named failure modes: sequencing, batching, delegation, occurrence count, message-id grouping, drop-if-uncomputable), the message-id-absence caveat, and the dependency-check-before-batching-finding requirement (including the data/control/resource/side-effect breadth) are all present in _analysis_prompt's output — plus a regression guard that the pre-existing mandatory redaction instruction wasn't crowded out.
• Ran the full test_observer.py suite locally after every commit: 27/27 passing.
• markdownlint-cli2 against the edited checkpoint.md: 0 issues (checked after every markdown edit).
• Swept plugins/session-flow/ for other references to checkpoint.md's internal Method step numbers (renumbered 4-7 → 5-8 by the insertion) — none found; SKILL.md's "step N" references are its own separate flow numbering, unaffected.
• CI green across all lanes on every commit, including review / review and security-review / security-review; every bot-review finding (3 from Codex, 1 from claude[bot] not already covered by the fixed ones) verified against actual code, classified, reacted to, replied to, and — for inline threads — resolved. One VALID-but-deferred finding routed to a follow-up issue (session-flow/running-retro: detached observer's distilled observations never preserve enough structure to compute sequencing/batching/delegation/dependency claims #1485) rather than expanding this PR's scope.

Related

Refs #1480 — recommendation 2 (raise the default analysis model tier), split out as a separate cost/quality decision, not touched here.
Refs #1471 — recommendation 3 (disclose partial-transcript ledger coverage truncation), tracked there rather than duplicated here.
Refs #1485 — follow-up filed from this PR's review, widened to cover both bot findings: preserve enough structure in the observer's distilled observations (a grouping key, plus tool-call inputs/result content) to compute — not just drop — sequencing/batching/delegation/dependency findings on the headless path.

---

This was generated by AI during work-loop execution.

🤖 Generated with Claude Code

[claude]

Claude finished @kyle-sexton's task in 3m 3s —— View job

---

PR Review

• Load tools and gather context
• Read changed files
• Review for correctness, security, and CLAUDE.md alignment
• Post review findings

---

Summary: Clean, well-scoped fix. The change correctly addresses recommendation 1 of #1473 with no scope creep, and the fallback behavior is defensively correct. One consistency gap worth noting — not a blocker.

---

Findings

Minor: message-ID grouping instruction is unachievable in the headless path

observer.py line 644–645:

Derive an ordering or batching claim (e.g. "ran sequentially," "no subagent \
delegation") by grouping tool-use events by API message id and inspecting the grouping. \

The summarize_record distillation function (lines 51–93) does not preserve the message id field — it extracts type, timestamp, tools, stop_reason, etc., but not the message identifier. So the headless analysis agent is instructed to group by a field that isn't present in the observations it receives.

The fallback clause (drop the claim rather than assert it uncomputed) is the correct behavior here, and it will fire correctly. But the specific guidance to group by API message id is only reachable in the in-session checkpoint path where the delegation subagent reads the full JSONL transcript. For the headless path, the model will look for a non-existent field and must infer to drop the claim rather than being told upfront that the field may be absent.

A clarifying note would make this explicit rather than implicit:

# Suggested addition after the message-id sentence:
Derive an ordering or batching claim ... by grouping tool-use events by API message id
and inspecting the grouping. Note: distilled observations may not carry message ids; \
if the field is absent, treat that claim as uncomputable and drop it.

Fix this →

---

Everything else looks correct

checkpoint.md — The new step 4 is well-articulated. It names the specific failure modes, gives concrete guidance (group by message id, count actual occurrences), states the consequence (drop rather than assert), and explains WHY (routes as if verified). The renumbering of steps 4-7 → 5-8 is correct. The PR description confirms a sweep for references to internal step numbers — no cross-references were found.

Delegation prompt template — The Do: summary correctly carries the compute-don't-assert rule into the operative text handed to the subagent. The rule's presence there is what matters for the in-session path, since the subagent receives a concrete filled-in prompt, not the Method section.

observer.py prompt structure — The rule appears twice: once as a dedicated block (detailed, with the message-ID grouping guidance) and once abbreviated in the Do: summary. This is intentional redundancy for emphasis — a valid prompt engineering technique. The Do: summary's phrasing (dropping it if it can't be computed from the observations) is actually cleaner than the main block for the headless case.

test_observer.py — The AnalysisPrompt class is well-structured:

• The class docstring explains why these tests exist (headless path has no delegation prompt to fall back on), which is important for prompt-engineering tests where the "what" is obvious
• Covers all named failure modes from the validation report
• The regression guard (test_redaction_rule_still_present) is a good addition — guarding against a new instruction crowding out a pre-existing mandatory one is exactly the right instinct
• All assertions match the actual prompt text: "MANDATORY redaction pass" matches line 635's uppercase form, "drop the claim" matches line 648, "occurrence count" matches line 642

CHANGELOG — Accurate, appropriately detailed, correctly references #1473. The description of the root cause ("0/2 on independently inferred structural claims — one routed as a tracker issue a human would have filed for a non-problem") gives useful context for readers evaluating whether the fix is worth the diff.

Version bump — 0.17.3 → 0.17.4 is correct for a bug fix. Both plugin.json and CHANGELOG.md are consistent.

CLAUDE.md compliance — No hardcoded paths, no PII/secrets, plugin-form-safe, versioned correctly.

---

The single finding is a clarity/guidance issue rather than a correctness bug — the fallback clause ensures the headless agent drops uncomputable claims even without the clarification. Ready to merge once the note is considered (or accepted as-is given the defensive fallback already covers it).

[claude]

Claude finished @kyle-sexton's task in 1m 26s —— View job

---

Security Review — PR #1482

Status: Complete

Todo:

• Load tools
• Read PR diff
• Review changed files for security issues
• Post findings

---

Findings

No security issues found in the changes introduced by this PR.

The five changed files consist of:

• plugin.json — version bump only
• CHANGELOG.md — documentation only
• checkpoint.md — static instruction prose added to a markdown document; no executable surface
• test_observer.py — test assertions over a string; no new attack surface
• observer.py — the only file with a code surface worth scrutinizing

observer.py — analysis of the diff

The entire diff in observer.py is new static string literals inserted into the _analysis_prompt f-string body (lines 640–661). No new variables are interpolated; no new parameters are added to the function signature; no new code paths are introduced. The security properties of this function are therefore unchanged by this PR.

For completeness, I reviewed the pre-existing injection surfaces in the unchanged context:

Surface	Risk	Mitigated?
{session_id} interpolated into Claude prompt	Attacker-controlled --session-id arg could inject LLM instructions	Tool is local; caller already has code execution; no host-system escalation possible
{observations} / {checkpoint} (file paths) in prompt	Model directed to read unexpected paths	Mitigated: --add-dir restricts Read tool to work_dir + checkpoint_dir only
Transcript observation data reaching the model	Prompt injection via crafted transcript content	Mitigated: trust-boundary declaration (line 629–631) tells model observations are untrusted data, not instructions; --tools Read prevents any write/exec tools being driven by injected content
self.session_id written to ledger frontmatter (line 512)	YAML frontmatter corruption via newlines in session_id	Pre-existing; not introduced by this PR; low severity (local ledger file, no deserialization chain)

None of these are introduced by this PR, and all have reasonable mitigations for a local-execution tool.

---

Summary: The PR is a prompt-text-only change. It adds defensive instructions to an AI analysis prompt and corresponding tests. No new injection surfaces, no permission changes, no new dependencies, no workflow modifications. Clean from a security standpoint.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2f76525dc8

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Claude finished @kyle-sexton's task in 1m 45s —— View job

---

PR Review — follow-up commit b42283c

• Load tools
• Read new commit changes
• Review for correctness and alignment
• Post findings

---

Summary: The follow-up commit cleanly addresses the finding flagged in the previous review pass (and echoed by the Codex review). No new issues found. Ready to merge.

---

Finding addressed: message-id absence caveat

observer.py lines 646–648:

Note: the distilled observations you have here may not carry a message-id field at all -- \
if it's absent, that claim is uncomputable from what you have, not a license to assert it \
from impression. \

This is exactly the right fix. The caveat is correctly scoped ("the distilled observations you have here") and prescribes the right behavior: absent field → uncomputable, not asserted. The phrasing "not a license to assert it from impression" directly closes the gap identified in the first review.

New test coverage

test_observer.py — test_message_id_absence_caveat_present:

Two assertions in one test:

1. Prompt contains "may not carry a message-id field" — matches the exact phrase at line 646.
2. summarize_record() output genuinely lacks an id key — which is the structural fact that motivated the prompt change. Testing the underlying fact rather than just the prose that describes it is the right level for a correctness guard.

I verified summarize_record() at lines 51–93: it builds {"t": ..., "ts": ...} and conditionally adds tools, say, stop_reason, tool_results, human, subtype, turn_boundary — never id. The test assertion self.assertNotIn("id", rec) is factually correct and will catch a future change to summarize_record that starts preserving message IDs (at which point the prompt caveat should be softened, not left as-is).

Everything else

CHANGELOG — The 0.17.4 entry was updated in this commit with a sentence describing the message-id clarification. Accurate.

No scope creep — The commit is tightly scoped to the one gap identified in review. No other prompt text was altered; the Do: summary and redaction rule are untouched.

All test assertions match actual prompt text — cross-checked against the _analysis_prompt f-string body:

Assertion	Matched text
"Compute, don't assert"	line 640
"sequencing"	line 642
"batching"	line 642
"delegation"	line 642
"occurrence count"	line 642
"message id"	line 645 "API message id"
"drop the claim"	line 650
"may not carry a message-id field"	line 646
"MANDATORY redaction pass"	line 635

All match. PR is correct and complete.

[claude]

Claude finished @kyle-sexton's task in 1m 24s —— View job

---

Security Review — PR #1482

Status: Complete

• Load tools
• Get PR diff (full, including new commit b42283c)
• Read changed files
• Conduct security analysis
• Post findings

---

No security issues found in the changes introduced by this PR.

The five changed files consist of:

• plugin.json — version bump only
• CHANGELOG.md — documentation only
• checkpoint.md — static instruction prose added to a markdown document; no executable surface
• test_observer.py — test assertions over an in-memory string return value; no new attack surface
• observer.py — the only file with a code surface worth scrutinizing

observer.py — analysis of the diff

The entire diff in observer.py (both commits 2f76525 and b42283c) is new static string literals inserted into the _analysis_prompt f-string body. No new variables are interpolated into the f-string: the only variable substitutions are {observations}, {checkpoint}, and {session_id}, all of which are unchanged from before this PR. No new code paths, function signatures, file-system operations, or subprocess invocations are introduced.

For completeness, I reviewed the pre-existing security surfaces in the unchanged context:

Surface	Risk	Mitigated?
{session_id} interpolated into Claude prompt	Attacker-controlled --session-id arg could inject LLM instructions	Tool is local; caller already has code execution; no host-system escalation possible
{observations} / {checkpoint} (file paths) in prompt	Model directed to read unexpected paths	Mitigated: --add-dir restricts Read tool to work_dir + checkpoint_dir only (lines 434–456)
Transcript observation data reaching the model	Prompt injection via crafted transcript content	Mitigated: trust-boundary declaration tells model observations are untrusted data, not instructions; --tools Read --strict-mcp-config prevents write/exec tools being driven by injected content
self.session_id written to ledger frontmatter (line 512)	YAML frontmatter corruption via newlines in session_id	Pre-existing; not introduced by this PR; low severity (local ledger file, no deserialization chain)

None of these are introduced by this PR, and all have reasonable mitigations for a local-execution tool.

Summary: The PR is a prompt-text-only change across commits 2f76525 and b42283c. It adds defensive instructions to an AI analysis prompt and corresponding tests. No new injection surfaces, no permission changes, no new dependencies, no workflow modifications. Clean from a security standpoint.

---

[kyle-sexton]

This was generated by AI during work-loop execution.

VALID (fixed now) — confirmed: summarize_record() (lines 51-93) does not preserve a message-id field in the distilled observations it produces; only type, timestamp, tools, say/human, stop_reason, etc. are extracted. Instructing the headless agent to group by a field that never exists in its own input was a real consistency gap.

Fixed in b42283c: added an explicit caveat immediately after the message-id sentence in _analysis_prompt — "the distilled observations you have here may not carry a message-id field at all -- if it's absent, that claim is uncomputable from what you have, not a license to assert it from impression." Also added test_message_id_absence_caveat_present, which asserts both the caveat text is present in the prompt and that summarize_record()'s actual output carries no id key (so the test would catch a future distillation change that added one back without updating the prompt, or vice versa).

CHANGELOG.md updated to describe the caveat alongside the original entry. All 26 tests pass locally.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b42283c082

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Claude finished @kyle-sexton's task in 2m 5s —— View job

---

PR Review — follow-up commit 8f29a28

• Load tools
• Read new commit changes
• Review for correctness and alignment
• Post findings

---

Summary: The commit cleanly addresses the Codex P2 finding: a correctly computed sequencing fact is not by itself proof of a missed batching opportunity — calls that ran in separate message-id groups can be genuinely data-dependent, making sequential execution correct. The fix extends the compute-don't-assert discipline to the judgment built on top of a structural fact. One observation follows (lower severity than the message-id finding from the previous round).

---

Observation: summarize_record() doesn't carry tool inputs or result content

observer.py lines 51–93 — the distillation function that feeds the headless path.

The new instruction tells the headless agent to "check the calls' tool inputs/results for that dependency" (line 655–656). But summarize_record() strips both:

• Tool call inputs — only the tool name is preserved (out["tools"] = [c.get("name") ...]), not its arguments
• Tool result content — only the count is preserved (out["tool_results"] = results), not the result values

So on the headless path the model is told to check data that was never written to the distilled observations file.

Why this is less severe than the message-id case: The dependency check instruction appears immediately after the drop the claim rather than assert it uncomputed clause, and that clause's scope is broad ("if the observations don't carry what's needed to compute a structural claim"). A model reading the block as a unit should correctly infer that absent inputs/results → dependency check uncomputable → drop the Efficiency finding. This is the conservative/correct outcome.

Why it's still worth a caveat: Without an explicit note, a model could try to infer a dependency from tool names alone (e.g., "WriteFile presumably uses what ReadFile returned") — which is exactly the assertion-from-impression failure mode the rule exists to prevent. The message-id caveat precedent suggests adding one here too:

# Suggested addition after the dependency-check sentence:
# "Note: distilled observations carry tool names and result counts but not
#  tool call inputs or result content — if those are absent, the dependency
#  check is uncomputable from what you have; drop the Efficiency finding
#  rather than infer dependency from tool names."

This is a suggestion rather than a blocker — the defensive fallback covers the correctness gap, and the commit is a net improvement.

---

Everything else looks correct

Scope — The change covers all four prompt surfaces consistently: checkpoint.md Method step 4, checkpoint.md delegation Do: summary, observer.py main block, observer.py Do: summary. No surface is left inconsistent.

Logic — "Computed sequencing fact ≠ missed batching opportunity when calls are data-dependent" is sound and correctly scoped. The instruction is surgical: it only gates the Efficiency routing decision, not the structural-fact computation itself.

Test assertions — Both assertions in test_dependency_check_before_batching_finding_present match actual prompt text:

• "missed batching opportunity" → line 652: "not by itself proof of a missed batching opportunity"
• "dependency" → line 655: "check the calls' tool inputs/results for that dependency"

The existing regression guards (test_redaction_rule_still_present, test_message_id_absence_caveat_present) are untouched and still valid.

CHANGELOG — Accurate. The update to the 0.17.4 entry correctly describes the extension: the compute-don't-assert discipline now covers the Efficiency judgment built on a structural fact, not only the fact itself.

No scope creep — Tightly bounded to the Codex P2 finding on checkpoint.md:32. The prior fixes (2f76525, b42283c) are untouched.

---

The observation above is a consistency / clarity note. The fallback clause already yields the correct outcome for the headless path (drop uncomputable Efficiency findings). Ready to merge as-is, or with a follow-up caveat in a subsequent commit if the pattern from b42283c should be applied uniformly.

[claude]

Claude finished @kyle-sexton's task in 1m 27s —— View job

---

Security Review — PR #1482

• Load tools and gather context
• Get PR diff (all 3 commits: 2f76525, b42283c, 8f29a28)
• Read changed files
• Conduct security analysis
• Post findings

---

No security issues found in the changes introduced by this PR.

Changed files reviewed

File	Nature
plugin.json	Version bump only (0.17.3 → 0.17.4)
CHANGELOG.md	Documentation only
checkpoint.md	Static instruction prose; no executable surface
test_observer.py	New AnalysisPrompt test class; no new attack surface
observer.py	Only file with a code surface worth scrutinizing

observer.py — analysis of the full diff

All three commits' changes to observer.py are new static string literals inserted into the _analysis_prompt f-string body (lines 640–669). No new variables are interpolated: the only variable substitutions remain {observations}, {checkpoint}, and {session_id}, all of which are unchanged from before this PR. No new code paths, function signatures, file-system operations, or subprocess invocations were introduced.

For completeness, I reviewed the pre-existing injection surfaces in the unchanged context:

Surface	Risk	Mitigated?
{session_id} interpolated into Claude prompt (line 661)	Attacker-controlled --session-id arg could inject LLM instructions	Tool is local; caller already has code execution — no host-system escalation possible
{observations} path in prompt (line 659)	Model directed to read at attacker-controlled path	Mitigated: --add-dir restricts Read tool to work_dir + checkpoint_dir only (lines 434–456)
Transcript observation data reaching the model	Prompt injection via crafted transcript content	Mitigated: trust-boundary declaration (line 629–631) tells model observations are untrusted data; --tools Read --strict-mcp-config prevents write/exec tools being driven by injected content
self.session_id written to YAML frontmatter (line 512)	YAML frontmatter corruption via newlines in session_id	Pre-existing; not introduced or worsened by this PR; low severity (local ledger file, no deserialization chain)

None of these are introduced by this PR, and none are worsened by it.

test_observer.py

The new AnalysisPrompt test class calls _analysis_prompt() with hardcoded safe values (/abs/obs.jsonl, /abs/checkpoint.md, sid) and asserts string containment. No new attack surface.

---

Summary: The PR is a prompt-text-only change across all three commits. It adds defensive instructions to the AI analysis prompt and corresponding tests. No new injection surfaces, no permission changes, no new dependencies, no workflow modifications. Clean from a security standpoint.

[claude]

Claude finished @kyle-sexton's task in 1m 24s —— View job

---

PR Review — commit 356ba72

• Load tools
• Read PR diff and changed files
• Review latest commit (356ba72) and full PR state
• Post findings

---

Summary: The final commit is a CI compliance cleanup — no logic changes, only test docstring improvements. The full four-commit PR is correct and ready to merge.

---

Commit 356ba72 — drop PR-number references from test comments

test_observer.py lines 110–135

Both rewrites remove the PR #1482 tracker citation and replace it with a self-contained reason that doesn't depend on the PR being findable post-merge. The revised docstrings are actually more informative than the originals:

• test_message_id_absence_caveat_present: Was "(PR #1482 review)" — now just ".". Clean.
• test_dependency_check_before_batching_finding_present: Was "Codex's PR #1482 review found the compute-don't-assert rule covered the structural fact but not the Efficiency judgment built on top of it." — now "The compute-don't-assert rule must cover the Efficiency judgment built on top of a computed structural fact, not only the fact itself." This rewrite is strictly better: it states the invariant the test enforces rather than the historical event that prompted writing the test. A future reader learns what the rule is, not just how it was discovered.

No logic, assertions, or test coverage changed.

---

Full PR state

All four commits together correctly implement recommendation 1 of #1473. The remaining observation from the previous review pass (tool inputs/results not preserved in summarize_record(), so the dependency-check instruction is uncomputable on the headless path) has the correct fallback: the drop-if-uncomputable clause fires and the headless agent drops the Efficiency finding rather than asserting it. The gap is a clarity issue, not a correctness one, and the previous review noted it was filed as a separate follow-up. Nothing new to add.

Files verified at HEAD (356ba72):

File	State
plugin.json	Version bump 0.17.3 → 0.17.4 ✓
CHANGELOG.md	Accurate entry covering all four commits ✓
checkpoint.md	Step 4 well-articulated; renumbering 4-7 → 5-8 correct ✓
observer.py	Rule present, message-id caveat present, dependency-check present ✓
test_observer.py	4 test methods, all assertions match actual prompt text ✓

Ready to merge.

[claude]

Claude finished @kyle-sexton's task in 1m 30s —— View job

---

Security Review — PR #1482

• Load tools
• Get PR diff (all 4 commits: 2f76525, b42283c, 8f29a28, 356ba72)
• Read changed files
• Conduct security analysis
• Post findings

---

No security issues found in the changes introduced by this PR.

Changed files reviewed

File	Nature
plugin.json	Version bump only (0.17.3 → 0.17.4)
CHANGELOG.md	Documentation only
checkpoint.md	Static instruction prose; no executable surface
test_observer.py	New AnalysisPrompt test class; no new attack surface
observer.py	Only file with a code surface worth scrutinizing

observer.py — analysis of the full diff

All four commits' changes to observer.py are new static string literals inserted into the _analysis_prompt f-string body (lines 640–669). No new variables are interpolated into the f-string: the only variable substitutions remain {observations}, {checkpoint}, and {session_id}, all of which are unchanged from before this PR. No new code paths, function signatures, file-system operations, or subprocess invocations were introduced.

The latest commit (356ba72) removed PR-number references from test code comments — purely cosmetic, zero security impact.

For completeness, I reviewed the pre-existing injection surfaces in the unchanged context:

Surface	Risk	Mitigated?
{session_id} interpolated into Claude prompt (line 660)	Attacker-controlled --session-id arg could inject LLM instructions	Tool is local; caller already has code execution — no host-system escalation possible
{observations} path in prompt (line 659)	Model directed to read at attacker-controlled path	Mitigated: --add-dir restricts Read tool to work_dir + checkpoint_dir only (lines 434–456)
Subprocess call (line 467)	Shell injection	Mitigated: subprocess.run(cmd, ...) uses a list, never shell=True
Transcript observation data reaching the model	Prompt injection via crafted transcript content	Mitigated: trust-boundary declaration (lines 629–631) tells model observations are untrusted data, not instructions; --tools Read --strict-mcp-config prevents write/exec tools being driven by injected content
self.session_id written to YAML frontmatter (line 512)	YAML frontmatter corruption via newlines in session_id	Pre-existing; not introduced or worsened by this PR; low severity (local ledger file, no deserialization chain)

None of these are introduced by this PR, and none are worsened by it.

test_observer.py

The new AnalysisPrompt test class calls _analysis_prompt() with hardcoded safe values (/abs/obs.jsonl, /abs/checkpoint.md, sid) and asserts string containment. No new attack surface.

---

Summary: The PR is a prompt-text-only change across all four commits. It adds defensive instructions to the AI analysis prompt and corresponding tests. No new injection surfaces, no permission changes, no new dependencies, no workflow modifications. Clean from a security standpoint.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 356ba7221f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Claude finished @kyle-sexton's task in 1m 29s —— View job

---

PR Review — commit eda8a2c

• Load tools
• Read latest commit diff
• Review for correctness and alignment
• Post findings

---

Summary: The commit cleanly addresses the Codex P2 finding on commit 8f29a28: the dependency check was narrowed to data flow (result-to-input), but side-effect dependencies — a directory mkdir'd before a Write into it, an Edit before a test that exercises it — are just as real a reason two calls had to be sequential. The fix broadens all four prompt surfaces consistently. No new issues found.

---

Finding addressed: dependency check narrowed to data flow only

All four prompt surfaces are updated consistently:

Surface	Before	After
checkpoint.md Method step 4	"a later call's input consumes an earlier call's result"	"data, control, resource, or side-effect — dependency between them"
checkpoint.md delegation Do:	"check for a genuine data dependency"	"check for a genuine data, control, resource, or side-effect dependency"
observer.py main block	same narrow form	same broadened form
observer.py Do: summary	"checking for a data dependency"	"checking for a data, control, resource, or side-effect dependency"

The concrete examples added (a directory created before a file is written into it, an edit made before a test that exercises it runs) are well-chosen — they are the exact failure-mode pattern the Codex finding described and are distinct from result-to-input flow.

Test assertions

Three new assertions added to test_dependency_check_before_batching_finding_present:

Assertion	Matched text
"missed batching opportunity"	observer.py line 652 ✓
"dependency"	observer.py lines 658, 670 ✓
"control"	observer.py lines 655, 670 ✓
"resource"	observer.py lines 655, 670 ✓
"side-effect"	observer.py lines 655, 670 ✓

All match. The prior regression guards (test_redaction_rule_still_present, test_message_id_absence_caveat_present) are untouched and still valid.

Remaining structural limitation (unchanged from prior review)

The headless path still can't compute dependency checks of any kind — summarize_record() carries only tool names, not inputs, results, or side-effect information. This means the dependency check is uncomputable on the headless path for data, control, resource, and side-effect dependencies alike. The drop-if-uncomputable clause fires correctly in all cases (absent context → uncomputable → drop the Efficiency finding), so the behavior is conservative and correct. This gap was identified in the 8f29a28 review, already filed as #1485, and is not worsened by this commit.

Full PR state at HEAD (eda8a2c)

All five commits together correctly implement recommendation 1 of #1473. The PR is correct and complete.

File	State
plugin.json	Version bump 0.17.3 → 0.17.4 ✓
CHANGELOG.md	Accurate entry covering all five commits ✓
checkpoint.md	Step 4 well-articulated; delegation Do: consistent ✓
observer.py	Rule, message-id caveat, and dependency check all present and broadened ✓
test_observer.py	All assertions match actual prompt text ✓

Ready to merge.

[claude]

Claude finished @kyle-sexton's task in 1m 31s —— View job

---

Security Review — PR #1482

• Load tools and gather context
• Read PR diff
• Read changed files
• Conduct security analysis
• Post findings

---

No security issues found in the changes introduced by this PR.

Changed files reviewed

File	Nature
plugin.json	Version bump only (0.17.3 → 0.17.4)
CHANGELOG.md	Documentation only
checkpoint.md	Static instruction prose added to a Markdown document; no executable surface
test_observer.py	New AnalysisPrompt test class asserting string containment; no new attack surface
observer.py	Only file with a code surface worth scrutinizing

observer.py — analysis of the full diff

All five commits' changes to observer.py are new static string literals inserted into the _analysis_prompt f-string body (lines 640–672). No new variables are interpolated into the f-string: the only variable substitutions remain {observations}, {checkpoint}, and {session_id} — all unchanged from before this PR. No new code paths, function signatures, file-system operations, subprocess invocations, or permission changes were introduced.

For completeness, I reviewed the pre-existing injection surfaces in the unchanged context:

Surface	Risk	Mitigated?
{session_id} interpolated into Claude prompt (line 663)	Attacker-controlled --session-id arg could inject LLM instructions	Tool is local; caller already has code execution — no host-system escalation possible
{observations} / {checkpoint} paths in prompt (lines 662, 666)	Model directed to read at attacker-controlled path	Mitigated: --add-dir restricts Read tool to work_dir + checkpoint_dir only (lines 434–456)
subprocess.run(cmd, ...) (line 467)	Shell injection	Mitigated: uses a list, never shell=True; prompt goes via input= to stdin, not into cmd
Transcript observation data reaching the model	Prompt injection via crafted transcript content	Mitigated: trust-boundary declaration (lines 629–631) tells model observations are untrusted data; --tools Read --strict-mcp-config prevents write/exec tools being driven by injected content
self.session_id written to YAML frontmatter (line 512)	YAML frontmatter corruption via embedded newlines in session_id	Pre-existing; not introduced or worsened by this PR; low severity (local ledger file, no deserialization chain)

None of these are introduced by this PR, and none are worsened by it.

test_observer.py

The new AnalysisPrompt test class calls _analysis_prompt() with hardcoded safe values (/abs/obs.jsonl, /abs/checkpoint.md, sid) and asserts string containment against the return value. No new attack surface.

---

Summary: The PR is a prompt-text-only change across all five commits. It adds defensive instructions to the AI analysis prompt and corresponding tests. No new injection surfaces, no permission changes, no new dependencies, no workflow modifications. Clean from a security standpoint.

[kyle-sexton]

This was generated by AI during work-loop execution.

VALID (deferred) — confirmed: summarize_record() preserves only tool names (out["tools"] = [c.get("name") ...]) and result counts (out["tool_results"] = results), never the actual input arguments or result content. So the dependency check the previous commit added is, like the message-id grouping, uncomputable on the headless path today.

Same interim mitigation as the message-id gap: the drop-if-uncomputable clause is broad enough ("if the observations don't carry what's needed to compute a structural claim, drop the claim") that it already covers this — a model reading the block as a unit should drop the Efficiency finding rather than infer dependency from tool names alone, which is the correct/conservative outcome.

Rather than leave it implicit, I widened the follow-up already filed for the message-id gap (#1485) to also cover this — same underlying schema limitation (summarize_record()'s distillation), same class of fix (preserve more structure in the distilled observations so the headless path can compute these findings instead of always dropping them). Added a matching acceptance-criteria bullet there rather than filing a second issue for what's really one root cause.

[claude]

Claude finished @kyle-sexton's task in 1m 38s —— View job

---

Security Review — PR #1482

• Load tools and gather context
• Get PR diff
• Read changed files for security analysis
• Post findings

---

No security issues found in the changes introduced by this PR.

Changed files reviewed

File	Nature
plugin.json	Version bump only (0.17.4 → 0.17.5)
CHANGELOG.md	Documentation only
checkpoint.md	Static instruction prose; no executable surface
test_observer.py	New AnalysisPrompt test class; no new attack surface
observer.py	Only file with a code surface worth scrutinizing

observer.py — analysis of the diff

All changes to observer.py in this PR are new static string literals inserted into the _analysis_prompt f-string body (lines 662–681) and an update to the Do: paragraph (lines 690–694). No new variables are interpolated into the f-string: the only variable substitutions remain {observations}, {checkpoint}, and {session_id} — all unchanged from before this PR. No new code paths, function signatures, file-system operations, subprocess invocations, or permission changes were introduced.

For completeness, I reviewed the pre-existing injection surfaces in the unchanged context:

Surface	Risk	Mitigated?
{session_id} interpolated into Claude prompt (line 685)	Attacker-controlled --session-id arg could inject LLM instructions	Tool is local; caller already has code execution — no host-system escalation possible
{observations} / {checkpoint} paths in prompt (lines 684, 688)	Model directed to read at attacker-controlled path	Mitigated: --add-dir restricts Read tool to work_dir + checkpoint_dir only (lines 434–456)
subprocess.run(cmd, ...) (line 488)	Shell injection	Mitigated: uses a list, never shell=True; prompt goes via input= to stdin, not into cmd
Transcript observation data reaching the model	Prompt injection via crafted transcript content	Mitigated: trust-boundary declaration (lines 651–653) tells model observations are untrusted data, not instructions; --tools Read --strict-mcp-config (lines 452–454) prevents write/exec tools being driven by injected content
self.session_id written to YAML frontmatter (line 534)	YAML frontmatter corruption via embedded newlines in session_id	Pre-existing; not introduced or worsened by this PR; low severity (local ledger file, no deserialization chain)

None of these are introduced by this PR, and none are worsened by it.

test_observer.py

The new AnalysisPrompt test class calls _analysis_prompt() with hardcoded safe values (/abs/obs.jsonl, /abs/checkpoint.md, sid) and asserts string containment against the return value. No new attack surface.

---

Summary: The PR is a prompt-text-only change. It adds defensive instructions to an AI analysis prompt and corresponding tests. No new injection surfaces, no permission changes, no new dependencies, no workflow modifications. Clean from a security standpoint.

[claude]

Claude finished @kyle-sexton's task in 2m 16s —— View job

---

PR Review — merge commit 5a144ee

• Load tools and gather context
• Read diff and changed files
• Review for correctness, security, and CLAUDE.md alignment
• Post findings

---

Summary: The merge commit correctly resolved a version conflict: origin/main already carried 0.17.4 (the console-window/mojibake fix, PR #1481), so this PR's changes were rebumped to 0.17.5. All prior review findings from previous rounds remain addressed. One minor cosmetic observation follows.

---

Version bump: 0.17.4 → 0.17.5 (post-merge correct)

The PR description says "0.17.3 → 0.17.4" but the merge resolved a conflict: main's 0.17.4 entry (from a sibling fix) landed before this PR merged, so the version was correctly advanced to 0.17.5. Both plugin.json and CHANGELOG.md at HEAD are consistent at 0.17.5. The CHANGELOG entry under ## [0.17.5] accurately describes all five commits' changes. No issue — just noting the PR description is now stale.

---

Minor: soft line-break inside delegation prompt Do: instruction

checkpoint.md lines 145–147:

... classify each finding
by category and
suggested resolution route; ...

This sits inside a fenced code block (the delegation prompt template), so line breaks are literal when the main agent pastes the filled-in template to the subagent. The original line read classify each finding by category and suggested resolution route; without a break; the new text around it pushed and to a dangling end-of-line. Models handle soft wraps well, but the split "and\nsuggested resolution route" is grammatically odd. Not a correctness bug — the subagent will read it correctly — but worth a one-word fix to avoid any ambiguity.

Fix this →

---

Everything else correct at HEAD

plugin.json — Version 0.17.5; consistent with CHANGELOG. ✓

CHANGELOG.md — ## [0.17.5] entry accurate: covers the compute-don't-assert rule, message-id absence caveat, dependency-check requirement, and the data/control/resource/side-effect breadth. The existing ## [0.17.4] and ## [0.17.3] entries are untouched. ✓

checkpoint.md Method step 4 — Well-articulated: names the failure modes, prescribes computation by message-id grouping, requires an occurrence count, mandates drop-not-assert for uncomputable claims, clarifies that computed sequentiality ≠ batching miss, and broadens "dependent" to cover all four dependency types with concrete examples. Renumbering 4–7 → 5–8 is correct. ✓

checkpoint.md delegation Do: summary — Carries the compute-don't-assert rule and the dependency-check-before-Efficiency-routing requirement into the operative text the subagent receives. Consistent with the Method section (minus the minor line-break above). ✓

observer.py _analysis_prompt — Rule appears in both the main block (detailed, with message-id absence caveat, all four dependency types, concrete examples) and the Do: summary (compact restatement). All text verified against the function at HEAD (lines 644–694). ✓

test_observer.py AnalysisPrompt class — Four methods; all assertions confirmed to match actual prompt text at HEAD:

Assertion	Location in prompt
"Compute, don't assert"	line 662
"sequencing" / "batching" / "delegation" / "occurrence count"	lines 664–665
"message id"	line 667
"may not carry a message-id field"	line 668
"drop the claim"	line 672
"missed batching opportunity"	line 674
"dependency"	lines 676, 680
"control" / "resource" / "side-effect"	lines 677, 680
"MANDATORY redaction pass"	line 657

The test_message_id_absence_caveat_present structural assertion (assertNotIn("id", rec) against summarize_record() output) remains accurate — summarize_record() at lines 51–93 never emits an id key. ✓

CLAUDE.md compliance — No hardcoded paths, no PII/secrets, plugin-form-safe (${CLAUDE_PLUGIN_ROOT} conventions unaffected), versioned correctly. ✓

Security — No new interpolated variables in the _analysis_prompt f-string; only static string literals added. Pre-existing injection surfaces ({session_id}, {observations}, {checkpoint}) unchanged and mitigated (local execution, --add-dir restriction, trust-boundary declaration, --tools Read). ✓

---

The single observation (line-break in the Do: template) is cosmetic and non-blocking. Ready to merge.