Repository navigation
fix(runtime): preserve admitted skill instructions across requests - #1117
Conversation
XuPeng-SH
left a comment
There was a problem hiding this comment.
Recommendation: REQUEST_CHANGES. Submitted as COMMENT because the authenticated account is the PR author.
[P1] Capture safety-governed bytes for client-pipeline skills before unconditional recovery.
At context.rs:1963–1965, recovery now consumes invoked-skill entries, but the client-pipeline activation owner still caches callback output unchanged at tool_phase.rs:154–159, before the shared tool pipeline sanitizes it. CLI execution and callback ingestion redact credentials but do not remove prompt-like lines.
Consequently, a line such as ignore previous instructions, stripped from ordinary tool-result delivery, returns through the new recovery attachment as runtime Skill instructions. Sanitized history differs from the cached body, so the presence check triggers this even when the original result remains in history; compaction is unnecessary.
An isolated regression probe reproduced the failure in the final provider request in all four combinations: retained/missing tool history × TailSuffix/AppendOnlyUserTail. Extend complete governed-body capture to the client-pipeline owner, and derive ordinary delivery, recovery and continuation from that same value. Add coverage through client-pipeline receipt → sanitization → subsequent provider request.
The principal #1101 repair otherwise addresses the reported mechanism: recovery no longer depends on a request-local compaction flag, complete results and active recovery frames suppress reinjection, and the unchanged three-re-entry limit permits subsequent authorized tool work. Reviewed activation, automatic activation, canonical pairing, frame lifetime and rehoming, continuation restore, final-budget recomposition, admission and lockout paths. No additional material PR regression was found. A separate redaction-marker bisection probe reproduced unchanged attachment-truncator behavior; it is excluded as pre-existing, not attributed to this PR.
Independent validation at 6c4250e:
- Skill suite: 283 passed, 1 ignored (
cargo test --locked -p astra-runtime --lib skill -- --nocapture). - Disjoint context/wire/interception/final-budget suite: 156 passed.
- Format, toolchain and diff checks: passed.
- Additional client-pipeline safety probe: failed as described above, all four combinations.
- Temporary probe changes removed; detached review checkout clean.
All nine changed files and the single PR commit were reviewed against merge-base 2238be3. Implementation: +105/−34, net +71; tests: +498/−169, net +329; documentation: +15/−0, net +15. No pure moves identified. Existing execution facts remain authoritative; recovery adds history scans and temporary indexes, with no new database calls visible in changed paths. CPU/memory performance was not benchmarked.
Full-workspace, live-provider, database and independent clippy runs were not performed. Final publication checks confirmed the same open non-draft head, matching author/authenticated account and no existing reviews or inline comments. Visible CI: 38 checks, 34 success, 3 skipped, 1 neutral; no commit-status contexts. The complete readable ruleset list contains no required-status rule, but classic branch protection remains inaccessible to the integration. Required-context/App completeness therefore cannot be certified; this manually requested source review is not merge-gate certification.
Capture complete safety-governed client skill callback bytes in the existing invocation facts and forward the same body to ordinary tool delivery. Extend loop coverage across client receipts, both wire layouts, recovery and continuation restore.
6c4250e to
ab46134
Compare
XuPeng-SH
left a comment
There was a problem hiding this comment.
Independent repair re-review: REQUEST_CHANGES recommended. COMMENT because the authenticated account is the author.
Reviewed head ab46134, base/merge-base 3d8af2e. Fresh activations pass the original client-pipeline sanitization regression, but two recovery gaps remain:
-
[P1] Govern existing continuation facts before unconditional recovery. context.rs:1954–1965 consumes cached bodies. Main previously stored raw bodies, while compatible continuation validation checks identities without governing content. An independent serialized-facts probe put a synthetic credential and
ignore previous instructionsback on the final provider wire in all four retained/missing-history × TailSuffix/AppendOnlyUserTail cases. Establish the governed-body invariant when accepting existing continuation facts, preserving opaque redaction markers. This is evidence from compatible serialized facts and source tracing, not live database adoption. -
[P2] Capture the body after PostToolUse governance. tool_phase.rs:133–164 caches the callback before the shared post-tool hook. In the existing loop fixture, a successful hook replaced synthetic
PRE_HOOK_PRIVATE_INSTRUCTIONSwithHOOK_APPROVED_INSTRUCTIONS. Ordinary history contained the approved output, but recovery restored the earlier callback in all four cases. Retained history delivered both bodies; missing history delivered only the pre-hook body. Capture the complete final governed output before presentation truncation, rather than maintaining a second pre-hook authority.
Independent validation:
- Exact-head skill suite: 283 passed, 1 ignored.
- Disjoint context/wire/interception/final-budget checks: 156 passed.
- Storage, owner-scope, budgeting and hook checks: 27 passed.
- Two adversarial probes: both failed, each confirming four layout/history combinations.
- Format, toolchain and diff checks passed. Range-diff confirms the original continuity commit was unchanged by rebase.
- Temporary test-only include removed; clean review worktree. No product logic or remote state changed by this verifier.
Full PR delta versus rebased main: implementation +116/−37 (net +79), tests +571/−178 (net +393), documentation +17/−0 (net +17); no pure file moves identified in repair accounting. Recovery continues to use existing execution facts rather than adding database storage or calls; additional scans/indexes were not performance-benchmarked.
Clippy, full-workspace, live-provider and database suites were not independently rerun in this review. Final checks confirmed exact open head/authorship and no duplicate current-head review. Visible CI has 39 checks: 34 success, 3 skipped, 1 neutral and the live regression / harness still running; it is not claimed passed. No commit-status contexts. Readable rulesets do not establish required-context/App completeness because classic protection remains inaccessible to this integration. This manually requested code review does not certify merge gates.
XuPeng-SH
left a comment
There was a problem hiding this comment.
Independent final code review: APPROVE recommended. Submitted as COMMENT because the authenticated account owns this PR. No actionable findings in the second repair; both findings in review 5613730381 are resolved.
Reviewed exact head 1c87634, base/merge-base 3d8af2e.
Closure evidence:
- Continuation acceptance now governs existing cached instruction bodies. The original serialized-facts probe passes all four retained/missing-history × TailSuffix/AppendOnlyUserTail combinations: neither the synthetic credential nor the stripped instruction reaches provider serialization.
- Client caching now uses the complete final post-hook source. The original replacement probe passes all four combinations: approved instructions occur exactly once and the pre-hook body is absent.
- An additional publication probe passes eight scenarios: long-body artifact persistence, I/O fallback, immutable identity conflict, hook failure, cancellation, failed callback, resource read and repeated invocation. It checks governed bytes, opaque-marker preservation, counters, activation turn and topology. Hard conflicts and unsettled/rejected results do not publish cache entries.
- The original continuity patch is unchanged by rebase. The existing ledger remains the single authority; no new cache, serialized format or database calls were added.
Independent validation reproduced 630 passed, 5 failed, 2 ignored, plus three independent adversarial/publication probe tests passed. The five broader failures repeat serially and diagnostics confirm the unchanged native-workspace prerequisite: the executor's /tmp is owned by UID65534 rather than required root. Three tests expose workspace_host_unsupported; the other two write assertions fail because that same guard denies writes. These are recorded limitations, not represented as passing tests. No host security settings were changed. Format, toolchain and diff checks passed; temporary test-only changes were removed and the worktree is clean.
Latest repair accounting: implementation +113/−85, net +28, tests +309/−9, net +300, docs +4/−2, net +2. Raw implementation churn includes relocation/refactoring of the existing receipt writer, not an additional parallel writer. New behavior is continuation sanitation, final-result publication gating, activation-only validation and shared-context plumbing. Pure relocation does not increase the reported net complexity. CLI wiring was source-audited, but CLI compilation/clippy, full-workspace, live-provider and database suites were not independently rerun by this verifier. No CPU/memory benchmark was performed.
Final GitHub read confirms the exact open head, author/authenticated account XuPeng-SH, and no duplicate current-head review. Visible CI: 34 success, 3 skipped, 1 neutral, 1 failed. The failed live regression/harness job reports 17/18 cases passed, with flash_rejected_delegation_preserves_parent_tools failing: argument validation rejected additional_properties before execution, expected follow-on tool sequence was incomplete, and the LLM judge was unavailable at resource_limit. This does not establish the requested-model validation was reached and is NOT claimed as harness acceptance or a negative quality verdict. Evidence: https://github.com/matrixorigin/Astra/actions/runs/39778721200/job/119824207833 . No remote CI was rerun. Separate targeted manual harness verification remains in progress.
The readable rulesets were rechecked and contain no required-status rule, and there are no commit-status contexts. Classic branch protection remains inaccessible to this integration, so authoritative required-context/App completeness is not certified. This is the requested source-code recommendation under the disclosed visibility limit, not a claim that all CI or merge gates passed.
Capability assessment: Unchanged: restores already admitted Skill instructions on the shared backbone; capability sources, admission, execution routes and supported profiles are unchanged.
Summary
Admitted Skill instructions could disappear after pre-turn compaction because recovery depended on a request-local boundary flag. Repeated activation then escalated to guidance that prematurely ended unfinished work. Recover missing instructions from the existing execution facts on every request, while recognizing complete activation results and active recovery frames to avoid duplication.
Capture the complete safety-governed instruction body at activation so ordinary delivery, recovery and continuation reuse the same bytes and redaction markers. Keep recovery within the existing attachment and final wire budgets. Re-entry limits continue to block repeated activation of the named Skill and guide continuation of authorized work.
Related issue
Closes #1101.
Change type
User and compatibility impact
Long tasks retain admitted Skill guidance across ordinary provider requests and continuation restore. No new configuration or public protocol fields. The obsolete internal assembly flag and superseded flag-based test assumptions are removed.
Architecture and complexity delta
Verification
Rust 1.97.0; disk-backed temporary/build directories; two build jobs. Merge base:
2238be3246e9959ebd3dff4d2d655b4c4767ea38.cargo test --locked -p astra-runtime --lib skill -- --nocapture: 283 passed, 1 existing ignored. Includes real ordinary/automatic activation, unchanged counters, authorized tool continuation after lockout, retained/missing bodies, actual provider request construction, canonical commit/continuation and safety-marker preservation.cargo test --locked -p astra-runtime --lib -- turn::llm::context:: turn::wire_assembly:: turn::agentic::tool_interception:: final_wire_budget --skip skill --test-threads=4: 156 passed. Disjoint from the preceding filter; 439 passed in total.cargo clippy --locked -p astra-runtime --lib --tests -- -D warnings: passed.make format-check,make toolchain-check,git diff --check: passed.79ca9398dde3036b3c04193a4c7d32f8e260d16672adcdc75104db9baa43eaab; no unresolved material blockers after resolving the safe-body finding.No full workspace, live-provider or database suite was run. Remote CI is pending.
Database verification: no schema, query, transaction, polling, artifact-storage or auxiliary-inference changes. Recovery consumes already loaded execution facts and request history; it adds no database reads or writes. Safety canonicalization occurs at activation, not repeatedly during request assembly.
Final checklist