feat: accept unique job id prefixes#6
Open
Boulea7 wants to merge 5 commits into
Open
Conversation
Change-Id: I5aa2b30645932d59288950d0e43cad788b43ff99
Reviewer's GuideAdds support for resolving job metadata tools by unique job_id prefixes, centralizing resolution/validation in SessionStore and server helpers, and documents prefix behavior across README variants and examples. Sequence diagram for resolving job_id prefixes in metadata toolssequenceDiagram
actor Client
participant agy_status_tool
participant _resolve_job_id_reference
participant SessionStore
participant Supervisor
Client->>agy_status_tool: agy_status_tool(job_id)
agy_status_tool->>_resolve_job_id_reference: _resolve_job_id_reference(safety, store, job_id)
_resolve_job_id_reference->>SessionStore: resolve_job_reference(reference)
alt exact id exists
SessionStore->>SessionStore: get_job(reference)
SessionStore-->>_resolve_job_id_reference: JobRecord
else unique prefix
SessionStore->>SessionStore: list_jobs(limit=None)
SessionStore-->>_resolve_job_id_reference: JobRecord
else no match
SessionStore-->>_resolve_job_id_reference: None
else ambiguous prefix
SessionStore--x _resolve_job_id_reference: ValueError("job_id reference is ambiguous ...")
end
alt resolved job_id
_resolve_job_id_reference-->>agy_status_tool: (job_id, None)
agy_status_tool->>Supervisor: status(job_id)
Supervisor-->>agy_status_tool: JobRecord | None
agy_status_tool-->>Client: StatusToolResponse
else error
_resolve_job_id_reference-->>agy_status_tool: (None, error)
agy_status_tool-->>Client: StatusToolResponse(success=False)
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughThis PR adds job-id prefix resolution to the MCP server, enabling users to reference long-running jobs by either their full ID or a unique prefix. The implementation adds a ChangesJob ID Prefix Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The job-id regexes are now duplicated between
server.pyandsession_store.py(_JOB_ID_PATTERN/_JOB_ID_REFERENCE_PATTERNvs_JOB_ID_RE/_JOB_ID_PREFIX_RE); consider centralizing these patterns (or at least their definitions) to avoid drift between layers. SessionStore.resolve_job_referencecurrently callslist_jobs(limit=None)and then filters in Python; if stores can grow large, you may want a more targeted lookup (e.g., early exit after finding >1 match or an optimized listing) to avoid scanning the full job set for every prefix resolution.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The job-id regexes are now duplicated between `server.py` and `session_store.py` (`_JOB_ID_PATTERN` / `_JOB_ID_REFERENCE_PATTERN` vs `_JOB_ID_RE` / `_JOB_ID_PREFIX_RE`); consider centralizing these patterns (or at least their definitions) to avoid drift between layers.
- `SessionStore.resolve_job_reference` currently calls `list_jobs(limit=None)` and then filters in Python; if stores can grow large, you may want a more targeted lookup (e.g., early exit after finding >1 match or an optimized listing) to avoid scanning the full job set for every prefix resolution.
## Individual Comments
### Comment 1
<location path="src/agy_mcp/session_store.py" line_range="41" />
<code_context>
# anything that could traverse out of the store root. Generated ids satisfy
# this regex (see generate_job_id).
_JOB_ID_RE = re.compile(r"^job_[A-Za-z0-9_-]{1,80}$")
+_JOB_ID_PREFIX_RE = re.compile(r"^job_[A-Za-z0-9_-]{0,80}$")
</code_context>
<issue_to_address>
**suggestion:** Consider disallowing the empty string as a job reference at the store level for consistency and to avoid pathological prefixes.
The server-side `_validate_job_id_reference` rejects empty references, but `_JOB_ID_PREFIX_RE` allows them via `{0,80}`. If `resolve_job_reference` is called with `""`, `record.job_id.startswith(reference)` will match every job, likely causing an unnecessary full scan and an "ambiguous" error. To align behavior and avoid this edge case, either change the prefix regex to `{1,80}` or add an explicit `if not reference: raise ValueError(...)` in `_validate_job_reference`.
```suggestion
_JOB_ID_PREFIX_RE = re.compile(r"^job_[A-Za-z0-9_-]{1,80}$")
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Pull request overview
This PR adds support for addressing background jobs by either a full job_id or a unique job_id prefix across the MCP metadata tools, and documents that behavior across user-facing READMEs and skill usage docs.
Changes:
- Add
SessionStore.resolve_job_reference()to resolve exact IDs or unique prefixes, with explicit ambiguity errors. - Update
agy_status,agy_read,agy_result, andagy_cancelto accept and resolve uniquejob_idprefixes. - Add tests for unique/ambiguous prefix handling and update documentation to describe the new behavior.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/agy_mcp/session_store.py |
Adds job-id prefix validation and resolve_job_reference() to map prefixes to stored jobs. |
src/agy_mcp/server.py |
Adds server-side validation + prefix resolution plumbing for job metadata tools. |
tests/test_session_store.py |
Adds unit tests for exact and ambiguous prefix resolution in SessionStore. |
tests/test_mcp_server.py |
Adds integration tests ensuring tools accept unique prefixes and reject ambiguous ones. |
src/agy_mcp/_skill_bodies/claude/references/usage.md |
Documents prefix support and ambiguity behavior for metadata tools. |
README.md |
Documents prefix support for job metadata tools (ZH). |
docs/README_ZH-TW.md |
Documents prefix support for job metadata tools (ZH-TW). |
docs/README_JA.md |
Documents prefix support for job metadata tools (JA). |
docs/README_EN.md |
Documents prefix support for job metadata tools (EN). |
docs/examples.md |
Adds examples/notes describing prefix support and ambiguity errors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # anything that could traverse out of the store root. Generated ids satisfy | ||
| # this regex (see generate_job_id). | ||
| _JOB_ID_RE = re.compile(r"^job_[A-Za-z0-9_-]{1,80}$") | ||
| _JOB_ID_PREFIX_RE = re.compile(r"^job_[A-Za-z0-9_-]{0,80}$") |
Comment on lines
+356
to
+363
| matches = [ | ||
| record for record in self.list_jobs(limit=None) | ||
| if record.job_id.startswith(reference) | ||
| ] | ||
| if len(matches) > 1: | ||
| raise ValueError("job_id reference is ambiguous; pass a longer job_id") | ||
| return matches[0] if matches else None | ||
|
|
| # (Phase 5 R3 security P2). | ||
| _MAX_JOB_ID_LEN = 84 | ||
| _JOB_ID_PATTERN = re.compile(r"^job_[A-Za-z0-9_-]{1,80}$") | ||
| _JOB_ID_REFERENCE_PATTERN = re.compile(r"^job_[A-Za-z0-9_-]{0,80}$") |
| try: | ||
| record = store.resolve_job_reference(reference) | ||
| except ValueError as exc: | ||
| return None, safety.redact(str(exc)) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/agy_mcp/_skill_bodies/claude/references/usage.md`:
- Around line 68-71: The packaged copy of
src/agy_mcp/_skill_bodies/claude/references/usage.md diverges from the source;
check for duplicate copies and packaging steps that copy or transform this file
by searching the repo for other usage.md instances and references to usage.md in
build scripts (as suggested by the provided shell commands), compare the
contents of any matches to the source file, and update either the packaging
script or the out-of-source copy so the packaged copy matches the canonical
file; ensure any install/build step (e.g., a packaging/prepare script, MANIFEST,
setup.cfg/pyproject.toml include/exclude rules) references the correct path and
that CI uses the same source before committing the synchronized change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ce157d40-e26a-4610-90a3-b432b47e81b2
📒 Files selected for processing (10)
README.mddocs/README_EN.mddocs/README_JA.mddocs/README_ZH-TW.mddocs/examples.mdsrc/agy_mcp/_skill_bodies/claude/references/usage.mdsrc/agy_mcp/server.pysrc/agy_mcp/session_store.pytests/test_mcp_server.pytests/test_session_store.py
Comment on lines
+68
to
+71
| The metadata tools accept a full `job_id` or a unique prefix. For example, | ||
| `agy_status("job_177986")` resolves to the matching stored job when exactly | ||
| one id starts with that prefix; ambiguous prefixes return `success=false` | ||
| with an explicit ambiguity error. |
There was a problem hiding this comment.
Investigate the pipeline drift test failure.
The CI pipeline reports a content mismatch between the source file and the packaged copy. The drift test indicates "additional/changed paragraph about job_id handling present in packaged copy." This suggests the build or packaging step may not be syncing the latest documentation correctly.
Run the following script to check if there are multiple copies of this file that need to be synchronized:
#!/bin/bash
# Find all copies of usage.md in the repository
fd -t f 'usage.md' --exec echo "Found: {}"
# Check if there's a build/packaging script that copies this file
rg -n 'usage\.md' --type=python --type=toml --type=yaml -C 3🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/agy_mcp/_skill_bodies/claude/references/usage.md` around lines 68 - 71,
The packaged copy of src/agy_mcp/_skill_bodies/claude/references/usage.md
diverges from the source; check for duplicate copies and packaging steps that
copy or transform this file by searching the repo for other usage.md instances
and references to usage.md in build scripts (as suggested by the provided shell
commands), compare the contents of any matches to the source file, and update
either the packaging script or the out-of-source copy so the packaged copy
matches the canonical file; ensure any install/build step (e.g., a
packaging/prepare script, MANIFEST, setup.cfg/pyproject.toml include/exclude
rules) references the correct path and that CI uses the same source before
committing the synchronized change.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 10 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Change-Id: Ie26683c8529026a4471b36da6cca7d5c105b2717
Change-Id: I2732d7c75f71c5c6dcfbd1ef3ecd1c959a24858d
Change-Id: I17c43d41da0dbaca7b9c79b14ca2ffb84526689a
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Change-Id: I7b60e129caaa8bbb44e5dc531eee1586adf446c8
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
Reference: inspired by gemini-plugin-cc/codex-plugin-cc status/result/cancel workflows, where short job references make result lookup less fragile.
Summary by Sourcery
Allow job metadata tools to accept full job IDs or unique prefixes and document this behavior across server, session store, and user-facing docs.
New Features:
Enhancements:
Documentation:
Tests:
Summary by cubic
Allow unique
job_idprefixes inagy_status,agy_read,agy_result, andagy_cancelto make lookup easier, with stricter validation, clearer errors, and optimized resolution that short-circuits exact IDs and avoids scanning metadata. Synced docs across READMEs, examples, and both Claude and Codex skill usage to explain prefix support.New Features
job_idor a unique prefix (e.g.,job_177986); ambiguous prefixes returnsuccess=falsewith a clear ambiguity error.SessionStorewith an optimized lookup; tests cover unique, ambiguous, bare-prefix, and lookup failure cases.Bug Fixes
job_), simplified fallback that preserves exact-id not-found behavior, and structured, redacted errors when reference lookup fails.Written for commit 43e96a1. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
New Features
agy_status,agy_read,agy_result,agy_cancel) now accept job IDs as either full identifiers or unique prefixes for convenience.Documentation