Skip to content

fix: Synchronize status detection with response completion#62

Merged
haofeif merged 1 commit into
awslabs:mainfrom
anilkmr-a2z:fix/status-completion-sync
Feb 6, 2026
Merged

fix: Synchronize status detection with response completion#62
haofeif merged 1 commit into
awslabs:mainfrom
anilkmr-a2z:fix/status-completion-sync

Conversation

@anilkmr-a2z

Copy link
Copy Markdown
Contributor

Summary

Fixes race condition where status detection returned COMPLETED before the agent's response was actually complete, causing extraction failures when MCP clients polled too early.

Problem

After PR #61 removed end-of-line anchors from prompt patterns to support trailing text, a timing bug was exposed:

  1. Status returned COMPLETED as soon as green arrow > appeared (response started)
  2. MCP client saw COMPLETED and called extraction
  3. Agent still processing: ⠸ Thinking... (no final prompt yet)
  4. Extraction failed: "Incomplete Q CLI response - no final prompt detected after response"

Root cause: Status detection and extraction logic were out of sync. Status said "done" but extraction couldn't succeed yet.

Solution

Synchronized status detection with extraction requirements:

Before:

if re.search(GREEN_ARROW_PATTERN, clean_output, re.MULTILINE):
    return TerminalStatus.COMPLETED  # Too early!

After:

green_arrows = list(re.finditer(GREEN_ARROW_PATTERN, clean_output, re.MULTILINE))
if green_arrows:
    last_arrow_pos = green_arrows[-1].end()
    idle_prompts = list(re.finditer(self._idle_prompt_pattern, clean_output))

    for prompt in idle_prompts:
        if prompt.start() > last_arrow_pos:
            return TerminalStatus.COMPLETED  # Only when extraction will succeed

    return TerminalStatus.PROCESSING  # Still working

Key insight: Status COMPLETED now guarantees extraction will succeed.

Changes

  • Modified get_status() in q_cli and kiro_cli providers
  • Status returns PROCESSING if response started but no final prompt yet
  • Status returns COMPLETED only when idle prompt appears AFTER green arrow
  • Added 10 comprehensive test cases (5 per provider)
  • Tests verify synchronization between status and extraction

Testing

✅ All 100 tests passing (50 Q CLI + 50 Kiro CLI)
✅ 99% code coverage for both providers
✅ Black formatting applied
✅ isort checks passed

New test scenarios:

  • Status PROCESSING when response started but incomplete
  • Status COMPLETED when prompt appears after response
  • Extraction succeeds when status is COMPLETED
  • Multiple prompts in buffer (edge case)
  • Synchronization guarantee validation

Impact

  • Eliminates "Incomplete Q CLI response" errors during handoffs
  • MCP clients now wait for actual completion before extraction
  • No breaking changes - only fixes timing bug
  • Applies to both Q CLI and Kiro CLI providers

Why Old Code Worked

The old prompt pattern with $ anchor didn't match prompts with trailing text, so it used stale prompts from previous interactions in the terminal buffer. This accidentally worked but was unreliable. The new code correctly identifies prompts but requires
proper completion detection.

🤖 Assisted by Amazon Q Developer

Fixes race condition where status returned COMPLETED before response was
actually complete, causing extraction failures when MCP client polled too
early.

Changes:
- Modified get_status() in q_cli and kiro_cli providers to check for idle
  prompt AFTER green arrow before returning COMPLETED
- Status now returns PROCESSING if response started but no final prompt yet
- Ensures status COMPLETED guarantees extraction will succeed
- Added comprehensive test cases for status detection edge cases
- Tests verify synchronization between status and extraction logic

The fix prevents 'Incomplete Q CLI response' errors by ensuring MCP clients
only attempt extraction when the agent has finished responding and the final
prompt has appeared.

Resolves issue where removing end-of-line anchor from prompt pattern exposed
timing bug in status detection. Old code used stale prompts from terminal
buffer; new code correctly identifies prompts but requires proper completion
detection.

🤖 Assisted by Amazon Q Developer (https://aws.amazon.com/q/developer)
@codecov-commenter

codecov-commenter commented Feb 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (main@484fd46). Learn more about missing BASE report.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #62   +/-   ##
=======================================
  Coverage        ?   20.17%           
=======================================
  Files           ?       30           
  Lines           ?     1403           
  Branches        ?        0           
=======================================
  Hits            ?      283           
  Misses          ?     1120           
  Partials        ?        0           
Flag Coverage Δ
unittests 20.17% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@haofeif haofeif self-requested a review February 6, 2026 05:57

@haofeif haofeif left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@haofeif haofeif merged commit 5302bc7 into awslabs:main Feb 6, 2026
9 checks passed
haofeif added a commit that referenced this pull request Mar 10, 2026
…104)

Claude Code renders inline (not alternate screen), so old spinner text
can persist in tmux scrollback after cursor-up overwrites fail to reach
it.  get_status() previously checked PROCESSING_PATTERN before
IDLE_PROMPT_PATTERN across the entire 200-line capture buffer, causing
stale spinners to return PROCESSING when the terminal was actually idle.

Use position-based comparison: find the last occurrence of both patterns
and let the more recent one decide the current state.  This matches the
approach already used by Q CLI and Kiro CLI providers (PR #62).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
gutosantos82 pushed a commit that referenced this pull request Apr 15, 2026
…104) (#106)

Claude Code renders inline (not alternate screen), so old spinner text
can persist in tmux scrollback after cursor-up overwrites fail to reach
it.  get_status() previously checked PROCESSING_PATTERN before
IDLE_PROMPT_PATTERN across the entire 200-line capture buffer, causing
stale spinners to return PROCESSING when the terminal was actually idle.

Use position-based comparison: find the last occurrence of both patterns
and let the more recent one decide the current state.  This matches the
approach already used by Q CLI and Kiro CLI providers (PR #62).

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
erikmackinnon pushed a commit to erikmackinnon/cli-agent-orchestrator that referenced this pull request Apr 18, 2026
…wslabs#104) (awslabs#106)

Claude Code renders inline (not alternate screen), so old spinner text
can persist in tmux scrollback after cursor-up overwrites fail to reach
it.  get_status() previously checked PROCESSING_PATTERN before
IDLE_PROMPT_PATTERN across the entire 200-line capture buffer, causing
stale spinners to return PROCESSING when the terminal was actually idle.

Use position-based comparison: find the last occurrence of both patterns
and let the more recent one decide the current state.  This matches the
approach already used by Q CLI and Kiro CLI providers (PR awslabs#62).

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
(cherry picked from commit 0344496)
haofeif added a commit that referenced this pull request May 2, 2026
… rebuild

Previous hardening commit wrote sanitisers that CodeQL didn't recognise as
taint-kills because the checks sat *after* Path() construction and
requests.get() received the caller-controlled source string.

- _SAFE_URL_PATH_RE validates parsed.path *before* the fetch; the URL handed
  to requests.get() is rebuilt as f"https://{safe_host}{parsed.path}" where
  safe_host is pulled from the allowlist literal. Reject query/fragment/
  userinfo which have no place in a static .md fetch.
- _FILE_PATH_RE validates the source string *before* Path(source).resolve()
  and Path(source).exists() — the fullmatch regex sits on the data-flow
  edge into each Path() sink.
- Add a CodeQL job to ci.yml (python + js/ts, security-and-quality suite)
  so future SSRF/path-injection regressions fail CI instead of trickling
  in as post-merge alerts.
- Add scripts/security-scan.sh for local trivy + codeql runs mirroring CI.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
haofeif added a commit that referenced this pull request May 2, 2026
#226)

* fix(install): harden agent-profile install against SSRF and path injection

Closes CodeQL py/full-ssrf and py/path-injection alerts on the install
path added in #166.

- URL downloads restricted to https:// with a host allowlist
  (github.com, raw.githubusercontent.com by default; extend via
  CAO_PROFILE_ALLOWED_HOSTS env var).
- Redirects disabled; explicit is_redirect rejection.
- (5, 30)s connect/read timeout to bound worker exposure.
- Filename / profile-name regex [A-Za-z0-9_-]{1,64} on every sink.
- New allow_file_source kwarg on install_agent(); HTTP API and
  (transitively) ops-MCP install_profile pass False so remote callers
  cannot coerce the server into reading arbitrary local files.

CLI behaviour unchanged.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* fix(install): close CodeQL #60/#61/#62 with pre-Path validation + URL rebuild

Previous hardening commit wrote sanitisers that CodeQL didn't recognise as
taint-kills because the checks sat *after* Path() construction and
requests.get() received the caller-controlled source string.

- _SAFE_URL_PATH_RE validates parsed.path *before* the fetch; the URL handed
  to requests.get() is rebuilt as f"https://{safe_host}{parsed.path}" where
  safe_host is pulled from the allowlist literal. Reject query/fragment/
  userinfo which have no place in a static .md fetch.
- _FILE_PATH_RE validates the source string *before* Path(source).resolve()
  and Path(source).exists() — the fullmatch regex sits on the data-flow
  edge into each Path() sink.
- Add a CodeQL job to ci.yml (python + js/ts, security-and-quality suite)
  so future SSRF/path-injection regressions fail CI instead of trickling
  in as post-merge alerts.
- Add scripts/security-scan.sh for local trivy + codeql runs mirroring CI.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* fix(install): close CodeQL #63 + drop conflicting workflow CodeQL job

Two follow-ups to the previous hardening commit:

1. Alert #63 (py/path-injection, install_service.py:235)
   The `elif allow_file_source and _FILE_PATH_RE.fullmatch(source)
   and Path(source).exists()` guard still tripped the scanner because
   CodeQL doesn't thread the regex sanitiser through the compound
   boolean into the Path() call. Fix: dispatch by pure string suffix
   (`source.endswith(".md")`) — no Path() in install_agent() at all.
   All path construction happens inside _download_agent(), which already
   regex-validates before `.resolve()`.

2. The workflow-based `codeql` job conflicted with the repo's existing
   default-setup CodeQL ("CodeQL analyses from advanced configurations
   cannot be processed when the default setup is enabled"). Dropped the
   job and left a comment in ci.yml explaining why; default setup already
   runs the Analyze (python) / Analyze (js-ts) checks on every PR.

3. SECURITY.md — documented CodeQL coverage, the host allowlist behaviour
   (`CAO_PROFILE_ALLOWED_HOSTS`), and the scripts/security-scan.sh wrapper.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* fix(install): reject `..` segments in _FILE_PATH_RE (closes CodeQL #61)

The previous regex used a character class that included `.` and `/`, so
`../../etc/passwd.md` matched and passed into `Path(source).resolve()`.
CodeQL was right to flag it — the sanitiser was weaker than advertised.

- Add a leading negative lookahead `(?!.*\.\.)` to the file-path regex so
  any `..` anywhere in the string rejects the source before Path() is
  constructed. Legitimate `./foo.md`, `/abs/foo.md`, `~/foo.md`, and
  `sub/dir/foo.md` all still work.
- Two new regression tests cover `../../etc/passwd.md` and embedded
  `/tmp/foo/../etc/passwd.md` traversal shapes.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* fix(install): move file-path handling out of install_service (closes CodeQL #64)

Earlier rounds kept `Path(user_input)` reachable inside `install_service` behind
a regex sanitiser. Every regex shape that still admitted a legitimate CLI path
like `./foo.md` also admitted `../../etc/passwd.md` without an unacceptable
normalise+prefix-check — so CodeQL kept correctly flagging the `.resolve()`
sink.

Structural fix: the shared service doesn't need a file-path branch at all.

- `install_service.install_agent()` now accepts only a bare profile name
  (`_PROFILE_NAME_RE`) or an https:// URL on the host allowlist.
- `cli/commands/install.py` grows a `_copy_local_profile_to_store()` helper
  that does the file reading, stem validation, and copy-into-store itself,
  then calls the service with the bare validated stem.
- `api/main.py` drops the `allow_file_source=False` kwarg — the parameter
  is gone; the service refuses filesystem paths for every caller.
- Tests: remove the file-path branches from the service suite, move that
  coverage to the CLI suite (`TestCopyLocalProfileToStore` + integration
  tests on file-source `cao install` invocations).

Full test suite (`test/ --ignore=test/e2e -m "not integration"`): 1581/1581
pass. End-to-end smoke of `cao install /tmp/smoke-agent.md --provider kiro_cli`
verified.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* style: black reformat test_install.py (extra blank line)

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants