[WILL NOT MERGE] a mix of 5 PRs merged together for demo purposes - #2504
Closed
ilblackdragon wants to merge 51 commits into
Closed
ilblackdragon wants to merge 51 commits into
ilblackdragon wants to merge 51 commits into
Conversation
The github_dev_workflow live test records HTTP exchanges including the github_token Bearer header. GitHub push protection correctly blocks this. The fixture is only useful locally for replay; the test skips gracefully without it.
Adds tests/e2e_github_dev_workflow.rs — a multi-turn live/replay test that
drives the developer-assistant + github-workflow skills end-to-end against
a synthetic nearai/ironclaw repository:
1. Setup — installs the wf-* mission set (excluding
wf-staging-review per the implement-but-don't-
auto-merge autonomy contract)
2. Issue opened — synthetic github.issue.opened webhook payload
3. Maintainer LGTM — pr.comment.created from a maintainer
4. PR review — non-maintainer review comment
5. CI failure — failing check_run
6. Approval — maintainer approval; asserts NO merge call ever
fires across the whole session
7. Digest — status report referencing the issue/PR
Webhook payloads are injected via TestRig::send_message with a
[GITHUB WEBHOOK] frame that matches what a real webhook→channel
adapter would emit. The mission OnSystemEvent firing path is covered
separately by mission.rs unit tests; this test exercises skill
behavior given the right inputs.
Adds two helpers to tests/support/live_harness.rs:
- trace_contains_tool_call(name, needle)
- assert_trace_contains_tool_call(name, needle, ctx)
Both scan ToolStarted.detail and ToolResult.preview for case-insensitive
substring matches, so behavior tests can assert *what the agent
actually called* without scraping the recorded trace JSON.
Drive-by cleanups from the extension-lifecycle merge:
- thread_ops.rs: drop orphaned RecordingStatusChannel + helper that
came from a dropped extension-lifecycle test variant
- bridge/router.rs: clippy needless_borrow on PendingGate args
- skills/mod.rs: SkillManifest no longer has metadata field; add
requires: GatingRequirements::default() to test fixture
- cargo fmt fallout in recording.rs / live_mission.rs / trace_llm.rs
The test is #[ignore]-tagged (live tier) and skips gracefully in replay
mode until tests/fixtures/llm_traces/live/github_dev_workflow_full_loop.json
is recorded with IRONCLAW_LIVE_TEST=1. Compile coverage is automatic
via the existing test matrix; live execution follows the same pattern
as e2e_live_personas.rs (manual recording + commit fixture).
cargo check --features libsql --tests: clean
cargo clippy --features libsql --tests --test e2e_github_dev_workflow -- -D warnings: clean
cargo test --features libsql --test e2e_github_dev_workflow -- --ignored: passes (skips, fixture missing)
Three additions to make the github_dev_workflow live test runnable: 1. **TestRigBuilder::with_secret(name, value)** — pre-seed credentials in the SecretsStore before the agent starts. The kernel pre-flight auth gate fires when a skill with a credential spec activates (e.g. the github skill needs github_token); without a stored credential the agent gets stuck in 'Authentication required' mode and can't make progress. Tests inject a fake/dummy value so the gate is satisfied — the test isn't actually hitting the credentialed API. Implementation: AppComponents.secrets_store is captured during build_all() and any pre-seeded (name, value) pairs are written via secrets_store.create() with user_id = config.owner_id. Already-exists errors are silenced so the helper is idempotent on seeded DBs. 2. **LiveTestHarnessBuilder::with_secret** — forwards to TestRigBuilder::with_secret. Plumbed through both build_live and build_replay so the same fixture works in both modes. 3. **dump_activity helper in e2e_github_dev_workflow.rs** — formats captured StatusUpdate stream (skill activations + every tool started/completed/result) to stderr. Used as a pre-assertion diagnostic so failing live runs surface the agent's actual tool sequence instead of an opaque panic on a workspace check. Test relaxations from running this against the real LLM: - verify_setup_landed accepts either developer-assistant OR github-workflow as the active skill (the deterministic selector picks based on keyword scoring + token budget; both routes are valid since github-workflow owns the mission templates) - final required-skills check drops developer-assistant in favor of github-workflow + github (the orchestrator persona is optional) - setup turn now pre-seeds github_token via with_secret cargo check --features libsql --tests: clean
Pivots the test from synthetic webhook simulation to a real end-to-end
integration test against the real nearai/ironclaw repo. Per project
owner: 'fully real live tests doing useful work on github repo... test
everything like it's live while recording all interactions to debug
what doesn't work and improve that'.
## Why the rewrite
The previous synthetic-event version injected fake GitHub payloads as
channel messages. With a real github_token in scope, the agent
attempted to fetch the fake issue 99001, got a 404, and helpfully
created 3 real issues + 3 real comments on nearai/ironclaw to
"reconcile" the discrepancy. The synthetic approach didn't surface
realistic failure modes anyway (auth gates, payload format mismatches,
rate limits), so we go all-in on real artifacts.
## New flow (2 turns + real artifact lifecycle)
1. Setup turn — agent installs the wf-* mission set for nearai/ironclaw
2. Test (NOT the agent) creates a real issue via direct REST API with
the title "[live-test {timestamp}] Add /metrics Prometheus endpoint"
and a real feature-request body.
3. Triage turn — test asks agent to triage issue #N. Agent reads via
github skill, generates a plan, posts a real comment back.
4. Verification — test polls api.github.com/issues/N/comments and
asserts at least one new comment exists since baseline. Comment
bodies are logged to stderr for human review (the most useful
debug output for iterating on skill quality).
5. Cleanup — std::panic::catch_unwind wraps the body so cleanup runs
regardless of pass/fail. Closes the issue with a final "live test
complete" comment. If cleanup itself fails, the issue URL is
printed for manual recovery.
## Test infrastructure additions
- TestRig.get_secret(name) — read decrypted secrets back from the
rig's SecretsStore. Required so the test can read the github_token
the harness pre-seeded via with_secrets(["github_token"]).
- TestRig captures secrets_store + owner_id from AppComponents during
build (needed for get_secret).
- github_api submodule inside the test file — direct REST helpers for
create_issue, list_issue_comments, post_issue_comment, close_issue.
Uses reqwest directly so the test has guaranteed GitHub access
regardless of skill selection / tool gating.
## Recording
- LLM trace fixture: tests/fixtures/llm_traces/live/github_dev_workflow_full_loop.json (65K)
- Session log: github_dev_workflow_full_loop.log (5.9K)
- Both committed so future runs can replay deterministically without
hitting real GitHub.
## What's NOT covered yet
Dropped from the previous version (can be added back as follow-ups):
- PR creation flow (agent opens a real PR with a real branch + real
code change)
- CI failure simulation (would need a real failing CI run)
- Mission OnSystemEvent firing via real webhooks (needs an HTTP
server registered as a GitHub webhook)
- Maintainer approval flow
This first version validates the most valuable slice: setup → react
to real issue → produce real comment → cleanup. If the agent's
comment quality is good, we expand from here.
cargo check --features libsql --tests: clean
cargo clippy --features libsql --tests --test e2e_github_dev_workflow -- -D warnings: clean
Live recording: passed in 85.9s
- Created issue #2185
- Agent posted 2 comments (full plan + follow-up)
- Closed issue #2185
… to *-setup
The persona orchestrator skills (developer-assistant, ceo-assistant,
trader-assistant, content-creator-assistant) are pure first-time
onboarding flows — their entire body is Steps 1-N of workspace setup,
mission registration, and calibration memory writes. After those steps
run successfully, there is nothing left for the skill to do, but the
deterministic selector kept evaluating them on every conversation
turn, burning ~3000 tokens of activation budget for work already
completed and risking partial re-runs of setup steps.
This commit makes setup skills opt-in to one-time activation:
## Mechanism: setup_marker exclusion
New optional field on ActivationCriteria:
activation:
setup_marker: commitments/.developer-setup-complete
Before scoring, the selector caller (Agent::select_active_skills)
collects every distinct setup_marker referenced by loaded skills,
checks the workspace for each via Workspace::exists(), and passes
the set of satisfied markers into prefilter_skills. Any skill whose
marker is in the satisfied set is excluded from scoring entirely
(returns None from the filter map, skipping the score_skill call).
The selector check is opt-in: skills without a setup_marker are
unaffected. Reactive operational skills (commitment-triage,
decision-capture, github, github-workflow, etc.) keep activating
on every matching message as before.
Tests:
- 4 unit tests in crates/ironclaw_skills/src/selector.rs covering
marker present/absent, marker mismatch, and skill-without-marker
unaffected paths
- All 152 ironclaw_skills tests pass
- Live e2e_github_dev_workflow run on real nearai/ironclaw passes
(issue #2186 created, comment posted, closed) in 88s
## Rename: *-assistant → *-setup
Per project owner: 'rename persona skills to -setup skills to make
it explicit they are called once'. The -assistant suffix obscured
the lifecycle — these are not always-on assistants, they are
one-time onboarding wizards.
Renamed directories (via git mv) and updated SKILL.md `name:`
fields:
- skills/ceo-assistant → skills/ceo-setup
- skills/content-creator-assistant → skills/content-creator-setup
- skills/developer-assistant → skills/developer-setup
- skills/trader-assistant → skills/trader-setup
All four now declare `setup_marker: commitments/.<name>-setup-complete`
and have a new final 'Step N: Mark setup complete' instructing the
agent to write the marker via memory_write after confirming setup
with the user. Different personas have different markers so they
remain independently triggerable in separate workspaces.
Cross-references updated:
- tests/e2e_live_personas.rs (4 persona test invocations)
- tests/e2e_github_dev_workflow.rs (doc comments)
- tests/e2e/LIVE_TOOL_FAILURES.md (1 reference)
- crates/ironclaw_skills/src/types.rs (doc comment example)
## Bump: SKILLS_MAX_CONTEXT_TOKENS default 4000 → 6000
The previous default was so tight that a setup skill (3000 tokens)
plus its companion github-workflow (2000) plus github (2000) would
overflow at 7000. Reactive operational skills like
commitment-triage, decision-capture, tech-debt-tracker often got
budget-evicted. With setup skills now excluded after onboarding,
the freed budget plus the bump to 6000 lets the most useful
combinations fit comfortably (e.g. github-workflow + github +
product-prioritization is now active in the live recording, where
previously product-prioritization would have been evicted).
## Plumbing changes
- ActivationCriteria gains pub setup_marker: Option<String>
(#[serde(default)], so existing skills are unaffected)
- prefilter_skills signature gains
&satisfied_setup_markers: &HashSet<String> (caller passes empty
set to disable filtering — used by all existing tests via the
prefilter_no_markers wrapper)
- Agent::select_active_skills is now async — it needs to
Workspace::exists() each marker. dispatcher.rs caller updated
to .await. Snapshots the skill list under the read lock then
drops the guard before any await to avoid holding a poisonable
RwLock across an await point.
cargo check --features libsql --tests: clean
cargo clippy --features libsql --tests --all-targets -- -D warnings: clean
cargo test -p ironclaw_skills: 152 passed
Live e2e_github_dev_workflow run: passes (88s)
…t-setup marker
Three orthogonal follow-ups to the skill lifecycle work.
## 1. Chain-loading via requires.skills (v1 Rust + v2 Python)
When a parent skill is selected by the scorer, its requires.skills
companions are now automatically loaded, bypassing the score filter.
Persona/bundle skills like developer-setup can finally work as
designed: the orchestrator declares which operational skills it
delegates to, and selecting the orchestrator pulls them all in.
- **v1 Rust** (crates/ironclaw_skills/src/selector.rs): extracted
skill_token_cost() and try_select() helpers used by both the
scored-selection loop and the new chain-loading pass. Companions
consume the same budget and respect max_candidates. Non-transitive
(depth 1 only) to keep behavior predictable.
- **v2 Python** (crates/ironclaw_engine/orchestrator/default.py):
select_skills() gains an inline chain-loading pass that mirrors
the Rust logic. Uses a name-indexed lookup built from the skill
list passed in by handle_list_skills. No closure-over-outer-var
tricks that Monty would reject — the inner try-add is inlined.
7 chain-load unit tests in selector.rs covering: pulls in
companions, skipped when parent not selected, respects budget,
skips companion with satisfied marker, non-transitive (depth 2
not pulled), missing companion silent, dedup across parents.
## 2. v2 setup_marker exclusion
The v2 engine's Python orchestrator handles skill selection via
handle_list_skills (Rust) -> select_skills (Python). Since
handle_list_skills already has the full project doc list in scope,
we filter there: any skill whose metadata.activation.setup_marker
is in the set of existing doc titles gets excluded before the
Python orchestrator ever sees it. Zero extra store calls — we
reuse the existing list_memory_docs_with_shared result to build
an O(1) title set.
This is the v2 parity of the v1 satisfied_setup_markers parameter
threaded through prefilter_skills. Both paths now implement the
same rule: a one-time setup skill whose marker file has been
written has finished its job and should not keep burning
activation budget.
## 3. commitment-setup gets a setup_marker
commitment-setup writes commitments/README.md as its first step,
so the marker is automatically set after a successful first run.
Added:
activation:
setup_marker: commitments/README.md
To re-trigger (e.g. migrate to a new schema), delete README.md
first. project-setup was NOT given a marker — it's per-repo,
invoked repeatedly, not a singleton (each call creates a new
projects/<owner>-<repo>/project.md).
## 4. Lifecycle integration test
tests/skill_setup_marker_lifecycle.rs drives a real agent turn
through the v1 selector pipeline (Agent::select_active_skills ->
Workspace::exists -> prefilter_skills) to verify that a setup
skill:
Phase 1: activates on the first matching message (marker absent)
Phase 2: marker file is written via workspace.write()
Phase 3: is excluded on the second matching message
The test asserts on the captured LLM system prompt content (via
rig.captured_llm_requests) rather than on StatusUpdate events so
it's agnostic to v1/v2 path differences in how skill activations
are announced. The skill's body contains a distinctive marker
string (LIFECYCLE-TEST-SKILL-BODY-MARKER-Z7Q) — if the skill was
selected, that string appears in the system prompt; if excluded,
it doesn't.
Cover matrix after this commit:
- v1 selector: 35 unit tests + 4 setup-marker tests + 7 chain-load tests
- v2 handle_list_skills marker exclusion: 1 integration test (lifecycle)
plus structural verification via cargo check (the filter uses the
existing list_memory_docs API, no new store calls to test)
- v2 Python select_skills chain-load: covered by the v1 unit tests
through shared semantic contract (both paths mirror the same
algorithm); a direct Python-level test would require spinning up
the Monty interpreter which is out of scope for this session.
Verification:
cargo test -p ironclaw_skills --lib: 159 passed
cargo test -p ironclaw_engine: 304 passed
cargo test --features libsql --test skill_setup_marker_lifecycle: 1 passed
cargo clippy --features libsql --tests --all-targets -- -D warnings: clean
V2SkillMetadata was missing the `requires` field entirely, so the
v1→v2 skill migration silently dropped `requires.skills` and the
chain-loading code I added to the v2 Python orchestrator in the
previous commit was effectively dead code — it always read an empty
companion list.
This was caught while writing an end-to-end chain-load test: the v1
test (through the Rust selector) passes, the v2 test (through the
Python orchestrator) was failing in a way that only made sense if
the companion metadata never reached Python. Inspection confirmed
`V2SkillMetadata` had no `requires` field, only `activation`.
## Fix
1. `V2SkillMetadata` gains `pub requires: GatingRequirements` with
`#[serde(default)]` for backwards compatibility (legacy
MemoryDocs in existing databases deserialize with an empty
`requires`).
2. `src/bridge/skill_migration.rs::v1_skill_to_memory_doc` now
copies `skill.manifest.requires.clone()` into the new field.
3. Four other explicit `V2SkillMetadata { ... }` literal
constructions updated with `requires: Default::default()`:
- `crates/ironclaw_engine/src/memory/skill_tracker.rs` (test helper)
- `crates/ironclaw_engine/src/runtime/mission.rs` (test helper)
- `crates/ironclaw_skills/src/v2.rs` (serde roundtrip test)
- `tests/engine_v2_skill_codeact.rs` (test fixture)
## New test: tests/skill_chain_load_lifecycle.rs
End-to-end lifecycle test for chain-loading. Writes three skills to
a tempdir:
- `parent-setup-test` — scored by a distinctive keyword, declares
two companions via `requires.skills`
- `companion-one-test` / `companion-two-test` — zero-scoring on
their own (keywords deliberately don't match)
Each skill body carries a distinctive marker string
(`CHAIN-LOAD-PARENT-BODY-J4V`, `CHAIN-LOAD-COMPANION-ONE-K5W`,
`CHAIN-LOAD-COMPANION-TWO-L6X`) that the test greps for in the
captured LLM system prompt via `rig.captured_llm_requests()`. If a
marker is present, the skill was injected into the prompt; if
absent, it wasn't.
Two test variants:
- **v1** (default rig, Rust selector path): **PASSES**. Proves the
chain-loading pass in `prefilter_skills` correctly pulls in both
companions despite their zero individual scores.
- **v2** (with_engine_v2, Python orchestrator path):
**`#[ignore]`d** with a detailed explanation. The v2 engine runs
a Python orchestrator that makes multiple LLM calls per user
message, but the default TestRig uses a single-turn TraceLlm that
exhausts after the first call — observing skill injection through
the v2 path needs a multi-turn TraceLlm harness or a dedicated v2
skill test rig. The structural wiring for v2 chain-loading
(V2SkillMetadata.requires + skill_migration copy + Python
select_skills chain-load pass) compiles and passes the 304-test
engine suite, so this is a test-harness gap, not a code gap.
When the multi-turn harness exists, flipping `#[ignore]` on the v2
test will exercise the full path.
Verification:
cargo test -p ironclaw_skills --lib: 159 passed
cargo test -p ironclaw_engine --lib: 304 passed
cargo test --features libsql --test skill_chain_load_lifecycle
-- --test-threads=1: 1 passed, 1 ignored
cargo test --features libsql --test skill_setup_marker_lifecycle
-- --test-threads=1: 1 passed
cargo clippy --features libsql --tests --all-targets -- -D warnings: clean
Also includes an updated fixture recording from the last live
`e2e_github_dev_workflow` run (issue #2204, agent posted 2 comments,
cleanup closed it). No functional difference; committed for
completeness since the fixture was modified on disk by the live run
and the test is hermetic in replay mode.
Use make_test_agent_with_status_channel instead of removed make_thread_ops_test_agent, StdMutex instead of TokioMutex, and fix String comparison direction. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…trator Addresses PR #2268 review feedback: the try_add closure was defined but never called since the logic was inlined for Monty compatibility. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Restore our branch's test helpers (SessionTurn, finish_turns_strict, with_skills_dir, loaded_skill_names, active_skill_names, etc.) that staging removed, while incorporating staging's new features (record_trace, with_no_trace_recording, secrets_store/owner_id accessors). Bridge the API gap with finish_turns_simple for tests using staging's (String, Vec<String>) tuple convention. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Portfolio WASM tool with full pipeline: - Indexer (fixture, dune, dune-replay backends) - Analyzer (6 protocol classifiers, health extraction, stablecoin detection) - Strategy filter (yield-floor, health-guard, LP impermanent-loss-watch) - Intent builder (fixture + solver backends, bounded checks, leg bundling) - Format (suggestion markdown, progress metric, widget state) 172 unit tests covering all modules including edge cases: - filter.rs: 33 tests (yield floor, health guard, LP watch, helpers) - bounded.rs: 16 tests (slippage, cost, chain allowlist, multi-leg) - parser.rs: 18 tests (delimiters, YAML, kind inference, real strategies) - fixture.rs: 14 tests (slippage calc, ID formats, payload structure) - analyzer: 18 tests (stablecoin detection, health extraction, debt/yield) - format.rs: 16 tests (totals, empty states, progress windowing) - widget.rs: 10 tests (rendering, intents, non-ready filtering) - types: 16 tests (parse_decimal, ChainSelector serde) - 14 YAML replay scenarios + 4 live Dune API tests (ignored by default) Share-gains feature: - Gateway-level IronClaw.api.share() modal with X, LinkedIn, Facebook, copy-to-clipboard, and download buttons - Portfolio widget generates SVG card showing gains (APY, annual savings, moves found) — no addresses or balances exposed - "Share gains" button appears only when portfolio has positive delta E2E Playwright tests (11 scenarios): - Skill discovery via API and settings UI - Chat integration (keyword + wallet address triggering) - Widget rendering with pre-seeded state (positions, totals, suggestions) - Share button visibility (present with gains, absent without) - Share modal lifecycle (opens with card image, social buttons, closes) Supporting changes: - E2E conftest: SKILLS_DIR points to workspace skills/ - Mock LLM: canned responses for portfolio/defi and wallet address patterns - Skill YAML, registry entry, capabilities JSON, 3 strategy docs, 4 scripts Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Addresses review comments from #2368: - XSS: widget renders all interpolated fields through escapeHtml(); share modal creates <img> via DOM API with data:image/ prefix check - OnceLock: protocol registry parsed once via std::sync::OnceLock - to_ascii_lowercase() for wallet address lookups (fixture + dune_replay) - bounded.rs: reject empty value_usd in single-leg slippage check - fixture.rs: compute min_out amount and value_usd separately - fixture.rs: clarify expires_at=0 comment (fixture = no expiry) - schema.json: add "dune-replay" to source enum - parser.rs: fix doc comment re kind inference (defaults, not inferred) - live_tests.rs: fix log placeholder (raw_count vs classified.len()) - intent.rs: expand kind comment to match SCHEMA.md Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Escape delta_vs_last_run_usd and next_mission_run in widget innerHTML - Add fixture test with amount != value_usd (stETH: 3.5 tokens / $12250) to verify the review fix separating amount from value_usd - Add empty-legs test for bundling.rs order_legs - Add comment explaining multi-leg empty value_usd tolerance in bounded.rs - WASM component builds successfully (754K release binary) via: cargo component build --release --target wasm32-wasip2 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Tighten share image validation to data:image/png only (was data:image/*) - Add ClipboardItem existence check to prevent runtime errors in some browsers - Fix SCHEMA.md to correctly attribute invariant enforcement (bounded.rs vs bundling.rs) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…new-project skill Adds project metrics types, mission cadence scheduling via gateway, and a /new-project skill for creating autonomous projects with goals, metrics, and missions. Includes gateway frontend enhancements for project views with metrics and goal tracking. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… new-project skill
Two fixes from trace analysis (trace_20260411T133641.json):
1. Skill rewrite: new-project skill now instructs the model to use
memory_write + mission_create directly instead of referencing
nonexistent project_create/project_update tools. Includes goals
and metrics when appropriate. Instructs sequential execution.
2. Template ref resolution: some OpenAI-format models (e.g. Qwen)
emit {{call_id.field}} references in parallel tool call arguments.
Added resolution pass in LlmBridgeAdapter that scans ActionCall
parameters for these patterns and resolves them from prior tool
results in the conversation history.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Playwright test that seeds mock project data via page.route() API interception, navigates to the Projects tab, drills into a project, and captures a screenshot showing goals, missions, and activity. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…s, add tests - Remove project_create/project_update/project_list tools and capability registration (skill uses memory_write + mission_create only) - Add ownership check on mission_create project_id override to prevent IDOR - Reject non-UUID project_id values explicitly instead of silent fallback - Add goals field to ProjectOverviewEntry so frontend drill-in renders them - Propagate store errors in overview instead of unwrap_or_default masking failures - Scope project widget CSS server-side via scope_css (prevents style leakage) - Fix template ref doc comment to match partial resolution semantics - Fix E2E mock widget response shape (bare array, not wrapped object) - Call crBackToOverview() on tab switch to tear down project widgets - Add caller-level test for template ref resolution through LlmBridgeAdapter - Clean up stale cargo-deny advisory ignores, add RUSTSEC-2026-0097 (rand) - Run cargo fmt Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
LLMs kept inventing a `search_issues` action and looping over
`/repos/{owner}/{repo}/pulls` for "my PRs" queries. Clarify the
GitHub tool surface in three places:
- `tools-src/github/src/lib.rs` and `registry/tools/github.json`:
enumerate the three real search actions and call out that
`search_issues_pull_requests` covers both. Add the canonical
`is:pr author:@me sort:updated-desc` recipe for cross-repo "my PRs".
- `skills/github/SKILL.md`: add an "Authenticated User & Cross-Repo
Queries" section with copy-paste recipes for `@me`, the search
endpoints with proper URL encoding, and the response-envelope
contract (`body` is parsed JSON for application/json, raw `str` for
diff endpoints — never call `json.loads()` on it, never write
`.get("body", body)` as a fallback).
Rewrite the code-review skill from a 6-bullet checklist into a paranoid-architect workflow that handles both local diffs and GitHub PRs end-to-end: - Two input shapes: local `git diff` or `owner/repo N` / `github.com/.../pull/N` URLs. - Step 1 wraps GitHub fetches in `async def` + `FINAL(await ...)` to avoid the closure-capture quirk that kept tripping LLMs (see the paired codeact preamble update); reads metadata, diff, and files via three sequential awaits instead of `asyncio.gather`. - Step 2 reads each changed file in full (raw media type, no base64 module needed) so reviews account for surrounding context. - Step 3 runs the change through six lenses: correctness, edge cases, security (with a real adversarial checklist), test coverage, docs, architecture. - Step 4 renders findings as a severity table and asks which to post. - Step 5 posts line-level comments via the PR comments endpoint with the captured head SHA, falling back to issue comments for multi-file findings. Bumps `requires.skills` to include `github` so the activation pulls in the GitHub API recipes via the chain-loader. Adds a live e2e test (`e2e_live_code_review.rs`) plus a recorded trace fixture (PR #2483) so the workflow is replayable without hitting GitHub.
Pure formatting — `cargo fmt` reflow of long lines and trailing-blank removal in the portfolio e2e tests. No logic changes.
github-actions
Bot
added
scope: agent
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements Engine v2, introducing workspace-backed autonomous projects, a project control room dashboard, and an enhanced skill activation system with chain-loading and setup-marker exclusion. It also expands attachment support to include documents and audio, ensuring persistence and indexing within project-local directories. Other notable changes include a new DeFi portfolio tracking tool and significant updates to the code review and commitment tracking skills. Feedback focuses on improving code maintainability by refactoring complex logic into helper functions—specifically for clock replies, message sending, and skill selection—and simplifying string formatting in the Python orchestrator.
Comment on lines
+158
to
+166
| if len(output) > 500: | ||
| preview = output[:500] + "..." | ||
| parts.append( | ||
| "[" + name + "] " + preview + | ||
| "\n(full result stored in state['" + name + "']; " | ||
| "do NOT retype the data — reference the variable in your next call.)" | ||
| ) | ||
| else: | ||
| parts.append("[" + name + "] " + output) |
Contributor
There was a problem hiding this comment.
The logic for formatting the preview output is becoming complex. Following repository guidelines, always truncate tool output for previews to a reasonable maximum length to prevent excessive memory usage and avoid leaking sensitive information.
if len(output) > 500:
preview = output[:500] + "..."
parts.append(f"[{name}] {preview}\n(full result stored in state['{name}']; do NOT retype the data — reference the variable in your next call.)")
else:
parts.append(f"[{name}] {output}")References
- Always truncate tool output for previews or status updates to a reasonable maximum length to prevent excessive memory/bandwidth usage and reduce the risk of leaking sensitive information.
Comment on lines
+860
to
+866
| let clock_reply: Option<ExtFunctionResult> = match os_call.function { | ||
| OsFunction::DateTimeNow => { | ||
| Some(ExtFunctionResult::Return(build_datetime_now(&os_call.args))) | ||
| } | ||
| OsFunction::DateToday => Some(ExtFunctionResult::Return(build_date_today())), | ||
| _ => None, | ||
| }; |
Contributor
There was a problem hiding this comment.
The match statement for clock replies is growing. Consider encapsulating this logic within a dedicated helper function to provide a simpler interface and keep execute_code_with_skills clean.
References
- Encapsulate complex data structures and logic within a helper function, and expose a simpler interface or data structure tailored to the caller's needs.
Comment on lines
+1174
to
1283
| if (_sendCooldown || _sendInFlight) return; | ||
| _sendInFlight = true; | ||
| try { | ||
| if (pendingAttachmentReads.length > 0) { | ||
| await Promise.all([...pendingAttachmentReads]); | ||
| } | ||
| const content = input.value.trim(); | ||
| if (!content && stagedAttachments.length === 0) return; | ||
|
|
||
| // Intercept approval keywords when an unresolved approval card is pending. | ||
| // Find the most recent unresolved card for the current thread (resolved cards | ||
| // linger 1.5s before removal; cards from other threads must not be matched). | ||
| const approvalCards = Array.from(document.querySelectorAll('.approval-card')); | ||
| const approvalCard = approvalCards.reverse().find(card => { | ||
| if (card.querySelector('.approval-resolved')) return false; | ||
| const cardThreadId = card.getAttribute('data-thread-id'); | ||
| return !cardThreadId || cardThreadId === currentThreadId; | ||
| }); | ||
| if (approvalCard && content) { | ||
| const lower = content.toLowerCase(); | ||
| let action = null; | ||
| if (['yes', 'y', 'approve', 'ok', '/approve', '/yes', '/y'].includes(lower)) { | ||
| action = 'approve'; | ||
| } else if (['always', 'a', 'yes always', 'approve always', '/always', '/a'].includes(lower)) { | ||
| action = 'always'; | ||
| } else if (['no', 'n', 'deny', 'reject', 'cancel', '/deny', '/no', '/n'].includes(lower)) { | ||
| action = 'deny'; | ||
| } | ||
| if (action) { | ||
| input.value = ''; | ||
| autoResizeTextarea(input); | ||
| input.focus(); | ||
| const requestId = approvalCard.getAttribute('data-request-id'); | ||
| const threadId = approvalCard.getAttribute('data-thread-id'); | ||
| if (requestId) { | ||
| sendApprovalAction(requestId, action, threadId); | ||
| } | ||
| return; | ||
| } | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| const userMsg = addMessage('user', content || '(images attached)'); | ||
| input.value = ''; | ||
| autoResizeTextarea(input); | ||
| input.focus(); | ||
|
|
||
| const body = { content, thread_id: currentThreadId || undefined, timezone: Intl.DateTimeFormat().resolvedOptions().timeZone }; | ||
| if (stagedImages.length > 0) { | ||
| body.images = stagedImages.map(img => ({ media_type: img.media_type, data: img.data })); | ||
| stagedImages = []; | ||
| renderImagePreviews(); | ||
| } | ||
|
|
||
| apiFetch('/api/chat/send', { | ||
| method: 'POST', | ||
| body: body, | ||
| }).catch((err) => { | ||
| // Handle rate limiting (429) | ||
| if (err.status === 429) { | ||
| showToast(I18n.t('chat.rateLimited'), 'error'); | ||
| _sendCooldown = true; | ||
| const sendBtn = document.getElementById('send-btn'); | ||
| if (sendBtn) sendBtn.disabled = true; | ||
| setTimeout(() => { | ||
| _sendCooldown = false; | ||
| if (sendBtn) sendBtn.disabled = false; | ||
| }, 2000); | ||
| } | ||
| // Keep the user message in DOM, add a retry link | ||
| if (userMsg) { | ||
| userMsg.classList.add('send-failed'); | ||
| userMsg.style.borderStyle = 'dashed'; | ||
| const retryLink = document.createElement('a'); | ||
| retryLink.className = 'retry-link'; | ||
| retryLink.href = '#'; | ||
| retryLink.textContent = I18n.t('common.retry'); | ||
| retryLink.addEventListener('click', (e) => { | ||
| e.preventDefault(); | ||
| if (userMsg.parentNode) userMsg.parentNode.removeChild(userMsg); | ||
| input.value = content; | ||
| sendMessage(); | ||
| const pendingAttachments = stagedAttachments.map(att => ({ ...att })); | ||
| const pendingCopyText = [ | ||
| content || '(files attached)', | ||
| ...pendingAttachments.map((att) => { | ||
| const suffix = [att.mime_type, att.size_label].filter(Boolean).join(' • '); | ||
| return suffix ? `[Attachment] ${att.filename || 'attachment'} (${suffix})` : `[Attachment] ${att.filename || 'attachment'}`; | ||
| }), | ||
| ].join('\n'); | ||
| const userMsg = addMessage('user', content || '(files attached)', { | ||
| attachments: pendingAttachments, | ||
| copyText: pendingCopyText, | ||
| }); | ||
| input.value = ''; | ||
| autoResizeTextarea(input); | ||
| input.focus(); | ||
|
|
||
| const body = { content, thread_id: currentThreadId || undefined, timezone: Intl.DateTimeFormat().resolvedOptions().timeZone }; | ||
| if (stagedAttachments.length > 0) { | ||
| body.attachments = stagedAttachments.map(att => ({ | ||
| mime_type: att.mime_type, | ||
| filename: att.filename, | ||
| data_base64: att.data_base64, | ||
| })); | ||
| stagedAttachments = []; | ||
| renderAttachmentPreviews(); | ||
| } | ||
|
|
||
| const sentThreadId = currentThreadId || null; | ||
| try { | ||
| await apiFetch('/api/chat/send', { | ||
| method: 'POST', | ||
| body: body, | ||
| }); | ||
| userMsg.appendChild(retryLink); | ||
| if (sentThreadId) { | ||
| hydratePendingGateFromHistory(sentThreadId, 12, 250); | ||
| } | ||
| } catch (err) { | ||
| // Handle rate limiting (429) | ||
| if (err.status === 429) { | ||
| showToast(I18n.t('chat.rateLimited'), 'error'); | ||
| _sendCooldown = true; | ||
| const sendBtn = document.getElementById('send-btn'); | ||
| if (sendBtn) sendBtn.disabled = true; | ||
| setTimeout(() => { | ||
| _sendCooldown = false; | ||
| if (sendBtn) sendBtn.disabled = false; | ||
| }, 2000); | ||
| } | ||
| // Keep the user message in DOM, add a retry link | ||
| if (userMsg) { | ||
| userMsg.classList.add('send-failed'); | ||
| userMsg.style.borderStyle = 'dashed'; | ||
| const retryLink = document.createElement('a'); | ||
| retryLink.className = 'retry-link'; | ||
| retryLink.href = '#'; | ||
| retryLink.textContent = I18n.t('common.retry'); | ||
| retryLink.addEventListener('click', (e) => { | ||
| e.preventDefault(); | ||
| if (userMsg.parentNode) userMsg.parentNode.removeChild(userMsg); | ||
| input.value = content; | ||
| sendMessage(); | ||
| }); | ||
| userMsg.appendChild(retryLink); | ||
| } | ||
| } | ||
| }); | ||
| } finally { | ||
| _sendInFlight = false; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
The sendMessage function is becoming quite large and handles multiple responsibilities (attachment processing, approval interception, API interaction). Consider breaking this into smaller, focused functions to improve maintainability.
Comment on lines
179
to
218
| for entry in scored { | ||
| if result.len() >= max_candidates { | ||
| break; | ||
| // Try to select the parent first. | ||
| if !try_select( | ||
| entry.skill, | ||
| &mut result, | ||
| &mut selected_names, | ||
| &mut budget_remaining, | ||
| max_candidates, | ||
| satisfied_setup_markers, | ||
| ) { | ||
| // Parent didn't fit or was already selected — don't try to | ||
| // chain-load companions for a parent that isn't in the set. | ||
| continue; | ||
| } | ||
| let declared_tokens = entry.skill.manifest.activation.max_context_tokens; | ||
| // Rough token estimate: ~0.25 tokens per byte (~4 bytes per token for English prose) | ||
| let approx_tokens = (entry.skill.prompt_content.len() as f64 * 0.25) as usize; | ||
| let raw_cost = if approx_tokens > declared_tokens * 2 { | ||
| tracing::warn!( | ||
| "Skill '{}' declares max_context_tokens={} but prompt is ~{} tokens; using actual estimate", | ||
| entry.skill.name(), | ||
| declared_tokens, | ||
| approx_tokens, | ||
| ); | ||
| approx_tokens | ||
| } else { | ||
| declared_tokens | ||
| }; | ||
| // Enforce a minimum token cost so max_context_tokens=0 can't bypass budgeting | ||
| let token_cost = raw_cost.max(1); | ||
| if token_cost <= budget_remaining { | ||
| budget_remaining -= token_cost; | ||
| result.push(entry.skill); | ||
|
|
||
| // Chain-load companions declared in requires.skills. | ||
| // Non-transitive: companions don't load their own companions. | ||
| for companion_name in &entry.skill.manifest.requires.skills { | ||
| let Some(companion) = by_name.get(companion_name.as_str()) else { | ||
| // Listed but not loaded — ignore silently. Persona | ||
| // bundles declare optional companions. | ||
| continue; | ||
| }; | ||
| if !try_select( | ||
| companion, | ||
| &mut result, | ||
| &mut selected_names, | ||
| &mut budget_remaining, | ||
| max_candidates, | ||
| satisfied_setup_markers, | ||
| ) { | ||
| tracing::debug!( | ||
| parent = %entry.skill.name(), | ||
| companion = %companion_name, | ||
| budget_remaining, | ||
| "chain-load skipped (already selected, budget full, or marker satisfied)" | ||
| ); | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
The nested loop structure for chain-loading companions is becoming deeply indented. Consider encapsulating the companion loading logic into a separate helper function to reduce complexity and improve readability.
References
- Encapsulate complex data structures and logic within a helper function, and expose a simpler interface or data structure tailored to the caller's needs.
This was referenced Apr 16, 2026
ilblackdragon
changed the title
ilblackdragon
added a commit
that referenced
this pull request
Apr 19, 2026
…ceptance Closes two gaps blocking v2 engine becoming the default: **Phase 4 — token + cost accounting** - Delete orphaned `crates/ironclaw_engine/src/executor/compaction.rs` (176 lines). The Python orchestrator (`default.py::compact_if_needed`) has owned compaction policy since #1557; the Rust module had no callers anywhere in the workspace. - Wire `cost_usd` in `LlmBridgeAdapter` by calling `LlmProvider::calculate_cost()` at both the no-tools and with-tools response paths. The engine's `Thread::total_cost_usd` accumulator and `max_budget_usd` gates were already plumbed — only the adapter was hardcoding 0.0. - Persist `total_cost_usd` through `ThreadArchiveSummary` round-trip in `store_adapter.rs`. Previously, rehydrating an archived thread silently dropped the cost to 0.0. `#[serde(default)]` keeps existing archive files deserializing cleanly. **Phase 6 — mission lifecycle acceptance** Three new integration tests in `bridge/effect_adapter.rs` driving `execute_action()` end-to-end (per `.claude/rules/testing.md` "Test Through the Caller"): - `mission_full_lifecycle_via_execute_action` — create → list → complete → list, asserting the `Completed` status surfaces through `mission_list` after `mission_complete`. - `mission_fire_returns_thread_id_for_manual_cadence_via_execute_action` — fresh manual mission fires successfully and returns a UUID thread_id rather than `not_fired`. - `mission_list_returns_all_user_missions_via_execute_action` — all three created missions appear in `mission_list` output. **Regression tests for cost wiring** Three new tests in `bridge/llm_adapter.rs`: - `complete_no_tools_populates_cost_usd_through_adapter` - `complete_with_tools_populates_cost_usd_through_adapter` - `complete_routes_subcalls_through_cheap_provider_for_cost` — pins that `depth > 0` is priced with the cheap provider, not the primary. Coordinated with in-flight work: skipped paths owned by #2504 (auth E2E), #2631 (paused-lease resume), #2570 (mission re-fire), #2549 (mission_get), #2452 (tool_calls persistence), #2621 (replay snapshot). Verified: `cargo fmt`, `cargo clippy --all --benches --tests --examples --all-features` (0 warnings), `cargo test -p ironclaw_engine` (409 passed), `cargo test -p ironclaw --lib` (5079 passed). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 of 5 tasks
ilblackdragon
added a commit
that referenced
this pull request
Apr 19, 2026
…ceptance Closes two gaps blocking v2 engine becoming the default: **Phase 4 — token + cost accounting** - Delete orphaned `crates/ironclaw_engine/src/executor/compaction.rs` (176 lines). The Python orchestrator (`default.py::compact_if_needed`) has owned compaction policy since #1557; the Rust module had no callers anywhere in the workspace. - Wire `cost_usd` in `LlmBridgeAdapter` by calling `LlmProvider::calculate_cost()` at both the no-tools and with-tools response paths. The engine's `Thread::total_cost_usd` accumulator and `max_budget_usd` gates were already plumbed — only the adapter was hardcoding 0.0. - Persist `total_cost_usd` through `ThreadArchiveSummary` round-trip in `store_adapter.rs`. Previously, rehydrating an archived thread silently dropped the cost to 0.0. `#[serde(default)]` keeps existing archive files deserializing cleanly. **Phase 6 — mission lifecycle acceptance** Three new integration tests in `bridge/effect_adapter.rs` driving `execute_action()` end-to-end (per `.claude/rules/testing.md` "Test Through the Caller"): - `mission_full_lifecycle_via_execute_action` — create → list → complete → list, asserting the `Completed` status surfaces through `mission_list` after `mission_complete`. - `mission_fire_returns_thread_id_for_manual_cadence_via_execute_action` — fresh manual mission fires successfully and returns a UUID thread_id rather than `not_fired`. - `mission_list_returns_all_user_missions_via_execute_action` — all three created missions appear in `mission_list` output. **Regression tests for cost wiring** Three new tests in `bridge/llm_adapter.rs`: - `complete_no_tools_populates_cost_usd_through_adapter` - `complete_with_tools_populates_cost_usd_through_adapter` - `complete_routes_subcalls_through_cheap_provider_for_cost` — pins that `depth > 0` is priced with the cheap provider, not the primary. Coordinated with in-flight work: skipped paths owned by #2504 (auth E2E), #2631 (paused-lease resume), #2570 (mission re-fire), #2549 (mission_get), #2452 (tool_calls persistence), #2621 (replay snapshot). Verified: `cargo fmt`, `cargo clippy --all --benches --tests --examples --all-features` (0 warnings), `cargo test -p ironclaw_engine` (409 passed), `cargo test -p ironclaw --lib` (5079 passed). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ilblackdragon
added a commit
that referenced
this pull request
Apr 19, 2026
…ceptance (#2660) * feat(engine-v2): Phase 4 cost tracking + Phase 6 mission lifecycle acceptance Closes two gaps blocking v2 engine becoming the default: **Phase 4 — token + cost accounting** - Delete orphaned `crates/ironclaw_engine/src/executor/compaction.rs` (176 lines). The Python orchestrator (`default.py::compact_if_needed`) has owned compaction policy since #1557; the Rust module had no callers anywhere in the workspace. - Wire `cost_usd` in `LlmBridgeAdapter` by calling `LlmProvider::calculate_cost()` at both the no-tools and with-tools response paths. The engine's `Thread::total_cost_usd` accumulator and `max_budget_usd` gates were already plumbed — only the adapter was hardcoding 0.0. - Persist `total_cost_usd` through `ThreadArchiveSummary` round-trip in `store_adapter.rs`. Previously, rehydrating an archived thread silently dropped the cost to 0.0. `#[serde(default)]` keeps existing archive files deserializing cleanly. **Phase 6 — mission lifecycle acceptance** Three new integration tests in `bridge/effect_adapter.rs` driving `execute_action()` end-to-end (per `.claude/rules/testing.md` "Test Through the Caller"): - `mission_full_lifecycle_via_execute_action` — create → list → complete → list, asserting the `Completed` status surfaces through `mission_list` after `mission_complete`. - `mission_fire_returns_thread_id_for_manual_cadence_via_execute_action` — fresh manual mission fires successfully and returns a UUID thread_id rather than `not_fired`. - `mission_list_returns_all_user_missions_via_execute_action` — all three created missions appear in `mission_list` output. **Regression tests for cost wiring** Three new tests in `bridge/llm_adapter.rs`: - `complete_no_tools_populates_cost_usd_through_adapter` - `complete_with_tools_populates_cost_usd_through_adapter` - `complete_routes_subcalls_through_cheap_provider_for_cost` — pins that `depth > 0` is priced with the cheap provider, not the primary. Coordinated with in-flight work: skipped paths owned by #2504 (auth E2E), #2631 (paused-lease resume), #2570 (mission re-fire), #2549 (mission_get), #2452 (tool_calls persistence), #2621 (replay snapshot). Verified: `cargo fmt`, `cargo clippy --all --benches --tests --examples --all-features` (0 warnings), `cargo test -p ironclaw_engine` (409 passed), `cargo test -p ironclaw --lib` (5079 passed). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(engine-v2): surface engine capability actions to LLM via available_actions Fixes the gap called out in the PR body: `EffectBridgeAdapter::available_actions` was only enumerating v1 `ToolRegistry` tools + latent OAuth actions, so engine-native capabilities like `missions` never appeared in the LLM's tools list even when a thread held an active lease for them. The LLM was therefore unable to call `mission_create` / `mission_list` / etc. via structured tool calls; the only ways to drive missions were CodeAct Python calls (which relied on the same `known_actions` set and hit the same gap) or `/routine` slash commands falling through to v1. Wire `CapabilityRegistry` into the adapter and iterate active leases to surface every leased, engine-registered capability action. Respects lease grant scope — a lease granting only `mission_list` does not leak `mission_create`. Skips the `"tools"` capability since that lease is already reconciled from the v1 path. Router wires the shared `Arc<CapabilityRegistry>` to both the adapter and `ThreadManager` at setup. Three new regression tests: - `available_actions_surfaces_leased_mission_capability` - `available_actions_respects_partial_lease_grant` - `available_actions_omits_capability_without_lease` Verified: `cargo fmt`, `cargo clippy --all --benches --tests --examples --all-features` (0 warnings), `cargo test -p ironclaw_engine` (409 passed), `cargo test -p ironclaw --lib` (5082 passed). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(engine-v2): close review gaps — archive round-trip, v1/engine merge, defensive filters Addresses gaps raised in PR #2660 review: - **Consolidate `use ironclaw_engine::{...}`** into a single grouped import in `effect_adapter.rs` (was split across two statements). - **Apply `is_v1_only_tool` / `is_v1_auth_tool` filters** to the engine capability path in `available_actions`. Defensive guardrail: a future engine capability that registers an action under a v1-denylisted name (`create_job`, `tool_auth`, ...) must not bypass the v2-isolation filters by virtue of coming through a different capability registry. - **`ThreadArchiveSummary` serialization round-trip tests** in `store_adapter.rs`: - `archive_summary_preserves_total_cost_usd_through_round_trip` — pins the regression the PR fixed (cost silently zeroed on rehydration). - `archive_summary_handles_legacy_json_without_total_cost_usd_field` — pins `#[serde(default)]` back-compat for archive files written before this PR. - **`available_actions` combined advertising tests** in `effect_adapter.rs`: - `available_actions_merges_v1_tools_with_engine_capabilities` — v1 tool + mission capability both surface on one call. - `available_actions_filters_v1_denylisted_names_from_engine_capabilities` — pins the new defensive filter. - **`cost_usd_from` subscription-billed-provider test** in `llm_adapter.rs`: - `complete_with_subscription_billed_provider_yields_zero_cost` — zero `cost_per_token` round-trips to exactly `0.0`, no NaN/Inf. Verified: `cargo fmt`, `cargo clippy --all --benches --tests --examples --all-features` (0 warnings), `cargo test -p ironclaw_engine` (409 passed), `cargo test -p ironclaw --lib` (5087 passed, +5 new). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(engine-v2): price cache tokens correctly in LlmBridgeAdapter Addresses PR #2660 review (gemini-code-assist + Copilot, L23/L115/L189): `cost_usd_from` only priced `input_tokens + output_tokens`, ignoring `cache_read_input_tokens` and `cache_creation_input_tokens`. For providers with prompt caching (Anthropic, OpenAI), this undercounted input cost and silently neutered the `max_budget_usd` gate. Extend the helper to mirror the canonical formula in `src/agent/cost_guard.rs::CostGuard::record_llm_call`: uncached_input = input_tokens - (cache_read + cache_write) cache_read_cost = input_rate * cache_read / cache_read_discount() cache_write_cost = input_rate * cache_write * cache_write_multiplier() cost = input_rate * uncached_input + cache_read_cost + cache_write_cost + output_rate * output_tokens All `LlmProvider` implementations already supply `cache_read_discount()` (default 1, Anthropic 10, OpenAI 2) and `cache_write_multiplier()` (default 1, Anthropic 1.25 for 5m / 2.0 for 1h) through the decorator chain, so no trait surgery is required. Regression test: `complete_prices_cache_tokens_with_discount_and_multiplier` uses Anthropic Sonnet 5m-TTL rates, exercises a 10k-input / 2k-read / 1k-write / 500-output response, and pins the correct total ($0.03285) against the old naive $0.0375 that would have undercounted ~14%. Verified: `cargo fmt`, `cargo clippy --all --benches --tests --examples --all-features` (0 warnings), `cargo test -p ironclaw_engine` (435 passed), `cargo test -p ironclaw --lib` (5136 passed). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Member
Author
|
Everything merged in separate PRs. |
theredspoon
pushed a commit
to theredspoon/ironclaw
that referenced
this pull request
Jun 21, 2026
…ceptance (nearai#2660) * feat(engine-v2): Phase 4 cost tracking + Phase 6 mission lifecycle acceptance Closes two gaps blocking v2 engine becoming the default: **Phase 4 — token + cost accounting** - Delete orphaned `crates/ironclaw_engine/src/executor/compaction.rs` (176 lines). The Python orchestrator (`default.py::compact_if_needed`) has owned compaction policy since nearai#1557; the Rust module had no callers anywhere in the workspace. - Wire `cost_usd` in `LlmBridgeAdapter` by calling `LlmProvider::calculate_cost()` at both the no-tools and with-tools response paths. The engine's `Thread::total_cost_usd` accumulator and `max_budget_usd` gates were already plumbed — only the adapter was hardcoding 0.0. - Persist `total_cost_usd` through `ThreadArchiveSummary` round-trip in `store_adapter.rs`. Previously, rehydrating an archived thread silently dropped the cost to 0.0. `#[serde(default)]` keeps existing archive files deserializing cleanly. **Phase 6 — mission lifecycle acceptance** Three new integration tests in `bridge/effect_adapter.rs` driving `execute_action()` end-to-end (per `.claude/rules/testing.md` "Test Through the Caller"): - `mission_full_lifecycle_via_execute_action` — create → list → complete → list, asserting the `Completed` status surfaces through `mission_list` after `mission_complete`. - `mission_fire_returns_thread_id_for_manual_cadence_via_execute_action` — fresh manual mission fires successfully and returns a UUID thread_id rather than `not_fired`. - `mission_list_returns_all_user_missions_via_execute_action` — all three created missions appear in `mission_list` output. **Regression tests for cost wiring** Three new tests in `bridge/llm_adapter.rs`: - `complete_no_tools_populates_cost_usd_through_adapter` - `complete_with_tools_populates_cost_usd_through_adapter` - `complete_routes_subcalls_through_cheap_provider_for_cost` — pins that `depth > 0` is priced with the cheap provider, not the primary. Coordinated with in-flight work: skipped paths owned by nearai#2504 (auth E2E), nearai#2631 (paused-lease resume), nearai#2570 (mission re-fire), nearai#2549 (mission_get), nearai#2452 (tool_calls persistence), nearai#2621 (replay snapshot). Verified: `cargo fmt`, `cargo clippy --all --benches --tests --examples --all-features` (0 warnings), `cargo test -p ironclaw_engine` (409 passed), `cargo test -p ironclaw --lib` (5079 passed). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * feat(engine-v2): surface engine capability actions to LLM via available_actions Fixes the gap called out in the PR body: `EffectBridgeAdapter::available_actions` was only enumerating v1 `ToolRegistry` tools + latent OAuth actions, so engine-native capabilities like `missions` never appeared in the LLM's tools list even when a thread held an active lease for them. The LLM was therefore unable to call `mission_create` / `mission_list` / etc. via structured tool calls; the only ways to drive missions were CodeAct Python calls (which relied on the same `known_actions` set and hit the same gap) or `/routine` slash commands falling through to v1. Wire `CapabilityRegistry` into the adapter and iterate active leases to surface every leased, engine-registered capability action. Respects lease grant scope — a lease granting only `mission_list` does not leak `mission_create`. Skips the `"tools"` capability since that lease is already reconciled from the v1 path. Router wires the shared `Arc<CapabilityRegistry>` to both the adapter and `ThreadManager` at setup. Three new regression tests: - `available_actions_surfaces_leased_mission_capability` - `available_actions_respects_partial_lease_grant` - `available_actions_omits_capability_without_lease` Verified: `cargo fmt`, `cargo clippy --all --benches --tests --examples --all-features` (0 warnings), `cargo test -p ironclaw_engine` (409 passed), `cargo test -p ironclaw --lib` (5082 passed). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test(engine-v2): close review gaps — archive round-trip, v1/engine merge, defensive filters Addresses gaps raised in PR nearai#2660 review: - **Consolidate `use ironclaw_engine::{...}`** into a single grouped import in `effect_adapter.rs` (was split across two statements). - **Apply `is_v1_only_tool` / `is_v1_auth_tool` filters** to the engine capability path in `available_actions`. Defensive guardrail: a future engine capability that registers an action under a v1-denylisted name (`create_job`, `tool_auth`, ...) must not bypass the v2-isolation filters by virtue of coming through a different capability registry. - **`ThreadArchiveSummary` serialization round-trip tests** in `store_adapter.rs`: - `archive_summary_preserves_total_cost_usd_through_round_trip` — pins the regression the PR fixed (cost silently zeroed on rehydration). - `archive_summary_handles_legacy_json_without_total_cost_usd_field` — pins `#[serde(default)]` back-compat for archive files written before this PR. - **`available_actions` combined advertising tests** in `effect_adapter.rs`: - `available_actions_merges_v1_tools_with_engine_capabilities` — v1 tool + mission capability both surface on one call. - `available_actions_filters_v1_denylisted_names_from_engine_capabilities` — pins the new defensive filter. - **`cost_usd_from` subscription-billed-provider test** in `llm_adapter.rs`: - `complete_with_subscription_billed_provider_yields_zero_cost` — zero `cost_per_token` round-trips to exactly `0.0`, no NaN/Inf. Verified: `cargo fmt`, `cargo clippy --all --benches --tests --examples --all-features` (0 warnings), `cargo test -p ironclaw_engine` (409 passed), `cargo test -p ironclaw --lib` (5087 passed, +5 new). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(engine-v2): price cache tokens correctly in LlmBridgeAdapter Addresses PR nearai#2660 review (gemini-code-assist + Copilot, L23/L115/L189): `cost_usd_from` only priced `input_tokens + output_tokens`, ignoring `cache_read_input_tokens` and `cache_creation_input_tokens`. For providers with prompt caching (Anthropic, OpenAI), this undercounted input cost and silently neutered the `max_budget_usd` gate. Extend the helper to mirror the canonical formula in `src/agent/cost_guard.rs::CostGuard::record_llm_call`: uncached_input = input_tokens - (cache_read + cache_write) cache_read_cost = input_rate * cache_read / cache_read_discount() cache_write_cost = input_rate * cache_write * cache_write_multiplier() cost = input_rate * uncached_input + cache_read_cost + cache_write_cost + output_rate * output_tokens All `LlmProvider` implementations already supply `cache_read_discount()` (default 1, Anthropic 10, OpenAI 2) and `cache_write_multiplier()` (default 1, Anthropic 1.25 for 5m / 2.0 for 1h) through the decorator chain, so no trait surgery is required. Regression test: `complete_prices_cache_tokens_with_discount_and_multiplier` uses Anthropic Sonnet 5m-TTL rates, exercises a 10k-input / 2k-read / 1k-write / 500-output response, and pins the correct total ($0.03285) against the old naive $0.0375 that would have undercounted ~14%. Verified: `cargo fmt`, `cargo clippy --all --benches --tests --examples --all-features` (0 warnings), `cargo test -p ironclaw_engine` (435 passed), `cargo test -p ironclaw --lib` (5136 passed). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Eleven scoped fixes split out of
feat/portfolio-tests-and-share(PR #2368) so they can land independently of the portfolio work. Targets staging.Engine
fix(engine): make FINAL/FINAL_VAR awaitable in CodeAct scripts—await FINAL(...)now resolves toNoneinstead of raisingTypeError: 'NoneType' object can't be awaitedand dropping the answer. BothFINAL(x)andawait FINAL(x)work.fix(engine): normalize typographic punctuation before skill activation—iOS/macOSautocorrect turnsI'mintoI'm; activation regexes authored with ASCII apostrophes silently failed to match.default.pynow folds 8 curly quotes + en/em dashes to ASCII before scoring. Fixes theceo-setupnon-activation report and any other skill with apostrophes in its patterns.fix(engine): default max_consecutive_errors to 5— bound runaway loops on wedged actions.Skills + UI
feat(events): SkillActivated carries activation feedback notes— adds an optionalfeedback: Vec<String>field through the SSE/StatusUpdate path with a gateway timeline card so chain-load reasons / marker exclusions can surface to the user.fix(skills): skill_install never prompts when skill is already loaded— mirrors the idempotent execute path inrequires_approvalso/ceo-setupfollow-up installs don't trigger a useless approval prompt.feat(skills): paranoid-architect code-review skill v2— full rewrite of the code-review skill, now handles both local diffs and GitHub PRs (owner/repo N), reads each changed file in full, six-lens review, can post line-level review comments. Recorded e2e fixture for PR feat(engine): add code execution failure categorization instrumentation #2483.docs(github): clarify search endpoints, response envelope, @me queries— kills the recurringsearch_issueshallucination, addsis:pr author:@merecipes, documents thebodyenvelope contract.Web gateway
feat(web): expose engine v2 threads in chat history and sidebar— fixes#/chat/<engine-thread-id>rendering empty and engine threads being invisible in the sidebar (v1 dual-write writes into the assistant conversation id, not the engine thread id). Bumps sidebar cap 50→500.Security
fix(security): redact credentials in HTTP exchange recorder and tool— recorded fixtures can no longer ship live tokens.RecordingHttpInterceptorscrubs credential headers + sensitive query params;HttpToolsnapshots caller headers BEFORE credential injection so injectedAuthorizationnever reaches the recorder. Also dedupes credential mappings (was causing GitHub 401s).Docs / chore
docs(codeact): note closure capture quirk and Rust regex limits— preamble paragraphs on the cross-block closure-capture trap and Rust regex differences.chore(tests): rustfmt portfolio test files— reflow only.Test plan
cargo test -p ironclaw_engine --lib— all green (35 scripting + 53 orchestrator)final_supports_await,final_var_supports_await,normalize_punctuation_folds_curly_quotes_and_dashes,select_skills_matches_curly_apostrophe_input,recording_http_interceptor_redacts_credentials,skill_install_skips_approval_when_already_loadedcargo clippy --all --benches --tests --examples --all-features(run before marking ready)