Stage strict prompt and output containment - #1509
Conversation
Signed-off-by: Chris0Jeky <jeky.tck@gmail.com>
Signed-off-by: Chris0Jeky <jeky.tck@gmail.com>
Signed-off-by: Chris0Jeky <jeky.tck@gmail.com>
Signed-off-by: Chris0Jeky <jeky.tck@gmail.com>
Signed-off-by: Chris0Jeky <jeky.tck@gmail.com>
Signed-off-by: Chris0Jeky <jeky.tck@gmail.com>
Signed-off-by: Chris0Jeky <jeky.tck@gmail.com>
Coordinator self-review findings and fix evidence — exact head
|
Exact-head correctionThe preceding self-review comment expanded the short commit prefix incorrectly. The actual local, remote, and PR head is |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a23bccd99f
ℹ️ 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".
Independent adversarial correctness/compatibility review - exact head
|
Independent adversarial security review — exact head a23bccdResult: CHANGES REQUESTED. No CRITICAL findings. This pass adds one new HIGH finding and extends the security impact/reproduction of one already-open HIGH/P2 finding. I read issue #1323, the PR body, every issue comment, review, and inline thread before posting. HIGH — ResponseHeadersRead removes the configured deadline from body readsAll three live providers now return from SendAsync after headers and then pass only the caller token into the bounded body loop: OpenAI lines 47–58, with the same shape in Gemini and Ollama; the reader awaits each stream read using only that token at LlmProviderResponseReader lines 29–41. The configured provider deadline is only HttpClient.Timeout (registration lines 85–89). Microsoft’s contract explicitly states that with ResponseHeadersRead, that timeout ends when headers arrive and content must be timed out separately: HttpCompletionOption remarks. This is exploitable as an availability failure even with the 1 MiB byte cap: a provider/proxy can return headers promptly, then stall or drip fewer than 1 MiB forever. TranscriptTriageWorker passes only the host stopping token and processes the lane sequentially (loop lines 52–65 and 106–115); one response can therefore hold the lane and its quota reservation until process shutdown. Fix by applying a linked, configured total deadline across both SendAsync and ReadBoundedUtf8Async (for OpenAI, Gemini, and Ollama, including tool responses), while preserving caller cancellation semantics. Add a provider-level regression whose handler returns headers immediately but whose content stream never completes or slow-drips; the existing cancellation tests delay before headers and do not exercise this gap. HIGH — extend the open empty-verdict finding beyond contractionsThis converges with the existing Codex thread, but fixing only I'll/we'll and Let's will leave a straightforward injection bypass. For example: None of the three source-signal regexes at LlmCaptureTriageExtractor lines 19–35 and 283–294 matches this explicit human next step. If the model follows the hostile line, strict JSON parsing accepts the empty array, the extractor returns EmptyExtraction (lines 219–239), and the service terminalizes the capture with no proposal (CaptureTriageService lines 172–186). Broaden the independent fail-closed signal/policy to cover common explicit imperatives/task markers as well as contractions, and add this combined task-plus-injection case through the extractor and service. If terminal false-empty suppression outside the narrow regex vocabulary is intentionally accepted, the threat model must state that residual explicitly rather than presenting the current source-signal fallback as the effective review rail. Existing open findings checkedI independently checked the three open Codex threads and the separate correctness/compatibility review. The raw-response/ChatService regression, unsafe deterministic-fallback failure, and schema/runtime parity drift remain unresolved; I am not duplicating them here. Verification and bounded positives
|
Signed-off-by: Chris0Jeky <jeky.tck@gmail.com>
Signed-off-by: Chris0Jeky <jeky.tck@gmail.com>
Signed-off-by: Chris0Jeky <jeky.tck@gmail.com>
Review findings resolved — exact head
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6d90a2b966
ℹ️ 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".
Final independent correctness/compatibility review — exact head
|
Final independent adversarial security/threat-containment review — exact head
|
Signed-off-by: Chris0Jeky <jeky.tck@gmail.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Second/final review-fix evidence at exact signed head
Verification: bounded final-fix review VERIFY; focused 242/242 twice (writer + verifier), full Still owed: exact-head hosted CI and final Codex review. Proposed ADR ratification, wider live/UI/resource acceptance, and merge remain outside this fix batch and are not claimed. |
Final exact-head hosted gate checkpointExact head The requested final Codex pass could not run because the connected review account reported its usage limit; the bounded independent final-fix verifier was clean and the request itself is recorded. This is not merge authorization: the Proposed ADR amendment, wider live/UI/resource acceptance, and maintainer decision remain outstanding. |
Status: IN REVIEW — EXACT-HEAD GATES ACTIVE
The complete second/final review-fix batch is published at exact signed head
ca09aec38e4fa38cee809c6fecc1c971ea5d8b09. All final findings are fixed and mapped; Required CI, CI Extended, CodeQL, and final Codex review are active.Summary
llm-triage.v2with collision-resistant untrusted-data framing and strict task-only output containmentScope and decision boundary
Refs #1323; this is the bounded prompt/output-containment tranche, not the whole issue. It deliberately leaves live-model verification, preview-XSS, consent/egress, image/OCR, and broader artefact-resource acceptance open. PDF decompressed-byte/object-count/single-parse-memory containment remains #1379 / ADR-0048.
The ADR amendment is Proposed pending maintainer ratification, so this PR is ready for technical review but maintainer-held for merge.
Review fixes
95a28a16: restrict raw preservation to the explicit triage mode and enforce a linked headers-plus-body provider deadlinecc825170: broaden conservative empty-verdict signals, sanitize deterministic titles, preserve version-specific v1 validation, and align v2 schema/runtime evidence rules6d90a2b9: reconcile canonical docs, verification commands, and residual-risk wordingca09aec3: close timeout-quota, false-proposal, completed-checklist, U+FEFF, and Unicode-scalar parity findingsVerification
NOT verified
ca09aec3