Add GitHub Issues storage backend for plans MCP - #1023
Conversation
📝 WalkthroughWalkthroughAdds a reusable plans/tasks/notes storage core and generic MCP server, then implements a GitHub Issues-backed backend with authentication, rate limiting, encoding, configuration, runtime serving, tests, documentation, and release packaging for a new binary. ChangesShared PlanStore core
GitHub Issues-backed backend
Workspace and release integration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Tick the box to add this pull request to the merge queue (same as
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (9)
docs/solutions/integration-issues/github-issues-storage-backend-2026-07-08.md-200-203 (1)
200-203: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winLabel the fenced example as TOML.
Markdownlint flags this block as an unlabeled fence (MD040). Since the snippet is TOML, the opening fence should specify
toml.Suggested fix
-``` +```toml # Cargo.toml jsonwebtoken = { version = "10", features = ["aws_lc_rs", "use_pem"] }</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@docs/solutions/integration-issues/github-issues-storage-backend-2026-07-08.md
around lines 200 - 203, The fenced example in the markdown note is unlabeled and
should be marked as TOML to satisfy markdownlint. Update the code block around
the Cargo.toml snippet to use a TOML fence, keeping the content unchanged; this
should be done in the docs markdown where the example appears.</details> <!-- cr-comment:v1:21882a823715c76ec1b75cab --> _Source: Linters/SAST tools_ </blockquote></details> <details> <summary>crates/harnx-mcp-plans-github/src/ratelimit.rs-110-136 (1)</summary><blockquote> `110-136`: _🎯 Functional Correctness_ | _🟡 Minor_ | _⚡ Quick win_ **Transient backoff granularity is lost, defeating the 250 ms base.** `transient_backoff` produces sub-second durations (base 250 ms), but `sleep_or_rate_limited` only accepts whole seconds and the call at Line 117 does `wait.as_secs().max(1)`. So the first retries (250 ms, 500 ms) all round to `1s`, and the configured `base_transient_backoff` has no observable effect below ~1 s. Consider passing a `Duration` for the transient path (keeping the whole-second cap comparison against `max_wait_secs`). <details> <summary>♻️ Sketch: sleep by Duration for transient path</summary> ```diff - if is_transient(status) && transient_retries < self.config.max_transient_retries - { - let wait = transient_backoff( - self.config.base_transient_backoff, - transient_retries, - ); - transient_retries += 1; - self.sleep_or_rate_limited(wait.as_secs().max(1)).await?; - continue; - } + if is_transient(status) && transient_retries < self.config.max_transient_retries + { + let wait = transient_backoff( + self.config.base_transient_backoff, + transient_retries, + ); + transient_retries += 1; + if wait.as_secs() > self.config.max_wait_secs { + return Err(StoreError::RateLimited { + retry_after_secs: wait.as_secs().max(1), + } + .into()); + } + self.sleeper.sleep(wait).await; + continue; + }🤖 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 `@crates/harnx-mcp-plans-github/src/ratelimit.rs` around lines 110 - 136, Transient retries are losing sub-second precision because the transient path in the ratelimit logic rounds `transient_backoff` down to whole seconds before calling `sleep_or_rate_limited`. Update the retry flow in `ratelimit.rs` so `transient_backoff` is passed through as a `Duration` for transient sleeps, while keeping the existing `max_wait_secs` limit check for rate-limited waits. Adjust `sleep_or_rate_limited` (and its call sites in the retry loop) to preserve the 250 ms base and only cap or reject excessive waits, rather than forcing `wait.as_secs().max(1)`.crates/harnx-mcp-plans-github/src/codec.rs-87-122 (1)
87-122: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
TaskFrontMatter::default().statusdiverges from the intended "open" default.
statususes#[serde(default = "default_status")], which only applies to missing-field deserialization — the derivedDefaultimpl (used as the fallback inparse_frontmatteron malformed YAML, line 682) still yieldsstatus: String::default()(i.e.""), not"open". A task with malformed front-matter would silently decode with an empty status instead of the intended default.🐛 Proposed fix
-#[derive(Debug, Clone, Default, Serialize, Deserialize, PartialEq, Eq)] +#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] #[serde(rename_all = "snake_case")] pub struct TaskFrontMatter { ... } + +impl Default for TaskFrontMatter { + fn default() -> Self { + Self { + client_id: None, + jira_key: None, + summary: None, + author: None, + assignee: None, + executor: None, + tags: Vec::new(), + status: default_status(), + dependencies: Vec::new(), + created_at: String::new(), + updated_at: None, + } + } +}Also applies to: 144-146, 666-685
🤖 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 `@crates/harnx-mcp-plans-github/src/codec.rs` around lines 87 - 122, TaskFrontMatter::default() does not match the intended default status, so malformed front-matter can fall back to an empty status instead of "open". Update the Default behavior for TaskFrontMatter (or the fallback path used by parse_frontmatter) so status is initialized via default_status, and make sure the same "open" default is preserved when serde deserialization fails. Verify the struct and the parse_frontmatter fallback both use the same default source for status.crates/harnx-mcp-plans-github/src/store_github.rs-478-503 (1)
478-503: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winThread
jira_keythrough the public plan/task API.add_plan/add_taskalways encode withNone, andPlanMetaUpdate/TaskMetaUpdatedon’t expose ajira_keyfield, so the store can only preserve an existing key from title/front-matter — it can’t set or change one through the API.🤖 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 `@crates/harnx-mcp-plans-github/src/store_github.rs` around lines 478 - 503, The public plan/task API is not carrying jira_key through, so add it to the plan/task metadata types and plumbing used by add_plan and add_task. Update PlanMetaUpdate and TaskMetaUpdate to expose jira_key, pass it into new_plan_to_issue/new_task_to_issue instead of always using None, and make sure the corresponding conversion/build paths preserve and allow updating the key consistently.crates/harnx-mcp-plans-core/src/server/handlers.rs-283-293 (1)
283-293: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAlreadyExists error messages show wrong identifiers.
In
handle_add_task(line 287), theAlreadyExistserror callsdisplay_id(&body)wherebodyis the task body text — not the task ID. The error message would display body content instead of the actual task ID. Inhandle_add_note(line 814), the same error uses a hardcoded empty string""for the note ID, making the message unhelpful when the user supplied a custom ID.In both cases, the ID is consumed by the
NewTask/NewNoteconstructor before the error handler runs. The fix is to extract the display ID before moving the struct into the store call.🐛 Proposed fix for handle_add_task
let body = params.body.unwrap_or_default(); + let task_id = id.unwrap_or_else(|| uuid::Uuid::new_v4().simple().to_string()[..8].to_string()); + let task_id_display = display_id(&task_id); let new_task = NewTask { - id: id.unwrap_or_else(|| uuid::Uuid::new_v4().simple().to_string()[..8].to_string()), + id: task_id, title: params.title, summary: params.summary, author: params.author, assignee: params.assignee, executor: params.executor, tags: params.tags, status: params.status, dependencies: params.dependencies, }; let task = self .store .add_task(&plan_id, new_task) .await .map_err(|err| match err { StoreError::AlreadyExists => ErrorData::invalid_params( format!( "task '{}' already exists in plan '{}'", - display_id(&body), + task_id_display, plan_name ), None, ), other => store_error_to_error_data(other), })?;Proposed fix for handle_add_note
+ let note_id = id.unwrap_or_else(|| { + uuid::Uuid::new_v4().simple().to_string()[..8].to_string() + }); + let note_id_display = display_note_id(¬e_id); let note = self .store .add_note( &plan_id, NewNote { - id: id.unwrap_or_else(|| { - uuid::Uuid::new_v4().simple().to_string()[..8].to_string() - }), + id: note_id, summary: params.summary, author: params.author, }, ) .await .map_err(|err| match err { StoreError::AlreadyExists => ErrorData::invalid_params( - format!("note '{}' already exists in plan '{}'", "", plan), + format!("note '{}' already exists in plan '{}'", note_id_display, plan), None, ), other => store_error_to_error_data(other), })?;Also applies to: 812-818
🤖 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 `@crates/harnx-mcp-plans-core/src/server/handlers.rs` around lines 283 - 293, The AlreadyExists handling in handle_add_task and handle_add_note is using the wrong identifier because the task/note input is moved into the NewTask/NewNote constructor before the error mapping runs. Capture the display ID from the incoming body before passing it to the store call, then use that saved ID in the StoreError::AlreadyExists branch instead of display_id(&body) or a hardcoded empty string. Keep the fix localized around handle_add_task and handle_add_note so the invalid_params message shows the actual user-supplied task/note ID.crates/harnx-mcp-plans-core/src/server/handlers.rs-572-576 (1)
572-576: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
read_plan_bodyerrors are silently swallowed inhandle_update_plan.
unwrap_or_default()discards all errors fromread_plan_body, including transientBackendfailures. If the read fails, the body is treated as empty, and the subsequentwrite_plan_bodyoverwrites the existing content with the edit applied to an empty string — potentially causing data loss.Only
NotFoundshould be swallowed (the plan may have been just created with an empty body); other errors should propagate.🛡️ Proposed fix
- let before_body = self - .store - .read_plan_body(&plan_id) - .await - .unwrap_or_default(); + let before_body = match self.store.read_plan_body(&plan_id).await { + Ok(body) => body, + Err(StoreError::NotFound) => String::new(), + Err(err) => return Err(store_error_to_error_data(err)), + };🤖 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 `@crates/harnx-mcp-plans-core/src/server/handlers.rs` around lines 572 - 576, The `handle_update_plan` flow is swallowing all `read_plan_body` failures via `unwrap_or_default()`, which can treat backend errors as an empty body and later overwrite content incorrectly. Update the `before_body` read in `handle_update_plan` to handle `read_plan_body` errors explicitly: allow only `NotFound` to fall back to an empty string, and propagate any other error from `self.store.read_plan_body(&plan_id).await` instead of defaulting. Keep the existing `write_plan_body` logic unchanged so it only runs with a valid `before_body`.crates/harnx-mcp-plans-core/src/conformance.rs-514-541 (1)
514-541: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFilter tests pass vacuously on empty results.
assert!(open_tasks.items.iter().all(|t| t.status == "open"))returnstruewhenitemsis empty, so the test never verifies that matching tasks are actually returned. The same applies to the tag filter assertion. A backend that silently returns zero items for every filter would pass.🛡️ Proposed fix
let open_tasks = store .list_tasks( &plan.id, TaskFilter { status: Some("open".to_string()), tag: None, }, None, ) .await .expect("list_tasks with status filter should succeed"); + assert!( + open_tasks.items.iter().any(|t| t.status == "open"), + "status filter should return at least one matching task" + ); assert!(open_tasks.items.iter().all(|t| t.status == "open")); let backend_tasks = store .list_tasks( &plan.id, TaskFilter { status: None, tag: Some("backend".to_string()), }, None, ) .await .expect("list_tasks with tag filter should succeed"); + assert!( + backend_tasks + .items + .iter() + .any(|t| t.tags.contains(&"backend".to_string())), + "tag filter should return at least one matching task" + ); assert!(backend_tasks .items .iter() .all(|t| t.tags.contains(&"backend".to_string())));🤖 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 `@crates/harnx-mcp-plans-core/src/conformance.rs` around lines 514 - 541, The `list_tasks` filter checks in `conformance.rs` are vacuously true on empty შედეგs, so they don’t verify that matching tasks are actually returned. Update the `open_tasks` and `backend_tasks` assertions to first confirm the filtered result sets are non-empty, then keep the existing per-item checks on `Task.status` and `Task.tags` so `store.list_tasks`, `TaskFilter`, and the related conformance tests fail if the backend returns zero matches.crates/harnx-mcp-plans-core/src/conformance.rs-54-58 (1)
54-58: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDoc example for
BackendCapabilitiesis missingrejects_invalid_create_ids.The module doc example shows only two fields, but the actual struct at lines 70–75 has three. Anyone copying this example would get a compile error.
📝 Proposed fix
//! run_conformance( //! store, //! BackendCapabilities { //! preserves_client_id: true, //! deletes_permanently: true, +//! rejects_invalid_create_ids: true, //! }, //! )🤖 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 `@crates/harnx-mcp-plans-core/src/conformance.rs` around lines 54 - 58, The module doc example for BackendCapabilities is incomplete because it omits the rejects_invalid_create_ids field shown by the actual BackendCapabilities struct. Update the example in conformance.rs where BackendCapabilities is constructed so it includes all current fields, including rejects_invalid_create_ids, and keep the example consistent with the struct definition to avoid copy-paste compile errors.crates/harnx-mcp-plans-github/src/runtime.rs-254-266 (1)
254-266: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRetention is coupled to delete behavior.
close_stale_planreturns early whendelete_is_closeis false, so--delete-behavior leavealso disables stale-plan cleanup. If that’s intentional, document it in the Retention section; otherwise give retention its own toggle.🤖 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 `@crates/harnx-mcp-plans-github/src/runtime.rs` around lines 254 - 266, The stale-plan cleanup logic in close_stale_plan is currently gated by delete_is_close, so retention behavior is unintentionally tied to delete behavior. Update the GitHubPlanStore config path used by close_stale_plan so stale-plan cleanup has its own explicit toggle, or if the coupling is intended, document that relationship in the Retention section; use the close_stale_plan function and config_ref access as the main places to adjust.
🧹 Nitpick comments (20)
crates/harnx-mcp-plans-github/src/client.rs (2)
459-485: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFlatten
ensure_labelto clear the CodeScene critical gate.CI reports "Bumpy Road Ahead" for the nested
match/if/matchhere. The logic is a get-then-create with two "already exists" tolerances; extracting the not-found and already-exists checks into small helpers (e.g.is_not_found(&err),is_already_exists(&err)) and using early returns would flatten the nesting and satisfy the gate.🤖 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 `@crates/harnx-mcp-plans-github/src/client.rs` around lines 459 - 485, The ensure_label flow is too deeply nested, triggering the CodeScene gate; flatten the get-then-create logic in client.rs by extracting the error-classification checks into small helpers such as is_not_found and is_already_exists, then use early returns in ensure_label instead of nested match/if blocks. Keep the existing behavior for get_label and create_label, but make the control flow linear by returning immediately on non-404 errors and tolerating the already-exists create cases through the helpers.Source: Pipeline failures
632-662: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPath segments and query values are not percent-encoded.
encode_path_segmentonly replaces/, andwith_queryinterpolates values verbatim. Any label containing a space or reserved character (GitHub's own defaults includehelp wantedandgood first issue) produces an invalid URL inget_labeland in thelabelsfilter oflist_issues. Use a real percent-encoding routine (e.g.percent-encoding, orreqwest::Urlquery builder) for both paths and query values.🤖 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 `@crates/harnx-mcp-plans-github/src/client.rs` around lines 632 - 662, Path segments and query values in client.rs are being built without proper percent-encoding, so endpoints like get_label and list_issues can produce invalid URLs for labels with spaces or reserved characters. Update encode_path_segment and with_query to use a real URL encoding approach (such as a percent-encoding helper or reqwest::Url query handling) instead of manual string replacement/interpolation. Make sure the fix is applied wherever label names or other dynamic path/query values flow through absolute_url, encode_path_segment, normalize_issue_state, and with_query.crates/harnx-mcp-plans-github/src/ratelimit.rs (1)
286-390: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReduce the duplicated request-closure across tests (CI duplication gate).
retries_on_retry_after_then_succeeds,retries_on_reset_header_then_succeeds, andreturns_rate_limited_when_wait_exceeds_thresholdrepeat the samesend_rate_limited(..., || { client.clone(); url.clone(); async move { client.get(url).send().await.map_err(Into::into) } })block. Extracting a small helper (e.g.send_get(&executor, &url)) would satisfy the CodeScene duplication advisory and shorten the tests.🤖 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 `@crates/harnx-mcp-plans-github/src/ratelimit.rs` around lines 286 - 390, The three rate-limit tests duplicate the same GET request closure passed to send_rate_limited, so extract that repeated logic into a small helper tied to send_rate_limited and RequestContext::new, such as a reusable send_get helper that takes the executor and URL. Update retries_on_retry_after_then_succeeds, retries_on_reset_header_then_succeeds, and returns_rate_limited_when_wait_exceeds_threshold to call the helper instead of inlining the client.clone/url.clone async block.Source: Pipeline failures
crates/harnx-mcp-plans-github/src/store_github/tests.rs (1)
939-956: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCodec-only test placed in the store's integration test module.
write_task_body_does_not_corrupt_front_matterexercisescrate::codec::issue_to_task/task_meta_update_to_issue_bodydirectly with nowiremockserver usage; it would fit better alongside the other codec round-trip tests incodec.rs.🤖 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 `@crates/harnx-mcp-plans-github/src/store_github/tests.rs` around lines 939 - 956, The test write_task_body_does_not_corrupt_front_matter is codec-only and does not use any store_github or wiremock behavior, so move it out of the store integration test module into the codec test area. Keep the assertions around issue_to_task and task_meta_update_to_issue_body together with the other codec round-trip coverage in codec.rs so the test lives with the symbols it exercises.crates/harnx-mcp-plans-github/src/codec.rs (1)
724-728: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead code:
rfc3339_to_timestampis unused outside tests.Marked
#[allow(dead_code)]— either wire it into the store's error-mapping/decode path or drop it.🤖 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 `@crates/harnx-mcp-plans-github/src/codec.rs` around lines 724 - 728, The helper rfc3339_to_timestamp in codec.rs is only used by tests and is currently suppressed with #[allow(dead_code)]. Either integrate it into the production decode/error-mapping flow in the store path where RFC3339 timestamps are converted to jiff::Timestamp, or remove the function entirely if it is no longer needed. If you keep it, reference the existing Timestamp parsing logic so the conversion happens through the same codec path instead of remaining test-only.crates/harnx-mcp-plans-github/src/store_github.rs (2)
399-414: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoffFragile substring-based error classification.
map_github_errorclassifies errors by lowercasing the anyhowDisplaystring and checking for substrings like"404","422","rate limit". Any change in wording from the client/HTTP layer (or a URL/body that happens to contain "404") silently miscategorizes errors as genericBackend, changing retry/observability behavior for callers.Consider having
GitHubClientreturn a typed error (e.g., an enum carrying the HTTP status code) instead of stringifiedanyhow::Error, so this mapping can match on structured data.🤖 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 `@crates/harnx-mcp-plans-github/src/store_github.rs` around lines 399 - 414, The error classification in map_github_error is too dependent on lowercased display strings and substring checks, which can misroute GitHub failures. Update GitHubClient and the error flow so the mapper receives structured error data (such as a typed enum or explicit HTTP status code) and switch map_github_error to match on those fields instead of parsing anyhow::Error text.
586-637: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffSub-issue JSON parsed as untyped
serde_json::Valueinstead of a typed struct.
list_tasks(andfind_task_membership) manually pull fields (number,title,body,created_at,updated_at) out of rawserde_json::Valuevia.get(...), unlike the typedIssueRecord/IssueCommentused elsewhere in this file. A typedSubIssueRecordstruct with#[serde(deserialize_with = ...)]as needed would reduce the risk of typos in field names silently producingNone/skipped items.🤖 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 `@crates/harnx-mcp-plans-github/src/store_github.rs` around lines 586 - 637, The sub-issue parsing in list_tasks is using raw serde_json::Value field lookups instead of a typed record, which is fragile. Introduce a typed SubIssueRecord (similar to IssueRecord/IssueComment in this module) and deserialize the GitHub sub-issue payload into it in list_tasks and find_task_membership, using serde helpers where needed for timestamps. Then update the task mapping to read fields from the typed struct instead of .get(...) chains so missing or renamed fields are caught by deserialization.crates/harnx-mcp-plans-core/src/server/handlers.rs (2)
456-484: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared task-creation logic to reduce duplication.
The batch task creation loop (
build_new_task→add_task→write_task_body→ collect successes/failures) is duplicated betweenhandle_add_planandhandle_update_plan. CodeScene flagged this as code duplication. Extracting a helper would keep the two paths consistent and reduce maintenance burden.♻️ Proposed refactor
+async fn create_tasks_batch<S: PlanStore>( + store: &S, + plan_id: &PlanId, + name: &str, + specs: &[TaskSpec], +) -> (Vec<String>, Vec<String>) { + let mut created_ids = Vec::new(); + let mut failures = Vec::new(); + for spec in specs { + match build_new_task(name, spec) { + Ok((new_task, task_body)) => match store.add_task(plan_id, new_task).await { + Ok(task) => { + if let Err(err) = store.write_task_body(plan_id, &task.id, &task_body).await { + failures.push(format!( + "{}: {}", + display_id(&task.id), + store_error_to_error_data(err).message + )); + } else { + created_ids.push(display_id(&task.id)); + } + } + Err(StoreError::AlreadyExists) => { + failures.push(format!("task already exists in plan '{}'", name)) + } + Err(err) => { + failures.push(store_error_to_error_data(err).message.to_string()) + } + }, + Err(err) => failures.push(err.message.to_string()), + } + } + (created_ids, failures) +}Then in both
handle_add_planandhandle_update_plan, replace the loop with:- let mut created_task_ids = Vec::new(); - let mut task_failures = Vec::new(); - for spec in task_specs { - match build_new_task(&name, spec) { - Ok((new_task, task_body)) => match self.store.add_task(&plan_id, new_task).await { - Ok(task) => { - if let Err(err) = self - .store - .write_task_body(&plan_id, &task.id, &task_body) - .await - { - task_failures.push(format!( - "{}: {}", - display_id(&task.id), - store_error_to_error_data(err).message - )); - } else { - created_task_ids.push(display_id(&task.id)); - } - } - Err(err) => { - task_failures.push(store_error_to_error_data(err).message.to_string()) - } - }, - Err(err) => task_failures.push(err.message.to_string()), - } - } + let (created_task_ids, task_failures) = + create_tasks_batch(self.store.as_ref(), &plan_id, &name, &task_specs).await;Note:
handle_add_plan's version is slightly different — it doesn't have the explicitStoreError::AlreadyExistsarm. The extracted helper uses thehandle_update_planversion (with the explicit arm), which is the more correct behavior.Also applies to: 618-649
🤖 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 `@crates/harnx-mcp-plans-core/src/server/handlers.rs` around lines 456 - 484, The batch task-creation flow is duplicated in handle_add_plan and handle_update_plan, so extract the shared build_new_task → add_task → write_task_body → collect successes/failures logic into a helper. Reuse that helper from both handlers, keeping the handle_update_plan behavior as the source of truth, including the explicit StoreError::AlreadyExists handling. Make sure the helper preserves the existing created_task_ids and task_failures reporting semantics and is easy to locate via build_new_task, add_task, write_task_body, handle_add_plan, and handle_update_plan.
397-422: 🚀 Performance & Scalability | 🔵 TrivialN+1 store calls in
handle_list_plans.For each plan,
list_tasksandlist_notesare called separately to compute counts. With the GitHub backend, this means 2N API requests for N plans, each subject to rate limiting. Consider adding acount_tasks/count_notesmethod or including counts in thePlanmodel /list_plansresult to avoid per-plan round-trips.🤖 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 `@crates/harnx-mcp-plans-core/src/server/handlers.rs` around lines 397 - 422, `handle_list_plans` is doing per-plan `list_tasks` and `list_notes` calls, creating an N+1 request pattern. Update the `handle_list_plans` flow to avoid counting via per-item round trips by adding bulk count support in the store layer (for example `count_tasks`/`count_notes`) or by returning counts from `list_plans`/the `Plan` model itself. Then use those precomputed counts when building each entry with `plan_summary_json`.crates/harnx-mcp-plans-core/src/server/mod.rs (1)
144-150: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove dead
result_with_diffduplicate.This
result_with_diff(which wraps the diff in a markdown code fence) is marked#[allow(dead_code)]becausehandlers.rsdefines its ownresult_with_diff(without the code fence) that shadows it viause super::*. Having two functions with the same name and different behavior is confusing for future maintainers.♻️ Proposed fix
-#[allow(dead_code)] -fn result_with_diff(summary: String, diff: String) -> Result<CallToolResult, ErrorData> { - if diff.is_empty() { - return result_text(summary); - } - result_text(format!("{summary}\n\n```diff\n{diff}\n```")) -}🤖 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 `@crates/harnx-mcp-plans-core/src/server/mod.rs` around lines 144 - 150, Remove the duplicate dead result_with_diff helper in server/mod.rs and keep only the version actually used by handlers.rs via use super::*. The existing #[allow(dead_code)] function that wraps diff output in a markdown code fence should be deleted, and any remaining call sites should continue to use the shared result_with_diff behavior from the module so there is a single source of truth.crates/harnx-mcp-plans-core/src/conformance.rs (2)
338-365: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
test_plan_duplicate_idandtest_already_exists_errorsare functionally identical.Both create a plan, attempt a duplicate, assert
AlreadyExistsif it errors, and clean up. The only difference is the plan ID string. Consider removing one or merging them to reduce duplication flagged by CodeScene.♻️ Proposed consolidation
Remove
test_plan_duplicate_id(lines 338–365) and keeptest_already_exists_errors(lines 941–968), which already covers the same contract. Then remove the call at line 89:test_plan_list_pagination(&store).await; - test_plan_duplicate_id(&store).await;Also applies to: 941-968
🤖 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 `@crates/harnx-mcp-plans-core/src/conformance.rs` around lines 338 - 365, `test_plan_duplicate_id` duplicates the behavior already covered by `test_already_exists_errors`; remove the redundant test and keep the existing one that asserts `StoreError::AlreadyExists` on duplicate `add_plan` calls. Update the test suite wiring in the conformance module so only the remaining `test_already_exists_errors` path is invoked, and keep the cleanup logic in that test intact.
288-336: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffCodeScene flags "Bumpy Road Ahead" and Code Duplication in pagination tests.
The pagination verification pattern (fetch page 1, fetch page 2, assert no ID overlap) is repeated across
test_plan_list_pagination,test_task_list_pagination, andtest_note_list_pagination. Extracting a helper would reduce both the nesting depth and the duplication.♻️ Proposed helper extraction
+async fn assert_pagination_no_overlap<S: PlanStore, T>( + store: &Arc<S>, + fetch_page: impl Fn(&Arc<S>, Option<PageToken>) -> Result<Page<T>, StoreError>, +) where + T: AsRef<str>, +{ + let page1 = fetch_page(store, None).await.expect("first page should succeed"); + assert!(!page1.items.is_empty(), "page 1 should have items"); + if let Some(next) = page1.next { + let page2 = fetch_page(store, Some(next)) + .await + .expect("page 2 should succeed"); + let page1_ids: std::collections::HashSet<_> = + page1.items.iter().map(|item| item.as_ref()).collect(); + for item in &page2.items { + assert!( + !page1_ids.contains(item.as_ref()), + "pagination pages should not overlap" + ); + } + } +} + async fn test_plan_list_pagination<S: PlanStore>(store: &Arc<S>) {Then each pagination test body simplifies to a single helper call.
🤖 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 `@crates/harnx-mcp-plans-core/src/conformance.rs` around lines 288 - 336, The pagination assertion logic is duplicated across the list pagination tests, including test_plan_list_pagination, test_task_list_pagination, and test_note_list_pagination. Extract the repeated “fetch first page, fetch next page, assert no item ID overlap” flow into a shared helper and have test_plan_list_pagination call it, so the test bodies stay shallow and consistent.Source: Pipeline failures
crates/harnx-mcp-plans/src/store_fs.rs (1)
119-263: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
anyhow::ResultoverResult<T, String>for internal helpers.
validate_plan_name,validate_id,serialize_task/parse_task_frontmatter,serialize_plan/parse_plan_frontmatter,serialize_note/parse_note_frontmatter,write_task,write_plan_file, andwrite_noteall returnResult<T, String>, discarding error context/source chains thatanyhow::Errorwould preserve. Call sites already wrap these intoStoreError::Backend(anyhow::anyhow!(err)), so switching the helpers themselves toanyhow::Result<T>(using.context(...)instead of.map_err(|e| e.to_string())) would be more idiomatic and preserve better diagnostics.As per coding guidelines, "Use anyhow::Result and anyhow::bail! for error handling throughout the Rust codebase."
♻️ Example refactor for one helper
-pub(crate) fn validate_plan_name(name: &str) -> Result<String, String> { +pub(crate) fn validate_plan_name(name: &str) -> anyhow::Result<String> { let normalized = normalize_plan_name(name); if normalized.is_empty() { - return Err("plan name must not be empty".to_string()); + anyhow::bail!("plan name must not be empty"); } if normalized.contains('/') || normalized.contains('\\') || normalized.contains("..") { - return Err(format!( + anyhow::bail!( "plan name '{}' must not contain path separators or '..'", name - )); + ); } Ok(normalized) }🤖 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 `@crates/harnx-mcp-plans/src/store_fs.rs` around lines 119 - 263, The helpers in store_fs currently erase error context by returning Result<T, String>; update validate_plan_name, validate_id, serialize_task/parse_task_frontmatter, serialize_plan/parse_plan_frontmatter, serialize_note/parse_note_frontmatter, write_task, write_plan_file, and write_note to use anyhow::Result instead. Replace manual map_err(|err| err.to_string()) conversions with anyhow context/bail handling so serde_yaml and std::fs failures preserve source details, and keep the call sites compatible with StoreError::Backend.Source: Coding guidelines
crates/harnx-mcp-plans-github/src/runtime.rs (1)
107-112: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRegex recompiled on every error.
redact_bearer_tokenscompiles a newRegexon each call. Since it's only on error paths this is low-impact, but astd::sync::OnceLock/once_cell::sync::Lazycached regex would avoid the repeated compilation cost.🤖 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 `@crates/harnx-mcp-plans-github/src/runtime.rs` around lines 107 - 112, The redact_bearer_tokens helper recompiles the same regex on every call, so cache it instead of creating a new Regex each time. Update redact_bearer_tokens in runtime.rs to use a shared OnceLock or once_cell::sync::Lazy for the compiled pattern, and keep the replacement behavior unchanged.crates/harnx-mcp-plans-github/tests/conformance_github.rs (1)
257-525: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
mount_mock_handlersflagged as Large Method by CodeScene.The function registers ~10 independent wiremock endpoints sequentially. Splitting into per-resource helpers (e.g.
mount_issue_endpoints,mount_comment_endpoints,mount_sub_issue_endpoints) would shrink it and address the advisory.🤖 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 `@crates/harnx-mcp-plans-github/tests/conformance_github.rs` around lines 257 - 525, `mount_mock_handlers` is doing too much by registering many unrelated WireMock routes in one large method. Refactor it by extracting the endpoint setup into smaller helper functions in the same test module, grouped by concern such as `mount_issue_endpoints`, `mount_comment_endpoints`, and `mount_sub_issue_endpoints`, and have `mount_mock_handlers` just orchestrate those calls while preserving the existing behavior and shared `state` handling.Source: Pipeline failures
crates/harnx-mcp-plans-github/tests/github_specific.rs (2)
431-466: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
jira_key_round_trips_in_titledoesn't actually assert a round-trip.The test creates a plan and only asserts
!plan.id.is_empty(). It never checks that the JIRA key/title prefix was preserved (e.g. viaplan.title/ajira_keyfield), so it wouldn't fail if JIRA round-tripping regressed.🤖 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 `@crates/harnx-mcp-plans-github/tests/github_specific.rs` around lines 431 - 466, The jira_key_round_trips_in_title test is too weak because it only checks that add_plan returns a non-empty id and never verifies the JIRA prefix survived the round-trip. Update this test in github_specific.rs to assert the created plan returned by store.add_plan(NewPlan) preserves the expected title and/or jira_key value from the mocked GitHub issue response. Use the jira_key and title fields on the plan object (or whatever accessor is used in this test suite) so the test fails if encoding/decoding of the JIRA key regresses.
24-40: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSubstantial boilerplate duplication (matches the CodeScene "Code Duplication" gate).
create_test_store,mock_issue_response, andmock_plan_issue_responseare duplicated near-verbatim inconformance_github.rs(and similar patterns recur across nearly every#[tokio::test]in this file: plan-issue GET mock + sub_issues GET mock + PATCH mock). Extracting these into a sharedtests/common.rs(or a#[cfg(test)]support module reused viainclude!/a dev-dependency path) would remove the duplication CodeScene is flagging and make future test additions cheaper.Also applies to: 45-78
🤖 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 `@crates/harnx-mcp-plans-github/tests/github_specific.rs` around lines 24 - 40, Extract the duplicated test setup into a shared helper module so the GitHub test cases stop repeating near-identical boilerplate. Move create_test_store, mock_issue_response, and mock_plan_issue_response into a reusable test support location (for example a common test module under cfg(test)) and update the tests in github_specific.rs to call those helpers instead of inlining the setup. Keep the helpers named so they are easy to reuse across conformance_github.rs and the other #[tokio::test] cases that set up the plan-issue GET, sub_issues GET, and PATCH mocks.Source: Pipeline failures
crates/harnx-mcp-plans-github/src/config.rs (1)
28-160: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
parse_fromis flagged as a Complex Method by CodeScene.The function mixes CLI-flag scanning, env fallbacks, per-field validation, and struct construction in one ~130-line body — matching the pipeline's "Complex Method" advisory. Consider extracting the
whileloop into a smallRawArgsstruct/parser and moving each field's env/CLI resolution (auth source, retention, plan label, delete behavior, rate limit) into dedicated helper functions, mirroring what's already done forapp_auth_from_env.🤖 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 `@crates/harnx-mcp-plans-github/src/config.rs` around lines 28 - 160, The parse_from function is too complex because it combines argument scanning, environment fallback logic, validation, and final config assembly in one body. Refactor it by extracting the CLI parsing loop into a small RawArgs-style parser and moving each resolution step into focused helpers, especially for auth source, retention_days, plan_label, delete_is_close, and max_wait_secs, similar to app_auth_from_env. Keep parse_from as a thin orchestration method that calls those helpers and then builds Self.Source: Pipeline failures
crates/harnx-mcp-plans-github/README.md (1)
50-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRetention section doesn't mention the
delete-behaviorcoupling.Per the
runtime.rsreview, the retention pass only closes stale plans whendelete_is_close/--delete-behaviorisclose. This isn't mentioned here — worth a note so operators using--delete-behavior leaveknow retention closing is also disabled.🤖 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 `@crates/harnx-mcp-plans-github/README.md` around lines 50 - 51, The Retention section is missing the coupling to delete behavior, so update the README text near the Retention description to note that the background closing pass only runs when `delete_is_close`/`--delete-behavior` is set to close. Mention that with `--delete-behavior leave`, stale plans are not closed by retention, and reference the existing retention logic in `runtime.rs` so operators can discover the behavior from the documentation.crates/harnx-mcp-plans-github/tests/live_e2e.rs (1)
96-375: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffRepeated store/plan/task/note setup-and-teardown across the five live tests (CodeScene Code Duplication).
Each
#[tokio::test]repeats: obtain store viacreate_live_store, create a plan (and often a task/note), then manually clean up on both success and error paths. A sharedLiveTestContexthelper (create-on-setup,Drop-based or explicit teardown) would remove this duplication and reduce the risk of a cleanup step being skipped on a new test.🤖 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 `@crates/harnx-mcp-plans-github/tests/live_e2e.rs` around lines 96 - 375, The five live_e2e_* tests duplicate the same store initialization, plan creation, and cleanup logic. Extract that setup/teardown into a shared helper such as a LiveTestContext around create_live_store, unique_plan_id, and the delete_* cleanup paths so the tests only describe their specific assertions. Reuse the helper from live_e2e_create_plan, live_e2e_create_task, live_e2e_create_note, live_e2e_pagination, and live_e2e_full_crud_cycle to remove repeated boilerplate and centralize teardown.Source: Pipeline failures
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6487ebd9-93c6-44ff-980e-25df8c48dda5
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (41)
.github/workflows/release.yamlCargo.tomlREADME.mdcrates/harnx-mcp-plans-core/Cargo.tomlcrates/harnx-mcp-plans-core/src/conformance.rscrates/harnx-mcp-plans-core/src/lib.rscrates/harnx-mcp-plans-core/src/model.rscrates/harnx-mcp-plans-core/src/server/handler.rscrates/harnx-mcp-plans-core/src/server/handlers.rscrates/harnx-mcp-plans-core/src/server/mod.rscrates/harnx-mcp-plans-core/src/server/params.rscrates/harnx-mcp-plans-core/src/store.rscrates/harnx-mcp-plans-github/.gitignorecrates/harnx-mcp-plans-github/Cargo.tomlcrates/harnx-mcp-plans-github/README.mdcrates/harnx-mcp-plans-github/src/auth.rscrates/harnx-mcp-plans-github/src/client.rscrates/harnx-mcp-plans-github/src/codec.rscrates/harnx-mcp-plans-github/src/config.rscrates/harnx-mcp-plans-github/src/lib.rscrates/harnx-mcp-plans-github/src/main.rscrates/harnx-mcp-plans-github/src/ratelimit.rscrates/harnx-mcp-plans-github/src/runtime.rscrates/harnx-mcp-plans-github/src/store_github.rscrates/harnx-mcp-plans-github/src/store_github/tests.rscrates/harnx-mcp-plans-github/tests/conformance_github.rscrates/harnx-mcp-plans-github/tests/github_specific.rscrates/harnx-mcp-plans-github/tests/live_e2e.rscrates/harnx-mcp-plans/Cargo.tomlcrates/harnx-mcp-plans/src/lib.rscrates/harnx-mcp-plans/src/main.rscrates/harnx-mcp-plans/src/server/handlers.rscrates/harnx-mcp-plans/src/server/mod.rscrates/harnx-mcp-plans/src/server/params.rscrates/harnx-mcp-plans/src/server/store.rscrates/harnx-mcp-plans/src/server/tests.rscrates/harnx-mcp-plans/src/store_fs.rscrates/harnx-mcp-plans/src/tests.rscrates/harnx-mcp-plans/tests/conformance_fs.rsdocs/solutions/integration-issues/github-issues-storage-backend-2026-07-08.mdexample_config/mcp_servers/plans-github.yaml
💤 Files with no reviewable changes (4)
- crates/harnx-mcp-plans/src/server/mod.rs
- crates/harnx-mcp-plans/src/server/params.rs
- crates/harnx-mcp-plans/src/server/handlers.rs
- crates/harnx-mcp-plans/src/server/store.rs
| pub fn issue_to_plan( | ||
| issue_number: u64, | ||
| title: &str, | ||
| body: Option<&str>, | ||
| created_at: Timestamp, | ||
| updated_at: Option<Timestamp>, | ||
| ) -> DecodedPlan { | ||
| let body = body.unwrap_or(""); | ||
| let (front, markdown_body) = parse_plan_frontmatter(body); | ||
|
|
||
| // Extract JIRA key from title if present | ||
| let (plan_title, jira_key_from_title) = extract_jira_key_from_title(title); | ||
|
|
||
| // Merge JIRA keys: prefer title prefix over front-matter | ||
| let jira_key = jira_key_from_title.or(front.jira_key); | ||
|
|
||
| // Create the Plan domain struct | ||
| // ID is the stringified issue number (authoritative) | ||
| // Note: front.client_id is preserved but not used as the domain ID | ||
| let plan = Plan { | ||
| id: issue_number.to_string(), | ||
| title: Some(plan_title), | ||
| summary: front.summary, | ||
| author: front.author, | ||
| assignee: front.assignee, | ||
| executor: front.executor, | ||
| git_branch: front.git_branch, | ||
| github_owner_repo: front.github_owner_repo, | ||
| created_at, | ||
| updated_at, | ||
| }; | ||
|
|
||
| DecodedPlan { | ||
| plan, | ||
| body: markdown_body, | ||
| jira_key, | ||
| client_id: front.client_id, | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Bundle decode args into a struct (CI gate failure).
CodeScene's advisory gate is failing on issue_to_plan for excess function arguments (5 params), and issue_to_task has the same shape with 6 params. Since RUSTFLAGS enforces -D warnings for clippy and this CodeScene gate is a separate CI check that's currently failing, this needs to be addressed before merge.
♻️ Suggested refactor: bundle GitHub issue metadata into a struct
+pub struct GithubIssueMeta<'a> {
+ pub number: u64,
+ pub title: &'a str,
+ pub body: Option<&'a str>,
+ pub created_at: Timestamp,
+ pub updated_at: Option<Timestamp>,
+}
+
-pub fn issue_to_plan(
- issue_number: u64,
- title: &str,
- body: Option<&str>,
- created_at: Timestamp,
- updated_at: Option<Timestamp>,
-) -> DecodedPlan {
+pub fn issue_to_plan(meta: GithubIssueMeta<'_>) -> DecodedPlan {Also applies to: 355-401
🤖 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 `@crates/harnx-mcp-plans-github/src/codec.rs` around lines 245 - 283, Refactor
the decode APIs to stop passing long argument lists: `issue_to_plan` (and the
matching `issue_to_task`) should accept a single metadata struct instead of
separate issue fields. Introduce a shared input type for the GitHub issue data,
update the call sites and any decode helpers in `codec.rs` to construct and pass
that struct, and keep the existing behavior inside `issue_to_plan` by reading
from the new grouped fields.
Source: Pipeline failures
| async fn run_stdio(store: Arc<GitHubPlanStore>, config: AppConfig) -> Result<()> { | ||
| eprintln!( | ||
| "harnx-mcp-plans-github v{}: starting (repo: {}/{}, label: {}, retention: {} days)", | ||
| env!("CARGO_PKG_VERSION"), | ||
| config.auth.repo.owner, | ||
| config.auth.repo.repo, | ||
| config.store.plan_label, | ||
| config.retention_days | ||
| ); | ||
|
|
||
| let server = PlansServer::with_meta(store.clone(), GITHUB_SERVER_META); | ||
| let transport = rmcp::transport::stdio(); | ||
| let service = server.serve(transport).await?; | ||
|
|
||
| if config.retention_days == 0 { | ||
| eprintln!("[retention] disabled"); | ||
| service.waiting().await?; | ||
| return Ok(()); | ||
| } | ||
|
|
||
| let mut retention_handle = tokio::spawn(retention_loop(store.clone(), config.retention_days)); | ||
| let service_handle = tokio::spawn(async move { service.waiting().await }); | ||
| tokio::pin!(service_handle); | ||
| let mut backoff = BASE_BACKOFF; | ||
|
|
||
| loop { | ||
| tokio::select! { | ||
| result = &mut *service_handle => { | ||
| retention_handle.abort(); | ||
| result??; | ||
| break; | ||
| } | ||
| result = &mut retention_handle => { | ||
| match result { | ||
| Err(err) => { | ||
| eprintln!("[retention] task failed: {err}"); | ||
| tokio::time::sleep(backoff).await; | ||
| backoff = (backoff * 2).min(MAX_BACKOFF); | ||
| } | ||
| Ok(()) => backoff = BASE_BACKOFF, | ||
| } | ||
| retention_handle = tokio::spawn(retention_loop(store.clone(), config.retention_days)); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Duplicated retention-supervision loop between run_stdio and run_http (CodeScene Large Method in run_http).
The tokio::select! retention supervisor (spawn, backoff on panic, respawn, abort-on-exit) at lines 134-158 and 218-240 is essentially identical logic duplicated across both transports. Extracting a shared helper (e.g. supervise_with_retention<F>(store, retention_days, primary: F) -> Result<()> or a small RetentionSupervisor that both call) would remove the duplication and address the CodeScene "Large Method" gate on run_http.
Also applies to: 163-243
🤖 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 `@crates/harnx-mcp-plans-github/src/runtime.rs` around lines 114 - 161, The
retention-supervision logic is duplicated in both transport runners, so extract
the shared spawn/backoff/respawn/abort loop into a reusable helper or small
supervisor type and have both run_stdio and run_http call it. Keep the existing
behavior the same by moving the tokio::select! loop,
retention_handle/service_handle coordination, and backoff handling into a shared
function such as supervise_with_retention or a RetentionSupervisor, then let the
transport-specific setup only provide the primary service future.
Source: Pipeline failures
| async fn add_task(&self, plan: &PlanId, new_task: NewTask) -> Result<Task, StoreError> { | ||
| let plan_number = Self::parse_plan_id(plan)?; | ||
|
|
||
| self.ensure_issue_is_plan(plan_number).await?; | ||
| let current_sub_count = self.count_sub_issues(plan_number).await?; | ||
| if current_sub_count >= MAX_SUB_ISSUES { | ||
| return Err(StoreError::InvalidParams(format!( | ||
| "plan {} already has maximum number of sub-issues ({})", | ||
| plan, MAX_SUB_ISSUES | ||
| ))); | ||
| } | ||
|
|
||
| let (title, body) = new_task_to_issue(plan, &new_task, None, ""); | ||
|
|
||
| let issue = self | ||
| .client | ||
| .create_issue(CreateIssue { | ||
| title, | ||
| body: Some(body), | ||
| labels: Vec::new(), | ||
| }) | ||
| .await | ||
| .map_err(map_github_error)?; | ||
|
|
||
| let task_number = issue.number; | ||
| let task_internal_id = issue.id; | ||
|
|
||
| self.client | ||
| .add_sub_issue(plan_number, task_internal_id) | ||
| .await | ||
| .map_err(map_github_error)?; | ||
|
|
||
| Ok(Task { | ||
| id: task_number.to_string(), | ||
| title: new_task.title, | ||
| summary: new_task.summary, | ||
| author: new_task.author, | ||
| assignee: new_task.assignee, | ||
| executor: new_task.executor, | ||
| tags: new_task.tags, | ||
| plan: plan.clone(), | ||
| status: new_task.status.unwrap_or_else(|| "open".to_string()), | ||
| created_at: Timestamp::now(), | ||
| updated_at: None, | ||
| dependencies: new_task.dependencies, | ||
| }) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
sed -n '620,730p' crates/harnx-mcp-plans-github/src/store_github.rsRepository: dobesv/harnx
Length of output: 3585
🏁 Script executed:
rg -n "add_sub_issue|create_issue\(|delete_issue|close_issue|add_sub" crates -g '*.rs'Repository: dobesv/harnx
Length of output: 1694
🏁 Script executed:
sed -n '180,430p' crates/harnx-mcp-plans-github/src/client.rs && printf '\n---\n' && sed -n '460,760p' crates/harnx-mcp-plans-github/src/store_github.rsRepository: dobesv/harnx
Length of output: 18368
Clean up the created issue if add_sub_issue fails
In crates/harnx-mcp-plans-github/src/store_github.rs:668-681, add_task creates the issue first and then links it to the plan. If linking fails, the issue is left open but untracked, and retries can pile up more orphans. Close/delete the new issue on failure, or make the create+link step atomic.
🤖 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 `@crates/harnx-mcp-plans-github/src/store_github.rs` around lines 652 - 698,
The add_task flow in store_github creates a GitHub issue before calling
add_sub_issue, so a link failure leaves an orphaned issue behind. Update
add_task to clean up the newly created issue if add_sub_issue returns an error,
using the existing issue result from create_issue and map_github_error handling,
or otherwise make the create-and-link sequence effectively atomic.
| fn issue_body_from_request(body: &serde_json::Value) -> Option<String> { | ||
| body["body"].as_str().map(|s| s.to_string()).or_else(|| { | ||
| body["body"].as_array().map(|parts| { | ||
| let mut out = String::new(); | ||
| for part in parts { | ||
| if let Some(s) = part.as_str() { | ||
| out.push_str(s); | ||
| } else if let Some(n) = part.as_u64() { | ||
| out.push_str(&n.to_string()); | ||
| } else if let Some(map) = part.as_object() { | ||
| if let Some(v) = map.get("str").and_then(|v| v.as_str()) { | ||
| out.push_str(v); | ||
| } | ||
| } | ||
| } | ||
| out | ||
| }) | ||
| }) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
issue_body_from_request flagged as "Bumpy Road Ahead" by CodeScene (critical gate).
The nested if let/else if let chain handling string/array/object body encodings is the source of the critical gate failure. Flattening via a match on part (or a small enum-based dispatch) would reduce nesting and satisfy the gate.
♻️ Possible simplification
- for part in parts {
- if let Some(s) = part.as_str() {
- out.push_str(s);
- } else if let Some(n) = part.as_u64() {
- out.push_str(&n.to_string());
- } else if let Some(map) = part.as_object() {
- if let Some(v) = map.get("str").and_then(|v| v.as_str()) {
- out.push_str(v);
- }
- }
- }
+ for part in parts {
+ match part {
+ serde_json::Value::String(s) => out.push_str(s),
+ serde_json::Value::Number(n) => out.push_str(&n.to_string()),
+ serde_json::Value::Object(map) => {
+ if let Some(v) = map.get("str").and_then(|v| v.as_str()) {
+ out.push_str(v);
+ }
+ }
+ _ => {}
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn issue_body_from_request(body: &serde_json::Value) -> Option<String> { | |
| body["body"].as_str().map(|s| s.to_string()).or_else(|| { | |
| body["body"].as_array().map(|parts| { | |
| let mut out = String::new(); | |
| for part in parts { | |
| if let Some(s) = part.as_str() { | |
| out.push_str(s); | |
| } else if let Some(n) = part.as_u64() { | |
| out.push_str(&n.to_string()); | |
| } else if let Some(map) = part.as_object() { | |
| if let Some(v) = map.get("str").and_then(|v| v.as_str()) { | |
| out.push_str(v); | |
| } | |
| } | |
| } | |
| out | |
| }) | |
| }) | |
| } | |
| fn issue_body_from_request(body: &serde_json::Value) -> Option<String> { | |
| body["body"].as_str().map(|s| s.to_string()).or_else(|| { | |
| body["body"].as_array().map(|parts| { | |
| let mut out = String::new(); | |
| for part in parts { | |
| match part { | |
| serde_json::Value::String(s) => out.push_str(s), | |
| serde_json::Value::Number(n) => out.push_str(&n.to_string()), | |
| serde_json::Value::Object(map) => { | |
| if let Some(v) = map.get("str").and_then(|v| v.as_str()) { | |
| out.push_str(v); | |
| } | |
| } | |
| _ => {} | |
| } | |
| } | |
| out | |
| }) | |
| }) | |
| } |
🤖 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 `@crates/harnx-mcp-plans-github/tests/conformance_github.rs` around lines 203 -
221, The `issue_body_from_request` helper has a deeply nested `if let`/`else if
let` chain for parsing `body` array elements, which is triggering the CodeScene
gate. Refactor the array handling in `issue_body_from_request` to use a flatter
`match` on each `part` (or equivalent enum-style dispatch) so the string,
number, and object cases are handled at one level without nested branching.
Source: Pipeline failures
| #[async_trait] | ||
| impl PlanStore for FilePlanStore { | ||
| async fn list_plans(&self, page: Option<PageToken>) -> Result<Page<Plan>, StoreError> { | ||
| let offset = parse_offset(page)?; | ||
| let mut plans = Vec::new(); | ||
| for dir in plan_dirs(&self.dir) { | ||
| let Some(name) = dir.file_name().and_then(OsStr::to_str) else { | ||
| continue; | ||
| }; | ||
| let normalized = normalize_plan_name(name); | ||
| let plan_path = plan_file_path(&self.dir, &normalized); | ||
| let record = if plan_path.exists() { | ||
| let content = std::fs::read_to_string(&plan_path) | ||
| .map_err(|err| StoreError::Backend(err.into()))?; | ||
| let fallback_created_at = std::fs::metadata(&plan_path) | ||
| .and_then(|metadata| metadata.modified()) | ||
| .ok() | ||
| .map(system_time_to_rfc3339) | ||
| .transpose()?; | ||
| parse_plan_record_from_content(&content, &normalized, fallback_created_at)? | ||
| } else { | ||
| PlanRecord { | ||
| front: PlanFrontMatter { | ||
| id: normalized.clone(), | ||
| created_at: String::new(), | ||
| ..Default::default() | ||
| }, | ||
| body: String::new(), | ||
| } | ||
| }; | ||
| plans.push(plan_record_to_domain(record)?); | ||
| } | ||
| let total = plans.len(); | ||
| let items = plans.into_iter().skip(offset).collect::<Vec<_>>(); | ||
| Ok(Page { | ||
| items, | ||
| next: next_page_token(total, total), | ||
| }) | ||
| } | ||
|
|
||
| async fn get_plan(&self, plan: &PlanId) -> Result<Plan, StoreError> { | ||
| let name = validate_plan_name(plan).map_err(StoreError::InvalidParams)?; | ||
| let path = plan_file_path(&self.dir, &name); | ||
| let content = if path.exists() { | ||
| std::fs::read_to_string(&path).map_err(|err| StoreError::Backend(err.into()))? | ||
| } else if plan_dir(&self.dir, &name).exists() { | ||
| String::new() | ||
| } else { | ||
| return Err(StoreError::NotFound); | ||
| }; | ||
| let fallback_created_at = if path.exists() { | ||
| std::fs::metadata(&path) | ||
| .and_then(|metadata| metadata.modified()) | ||
| .ok() | ||
| .map(system_time_to_rfc3339) | ||
| .transpose()? | ||
| } else { | ||
| None | ||
| }; | ||
| let record = parse_plan_record_from_content(&content, &name, fallback_created_at)?; | ||
| plan_record_to_domain(PlanRecord { | ||
| front: record.front, | ||
| body: String::new(), | ||
| }) | ||
| } | ||
|
|
||
| async fn add_plan(&self, new_plan: NewPlan) -> Result<Plan, StoreError> { | ||
| let name = validate_plan_name(&new_plan.id).map_err(StoreError::InvalidParams)?; | ||
| let dir = plan_dir(&self.dir, &name); | ||
| if dir.exists() { | ||
| return Err(StoreError::AlreadyExists); | ||
| } | ||
| std::fs::create_dir_all(&dir).map_err(|err| StoreError::Backend(err.into()))?; | ||
|
|
||
| let record = PlanRecord { | ||
| front: PlanFrontMatter { | ||
| id: name.clone(), | ||
| title: new_plan.title, | ||
| summary: new_plan.summary, | ||
| author: new_plan.author, | ||
| assignee: new_plan.assignee, | ||
| executor: new_plan.executor, | ||
| git_branch: new_plan.git_branch, | ||
| github_owner_repo: new_plan.github_owner_repo, | ||
| created_at: now_iso(), | ||
| updated_at: None, | ||
| }, | ||
| body: String::new(), | ||
| }; | ||
| let serialized = | ||
| serialize_plan(&record).map_err(|err| StoreError::Backend(anyhow::anyhow!(err)))?; | ||
| write_plan_file(&plan_file_path(&self.dir, &name), &serialized) | ||
| .map_err(|err| StoreError::Backend(anyhow::anyhow!(err)))?; | ||
| plan_record_to_domain(record) | ||
| } | ||
|
|
||
| async fn update_plan_meta( | ||
| &self, | ||
| plan: &PlanId, | ||
| update: PlanMetaUpdate, | ||
| ) -> Result<Plan, StoreError> { | ||
| let name = validate_plan_name(plan).map_err(StoreError::InvalidParams)?; | ||
| let dir = plan_dir(&self.dir, &name); | ||
| std::fs::create_dir_all(&dir).map_err(|err| StoreError::Backend(err.into()))?; | ||
| let path = plan_file_path(&self.dir, &name); | ||
| let (existing, body) = if path.exists() { | ||
| let content = | ||
| std::fs::read_to_string(&path).map_err(|err| StoreError::Backend(err.into()))?; | ||
| let (front, body) = parse_plan_frontmatter(&content, &name) | ||
| .map_err(|err| StoreError::Backend(anyhow::anyhow!(err)))?; | ||
| (front, body) | ||
| } else { | ||
| ( | ||
| PlanFrontMatter { | ||
| id: name.clone(), | ||
| created_at: now_iso(), | ||
| ..Default::default() | ||
| }, | ||
| String::new(), | ||
| ) | ||
| }; | ||
| let record = PlanRecord { | ||
| front: PlanFrontMatter { | ||
| id: name.clone(), | ||
| title: update.title.or(existing.title), | ||
| summary: update.summary.or(existing.summary), | ||
| author: update.author.or(existing.author), | ||
| assignee: update.assignee.or(existing.assignee), | ||
| executor: update.executor.or(existing.executor), | ||
| git_branch: update.git_branch.or(existing.git_branch), | ||
| github_owner_repo: update.github_owner_repo.or(existing.github_owner_repo), | ||
| created_at: if existing.created_at.is_empty() { | ||
| now_iso() | ||
| } else { | ||
| existing.created_at | ||
| }, | ||
| updated_at: Some(now_iso()), | ||
| }, | ||
| body, | ||
| }; | ||
| let serialized = | ||
| serialize_plan(&record).map_err(|err| StoreError::Backend(anyhow::anyhow!(err)))?; | ||
| write_plan_file(&path, &serialized) | ||
| .map_err(|err| StoreError::Backend(anyhow::anyhow!(err)))?; | ||
| plan_record_to_domain(record) | ||
| } | ||
|
|
||
| async fn delete_plan(&self, plan: &PlanId) -> Result<(), StoreError> { | ||
| let name = validate_plan_name(plan).map_err(StoreError::InvalidParams)?; | ||
| let dir = plan_dir(&self.dir, &name); | ||
| if !dir.exists() { | ||
| return Err(StoreError::NotFound); | ||
| } | ||
| std::fs::remove_dir_all(&dir).map_err(|err| StoreError::Backend(err.into()))?; | ||
| Ok(()) | ||
| } | ||
|
|
||
| async fn read_plan_body(&self, plan: &PlanId) -> Result<String, StoreError> { | ||
| let name = validate_plan_name(plan).map_err(StoreError::InvalidParams)?; | ||
| let path = plan_file_path(&self.dir, &name); | ||
| let body = if path.exists() { | ||
| std::fs::read_to_string(&path).map_err(|err| StoreError::Backend(err.into()))? | ||
| } else if plan_dir(&self.dir, &name).exists() { | ||
| String::new() | ||
| } else { | ||
| return Err(StoreError::NotFound); | ||
| }; | ||
| let (_, content) = parse_plan_frontmatter(&body, &name) | ||
| .map_err(|err| StoreError::Backend(anyhow::anyhow!(err)))?; | ||
| Ok(content) | ||
| } | ||
|
|
||
| async fn write_plan_body(&self, plan: &PlanId, body: &str) -> Result<(), StoreError> { | ||
| let name = validate_plan_name(plan).map_err(StoreError::InvalidParams)?; | ||
| let dir = plan_dir(&self.dir, &name); | ||
| std::fs::create_dir_all(&dir).map_err(|err| StoreError::Backend(err.into()))?; | ||
| let path = plan_file_path(&self.dir, &name); | ||
| let existing = if path.exists() { | ||
| let content = | ||
| std::fs::read_to_string(&path).map_err(|err| StoreError::Backend(err.into()))?; | ||
| let (front, _) = parse_plan_frontmatter(&content, &name) | ||
| .map_err(|err| StoreError::Backend(anyhow::anyhow!(err)))?; | ||
| front | ||
| } else { | ||
| PlanFrontMatter { | ||
| id: name.clone(), | ||
| created_at: now_iso(), | ||
| ..Default::default() | ||
| } | ||
| }; | ||
| let record = PlanRecord { | ||
| front: existing, | ||
| body: body.to_string(), | ||
| }; | ||
| let serialized = | ||
| serialize_plan(&record).map_err(|err| StoreError::Backend(anyhow::anyhow!(err)))?; | ||
| write_plan_file(&path, &serialized).map_err(|err| StoreError::Backend(anyhow::anyhow!(err))) | ||
| } | ||
|
|
||
| async fn list_tasks( | ||
| &self, | ||
| plan: &PlanId, | ||
| filter: TaskFilter, | ||
| page: Option<PageToken>, | ||
| ) -> Result<Page<Task>, StoreError> { | ||
| let plan_name = validate_plan_name(plan).map_err(StoreError::InvalidParams)?; | ||
| let offset = parse_offset(page)?; | ||
| let tasks = list_tasks_scan( | ||
| &self.dir, | ||
| Some(&plan_name), | ||
| filter.tag.as_deref(), | ||
| filter.status.as_deref(), | ||
| ); | ||
| let total = tasks.len(); | ||
| let items = tasks | ||
| .into_iter() | ||
| .skip(offset) | ||
| .map(task_record_to_domain) | ||
| .collect::<Result<Vec<_>, _>>()?; | ||
| Ok(Page { | ||
| items, | ||
| next: next_page_token(total, total), | ||
| }) | ||
| } | ||
|
|
||
| async fn get_task(&self, plan: &PlanId, task: &String) -> Result<Task, StoreError> { | ||
| let plan_name = validate_plan_name(plan).map_err(StoreError::InvalidParams)?; | ||
| let task = read_task(&self.dir, &plan_name, task).map_err(|_| StoreError::NotFound)?; | ||
| task_record_to_domain(task) | ||
| } | ||
|
|
||
| async fn add_task(&self, plan: &PlanId, new_task: NewTask) -> Result<Task, StoreError> { | ||
| let plan_name = validate_plan_name(plan).map_err(StoreError::InvalidParams)?; | ||
| let plan_path = plan_dir(&self.dir, &plan_name); | ||
| if !plan_path.exists() { | ||
| std::fs::create_dir_all(&plan_path).map_err(|err| StoreError::Backend(err.into()))?; | ||
| } | ||
| let id = validate_id(&new_task.id).map_err(StoreError::InvalidId)?; | ||
| if task_file_path(&self.dir, &plan_name, &id).exists() { | ||
| return Err(StoreError::AlreadyExists); | ||
| } | ||
| let now = now_iso(); | ||
| let task = TaskRecord { | ||
| front: TaskFrontMatter { | ||
| id: id.clone(), | ||
| title: new_task.title, | ||
| summary: new_task.summary, | ||
| author: new_task.author, | ||
| assignee: new_task.assignee, | ||
| executor: new_task.executor, | ||
| tags: new_task.tags, | ||
| plan: plan_name.clone(), | ||
| status: new_task.status.unwrap_or_else(default_open_status), | ||
| created_at: now, | ||
| updated_at: None, | ||
| dependencies: new_task.dependencies, | ||
| }, | ||
| body: String::new(), | ||
| }; | ||
| write_task(&self.dir, &task).map_err(|err| StoreError::Backend(anyhow::anyhow!(err)))?; | ||
| task_record_to_domain(task) | ||
| } | ||
|
|
||
| async fn update_task_meta( | ||
| &self, | ||
| plan: &PlanId, | ||
| task: &String, | ||
| update: TaskMetaUpdate, | ||
| ) -> Result<Task, StoreError> { | ||
| let plan_name = validate_plan_name(plan).map_err(StoreError::InvalidParams)?; | ||
| let mut task = read_task(&self.dir, &plan_name, task).map_err(|_| StoreError::NotFound)?; | ||
| if let Some(title) = update.title { | ||
| task.front.title = title; | ||
| } | ||
| if let Some(summary) = update.summary { | ||
| task.front.summary = Some(summary); | ||
| } | ||
| if let Some(author) = update.author { | ||
| task.front.author = Some(author); | ||
| } | ||
| if let Some(assignee) = update.assignee { | ||
| task.front.assignee = Some(assignee); | ||
| } | ||
| if let Some(executor) = update.executor { | ||
| task.front.executor = Some(executor); | ||
| } | ||
| if let Some(tags) = update.tags { | ||
| task.front.tags = tags; | ||
| } | ||
| if let Some(status) = update.status { | ||
| task.front.status = status; | ||
| } | ||
| if let Some(dependencies) = update.dependencies { | ||
| task.front.dependencies = dependencies; | ||
| } | ||
| task.front.updated_at = Some(now_iso()); | ||
| write_task(&self.dir, &task).map_err(|err| StoreError::Backend(anyhow::anyhow!(err)))?; | ||
| task_record_to_domain(task) | ||
| } | ||
|
|
||
| async fn delete_task(&self, plan: &PlanId, task: &String) -> Result<(), StoreError> { | ||
| let plan_name = validate_plan_name(plan).map_err(StoreError::InvalidParams)?; | ||
| let path = task_file_path(&self.dir, &plan_name, task); | ||
| if !path.exists() { | ||
| return Err(StoreError::NotFound); | ||
| } | ||
| std::fs::remove_file(path).map_err(|err| StoreError::Backend(err.into()))?; | ||
| Ok(()) | ||
| } | ||
|
|
||
| async fn read_task_body(&self, plan: &PlanId, task: &String) -> Result<String, StoreError> { | ||
| let plan_name = validate_plan_name(plan).map_err(StoreError::InvalidParams)?; | ||
| Ok(read_task(&self.dir, &plan_name, task) | ||
| .map_err(|_| StoreError::NotFound)? | ||
| .body) | ||
| } | ||
|
|
||
| async fn write_task_body( | ||
| &self, | ||
| plan: &PlanId, | ||
| task: &String, | ||
| body: &str, | ||
| ) -> Result<(), StoreError> { | ||
| let plan_name = validate_plan_name(plan).map_err(StoreError::InvalidParams)?; | ||
| let mut task = read_task(&self.dir, &plan_name, task).map_err(|_| StoreError::NotFound)?; | ||
| task.body = body.to_string(); | ||
| write_task(&self.dir, &task).map_err(|err| StoreError::Backend(anyhow::anyhow!(err))) | ||
| } | ||
|
|
||
| async fn list_notes( | ||
| &self, | ||
| plan: &PlanId, | ||
| page: Option<PageToken>, | ||
| ) -> Result<Page<Note>, StoreError> { | ||
| let plan = validate_plan_name(plan).map_err(StoreError::InvalidParams)?; | ||
| let offset = parse_offset(page)?; | ||
| let mut notes = Vec::new(); | ||
| let dir = notes_dir(&self.dir, &plan); | ||
| if dir.exists() { | ||
| let mut entries = std::fs::read_dir(&dir) | ||
| .map_err(|err| StoreError::Backend(err.into()))? | ||
| .filter_map(Result::ok) | ||
| .map(|entry| entry.path()) | ||
| .filter(|path| path.extension().and_then(OsStr::to_str) == Some("md")) | ||
| .collect::<Vec<_>>(); | ||
| entries.sort(); | ||
| for path in entries { | ||
| let content = std::fs::read_to_string(&path) | ||
| .map_err(|err| StoreError::Backend(err.into()))?; | ||
| let (front, body) = parse_note_frontmatter(&content) | ||
| .map_err(|err| StoreError::Backend(anyhow::anyhow!(err)))?; | ||
| notes.push(note_record_to_domain(NoteRecord { front, body })?); | ||
| } | ||
| } | ||
| let total = notes.len(); | ||
| let items = notes.into_iter().skip(offset).collect::<Vec<_>>(); | ||
| Ok(Page { | ||
| items, | ||
| next: next_page_token(total, total), | ||
| }) | ||
| } | ||
|
|
||
| async fn get_note(&self, plan: &PlanId, note: &String) -> Result<Note, StoreError> { | ||
| let plan = validate_plan_name(plan).map_err(StoreError::InvalidParams)?; | ||
| let path = note_file_path(&self.dir, &plan, note); | ||
| if !path.exists() { | ||
| return Err(StoreError::NotFound); | ||
| } | ||
| let content = | ||
| std::fs::read_to_string(&path).map_err(|err| StoreError::Backend(err.into()))?; | ||
| let (front, body) = parse_note_frontmatter(&content) | ||
| .map_err(|err| StoreError::Backend(anyhow::anyhow!(err)))?; | ||
| let _ = body; | ||
| note_record_to_domain(NoteRecord { | ||
| front, | ||
| body: String::new(), | ||
| }) | ||
| } | ||
|
|
||
| async fn add_note(&self, plan: &PlanId, new_note: NewNote) -> Result<Note, StoreError> { | ||
| let plan = validate_plan_name(plan).map_err(StoreError::InvalidParams)?; | ||
| std::fs::create_dir_all(notes_dir(&self.dir, &plan)) | ||
| .map_err(|err| StoreError::Backend(err.into()))?; | ||
| let id = validate_id(&new_note.id).map_err(StoreError::InvalidId)?; | ||
| if note_file_path(&self.dir, &plan, &id).exists() { | ||
| return Err(StoreError::AlreadyExists); | ||
| } | ||
| let note = NoteRecord { | ||
| front: NoteFrontMatter { | ||
| id, | ||
| summary: new_note.summary, | ||
| author: new_note.author, | ||
| created_at: now_iso(), | ||
| updated_at: None, | ||
| }, | ||
| body: String::new(), | ||
| }; | ||
| write_note(&self.dir, &plan, ¬e) | ||
| .map_err(|err| StoreError::Backend(anyhow::anyhow!(err)))?; | ||
| note_record_to_domain(note) | ||
| } | ||
|
|
||
| async fn update_note_meta( | ||
| &self, | ||
| plan: &PlanId, | ||
| note: &String, | ||
| update: NoteMetaUpdate, | ||
| ) -> Result<Note, StoreError> { | ||
| let plan = validate_plan_name(plan).map_err(StoreError::InvalidParams)?; | ||
| let path = note_file_path(&self.dir, &plan, note); | ||
| if !path.exists() { | ||
| return Err(StoreError::NotFound); | ||
| } | ||
| let content = | ||
| std::fs::read_to_string(&path).map_err(|err| StoreError::Backend(err.into()))?; | ||
| let (mut front, body) = parse_note_frontmatter(&content) | ||
| .map_err(|err| StoreError::Backend(anyhow::anyhow!(err)))?; | ||
| if let Some(summary) = update.summary { | ||
| front.summary = Some(summary); | ||
| } | ||
| if let Some(author) = update.author { | ||
| front.author = Some(author); | ||
| } | ||
| front.updated_at = Some(now_iso()); | ||
| let note = NoteRecord { front, body }; | ||
| write_note(&self.dir, &plan, ¬e) | ||
| .map_err(|err| StoreError::Backend(anyhow::anyhow!(err)))?; | ||
| note_record_to_domain(note) | ||
| } | ||
|
|
||
| async fn delete_note(&self, plan: &PlanId, note: &String) -> Result<(), StoreError> { | ||
| let plan = validate_plan_name(plan).map_err(StoreError::InvalidParams)?; | ||
| let path = note_file_path(&self.dir, &plan, note); | ||
| if !path.exists() { | ||
| return Err(StoreError::NotFound); | ||
| } | ||
| std::fs::remove_file(path).map_err(|err| StoreError::Backend(err.into()))?; | ||
| Ok(()) | ||
| } | ||
|
|
||
| async fn read_note_body(&self, plan: &PlanId, note: &String) -> Result<String, StoreError> { | ||
| let plan = validate_plan_name(plan).map_err(StoreError::InvalidParams)?; | ||
| let path = note_file_path(&self.dir, &plan, note); | ||
| if !path.exists() { | ||
| return Err(StoreError::NotFound); | ||
| } | ||
| let content = | ||
| std::fs::read_to_string(&path).map_err(|err| StoreError::Backend(err.into()))?; | ||
| let (_, body) = parse_note_frontmatter(&content) | ||
| .map_err(|err| StoreError::Backend(anyhow::anyhow!(err)))?; | ||
| Ok(body) | ||
| } | ||
|
|
||
| async fn write_note_body( | ||
| &self, | ||
| plan: &PlanId, | ||
| note: &String, | ||
| body: &str, | ||
| ) -> Result<(), StoreError> { | ||
| let plan = validate_plan_name(plan).map_err(StoreError::InvalidParams)?; | ||
| let path = note_file_path(&self.dir, &plan, note); | ||
| if !path.exists() { | ||
| return Err(StoreError::NotFound); | ||
| } | ||
| let content = | ||
| std::fs::read_to_string(&path).map_err(|err| StoreError::Backend(err.into()))?; | ||
| let (front, _) = parse_note_frontmatter(&content) | ||
| .map_err(|err| StoreError::Backend(anyhow::anyhow!(err)))?; | ||
| let note = NoteRecord { | ||
| front, | ||
| body: body.to_string(), | ||
| }; | ||
| write_note(&self.dir, &plan, ¬e).map_err(|err| StoreError::Backend(anyhow::anyhow!(err))) | ||
| } | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Blocking filesystem I/O inside async PlanStore methods.
Every method in this impl PlanStore for FilePlanStore block (list_plans, get_plan, add_plan, update_plan_meta, delete_plan, read_plan_body, write_plan_body, and the task/note equivalents) calls std::fs::* synchronously from an async fn. These block the Tokio worker thread for the duration of each disk operation, stealing capacity from all other tasks scheduled on that thread — including concurrent MCP tool calls and the background cleanup_loop, which is shared on the same runtime (see main.rs). Notably, run_cleanup_pass/plan_last_activity in this same file correctly offload their std::fs calls via tokio::task::spawn_blocking; the CRUD path should follow the same pattern (or switch to tokio::fs for simple read/write calls).
As per coding guidelines, "All I/O must be async via Tokio using async fn and .await."
🚀 Example fix pattern (apply throughout the impl block)
async fn read_plan_body(&self, plan: &PlanId) -> Result<String, StoreError> {
let name = validate_plan_name(plan).map_err(StoreError::InvalidParams)?;
- let path = plan_file_path(&self.dir, &name);
- let body = if path.exists() {
- std::fs::read_to_string(&path).map_err(|err| StoreError::Backend(err.into()))?
- } else if plan_dir(&self.dir, &name).exists() {
- String::new()
- } else {
- return Err(StoreError::NotFound);
- };
+ let path = plan_file_path(&self.dir, &name);
+ let dir_path = plan_dir(&self.dir, &name);
+ let body = tokio::task::spawn_blocking(move || -> Result<String, StoreError> {
+ if path.exists() {
+ std::fs::read_to_string(&path).map_err(|err| StoreError::Backend(err.into()))
+ } else if dir_path.exists() {
+ Ok(String::new())
+ } else {
+ Err(StoreError::NotFound)
+ }
+ })
+ .await
+ .map_err(|err| StoreError::Backend(err.into()))??;
let (_, content) = parse_plan_frontmatter(&body, &name)
.map_err(|err| StoreError::Backend(anyhow::anyhow!(err)))?;
Ok(content)
}🤖 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 `@crates/harnx-mcp-plans/src/store_fs.rs` around lines 548 - 1022, The async
PlanStore CRUD methods are doing blocking std::fs I/O on Tokio worker threads,
which can stall concurrent MCP requests and cleanup work. Update the
FilePlanStore impl methods such as list_plans, get_plan, add_plan,
update_plan_meta, delete_plan, read_plan_body, write_plan_body, and the
task/note equivalents to use tokio::fs or offload filesystem work with
tokio::task::spawn_blocking, following the same async pattern already used by
run_cleanup_pass/plan_last_activity in this module.
Source: Coding guidelines
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
crates/harnx-mcp-plans-github/README.md (1)
22-25: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winWarn about PAT exposure through
--token.Command-line arguments may be visible in process listings and shell history. Prefer
GITHUB_TOKENin the documented example, or clearly warn operators to use the flag only in controlled environments.🤖 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 `@crates/harnx-mcp-plans-github/README.md` around lines 22 - 25, Update the options documentation near the `--token` entry to warn that command-line tokens can appear in process listings and shell history; make the documented usage example prefer `GITHUB_TOKEN`, and state that `--token` should only be used in controlled environments.
🤖 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 `@crates/harnx-mcp-plans-github/README.md`:
- Around line 30-32: Update the HTTP options documentation to clearly state that
Streamable HTTP is unauthenticated and must remain bound to loopback by default;
either remove or strongly caution against using `--host 0.0.0.0`, and document
that wider exposure requires an authenticated proxy boundary protecting plan
mutations.
In `@crates/harnx-mcp-plans-github/src/config.rs`:
- Around line 262-286: run_git_with_timeout currently performs synchronous
process and timeout waits during parse_from_env_and_args startup, blocking Tokio
workers. Refactor the git detection path, including parse_from_env_and_args and
run_git_with_timeout, to use Tokio-safe execution: either run the synchronous
command through tokio::task::spawn_blocking or replace it with
tokio::process::Command and an async tokio::time::timeout, propagating timeout,
spawn, and output errors appropriately.
In
`@docs/solutions/integration-issues/github-issues-storage-backend-2026-07-08.md`:
- Around line 207-210: Update the Cargo.toml fenced code block in the
documentation to use the toml language identifier by changing the opening fence
to ```toml, preserving the existing dependency snippet.
- Around line 243-257: Update the documented atomic-create fix to use Tokio
APIs: make the enclosing operation async, replace std::fs::create_dir with
tokio::fs::create_dir(...).await while preserving AlreadyExists handling, and
use tokio::fs::OpenOptions with create_new(true) plus await for task/note files.
Remove blocking std::fs calls from the example.
- Around line 54-70: Update the PlanStore example to use async fn for every
store operation, including list_plans, get_plan, add_plan, update_plan_meta,
delete_plan, read_plan_body, and write_plan_body, matching the actual trait
contract; alternatively label the snippet explicitly as pseudocode if it is
intentionally illustrative.
In `@example_config/mcp_servers/plans-github.yaml`:
- Around line 15-16: Remove the literal token placeholder from the example
configuration near GITHUB_TOKEN, and reference the environment variable
indirectly or leave the setting unset with a prominent warning not to commit
real secrets. Preserve the configuration’s guidance for supplying a personal
access token securely.
---
Nitpick comments:
In `@crates/harnx-mcp-plans-github/README.md`:
- Around line 22-25: Update the options documentation near the `--token` entry
to warn that command-line tokens can appear in process listings and shell
history; make the documented usage example prefer `GITHUB_TOKEN`, and state that
`--token` should only be used in controlled environments.
🪄 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 Plus
Run ID: 7f24d1ba-6ef3-4570-9e45-b656f49f0437
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (30)
.github/workflows/release.yamlCargo.tomlREADME.mdcrates/harnx-mcp-plans-core/Cargo.tomlcrates/harnx-mcp-plans-core/src/conformance.rscrates/harnx-mcp-plans-core/src/lib.rscrates/harnx-mcp-plans-core/src/model.rscrates/harnx-mcp-plans-core/src/server/handler.rscrates/harnx-mcp-plans-core/src/server/handlers.rscrates/harnx-mcp-plans-core/src/server/mod.rscrates/harnx-mcp-plans-core/src/server/params.rscrates/harnx-mcp-plans-core/src/store.rscrates/harnx-mcp-plans-github/.gitignorecrates/harnx-mcp-plans-github/Cargo.tomlcrates/harnx-mcp-plans-github/README.mdcrates/harnx-mcp-plans-github/src/auth.rscrates/harnx-mcp-plans-github/src/client.rscrates/harnx-mcp-plans-github/src/codec.rscrates/harnx-mcp-plans-github/src/config.rscrates/harnx-mcp-plans-github/src/lib.rscrates/harnx-mcp-plans-github/src/main.rscrates/harnx-mcp-plans-github/src/ratelimit.rscrates/harnx-mcp-plans-github/src/runtime.rscrates/harnx-mcp-plans-github/src/store_github.rscrates/harnx-mcp-plans-github/src/store_github/tests.rscrates/harnx-mcp-plans-github/tests/conformance_github.rscrates/harnx-mcp-plans-github/tests/github_specific.rscrates/harnx-mcp-plans-github/tests/live_e2e.rsdocs/solutions/integration-issues/github-issues-storage-backend-2026-07-08.mdexample_config/mcp_servers/plans-github.yaml
✅ Files skipped from review due to trivial changes (2)
- crates/harnx-mcp-plans-github/.gitignore
- README.md
🚧 Files skipped from review as they are similar to previous changes (23)
- crates/harnx-mcp-plans-core/Cargo.toml
- crates/harnx-mcp-plans-github/src/main.rs
- crates/harnx-mcp-plans-core/src/lib.rs
- crates/harnx-mcp-plans-github/Cargo.toml
- crates/harnx-mcp-plans-core/src/model.rs
- .github/workflows/release.yaml
- crates/harnx-mcp-plans-core/src/server/mod.rs
- crates/harnx-mcp-plans-core/src/server/handler.rs
- crates/harnx-mcp-plans-core/src/store.rs
- crates/harnx-mcp-plans-github/src/lib.rs
- crates/harnx-mcp-plans-github/tests/live_e2e.rs
- Cargo.toml
- crates/harnx-mcp-plans-core/src/conformance.rs
- crates/harnx-mcp-plans-core/src/server/params.rs
- crates/harnx-mcp-plans-github/src/runtime.rs
- crates/harnx-mcp-plans-github/src/store_github.rs
- crates/harnx-mcp-plans-github/tests/conformance_github.rs
- crates/harnx-mcp-plans-github/src/ratelimit.rs
- crates/harnx-mcp-plans-github/tests/github_specific.rs
- crates/harnx-mcp-plans-core/src/server/handlers.rs
- crates/harnx-mcp-plans-github/src/codec.rs
- crates/harnx-mcp-plans-github/src/client.rs
- crates/harnx-mcp-plans-github/src/store_github/tests.rs
| | `--http` | - | - | Serve MCP over Streamable HTTP instead of stdio. | | ||
| | `--host <addr>` | - | `127.0.0.1` | Bind address for HTTP mode. Set explicitly to `0.0.0.0` or another interface for wider exposure. | | ||
| | `--port <N>` | - | `3000` | Bind port for HTTP mode. | |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n 'Authorization|Bearer|auth|middleware|StreamableHttp|0\.0\.0\.0' \
crates/harnx-mcp-plans-github/srcRepository: dobesv/harnx
Length of output: 9552
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== runtime.rs outline =="
ast-grep outline crates/harnx-mcp-plans-github/src/runtime.rs --view expanded || true
echo
echo "== runtime.rs relevant lines =="
sed -n '140,230p' crates/harnx-mcp-plans-github/src/runtime.rs
echo
echo "== README lines around HTTP options =="
sed -n '20,40p' crates/harnx-mcp-plans-github/README.md
echo
echo "== search for HTTP auth / middleware / transport config =="
rg -n 'StreamableHttpServerConfig|StreamableHttpService|middleware|Authorization|bearer|auth|authenticate|token|proxy|0\.0\.0\.0|127\.0\.0\.1' crates/harnx-mcp-plans-github/src crates/harnx-mcp-plans-github/README.mdRepository: dobesv/harnx
Length of output: 22631
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Cargo.toml dependency versions =="
sed -n '1,220p' crates/harnx-mcp-plans-github/Cargo.toml
echo
echo "== runtime.rs lines 160-210 =="
sed -n '160,210p' crates/harnx-mcp-plans-github/src/runtime.rs
echo
echo "== lib.rs exports =="
sed -n '1,80p' crates/harnx-mcp-plans-github/src/lib.rs
echo
echo "== search for any inbound request checks in MCP server code =="
rg -n 'Authorization|auth.*header|header.*auth|SessionManager|NeverSessionManager|Require.*Auth|reject.*request|middleware|tower::Service|axum::middleware|Extension<|State<' crates/harnx-mcp-plans-github/srcRepository: dobesv/harnx
Length of output: 4447
Keep HTTP loopback-only unless auth is added
crates/harnx-mcp-plans-github/README.md:30-32 The Streamable HTTP path is unauthenticated (NeverSessionManager), so --host 0.0.0.0 would expose plan mutations to any reachable client. Keep the default loopback-only guidance or document the required authenticated proxy boundary.
🤖 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 `@crates/harnx-mcp-plans-github/README.md` around lines 30 - 32, Update the
HTTP options documentation to clearly state that Streamable HTTP is
unauthenticated and must remain bound to loopback by default; either remove or
strongly caution against using `--host 0.0.0.0`, and document that wider
exposure requires an authenticated proxy boundary protecting plan mutations.
| ``` | ||
| # Cargo.toml | ||
| jsonwebtoken = { version = "10", features = ["aws_lc_rs", "use_pem"] } | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language identifier to the Cargo.toml fence.
Use toml after the opening fence so Markdown lint passes and syntax highlighting is correct.
-```
+```toml🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 207-207: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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
`@docs/solutions/integration-issues/github-issues-storage-backend-2026-07-08.md`
around lines 207 - 210, Update the Cargo.toml fenced code block in the
documentation to use the toml language identifier by changing the opening fence
to ```toml, preserving the existing dependency snippet.
Source: Linters/SAST tools
| **Fix (Commit 7f2d1bde):** Atomic exclusive create on leaf directory: | ||
|
|
||
| ```rust | ||
| let dir = plan_dir(&self.dir, &name); | ||
| match std::fs::create_dir(&dir) { // NOT create_dir_all! | ||
| Ok(()) => {} | ||
| Err(err) if err.kind() == std::io::ErrorKind::AlreadyExists => { | ||
| return Err(StoreError::AlreadyExists); | ||
| } | ||
| Err(err) => return Err(StoreError::Backend(err.into())), | ||
| } | ||
| // Parent dirs created separately with create_dir_all if needed | ||
| ``` | ||
|
|
||
| For task/note files, use `OpenOptions::create_new(true)` — fails if file exists. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Use Tokio filesystem APIs in the documented atomic-create fix.
The example uses blocking std::fs::create_dir; replace it with tokio::fs::create_dir(...).await and the Tokio OpenOptions equivalent for file creation.
As per coding guidelines, all I/O must be async via Tokio using async fn and .await.
🤖 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
`@docs/solutions/integration-issues/github-issues-storage-backend-2026-07-08.md`
around lines 243 - 257, Update the documented atomic-create fix to use Tokio
APIs: make the enclosing operation async, replace std::fs::create_dir with
tokio::fs::create_dir(...).await while preserving AlreadyExists handling, and
use tokio::fs::OpenOptions with create_new(true) plus await for task/note files.
Remove blocking std::fs calls from the example.
Source: Coding guidelines
Introduce a GitHub Issues-backed storage backend for the plans MCP server, extracting the backend-agnostic layer into a shared `harnx-mcp-plans-core` crate (PlanStore trait + handler/schema/tool layer + conformance suite) and adding `harnx-mcp-plans-github` (issue=plan, sub-issue=task, comment=note; YAML front-matter metadata; PAT + GitHub App auth; rate-limit-aware executor; delete=close + retention; read-side dedupe; pagination without Search API). The filesystem backend is migrated to `FilePlanStore` behind the same trait with existing behavior preserved. Repo auto-detection: the target GitHub repo is detected from the git `origin` remote of the working directory (github.com only) — there is no `--repo` flag or `GITHUB_OWNER_REPO`. Startup fails fast if not in a git repo, no origin, non-github origin, or unparseable. Git invoked via fixed argv with a bounded timeout. Hardening from PR review: - HTTP: keep rmcp default host allowlist and default bind to 127.0.0.1 (drop disable_allowed_hosts) to prevent DNS-rebinding exposure. - config: normalize base_url trailing slash consistently. - FilePlanStore: fix TOCTOU on create (atomic exclusive create) and lost-update races via per-entity async locks across all RMW/delete paths. Issue: #949
Review comment responses (Atlas)Force-pushed Fixed in this push:
Already addressed in the current code (comments re-anchored from an earlier commit snapshot):
Deferred (non-blocking follow-ups, tracked in the solution doc's "Known Limitations"):
CI note: the previous failing run was a pre-existing flaky |
There was a problem hiding this comment.
Gates Failed
(14 new files with code health below 10.00)
(6 files with Bumpy Road Ahead, Low Cohesion, Deep, Nested Complexity)
(14 files with Code Duplication, Large Method, Complex Method, Excess Number of Function Arguments, Complex Conditional, String Heavy Function Arguments)
Our agent can fix these. Install it.
Gates Passed
Reason for failure
| New code is healthy | Violations | Code Health Impact | |
|---|---|---|---|
| handlers.rs | 7 rules | 5.66 | Suppress |
| store_github.rs | 2 rules | 7.11 | Suppress |
| conformance.rs | 3 rules | 7.17 | Suppress |
| config.rs | 5 rules | 8.13 | Suppress |
| tests.rs | 3 rules | 8.28 | Suppress |
| conformance_github.rs | 3 rules | 8.34 | Suppress |
| client.rs | 2 rules | 8.68 | Suppress |
| mod.rs | 2 rules | 8.82 | Suppress |
| live_e2e.rs | 2 rules | 9.00 | Suppress |
| ratelimit.rs | 1 rule | 9.39 | Suppress |
| github_specific.rs | 1 rule | 9.39 | Suppress |
| runtime.rs | 1 rule | 9.59 | Suppress |
| handler.rs | 1 rule | 9.60 | Suppress |
| codec.rs | 1 rule | 9.69 | Suppress |
| Enforce critical code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| handlers.rs | 3 critical rules | 5.66 | Suppress |
| store_github.rs | 1 critical rule | 7.11 | Suppress |
| conformance.rs | 1 critical rule | 7.17 | Suppress |
| config.rs | 1 critical rule | 8.13 | Suppress |
| conformance_github.rs | 1 critical rule | 8.34 | Suppress |
| client.rs | 1 critical rule | 8.68 | Suppress |
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| handlers.rs | 4 advisory rules | 5.66 | Suppress |
| store_github.rs | 1 advisory rule | 7.11 | Suppress |
| conformance.rs | 2 advisory rules | 7.17 | Suppress |
| config.rs | 4 advisory rules | 8.13 | Suppress |
| tests.rs | 3 advisory rules | 8.28 | Suppress |
| conformance_github.rs | 2 advisory rules | 8.34 | Suppress |
| client.rs | 1 advisory rule | 8.68 | Suppress |
| mod.rs | 2 advisory rules | 8.82 | Suppress |
| live_e2e.rs | 2 advisory rules | 9.00 | Suppress |
| ratelimit.rs | 1 advisory rule | 9.39 | Suppress |
| github_specific.rs | 1 advisory rule | 9.39 | Suppress |
| runtime.rs | 1 advisory rule | 9.59 | Suppress |
| handler.rs | 1 advisory rule | 9.60 | Suppress |
| codec.rs | 1 advisory rule | 9.69 | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/harnx-mcp-plans-github/tests/github_specific.rs (1)
45-78: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate
mock_issue_responseandmock_plan_issue_responseto resolve CodeScene duplication.The two helpers differ only by the
labelsfield. A single function with an optionallabelsparameter eliminates the duplication flagged by CodeScene while keeping call sites readable.♻️ Proposed consolidation
fn mock_issue_response(id: u64, number: u64, title: &str, body: &str) -> String { + mock_issue_response_with_labels(id, number, title, body, None) +} + +fn mock_plan_issue_response(id: u64, number: u64, title: &str, body: &str) -> String { + mock_issue_response_with_labels( + id, + number, + title, + body, + Some(vec![serde_json::json!({"id": 1, "name": "harnx-plan"})]), + ) +} + +fn mock_issue_response_with_labels( + id: u64, + number: u64, + title: &str, + body: &str, + labels: Option<Vec<serde_json::Value>>, +) -> String { let created = Timestamp::now().to_string(); serde_json::json!({ "id": id, "number": number, "title": title, "body": body, "node_id": format!("I_kwDOA{}", id), "state": "open", + ...(labels.map_or(serde_json::Value::Null, |l| serde_json::Value::Array(l))), "created_at": created, "updated_at": created, "comments": 0 }) .to_string() }🤖 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 `@crates/harnx-mcp-plans-github/tests/github_specific.rs` around lines 45 - 78, Consolidate mock_issue_response and mock_plan_issue_response into one helper by adding an optional labels parameter to mock_issue_response and conditionally including the labels field in its JSON output. Update all existing call sites to pass labels when creating plan issues and None otherwise, then remove mock_plan_issue_response.Source: Pipeline failures
🤖 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 `@crates/harnx-mcp-plans-github/src/config.rs`:
- Around line 110-113: The base URL construction in the configuration
initialization bypasses validation performed by auth.rs::normalize_base_url.
Apply normalize_base_url to the selected argument/environment/default URL before
constructing AuthConfig, propagate a clear configuration error when
normalization fails, and add tests covering malformed URLs and unsupported
schemes for both affected paths.
- Around line 288-303: Make app_auth_from_env asynchronous and await
crate::auth::load_private_key, which must use tokio::fs::read_to_string for file
paths instead of std::fs::read_to_string. Propagate async through every caller
of app_auth_from_env and the configuration entrypoint, updating call sites to
await the resulting futures while preserving existing error context.
---
Nitpick comments:
In `@crates/harnx-mcp-plans-github/tests/github_specific.rs`:
- Around line 45-78: Consolidate mock_issue_response and
mock_plan_issue_response into one helper by adding an optional labels parameter
to mock_issue_response and conditionally including the labels field in its JSON
output. Update all existing call sites to pass labels when creating plan issues
and None otherwise, then remove mock_plan_issue_response.
🪄 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 Plus
Run ID: dc8916f7-cfeb-4cd8-bd89-709182649312
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (30)
.github/workflows/release.yamlCargo.tomlREADME.mdcrates/harnx-mcp-plans-core/Cargo.tomlcrates/harnx-mcp-plans-core/src/conformance.rscrates/harnx-mcp-plans-core/src/lib.rscrates/harnx-mcp-plans-core/src/model.rscrates/harnx-mcp-plans-core/src/server/handler.rscrates/harnx-mcp-plans-core/src/server/handlers.rscrates/harnx-mcp-plans-core/src/server/mod.rscrates/harnx-mcp-plans-core/src/server/params.rscrates/harnx-mcp-plans-core/src/store.rscrates/harnx-mcp-plans-github/.gitignorecrates/harnx-mcp-plans-github/Cargo.tomlcrates/harnx-mcp-plans-github/README.mdcrates/harnx-mcp-plans-github/src/auth.rscrates/harnx-mcp-plans-github/src/client.rscrates/harnx-mcp-plans-github/src/codec.rscrates/harnx-mcp-plans-github/src/config.rscrates/harnx-mcp-plans-github/src/lib.rscrates/harnx-mcp-plans-github/src/main.rscrates/harnx-mcp-plans-github/src/ratelimit.rscrates/harnx-mcp-plans-github/src/runtime.rscrates/harnx-mcp-plans-github/src/store_github.rscrates/harnx-mcp-plans-github/src/store_github/tests.rscrates/harnx-mcp-plans-github/tests/conformance_github.rscrates/harnx-mcp-plans-github/tests/github_specific.rscrates/harnx-mcp-plans-github/tests/live_e2e.rsdocs/solutions/integration-issues/github-issues-storage-backend-2026-07-08.mdexample_config/mcp_servers/plans-github.yaml
✅ Files skipped from review due to trivial changes (3)
- crates/harnx-mcp-plans-core/Cargo.toml
- crates/harnx-mcp-plans-github/.gitignore
- README.md
🚧 Files skipped from review as they are similar to previous changes (21)
- example_config/mcp_servers/plans-github.yaml
- crates/harnx-mcp-plans-github/Cargo.toml
- crates/harnx-mcp-plans-github/src/lib.rs
- crates/harnx-mcp-plans-core/src/lib.rs
- crates/harnx-mcp-plans-core/src/server/handler.rs
- .github/workflows/release.yaml
- Cargo.toml
- crates/harnx-mcp-plans-github/tests/conformance_github.rs
- crates/harnx-mcp-plans-core/src/conformance.rs
- crates/harnx-mcp-plans-core/src/store.rs
- crates/harnx-mcp-plans-core/src/server/params.rs
- crates/harnx-mcp-plans-core/src/server/handlers.rs
- crates/harnx-mcp-plans-github/tests/live_e2e.rs
- crates/harnx-mcp-plans-core/src/model.rs
- crates/harnx-mcp-plans-github/src/ratelimit.rs
- crates/harnx-mcp-plans-github/src/runtime.rs
- crates/harnx-mcp-plans-github/src/store_github/tests.rs
- crates/harnx-mcp-plans-github/src/client.rs
- crates/harnx-mcp-plans-github/src/store_github.rs
- crates/harnx-mcp-plans-github/src/codec.rs
- crates/harnx-mcp-plans-core/src/server/mod.rs
| let base_url = first_non_empty(base_url_arg, env("GITHUB_API_URL")) | ||
| .unwrap_or_else(|| DEFAULT_GITHUB_API_URL.to_string()) | ||
| .trim_end_matches('/') | ||
| .to_string(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate the API base URL, not just its trailing slash.
This path directly constructs AuthConfig, accepting malformed URLs and unsupported schemes that auth.rs::normalize_base_url rejects. Reuse that normalizer and add invalid URL/scheme tests.
Also applies to: 423-435
🤖 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 `@crates/harnx-mcp-plans-github/src/config.rs` around lines 110 - 113, The base
URL construction in the configuration initialization bypasses validation
performed by auth.rs::normalize_base_url. Apply normalize_base_url to the
selected argument/environment/default URL before constructing AuthConfig,
propagate a clear configuration error when normalization fails, and add tests
covering malformed URLs and unsupported schemes for both affected paths.
| fn app_auth_from_env<F>(env: &F) -> Result<Option<AppAuthConfig>> | ||
| where | ||
| F: Fn(&str) -> Option<String>, | ||
| { | ||
| let app_id = first_non_empty(None, env("GITHUB_APP_ID")); | ||
| let private_key = first_non_empty(None, env("GITHUB_APP_PRIVATE_KEY")); | ||
| let installation_id = first_non_empty(None, env("GITHUB_APP_INSTALLATION_ID")); | ||
|
|
||
| match (app_id, private_key, installation_id) { | ||
| (None, None, None) => Ok(None), | ||
| (Some(app_id), Some(private_key), Some(installation_id)) => Ok(Some(AppAuthConfig { | ||
| app_id, | ||
| private_key_pem: crate::auth::load_private_key(&private_key) | ||
| .context("load GitHub App private key")?, | ||
| installation_id, | ||
| })), |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Load GitHub App private keys asynchronously.
auth::load_private_key uses std::fs::read_to_string when given a path. Convert it to tokio::fs::read_to_string and propagate async through the configuration entrypoint.
As per coding guidelines, “All I/O must be async via Tokio using async fn and .await.” <coding_guidelines>
🤖 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 `@crates/harnx-mcp-plans-github/src/config.rs` around lines 288 - 303, Make
app_auth_from_env asynchronous and await crate::auth::load_private_key, which
must use tokio::fs::read_to_string for file paths instead of
std::fs::read_to_string. Propagate async through every caller of
app_auth_from_env and the configuration entrypoint, updating call sites to await
the resulting futures while preserving existing error context.
Source: Coding guidelines
1 participant
Refactors the harnx-mcp-plans crate into a shared core library and adds a new GitHub Issues-backed storage provider. The refactoring extracts the PlanStore trait and common tool handlers into harnx-mcp-plans-core, ensuring consistent behavior across different storage backends.
The new harnx-mcp-plans-github crate implements the PlanStore trait using GitHub Issues, mapping plans to issues, tasks to sub-issues, and notes to comments. Metadata is persisted via YAML front-matter in issue bodies. Features include PAT and GitHub App authentication, rate-limit handling with backoff, background retention, and JIRA cross-referencing.
Includes a comprehensive conformance test suite used to validate both filesystem and GitHub backends.
Issue: #949
Summary by CodeRabbit