Skip to content

fix(qoder): match underscore workspace keys - #170

Open
chiga0 wants to merge 1 commit into
alibaba:mainfrom
chiga0:feat/qoder-session-workspace-key
Open

fix(qoder): match underscore workspace keys#170
chiga0 wants to merge 1 commit into
alibaba:mainfrom
chiga0:feat/qoder-session-workspace-key

Conversation

@chiga0

@chiga0 chiga0 commented Aug 4, 2026

Copy link
Copy Markdown

Summary

  • normalize underscores in Qoder project workspace keys
  • keep the transform agent-specific so other session lookup behavior is unchanged
  • add coverage for slash and underscore path encoding

Validation

  • make fmt
  • make verify
  • env -u QODER_PERSONAL_ACCESS_TOKEN make test
  • real Qoder evaluation now resolves the native JSONL transcript; DataAgent base eval: 12/12 PASS, real integration eval: 16/16 PASS

@chiga0
chiga0 requested a review from hittyt as a code owner August 4, 2026 10:30
@CLAassistant

Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


秦奇 seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

findExtra string
// workspaceKeyTransform applies agent-specific path encoding after the
// shared runtime workspace key has been normalized.
workspaceKeyTransform func(string) string

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

🟠 Major: Make workspace-key encoding fully agent-owned

workspaceKeyTransform func(string) string only post-processes workspaceKeyForRuntime, but that shared result already embeds Claude-specific path semantics. As a result, the new abstraction is still incomplete:

  • Qoder replaces more than underscores. Existing Qoder project directories encode punctuation such as .codex with -, while this patch preserves the dot on POSIX.
  • Qwen uses the same shared lookup without a transform, although upstream sanitizeCwd replaces every non-alphanumeric character with - and lowercases Windows paths. An underscore-containing Qwen workspace therefore has the same silent session-lookup/token-accounting failure class.

Please make the lookup accept a complete agent-owned encoder, e.g. type workspaceKeyEncoder func(Runtime) string, and require Claude, Qoder, and Qwen to each provide their full key implementation. The shared layer should continue to own session discovery, Windows cygpath conversion, download, and parsing; shared path preparation may remain a helper.

Please include table-driven coverage for _, ., :, Windows slash styles and 8.3 paths, plus Qwen Windows casing, and exercise the complete Qoder/Qwen runtime cwd -> key -> session lookup path. A real Windows canary can be supplied by maintainers if the contributor has no Windows environment.

Qwen reference: https://github.com/QwenLM/qwen-code/blob/7edc16ba11e1c4c3892c4f2664bd61d48cbf3cde/packages/core/src/utils/paths.ts


Source: manual evidence review @ b8b7911, cross-checked against the prior Claude Windows session recovery and Qwen upstream storage code.

@zpzjzj zpzjzj left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-reviewed b8b7911 together with the earlier Claude Windows session recovery. The observed Qoder underscore fix is valid and the local/remote test gates are green, but I left one focused inline request to make workspace-key encoding a complete agent-owned contract rather than a post-transform on Claude-oriented normalization. The same contract should cover Qwen, whose upstream storage encoding has the same punctuation behavior plus Windows lowercasing.

One additional repository requirement remains: please add a concise Unreleased / Fixed entry to CHANGELOG.md. This is user-visible because affected Qoder/Qwen runs can otherwise complete grading while silently losing the native transcript and token accounting.

Expected contributor scope for this PR:

  • implement the agent-owned encoder contract and wire Claude/Qoder/Qwen explicitly;
  • add focused table-driven and lookup-path tests;
  • update the changelog and resolve the CLA identity/signing issue.

A live Windows canary is useful but does not need to be supplied by the external contributor if no Windows environment is available; maintainers can provide that final verification.

Current status: CI, E2E, Lint, and CodeQL pass; CLA is still pending. Overall: ⚠️ conditionally mergeable after the inline design item, changelog, and CLA are addressed.

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