Skip to content

feat(cli): add --conversation-id global flag (#716) - #718

Merged
padak merged 2 commits into
mainfrom
claude/issue-716-conversation-id-flag
Aug 30, 2026
Merged

feat(cli): add --conversation-id global flag (#716)#718
padak merged 2 commits into
mainfrom
claude/issue-716-conversation-id-flag

Conversation

@padak

@padak padak commented Aug 29, 2026

Copy link
Copy Markdown
Member

What

skills/kbagent/SKILL.md rule 2 instructs the agent to set the observability conversation ID with a
one-time export KBAGENT_CONVERSATION_ID="<unique-id>". That instruction cannot be satisfied in an
agent harness.
Claude Code's Bash tool persists the working directory but not environment
variables, so the export is gone by the next tool call, and there was no flag alternative
(kbagent --help | grep -c conversation → 0).

The only way to comply was therefore to re-prepend the export to every command. Because Claude Code
permission allow-rules are prefix matches on the command string, a command starting with
export ... can never match Bash(kbagent --json workspace query:*) — so kbagent became impossible
to allowlist, every invocation fell through to the safety classifier, and a rule intended to
improve observability degraded the agent experience instead.

This PR implements option 1 from the issue (the flag) and option 2 (the doc correction).

How

A --conversation-id global option on the Typer callback, published into the environment rather
than threaded through the Typer context — that is where every consumer already reads it:

  • BaseHttpClient.__init__ stamps X-Conversation-ID from os.environ for all seven HTTP clients;
  • doctor's conversation_id check reports on it;
  • scheduled-agent / cli_command subprocesses inherit it for free;
  • serve already sets it exactly this way (commands/serve.py:271).

Precedence: an explicit flag beats an inherited KBAGENT_CONVERSATION_ID (it is the more specific
instruction). With neither set, the header stays omitted, exactly as before — this is purely
additive.

Not done here

Options 3 and 4 from the issue are product decisions rather than bug fixes, so they are deliberately
left out:

  • Auto-generating an ID when unset changes what the platform's observability data looks like
    (every unset invocation would mint a fresh unique ID), which is a call for whoever consumes that
    telemetry.
  • Shipping recommended allow-rules with the plugin is a plugin-config decision about which verbs
    are safe to pre-approve on someone else's machine.

How it was tested

New TestConversationIdFlag in tests/test_cli.py (5 cases): flag sets the ID, flag wins over the
env var, env var still honoured without the flag, neither set leaves it unset, and — the one that
matters — the flag actually reaches the outgoing X-Conversation-ID header on a constructed client,
not just the environment.

Also verified end-to-end out-of-process: kbagent --json --conversation-id kbagent-test-4242 doctor
leaves the env set and a subsequently constructed client carries
x-conversation-id: kbagent-test-4242.

make check exits 0.

Doc surfaces

Every surface that carried the unsatisfiable instruction now states that a standalone export does
not survive between tool calls, and points at the flag (or the harness's own env block for a whole
session):

  • plugins/kbagent/skills/kbagent/SKILL.md rule 2 — the line the issue quotes.
  • plugins/kbagent/skills/kbagent/references/gotchas.md "Conversation ID" — rewritten, tagged
    (since vNEXT, #716).
  • src/keboola_agent_cli/commands/context.py AGENT_CONTEXT — both the "IMPORTANT: Set Conversation
    ID" block and the env-var table.
  • CLAUDE.md global-options line.

One process note worth flagging: a (since vNEXT) gate cannot be written on the # comment
lines inside CLAUDE.md's ## All CLI Commands fenced block — scripts/check_version_gates.py parses
them as ATX markdown headings, and a placeholder in a heading is always fatal. The version gate for
this entry therefore lives in gotchas.md, with a comment in CLAUDE.md explaining why.

No CLI command was added / renamed / removed (this is a global option), so the command-sync
surfaces are untouched; make command-sync-check and make skill-check are green.

No version bump and no changelog.py entry — that belongs to the release PR.

Fixes #716

padak added 2 commits August 29, 2026 07:21
`skills/kbagent/SKILL.md` rule 2 told the agent to set the observability
conversation ID with a one-time `export KBAGENT_CONVERSATION_ID=...`. That
instruction cannot be satisfied in an agent harness: Claude Code's Bash tool
persists the working directory but not environment variables, so the export
is gone by the next tool call. With no flag alternative, the only way to
comply was to re-prepend the export to every command -- and since permission
allow-rules are prefix matches on the command string, a command starting with
`export ...` can never match `Bash(kbagent --json workspace query:*)`. Every
invocation then fell through to the safety classifier and prompted, so a rule
meant to improve observability degraded the agent experience instead.

Adds a `--conversation-id` global option. The value is published into the
environment in the Typer callback, which is where every consumer already
reads it: `BaseHttpClient.__init__` stamps the header from os.environ for all
seven clients, `doctor` reports on it, and scheduled-agent subprocesses
inherit it. `serve` already sets it the same way. An explicit flag wins over
an inherited env var; with neither, the header stays omitted as before.

Docs corrected on every surface that carried the unsatisfiable instruction
(SKILL.md rule 2, gotchas.md, context.py AGENT_CONTEXT, CLAUDE.md), each now
stating that a standalone `export` does not survive between tool calls and
pointing at the flag, or at the harness's own env block for a whole session.

Not done here: auto-generating an ID when unset, and shipping recommended
allow-rules with the plugin (options 3 and 4 in the issue) -- both are
product decisions rather than bug fixes.

@padak padak left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Independent verification review (issue #716)

Re-verified the issue and the fix from a clean worktree, independently of the session that authored this PR.

Issue reproduction — confirmed valid

  • KBAGENT_CONVERSATION_ID is read only from os.environ at client construction (http_base.py:111-113); no flag existed anywhere in the CLI.
  • The one-time-export instruction was present on all three agent-facing surfaces (SKILL.md:36, gotchas.md:2405, context.py AGENT_CONTEXT), and none of them acknowledged that agent harnesses do not persist shell state.
  • The permission-rule consequence is real: Claude Code allow-rules are prefix matches, so export ...; kbagent ... compound commands can never match Bash(kbagent ...:*).

Fix verification

  • Design: publishing the flag into os.environ in the root callback is the right minimal seam — all seven HTTP clients read the env var lazily (after the callback), doctor picks it up, subprocesses inherit it, and serve already respects a pre-set value when auto-generating its own ID, so kbagent --conversation-id X serve pins the serve session for free.
  • Tests: ran tests/test_cli.py::TestConversationIdFlag locally on this branch — 5/5 pass, including the wire-level assertion that the header actually lands on a constructed httpx client. Env hygiene via patch.dict is correct (no leakage between tests).
  • Version gating: verified the CLAUDE.md claim — check_version_gates.py's HEADING_RE (^ {0,3}#{1,6} ) is line-based with no fence awareness, so a vNEXT on the # --conversation-id comment line inside the code block would indeed be fatal on every PR. Keeping the (since vNEXT, #716) gate in gotchas.md only is correct.
  • Doc surfaces (convention #17): context.py, CLAUDE.md, SKILL.md, gotchas.md all updated. keboola-expert.md and commands-reference.md never mentioned conversation ID, so skipping them is right (no drift introduced).
  • Feature-PR rules: no version bump, no changelog.py entry — correct per the release process.

Nits (non-blocking)

  1. The four env-propagation tests discard the CLI result without asserting exit code 0. If project list ever started failing in the callback path, these tests would keep passing on a half-executed command. One assert result.exit_code == 0 per test would pin that down.
  2. Undocumented edge: an explicit empty flag (--conversation-id "") overwrites a set env var with "", which suppresses the header entirely. Harmless (and arguably a feature — an explicit opt-out), but worth a sentence in gotchas.md if anyone ever hits it.

Verdict

Fix is correct, minimal, and complete for options 1+2 of the issue; deferring options 3 (auto-generated fallback ID) and 4 (shipped allow-rules) as product decisions is the right call. Ready to merge from my side.

@padak
padak marked this pull request as ready for review August 30, 2026 11:16
@padak
padak merged commit a985249 into main Aug 30, 2026
4 checks passed
@padak
padak deleted the claude/issue-716-conversation-id-flag branch August 30, 2026 11:18
padak added a commit that referenced this pull request Aug 30, 2026
Batches the seven PRs merged since v0.91.0 into one version bump, one
changelog entry and one set of resolved version gates:

- #719 (#714) `flow triggers` -- table triggers, not just cron
- #717 (#711) a 401 is no longer automatically blamed on the token
- #722 (#704) setup completes in chat; skill covers setup + logout
- #718 (#716) `--conversation-id` global flag
- #706 223 stale version gates retired at the 0.80.0 floor
- #702 release process enforced rather than remembered
- #721 `get_flow_detail` docstring fix

Includes a curated What's-new entry (#717's error rework is UI-visible on
the Semantic Layer page) and the step 8-11 silent-drift review.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant