fix(qodercli): keep child tools on the evaluation session - #219
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7d0effe58f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| for _, key := range []string{ | ||
| "CODEX_THREAD_ID", | ||
| "CLAUDE_CODE_SESSION_ID", | ||
| "QODERWORK_SOURCE_CHAT_ID", | ||
| } { |
There was a problem hiding this comment.
Clear the inactive Qoder edition's session variable
When skill-up is launched beneath the opposite Qoder edition—for example, a CN evaluation started from global qodercli—the inherited QODER_SESSION_ID remains set while this code adds QODERCN_SESSION_ID (and vice versa). NoneRuntime begins with os.Environ() and overlays these entries, so both exact identities reach Qoder and its Bash subprocesses, recreating the host-identity conflict this change is intended to prevent. Clear the inactive profile's session variable alongside the other inherited host identities.
Useful? React with 👍 / 👎.
| envVars := a.qoderRunEnvVars(sessionID) | ||
| opts = a.mergeExecOptionsEnv(ctx, opts, envVars, a.buildAgentObservabilityAttrs(nil)) | ||
| a.pinQoderSessionEnv(&opts, sessionID) |
There was a problem hiding this comment.
Reject a different ID on resumed turns
When a resumed -r invocation reports a different session_id—for example, if Qoder starts a replacement session because the requested one is unavailable—buildSessionResult captures that ID, but the following assignment silently overwrites it with the requested ID. The evaluator then sends later turns to the old session while grading artifacts from the replacement session. Apply the same mismatch validation used by Run before retaining the resumed ID.
Useful? React with 👍 / 👎.
7d0effe to
f30cdf9
Compare
JHWang-1997
left a comment
There was a problem hiding this comment.
The initial-session change is sound, but the resumed-turn path introduces a deterministic Qoder CLI argument conflict.
RunTurn builds qodercli -r <sessionID> and then calls qoderRunEnvVars(sessionID) / pinQoderSessionEnv, which also exports QODER_SESSION_ID=<sessionID> (or QODERCN_SESSION_ID for CN). Qoder maps that environment variable to its --session-id option, so the effective invocation combines --resume and --session-id without --fork-session.
Qoder CLI rejects that combination before authentication or model execution. Reproduction with the official 1.1.27 package:
QODER_SESSION_ID=11111111-1111-4111-8111-111111111111 \
qodercli \
--resume 11111111-1111-4111-8111-111111111111 \
--print --output-format json testOutput:
--session-id can only be used with --continue or --resume when --fork-session is also specified.
The process exits with code 42. I reproduced the same behavior with Qoder CLI 1.1.29. Consequently, the first turn can succeed, but every subsequent RunTurn fails at argument validation. The current fake runtime accepts the combination, so TestQoderCLIRunTurn_ResumeUsesCorrectFlag does not expose the incompatibility.
Please avoid exporting the Qoder session environment variable on the resume path and use only -r <sessionID>, unless Qoder adds a separate child-process propagation mechanism that does not participate in CLI session selection. A parser-level or integration contract test for this combination would also prevent regression.
|
Addressed the resume-path conflict in the latest revision:
Validation: |
f30cdf9 to
7ffddb8
Compare
|
Thanks for addressing the resume-path conflict. I checked the latest revision, and clearing both session environment variables before invoking Qoder with There is just one behavior I would like to clarify before resolving the review. The PR description currently says that resumed turns and their child tools observe the same session identity, while the updated resume path intentionally removes the session variables from Qoder's inherited environment. If Qoder re-populates the restored session ID for child processes after handling To be clear, I am not suggesting restoring |
7ffddb8 to
6d314db
Compare
Problem
The built-in Qoder engine starts an evaluation, parses Qoder's returned
session_id, and later uses that value for--resume. However, the session isonly learned after the Qoder process exits.
With qodercli 1.1.27,
--session-id <id>selects the Qoder conversation but doesnot backfill
QODER_SESSION_IDinto Bash/tool subprocesses. If the variable wasalready present in the launcher environment, those subprocesses do inherit it.
This creates two generic failure modes for an evaluation harness:
traces with the Qoder conversation that invoked them; and
QODER_SESSION_IDcan disagree with Qoder's newlygenerated session, so the parent result and child tools identify different
conversations.
Minimal observation:
The CN binary has the same launcher contract through
QODERCN_SESSION_ID.Change
Run.QODER_SESSION_ID/QODERCN_SESSION_IDafter configured and runtimeenvironment layers are merged, so stale values cannot override the run.
session_idto agree with the launcher-assigned value.RunTurn.This makes skill-up the owner of the lifecycle it already orchestrates: one
evaluation run has one Qoder session identity, and resumed turns plus child tools
observe that same identity.
Scope and compatibility
by the launcher instead of being learned after process exit. They remain normal
UUID session IDs and continue to be returned in
SessionResult.Verification
make verifymake testprevention, Qoder CN mapping, returned-ID agreement, and resume continuity.
session_idand the value visible to its Bash subprocess.