Repository navigation
Two-way GitHub Sync (Cycle 0021) - #7
Merged
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 14 minutes and 41 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (19)
WalkthroughReplaces one-way GitHub sync with explicit two-way operations: Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Runner
participant GitHubAdapter as GitHub Adapter
participant Workspace as Local Workspace
participant GitHub as GitHub API
rect rgba(100,150,200,0.5)
note over CLI,GitHub: Push Flow (Local → Remote)
CLI->>GitHubAdapter: pushBacklog()
loop For each backlog item
alt github_issue_id exists
GitHubAdapter->>GitHub: PATCH /issues/{id}
GitHub-->>GitHubAdapter: updated issue
GitHubAdapter->>Workspace: (no frontmatter ID change)
Workspace-->>GitHubAdapter: ack
GitHubAdapter-->>CLI: {action: 'push'}
else github_issue_id missing
GitHubAdapter->>GitHub: POST /issues
GitHub-->>GitHubAdapter: created issue
GitHubAdapter->>Workspace: updateBody() + write frontmatter (github_issue_id,url)
Workspace-->>GitHubAdapter: persisted
GitHubAdapter-->>CLI: {action: 'create'}
end
end
end
rect rgba(200,150,100,0.5)
note over CLI,GitHub: Pull Flow (Remote → Local)
CLI->>GitHubAdapter: pullBacklog()
loop For each backlog item
alt github_issue_id missing
GitHubAdapter-->>CLI: {action: 'skip'}
else github_issue_id exists
GitHubAdapter->>GitHub: GET /issues/{id}
GitHubAdapter->>GitHub: GET /issues/{id}/comments
GitHub-->>GitHubAdapter: {state, labels, comments}
GitHubAdapter->>Workspace: updateBody() + set github_labels + append ## GitHub Comments (if absent)
Workspace-->>GitHubAdapter: persisted
alt state == 'closed'
GitHubAdapter->>Workspace: moveBacklogItem(→graveyard)
Workspace-->>GitHubAdapter: moved
end
GitHubAdapter-->>CLI: {action: 'pull'}
end
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c280ec2efc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| const [owner, repo] = repoFull.split('/'); | ||
| const adapter = new GitHubAdapter({ |
There was a problem hiding this comment.
method_sync_github creates new GitHubAdapter(...), but this module never imports GitHubAdapter, so calling the tool throws a ReferenceError at runtime and the new MCP GitHub sync path cannot execute. This is a hard failure for every method_sync_github call until the import is added.
Useful? React with 👍 / 👎.
| const log: string[] = []; | ||
| if (push) { | ||
| const results = await adapter.pushBacklog(); | ||
| log.push(...results.filter(r => !r.skipped).map(r => `${r.action === 'create' ? 'Created' : 'Updated'} GitHub Issue #${r.issue?.number} from ${r.path}`)); |
There was a problem hiding this comment.
This branch formats every non-skipped pushBacklog() result as "Created/Updated" even when the adapter returned error (the adapter captures API failures per item), so failed syncs are reported as successful with #undefined and the tool response is not marked as an error. That can silently hide failed writes and leave callers believing the backlog is synchronized when it is not.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
Already addressed in commit 6566f98 — error results are now filtered and surfaced with hasError flag. See src/mcp.ts:176-179 and src/mcp.ts:191-194. ✅
Comment on lines
+130
to
+132
| if (!localBody.includes('## GitHub Comments')) { | ||
| this.workspace.updateBody(relativePath, localBody + commentSection); | ||
| } |
There was a problem hiding this comment.
The pull path only appends comments when the local body does not yet contain ## GitHub Comments; after the first pull, subsequent pulls never update that section even if new remote comments are added or edited. This causes persistent local drift for comment data, which contradicts pull sync expectations.
Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment.
By design — comment sync is documented as additive-only in the retro's New Debt section. Full comment diffing is out of scope for this cycle. ✅
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/design/0021-two-way-github-sync/two-way-github-sync.md`:
- Around line 63-66: Update the doc to resolve the inconsistency between the
"Non-goals" section and the described pull behavior: either narrow the Non-goal
bullet to say "Conflicts resolution (filesystem wins for title/body content on
push; other metadata like labels, status, and comments may be updated on pull)"
or change the pull behavior text to state that pulls will not override local
frontmatter/title/body and will only fetch remote labels/status/comments; ensure
you update both the "Non-goals" heading and the pull behavior sentences that
mention updating frontmatter/labels/status/comments so they consistently reflect
the chosen authoritative source.
- Around line 30-35: The design omits behavior when both --push and --pull are
provided for the "method sync github" command; add a single clarifying sentence
and a playback question in that section stating whether the flags are mutually
exclusive or should run sequentially (and if sequential, explicitly state the
order, e.g., push then pull), and note that current implementation in the CLI
flag handling and MCP sync logic runs both sequentially—so update the doc to
choose one behavior or explicitly document the current push-then-pull behavior
to avoid ambiguity.
- Around line 30-31: Update the design doc so the "Hill" section explicitly
states that the sync github command defaults to push when neither --push nor
--pull is provided: mention that `sync github` uses push=true by default
(consistent with the CLI parsing in cli-args.ts and behavior in mcp.ts) and then
adjust the checklist line for `method sync github --push (or default)` to remove
ambiguity (either leave “--push (default)” or rewrite the Hill paragraph to
present push/pull symmetry and then call out the default explicitly).
- Around line 15-24: Remove the single trailing spaces on the affected markdown
lines under the "## Hill" section—specifically the lines containing the sentence
after the "## Hill" header, the line ending "the authority; the adapter will:",
and the list items for "Push" and "Pull"—so each of those lines has no trailing
whitespace (fixes MD009 from markdownlint-cli2); simply edit the
two-way-github-sync.md content to trim the trailing space characters from the
"## Hill" header block and the "Push"/"Pull" list lines.
In `@docs/invariants/sponsor-abstractness.md`:
- Around line 19-25: Update the "How do you check?" section to note that this
invariant is also enforced automatically by the repository's documentation test
suite (tests/docs.test.ts); explicitly add a short sentence such as "This is
enforced by the automated docs test (tests/docs.test.ts)" so readers know the
check is run by the test harness rather than only manual inspection. Reference
the "How do you check?" heading and the tests/docs.test.ts file in the new
sentence to make the enforcement mechanism discoverable.
In `@docs/method/retro/0021-two-way-github-sync/two-way-github-sync.md`:
- Around line 27-35: Remove the trailing whitespace characters in the "Drift"
and "New Debt" section headers/content so the markdown lines no longer end with
spaces; edit the lines containing "## Drift" and "## New Debt" (and the adjacent
bullet text that currently has trailing spaces) to trim end-of-line spaces for
lint compliance.
In `@docs/method/retro/0021-two-way-github-sync/witness/verification.md`:
- Around line 12-24: Add explicit language specifiers to the fenced code blocks
that contain the terminal output so the linter stops warning and rendering is
consistent: change the ``` fences around the block containing "> method@0.2.0
test" to ```text and likewise change the fences around the block containing "No
playback-question drift found." to ```text (these are the blocks at the shown
snippets around lines 12 and 28-32).
In `@README.md`:
- Around line 10-11: Remove the single trailing spaces at the ends of the two
lines containing ' "Repository Operator", "System Architect"). Both must agree
before work' so they conform to Markdown linting (use 0 trailing spaces for a
normal sentence continuation); update the two lines to have no trailing
whitespace and verify with your linter or run a trim-whitespace check to ensure
no other trailing spaces remain.
In `@src/adapters/github.ts`:
- Around line 82-95: The code uses Number.parseInt(frontmatter.github_issue_id,
10) which will accept malformed values like "42abc" or produce NaN; add a strict
numeric validation before calling updateIssue: check that
frontmatter.github_issue_id is a string matching /^\d+$/ (or otherwise a safe
integer) and only then parseInt and call updateIssue; if it fails validation,
treat it as missing (fallback to this.createIssue and updateFrontmatter) or
surface a clear error. Apply the same strict validation to the other occurrence
that uses Number.parseInt (the push/pull block referencing github_issue_id) so
both update and create paths handle user-edited frontmatter safely.
- Around line 75-80: pushItem currently sends the entire local markdown body
(including the generated "## GitHub Comments" transcript created by pullItem) to
createIssue/updateIssue, causing the mirrored comments to be written back to
GitHub; update pushItem to strip the generated comments block before building
title/body for createIssue/updateIssue (detect the "## GitHub Comments" header
or the specific markers used by pullItem and remove that section) and ensure
pullItem continues to replace/insert that block on pulls so the local transcript
is regenerated only from GitHub comments. Reference: pushItem, pullItem,
createIssue, updateIssue, and the "## GitHub Comments" block.
In `@src/cli-args.ts`:
- Around line 55-64: The sync argument parsing currently accepts stray args
(e.g., typos like "--pul") and silently treats them as defaults; update the
parsing logic that checks rest[0] ('github' and 'ship'), finalPush, and pull so
it first validates that only the allowed flags are present: for adapter 'github'
allow exclusively '--push' and '--pull' (no other tokens besides those two), and
for adapter 'ship' require no extra flags; if any unknown flag or extra arg is
found throw the existing MethodError with the Usage message instead of
proceeding to return the sync object. Ensure you reference and adjust the
branches that produce { command: 'sync', adapter: 'github', push: finalPush,
pull } and { command: 'sync', adapter: 'ship' } to run only after validation.
In `@src/cli.ts`:
- Around line 112-137: Add an explicit check before calling
adapter.pushBacklog()/adapter.pullBacklog(): if both parsed.push and parsed.pull
are falsy, write an error/warning via stderr using the existing alert/helper
(e.g., message "No sync direction specified. Use --push and/or --pull.") and
return a non-zero exit code (e.g., return 1) instead of falling through to
return 0; update the branch near parsed.push/parsed.pull in cli.ts so that
parsed.push, parsed.pull, adapter.pushBacklog, adapter.pullBacklog and the final
return are skipped when this validation fails.
- Around line 118-122: The success messages use optional chaining
result.issue?.number which can render "undefined"; update the branches handling
result.action === 'create' and 'push' (the code that calls stdout.write with
alert) to first assert or guard that result.issue exists and has a number:
either throw/log an error if issue is required (e.g., if (!result.issue?.number)
throw new Error(...)) or use a safe fallback when composing the message (e.g.,
const issueLabel = result.issue?.number ?? '<unknown>'; then call
stdout.write(alert(...`#${issueLabel}`...))). Ensure you change both the
'create' and 'push' branches (and any other similar branches) so the alert never
interpolates an undefined number and reference the existing stdout.write/alert
calls and result.action check when applying the fix.
In `@src/index.ts`:
- Around line 340-363: In moveBacklogItem, fix the target directory mapping so
the special 'root' lane resolves to the backlog directory (docs/method/backlog)
rather than BACKLOG_DIR + '/root' — i.e., when targetLane === 'root' use
resolve(this.root, BACKLOG_DIR) (or the equivalent backlog path used by
status()), and replace the local slash-only basename helper with the
platform-aware path.basename from the path module so Windows paths are parsed
correctly; ensure the comparisons that use resolve(fullPath, targetPath) still
work after switching to path.basename and the corrected targetDir.
In `@src/mcp.ts`:
- Around line 169-179: The code treats any non-skipped result from
adapter.pushBacklog() / adapter.pullBacklog() as a success message even when
result.error is set; update the mapping to explicitly check for r.error and
handle three cases: successful items (!r.skipped && !r.error) should produce the
current success lines, errored items (!r.skipped && r.error) should produce a
clear error line that includes r.path, r.issue?.number and r.error message, and
skipped items remain filtered out; also ensure the final returned payload (the
object returned at the end) reflects that there was an error by setting an
appropriate isError flag (e.g., set isError: true when any r.error was present)
so callers don’t treat failures as successes—look for pushBacklog, pullBacklog,
results, log and the final return { content: [...] } to implement this.
- Around line 162-167: Add the missing import for GitHubAdapter and change the
MCP handler's sync-result processing to surface per-item errors: import
GitHubAdapter so the new const adapter = new GitHubAdapter(...) compiles, then
when handling the sync results (the array of results used to compute filtered
results and the response flag isError) include items that have an error (not
just !r.skipped) and if any result has r.error set, set isError: true on the
response and include those error items in the returned diagnostics instead of
dropping them; update the logic around the result filtering and the response
isError calculation in the MCP handler that references the results array to
reflect these changes.
In `@tests/docs.test.ts`:
- Around line 143-156: The test "enforces sponsor abstractness in design
documents" currently skips files when the sponsors regex (sponsorsMatch) doesn't
capture groups and only checks for an "@" prefix, so update the test to fail
explicitly when a "## Sponsors" heading exists but the expected format didn't
match (inspect content for /^## Sponsors\b/ and throw/expect failure if present
but sponsorsMatch is undefined), and strengthen literal-name detection by
expanding the validation for human/agent (variables human and agent) to reject
common literal patterns — e.g., a deny-list of known agent names and a regex
that flags single capitalized words or all-letter tokens (like /^[A-Z][a-z]+$/
or /^(Gemini|Claude|James|Bard|GPT|GPT-\d+)/i) — so the test asserts these
patterns are invalid rather than only checking for an "@" prefix.
In `@tests/github-adapter.test.ts`:
- Around line 169-175: Remove the two tautological test cases in
tests/github-adapter.test.ts that only contain comments (the it() blocks whose
titles mention `GitHubAdapter.pushBacklog()` / `GitHubAdapter.pullBacklog()` and
claiming remote-to-local/local-to-remote proof); either delete these empty specs
or replace them with real integration tests that exercise the CLI/MCP
entrypoints and assert observable side effects, referencing the actual methods
`GitHubAdapter.pushBacklog` and `GitHubAdapter.pullBacklog` (or the CLI entry
functions) when you implement the meaningful checks.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 49f128f7-eb32-4d72-84a8-8d58781e3308
📒 Files selected for processing (31)
CHANGELOG.mdREADME.mddocs/BEARING.mddocs/VISION.mddocs/design/0009-generated-signpost-provenance/generated-signpost-provenance.mddocs/design/0010-yaml-frontmatter-schema/yaml-frontmatter-schema.mddocs/design/0011-library-api-surface/library-api-surface.mddocs/design/0012-mcp-server/mcp-server.mddocs/design/0013-executive-summary-protocol/executive-summary-protocol.mddocs/design/0014-github-issue-adapter/github-issue-adapter.mddocs/design/0015-git-branch-workflow-policy/git-branch-workflow-policy.mddocs/design/0016-system-style-javascript-adoption/system-style-javascript-adoption.mddocs/design/0017-behavior-spike-convention/behavior-spike-convention.mddocs/design/0018-ship-sync-automation/ship-sync-automation.mddocs/design/0019-config-management/config-management.mddocs/design/0020-automated-witness-capture/automated-witness-capture.mddocs/design/0021-two-way-github-sync/two-way-github-sync.mddocs/invariants/sponsor-abstractness.mddocs/method/backlog/inbox/PROCESS_i18n-string-extraction.mddocs/method/backlog/up-next/PROCESS_two-way-github-sync.mddocs/method/legends/PROCESS.mddocs/method/process.mddocs/method/retro/0021-two-way-github-sync/two-way-github-sync.mddocs/method/retro/0021-two-way-github-sync/witness/verification.mdsrc/adapters/github.tssrc/cli-args.tssrc/cli.tssrc/index.tssrc/mcp.tstests/docs.test.tstests/github-adapter.test.ts
💤 Files with no reviewable changes (1)
- docs/method/backlog/up-next/PROCESS_two-way-github-sync.md
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/method/backlog/bad-code/PROCESS_async-exec-refactor.md`:
- Around line 8-10: The backlog item is too vague—refactor Workspace.execCommand
to an async non-blocking API and add concrete success criteria: replace execSync
usage in Workspace.execCommand with an asynchronous implementation (e.g., using
child_process.spawn/exec with Promises) that preserves existing behavior for
METHOD_TEST inputs, matches current stdout/stderr output format on success and
error, supports a configurable timeout and cooperative cancellation (accept an
AbortSignal or timeout ms), and preserves exit-code semantics and thrown errors;
add unit/integration tests that verify API shape (async function returning {
stdout, stderr, code }), timeout cancellation behavior, parity with existing
METHOD_TEST witness output, and identical error messages for failing commands.
In `@docs/method/backlog/inbox/PROCESS_semantic-drift-detector.md`:
- Around line 8-10: Update the PROCESS_semantic-drift-detector doc to replace
aspirational language about "method drift" with concrete, testable rules:
specify the LLM semantic similarity threshold (e.g., cosine similarity >= 0.80
or LLM confidence >= 0.9) for automatic match, a lower threshold range (e.g.,
0.65–0.80) that triggers human review, and a fail state below the lower
threshold that marks the case as non-matching; describe clear fallback behavior
(retry with alternative prompt/template, fall back to lexical matching, or queue
for manual triage) and list observable failure modes to log (low confidence,
timeout, ambiguous multi-intent) so the PROCESS_semantic-drift-detector can be
implemented and tested deterministically.
In `@docs/method/retro/0021-two-way-github-sync/witness/verification.md`:
- Line 17: Replace the machine-specific absolute path shown in the witness
output string "RUN v4.1.2 /Users/james/git/method" with a repo-relative path or
neutral placeholder (e.g., "RUN v4.1.2 ./method" or "RUN v4.1.2 <repo-root>")
so the witness no longer leaks local filesystem details; update the output text
in verification.md to use that repo-relative/placeholder form wherever the
absolute path appears.
In `@src/adapters/github.ts`:
- Around line 64-73: The helper getAllBacklogItems uses a loose any for the
status parameter; replace it with an explicit type (or import an existing
WorkspaceStatus) that models the known shape returned by workspace.status():
i.e., an object with backlog containing inbox, asap, up-next, cool-ideas,
bad-code and root (each the appropriate array/item type). Update the signature
of getAllBacklogItems(status: WorkspaceStatus) (or the newly declared interface
name) and adjust the code to use that type (optionally add safe checks/optional
chaining if properties may be missing).
In `@src/index.ts`:
- Around line 323-338: The updateBody function is adding an extra leading
newline when frontmatter is empty; change the newContent construction in
updateBody to conditionally include the separator newline only when frontmatter
is non-empty (i.e., use frontmatter ? `${frontmatter}\n#
${title}\n\n${newBody.trim()}\n` : `# ${title}\n\n${newBody.trim()}\n`) so files
without YAML begin immediately with the heading; update references to
frontmatter, title and newBody in the updateBody method accordingly.
- Around line 340-369: In moveBacklogItem, the no-op branch returns the raw
input path which can be absolute, causing inconsistency with the other return
path; change the branch that checks fullPath === targetPath to return
relative(this.root, targetPath) (same as the successful move case) so the
function always returns a workspace-relative path; locate the check using the
variables fullPath, targetPath and the path parameter and replace the return
path with relative(this.root, targetPath).
In `@src/mcp.ts`:
- Around line 147-203: Replace the single generic thrown Error in the
method_sync_github request handler with the more specific checks used in cli.ts:
verify workspace.config.github_token and throw a clear "GitHub token missing"
message if absent, verify workspace.config.github_repo exists and contains '/'
and throw a distinct "GitHub repo invalid; must be owner/repo" message if
malformed; keep the existing use of owner/repo derived from repoFull and
preserve downstream logic in the method_sync_github block (variables: token,
repoFull, owner, repo, adapter.pushBacklog/pullBacklog) so the MCP response
behavior and hasError logging remain unchanged.
In `@tests/github-adapter.test.ts`:
- Around line 126-167: Replace the weak assertion that uses
readFileSync(...).toBeDefined() with explicit existence checks: assert the
graveyard file exists (e.g., use fs.existsSync(join(root,
'docs/method/graveyard/FEAT_closed.md')) and expect it to be true or assert
readFileSync does not throw), and also assert the original backlog file no
longer exists (fs.existsSync(join(root,
'docs/method/backlog/inbox/FEAT_closed.md')) should be false). Update the test
around the call to GitHubAdapter.pullBacklog() to use these explicit checks
instead of toBeDefined; refer to readFileSync, existsSync, join,
adapter.pullBacklog, Workspace.captureIdea and updateFrontmatter to locate and
modify the relevant assertions.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6b613863-2874-4883-af8f-d0fb6e858fd0
📒 Files selected for processing (37)
CHANGELOG.mdREADME.mddocs/design/0009-generated-signpost-provenance/generated-signpost-provenance.mddocs/design/0010-yaml-frontmatter-schema/yaml-frontmatter-schema.mddocs/design/0011-library-api-surface/library-api-surface.mddocs/design/0012-mcp-server/mcp-server.mddocs/design/0013-executive-summary-protocol/executive-summary-protocol.mddocs/design/0014-github-issue-adapter/github-issue-adapter.mddocs/design/0015-git-branch-workflow-policy/git-branch-workflow-policy.mddocs/design/0016-system-style-javascript-adoption/system-style-javascript-adoption.mddocs/design/0017-behavior-spike-convention/behavior-spike-convention.mddocs/design/0018-ship-sync-automation/ship-sync-automation.mddocs/design/0019-config-management/config-management.mddocs/design/0020-automated-witness-capture/automated-witness-capture.mddocs/design/0021-two-way-github-sync/two-way-github-sync.mddocs/invariants/sponsor-abstractness.mddocs/method/backlog/bad-code/PROCESS_async-exec-refactor.mddocs/method/backlog/inbox/PROCESS_interactive-scaffolder.mddocs/method/backlog/inbox/PROCESS_multi-forge-adapter.mddocs/method/backlog/inbox/PROCESS_semantic-drift-detector.mddocs/method/process.mddocs/method/retro/0016-system-style-javascript-adoption/system-style-javascript-adoption.mddocs/method/retro/0017-behavior-spike-convention/behavior-spike-convention.mddocs/method/retro/0018-ship-sync-automation/ship-sync-automation.mddocs/method/retro/0019-config-management/config-management.mddocs/method/retro/0019-config-management/witness/verification.mddocs/method/retro/0020-automated-witness-capture/automated-witness-capture.mddocs/method/retro/0021-two-way-github-sync/two-way-github-sync.mddocs/method/retro/0021-two-way-github-sync/witness/verification.mdsrc/adapters/github.tssrc/cli-args.tssrc/cli.tssrc/index.tssrc/mcp.tstests/docs.test.tstests/github-adapter.test.tstests/witness.test.ts
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
src/adapters/github.ts (1)
130-140:⚠️ Potential issue | 🟠 MajorComments are pulled exactly once and never refreshed.
Line 137 skips the update entirely if
'## GitHub Comments'already exists in the body. If new comments are added on GitHub after the first pull, subsequent pulls will never fetch them. The previous review suggested stripping the section before re-adding, which was implemented for push (lines 83-87) but not for pull.🔁 Proposed fix: Strip existing comments and always refresh
// Update local body with comments if any - if (remoteComments.length > 0) { - const fullPath = resolve(this.workspace.root, relativePath); - const localBody = readBody(fullPath); - const commentSection = '\n\n## GitHub Comments\n\n' + remoteComments.map(c => `**@${c.user}**: ${c.body}`).join('\n\n---\n\n'); - - // Simple avoid-duplication check - if (!localBody.includes('## GitHub Comments')) { - this.workspace.updateBody(relativePath, localBody + commentSection); - } - } + const fullPath = resolve(this.workspace.root, relativePath); + let localBody = readBody(fullPath); + + // Strip existing comments section + const commentHeader = '## GitHub Comments'; + if (localBody.includes(commentHeader)) { + localBody = localBody.split(commentHeader)[0]?.trim() ?? localBody; + } + + const commentSection = remoteComments.length > 0 + ? '\n\n## GitHub Comments\n\n' + remoteComments.map(c => `**@${c.user}**: ${c.body}`).join('\n\n---\n\n') + : ''; + this.workspace.updateBody(relativePath, `${localBody}${commentSection}`);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/adapters/github.ts` around lines 130 - 140, The pull logic currently skips updating when '## GitHub Comments' exists, causing stale comments; change the block that builds commentSection (using remoteComments, fullPath, readBody, localBody, this.workspace.updateBody, resolve(this.workspace.root, relativePath)) to remove any existing '## GitHub Comments' section from localBody before appending the freshly constructed commentSection so the code always refreshes comments on pull (mirror the strip-and-replace approach used for push).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/method/backlog/inbox/PROCESS_semantic-drift-detector.md`:
- Around line 19-24: Update the "Fallback Behavior" section to (1) define
"lexical normalized matching" precisely (e.g., normalization steps and matching
metric such as case folding, punctuation removal, tokenization, and chosen
similarity measure like exact match or Levenshtein/token-overlap) and link or
reference its spec; (2) specify exact triggers for each failure mode—define the
numeric threshold and source for low_confidence (e.g., llmConfidence < 0.7), the
timeout boundary and layer it applies to (e.g., LLM API call timeout = 5s HTTP
request), and a clear definition for ambiguous_multi_intent (e.g., LLM returns
>1 intent with similar scores within X% or top intents’ confidence gap < Y); and
(3) provide a concrete logging schema and destination by adopting the proposed
JSON shape (include timestamp ISO8601, mode, testDescription, playbackQuestion,
partialResult with fields like cosine and llmConfidence), state where to write
logs (stderr/structured log sink or specific logging service) and the format
(JSON structured logs), so implementers can instrument functions that call the
semantic drift detector to emit these logs.
- Around line 8-24: Specify exact LLM, embedding model, prompt, similarity calc,
persistence, workflow and SLAs: use OpenAI GPT-4o-mini (or configurable
provider) for reasoning and OpenAI text-embedding-3-small for embeddings;
compute cosine similarity between embeddings of {{testDescription}} and
{{playbackQuestion}} (normalize vectors, use stable library), treat similarity
>=0.80 or LLM JSON "confidence" >=0.9 as Automatic Match, 0.65–0.80 as Near
Miss, <0.65 as Non-Match; include exact prompt template that asks the LLM to
reply with JSON {"match":true|false,"confidence":0.0-1.0,"reasoning":"..."} and
log full LLM response; on LLM timeout/failure fall back to existing
lexical_normalized_match() and log failure reason tags
low_confidence/timeout/ambiguous_multi_intent; implement human review by
appending entries to drift-review-queue.json and expose method drift review CLI
(method names: enqueueDriftReview(), reviewDriftQueue(), applyManualOverride())
where reviewers approve/reject and approved overrides are written to
drift-manual-overrides.json; persist every attempt to drift-audit.jsonl with
inputs, cosine, LLM confidence, LLM reasoning, final classification, timestamp
and commit SHA; enforce performance/cost: p95 <2s, skip LLM when >3 concurrent
LLM requests or when daily LLM call count >1000 (use in-memory counter +
persistent quota) and fallback to lexical matching when limits hit.
- Around line 14-17: The matching rules conflate cosine similarity and LLM
confidence and lack conflict resolution; fix by choosing either Option A or B:
Option A — pick a single primary metric (e.g., "cosine similarity") and rewrite
the rules for Automatic Match / Human Review / Non-Match to use that metric as
primary (e.g., Automatic if cosine >= 0.80, Human Review if 0.65 <= cosine <
0.80, Non-Match if cosine < 0.65) and state that LLM confidence (LLM confidence)
is only used as a tie-breaker when values fall exactly on thresholds (describe
exact tie-break behavior); OR Option B — define explicit AND/OR logic for every
combination of ranges for both "cosine similarity" and "LLM confidence" (provide
a small decision table mapping all combinations to Automatic Match, Human
Review, or Non-Match), and replace the ambiguous generic terms "similarity" and
"confidence" with the explicit metric names across the rules ("cosine
similarity" and "LLM confidence") so there is no ambiguity about which metric
drives each decision.
- Around line 8-10: The doc refers to `method drift` without definition; add a
clear definition or a link to its spec in the PROCESS_semantic-drift-detector
document: state whether `method drift` is a CLI command, module, service, or
background process, provide its purpose and expected behavior, and include a
cross-reference (URL or internal doc anchor) to the canonical spec or
implementation (e.g., the module/class/function that implements it such as
"method drift" handler in the semantic-drift-detector component) so readers can
find the source and extend it for LLM-based semantic matching.
In `@src/adapters/github.ts`:
- Around line 182-188: In fetchComments, guard against deleted/ghost GitHub
users since c.user can be null; change the mapping to safely read login (e.g.,
use c.user?.login or c.user && c.user.login) and provide a sensible fallback
like "ghost" or "deleted user" for the user field so c.user.login cannot throw;
ensure the returned array from fetchComments still conforms to Promise<{ user:
string; body: string }[]> by always supplying a string user and the original
c.body.
- Around line 190-209: Change ghFetch's loose Promise<any> return to a typed
result: update the ghFetch method signature to use a generic like ghFetch<T =
unknown>(...) : Promise<T> (or at minimum Promise<unknown>) and cast the parsed
JSON to T when returning (e.g., return response.json() as Promise<T>); update
callers to supply/assert the expected T where needed. This preserves runtime
behavior while restoring type safety in the adapter and prevents propagating any
throughout the codebase; refer to the ghFetch method in this file when making
the change.
- Around line 143-148: The code assumes forward slashes by doing
relativePath.split('/')[3], which breaks on Windows; normalize and split using
the platform separator instead (import node's path, call
path.normalize(relativePath) then split on path.sep to derive parts and set
currentLane = parts[3] || 'root') and then call this.workspace.moveBacklogItem
as before (references: relativePath, currentLane,
this.workspace.moveBacklogItem, remoteIssue).
In `@src/mcp.ts`:
- Around line 158-168: The repo validation incorrectly allows "/" because it
only checks includes('/'); update the validation around repoFull in mcp.ts so
that after splitting repoFull (e.g., const [owner, repo] = repoFull.split('/')),
you verify both owner and repo are non-empty (and optionally trimmed) and throw
the existing Error if either is falsy; then pass owner and repo (without
non-null assertions) into the GitHubAdapter constructor to avoid creating
malformed API URLs.
---
Duplicate comments:
In `@src/adapters/github.ts`:
- Around line 130-140: The pull logic currently skips updating when '## GitHub
Comments' exists, causing stale comments; change the block that builds
commentSection (using remoteComments, fullPath, readBody, localBody,
this.workspace.updateBody, resolve(this.workspace.root, relativePath)) to remove
any existing '## GitHub Comments' section from localBody before appending the
freshly constructed commentSection so the code always refreshes comments on pull
(mirror the strip-and-replace approach used for push).
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 429cd09f-8ec0-4a6f-bd01-8028be652037
📒 Files selected for processing (7)
docs/method/backlog/bad-code/PROCESS_async-exec-refactor.mddocs/method/backlog/inbox/PROCESS_semantic-drift-detector.mddocs/method/retro/0021-two-way-github-sync/witness/verification.mdsrc/adapters/github.tssrc/index.tssrc/mcp.tstests/github-adapter.test.ts
Comment on lines
+8
to
+24
| Extend `method drift` to use LLM-based semantic matching for cases where | ||
| test descriptions don't exactly match playback questions but are | ||
| conceptually identical. | ||
|
|
||
| ## Matching Rules | ||
|
|
||
| - **Automatic Match**: Cosine similarity >= 0.80 or LLM confidence >= 0.9. | ||
| - **Human Review**: Similarity between 0.65 and 0.80 triggers a "Near Miss" | ||
| hint requiring manual confirmation. | ||
| - **Non-Match**: Marked as drift if confidence is below 0.65. | ||
|
|
||
| ## Fallback Behavior | ||
|
|
||
| - If the LLM call fails or times out, fall back to the existing lexical | ||
| normalized matching. | ||
| - Explicitly log failure modes: `low_confidence`, `timeout`, | ||
| `ambiguous_multi_intent`. |
There was a problem hiding this comment.
Critical implementation details are missing.
Even after fixing the logic issues, an engineer still can't implement this spec because it omits:
-
LLM provider & model: Which LLM? GPT-4? Claude? A local model? Which version? This affects both cost and behavior.
-
Prompt engineering: What prompt is sent to the LLM? Reproducibility requires the exact template.
-
Cosine similarity computation: Similarity between what vectors? Sentence embeddings? If so, which embedding model? How are they generated?
-
Human review workflow: Line 15-16 says "triggers a 'Near Miss' hint requiring manual confirmation" but doesn't say:
- Where does the hint appear? (CLI output? A dashboard? A Slack message?)
- Who confirms it? (Any developer? A specific reviewer?)
- How is the confirmation recorded? (A file edit? A database entry?)
- What happens if confirmation is delayed or never happens?
-
Auditability: The past review comment explicitly asked for "store score + rationale artifact" but there's no mention of persisting match scores, LLM reasoning, or decisions for later audit.
-
Performance & cost: No SLAs for latency (can drift detection block CI for 30 seconds?) or cost limits (LLM calls add $$).
📋 Missing sections to add
+## Implementation Details
+
+- **LLM**: OpenAI GPT-4o-mini via `openai` SDK (fallback: local sentence-transformers if API unavailable).
+- **Embedding model**: `text-embedding-3-small` for cosine similarity.
+- **Prompt template**:
+ ```
+ Compare these two questions for semantic equivalence:
+ A: {{testDescription}}
+ B: {{playbackQuestion}}
+ Reply with JSON: {"match": true|false, "confidence": 0.0-1.0, "reasoning": "..."}
+ ```
+- **Performance target**: < 2s p95 latency; skip LLM if >3 concurrent requests queued.
+- **Cost cap**: Max 1000 LLM calls/day; fallback to lexical after quota.
+
+## Human Review Workflow
+
+When "Near Miss" is triggered:
+1. Append entry to `drift-review-queue.json` with inputs + scores.
+2. Developer runs `method drift review` CLI to view queue and approve/reject.
+3. Approved matches are cached in `drift-manual-overrides.json` to skip future LLM calls.
+
+## Auditability
+
+Persist all match attempts to `drift-audit.jsonl`:
+- Inputs (testDescription, playbackQuestion)
+- Scores (cosine, LLM confidence)
+- LLM reasoning text
+- Final classification (auto-match | near-miss | non-match)
+- Timestamp & commit SHA🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/method/backlog/inbox/PROCESS_semantic-drift-detector.md` around lines 8
- 24, Specify exact LLM, embedding model, prompt, similarity calc, persistence,
workflow and SLAs: use OpenAI GPT-4o-mini (or configurable provider) for
reasoning and OpenAI text-embedding-3-small for embeddings; compute cosine
similarity between embeddings of {{testDescription}} and {{playbackQuestion}}
(normalize vectors, use stable library), treat similarity >=0.80 or LLM JSON
"confidence" >=0.9 as Automatic Match, 0.65–0.80 as Near Miss, <0.65 as
Non-Match; include exact prompt template that asks the LLM to reply with JSON
{"match":true|false,"confidence":0.0-1.0,"reasoning":"..."} and log full LLM
response; on LLM timeout/failure fall back to existing
lexical_normalized_match() and log failure reason tags
low_confidence/timeout/ambiguous_multi_intent; implement human review by
appending entries to drift-review-queue.json and expose method drift review CLI
(method names: enqueueDriftReview(), reviewDriftQueue(), applyManualOverride())
where reviewers approve/reject and approved overrides are written to
drift-manual-overrides.json; persist every attempt to drift-audit.jsonl with
inputs, cosine, LLM confidence, LLM reasoning, final classification, timestamp
and commit SHA; enforce performance/cost: p95 <2s, skip LLM when >3 concurrent
LLM requests or when daily LLM call count >1000 (use in-memory counter +
persistent quota) and fallback to lexical matching when limits hit.
Owner
Author
There was a problem hiding this comment.
This is an inbox backlog item, not a design doc — it intentionally defers implementation decisions (LLM provider, embedding model, etc.) to the cycle that pulls it. The matching thresholds and fallback behavior are specified as actionable constraints. Provider/model selection belongs in the design phase. ✅
There was a problem hiding this comment.
Rate Limit Exceeded
@flyingrobots have exceeded the limit for the number of chat messages per hour. Please wait 2 minutes and 7 seconds before sending another message.
Comment on lines
+8
to
+10
| Extend `method drift` to use LLM-based semantic matching for cases where | ||
| test descriptions don't exactly match playback questions but are | ||
| conceptually identical. |
There was a problem hiding this comment.
Define method drift or link to its specification.
Line 8 references `method drift` as if it's a known entity, but there's zero context about what it is (a CLI command? a module? a background process?) or where it lives in the codebase. An implementer can't "extend" something that isn't defined.
🔧 Proposed fix
-Extend `method drift` to use LLM-based semantic matching for cases where
-test descriptions don't exactly match playback questions but are
-conceptually identical.
+Extend the `method drift` detector (see `src/drift/detector.ts`) to use
+LLM-based semantic matching when test descriptions and playback questions
+are conceptually identical but lexically distinct.
+
+**Inputs**:
+- `testDescription`: String extracted from test file metadata.
+- `playbackQuestion`: String from the playback verification artifact.📝 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.
Suggested change
| Extend `method drift` to use LLM-based semantic matching for cases where | |
| test descriptions don't exactly match playback questions but are | |
| conceptually identical. | |
| Extend the `method drift` detector (see `src/drift/detector.ts`) to use | |
| LLM-based semantic matching when test descriptions and playback questions | |
| are conceptually identical but lexically distinct. | |
| **Inputs**: | |
| - `testDescription`: String extracted from test file metadata. | |
| - `playbackQuestion`: String from the playback verification artifact. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/method/backlog/inbox/PROCESS_semantic-drift-detector.md` around lines 8
- 10, The doc refers to `method drift` without definition; add a clear
definition or a link to its spec in the PROCESS_semantic-drift-detector
document: state whether `method drift` is a CLI command, module, service, or
background process, provide its purpose and expected behavior, and include a
cross-reference (URL or internal doc anchor) to the canonical spec or
implementation (e.g., the module/class/function that implements it such as
"method drift" handler in the semantic-drift-detector component) so readers can
find the source and extend it for LLM-based semantic matching.
Comment on lines
+14
to
+17
| - **Automatic Match**: Cosine similarity >= 0.80 or LLM confidence >= 0.9. | ||
| - **Human Review**: Similarity between 0.65 and 0.80 triggers a "Near Miss" | ||
| hint requiring manual confirmation. | ||
| - **Non-Match**: Marked as drift if confidence is below 0.65. |
There was a problem hiding this comment.
The matching rules are logically inconsistent and unimplementable.
You've defined two metrics (cosine similarity, LLM confidence) but the rules conflate them:
-
Line 14 OR logic is broken: "Cosine >= 0.80 or LLM >= 0.9" creates ambiguous cases. What happens when cosine = 0.75 and LLM confidence = 0.95? Or when cosine = 0.85 and LLM confidence = 0.4? Both metrics satisfied → auto-match, but which metric actually drove the decision?
-
Lines 15-17 drop the metric names: You switch to generic "similarity" and "confidence" without specifying which. Is "similarity between 0.65 and 0.80" referring to cosine similarity, LLM confidence, or both?
-
No conflict resolution: When the two metrics disagree (one says match, one says no-match), which wins? There's no precedence rule.
You must either:
- Option A: Pick ONE primary metric (cosine OR LLM, not both) and use the other only as a tie-breaker.
- Option B: Define explicit AND/OR logic for ALL threshold ranges with a decision table showing every combination.
🔥 Proposed fix: Single-metric design (Option A)
## Matching Rules
-- **Automatic Match**: Cosine similarity >= 0.80 or LLM confidence >= 0.9.
-- **Human Review**: Similarity between 0.65 and 0.80 triggers a "Near Miss"
- hint requiring manual confirmation.
-- **Non-Match**: Marked as drift if confidence is below 0.65.
+Use **cosine similarity** of embedding vectors as the primary metric.
+LLM confidence is logged but not used for classification.
+
+- **Automatic Match**: Cosine similarity >= 0.80.
+- **Human Review** (Near Miss): Cosine similarity in [0.65, 0.80).
+ Flag for manual confirmation with LLM reasoning as context.
+- **Non-Match**: Cosine similarity < 0.65. Marked as drift.
+
+**Embedding source**: OpenAI `text-embedding-3-small` model.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/method/backlog/inbox/PROCESS_semantic-drift-detector.md` around lines 14
- 17, The matching rules conflate cosine similarity and LLM confidence and lack
conflict resolution; fix by choosing either Option A or B: Option A — pick a
single primary metric (e.g., "cosine similarity") and rewrite the rules for
Automatic Match / Human Review / Non-Match to use that metric as primary (e.g.,
Automatic if cosine >= 0.80, Human Review if 0.65 <= cosine < 0.80, Non-Match if
cosine < 0.65) and state that LLM confidence (LLM confidence) is only used as a
tie-breaker when values fall exactly on thresholds (describe exact tie-break
behavior); OR Option B — define explicit AND/OR logic for every combination of
ranges for both "cosine similarity" and "LLM confidence" (provide a small
decision table mapping all combinations to Automatic Match, Human Review, or
Non-Match), and replace the ambiguous generic terms "similarity" and
"confidence" with the explicit metric names across the rules ("cosine
similarity" and "LLM confidence") so there is no ambiguity about which metric
drives each decision.
Comment on lines
+19
to
+24
| ## Fallback Behavior | ||
|
|
||
| - If the LLM call fails or times out, fall back to the existing lexical | ||
| normalized matching. | ||
| - Explicitly log failure modes: `low_confidence`, `timeout`, | ||
| `ambiguous_multi_intent`. |
There was a problem hiding this comment.
Fallback and logging are under-specified.
Three problems:
-
"Lexical normalized matching" is undefined: Line 21-22 assumes this exists but provides no reference. Is it case-insensitive string comparison? Levenshtein distance? Token overlap? Link to the spec or define it inline.
-
Failure mode triggers are not specified: You list
low_confidence,timeout,ambiguous_multi_intent(line 23-24) but don't define when each is emitted:low_confidence: Is this when LLM confidence < some threshold? Which threshold?timeout: After how many milliseconds/seconds? Is this an HTTP timeout? LLM API timeout?ambiguous_multi_intent: What does this mean? When does the LLM detect "multi-intent"?
-
Logging details are missing: You say "explicitly log" but don't specify:
- What to log: Just the failure mode string? Or also the inputs (testDescription, playbackQuestion), the partial results, timestamps?
- Where to log: stderr? A structured log file? A database table?
- Format: JSON? Plain text? Structured logging framework?
🪵 Proposed logging spec
## Fallback Behavior
-- If the LLM call fails or times out, fall back to the existing lexical
- normalized matching.
-- Explicitly log failure modes: `low_confidence`, `timeout`,
- `ambiguous_multi_intent`.
+If semantic matching fails, fall back to **lexical normalized matching**
+(case-insensitive exact match after stripping punctuation; see
+`src/drift/lexical.ts`).
+
+**Failure modes** (emit structured JSON log to `drift.log`):
+- `low_confidence`: LLM confidence < 0.5 after 2 retry attempts.
+- `timeout`: LLM API call exceeds 5000ms timeout.
+- `ambiguous_multi_intent`: LLM response indicates >1 distinct question intent.
+
+**Log schema**:
+```json
+{
+ "timestamp": "ISO8601",
+ "mode": "low_confidence" | "timeout" | "ambiguous_multi_intent",
+ "testDescription": "string",
+ "playbackQuestion": "string",
+ "partialResult": { "cosine"?: number, "llmConfidence"?: number }
+}
+```🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/method/backlog/inbox/PROCESS_semantic-drift-detector.md` around lines 19
- 24, Update the "Fallback Behavior" section to (1) define "lexical normalized
matching" precisely (e.g., normalization steps and matching metric such as case
folding, punctuation removal, tokenization, and chosen similarity measure like
exact match or Levenshtein/token-overlap) and link or reference its spec; (2)
specify exact triggers for each failure mode—define the numeric threshold and
source for low_confidence (e.g., llmConfidence < 0.7), the timeout boundary and
layer it applies to (e.g., LLM API call timeout = 5s HTTP request), and a clear
definition for ambiguous_multi_intent (e.g., LLM returns >1 intent with similar
scores within X% or top intents’ confidence gap < Y); and (3) provide a concrete
logging schema and destination by adopting the proposed JSON shape (include
timestamp ISO8601, mode, testDescription, playbackQuestion, partialResult with
fields like cosine and llmConfidence), state where to write logs
(stderr/structured log sink or specific logging service) and the format (JSON
structured logs), so implementers can instrument functions that call the
semantic drift detector to emit these logs.
Comment on lines
+143
to
+148
| if (remoteIssue.state === 'closed') { | ||
| const currentLane = relativePath.split('/')[3] || 'root'; | ||
| if (currentLane !== 'graveyard') { | ||
| this.workspace.moveBacklogItem(relativePath, 'graveyard'); | ||
| } | ||
| } |
There was a problem hiding this comment.
Path splitting by '/' breaks on Windows.
relativePath.split('/')[3] assumes forward slashes. On Windows, node:path/relative uses backslashes. This will fail to detect the lane correctly.
🔧 Proposed fix: Use platform-agnostic path parsing
+import { basename, dirname, relative, resolve, sep } from 'node:path';
...
// Handle closed status
if (remoteIssue.state === 'closed') {
- const currentLane = relativePath.split('/')[3] || 'root';
+ const parts = relativePath.split(sep);
+ // Expected structure: docs/method/backlog/{lane}/file.md
+ const currentLane = parts[3] || 'root';
if (currentLane !== 'graveyard') {
this.workspace.moveBacklogItem(relativePath, 'graveyard');
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/adapters/github.ts` around lines 143 - 148, The code assumes forward
slashes by doing relativePath.split('/')[3], which breaks on Windows; normalize
and split using the platform separator instead (import node's path, call
path.normalize(relativePath) then split on path.sep to derive parts and set
currentLane = parts[3] || 'root') and then call this.workspace.moveBacklogItem
as before (references: relativePath, currentLane,
this.workspace.moveBacklogItem, remoteIssue).
Comment on lines
+182
to
+188
| private async fetchComments(number: number): Promise<{ user: string; body: string }[]> { | ||
| const data = (await this.ghFetch(`/repos/${this.owner}/${this.repo}/issues/${number}/comments`)) as any[]; | ||
| return data.map((c: any) => ({ | ||
| user: c.user.login, | ||
| body: c.body, | ||
| })); | ||
| } |
There was a problem hiding this comment.
Null user crashes when GitHub user is deleted (ghost user).
GitHub API can return user: null for comments from deleted accounts. c.user.login will throw TypeError: Cannot read properties of null.
🛡️ Proposed fix: Guard against null user
private async fetchComments(number: number): Promise<{ user: string; body: string }[]> {
const data = (await this.ghFetch(`/repos/${this.owner}/${this.repo}/issues/${number}/comments`)) as any[];
return data.map((c: any) => ({
- user: c.user.login,
+ user: c.user?.login ?? 'ghost',
body: c.body,
}));
}📝 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.
Suggested change
| private async fetchComments(number: number): Promise<{ user: string; body: string }[]> { | |
| const data = (await this.ghFetch(`/repos/${this.owner}/${this.repo}/issues/${number}/comments`)) as any[]; | |
| return data.map((c: any) => ({ | |
| user: c.user.login, | |
| body: c.body, | |
| })); | |
| } | |
| private async fetchComments(number: number): Promise<{ user: string; body: string }[]> { | |
| const data = (await this.ghFetch(`/repos/${this.owner}/${this.repo}/issues/${number}/comments`)) as any[]; | |
| return data.map((c: any) => ({ | |
| user: c.user?.login ?? 'ghost', | |
| body: c.body, | |
| })); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/adapters/github.ts` around lines 182 - 188, In fetchComments, guard
against deleted/ghost GitHub users since c.user can be null; change the mapping
to safely read login (e.g., use c.user?.login or c.user && c.user.login) and
provide a sensible fallback like "ghost" or "deleted user" for the user field so
c.user.login cannot throw; ensure the returned array from fetchComments still
conforms to Promise<{ user: string; body: string }[]> by always supplying a
string user and the original c.body.
Comment on lines
+190
to
+209
| private async ghFetch(endpoint: string, options: RequestInit = {}): Promise<any> { | ||
| const url = endpoint.startsWith('http') ? endpoint : `https://api.github.com${endpoint}`; | ||
| const response = await fetch(url, { | ||
| ...options, | ||
| headers: { | ||
| Authorization: `Bearer ${this.token}`, | ||
| Accept: 'application/vnd.github+json', | ||
| 'X-GitHub-Api-Version': '2022-11-28', | ||
| 'User-Agent': 'METHOD-CLI', | ||
| ...options.headers, | ||
| }, | ||
| }); | ||
|
|
||
| if (!response.ok) { | ||
| const errorBody = await response.text(); | ||
| throw new Error(`GitHub API error: ${response.status} ${response.statusText}\n${errorBody}`); | ||
| } | ||
|
|
||
| const data = (await response.json()) as any; | ||
| return response.json(); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor: any return type on ghFetch weakens type safety.
Consider returning Promise<unknown> and letting callers assert/narrow, or use generics. Not blocking, but this propagates any throughout the adapter.
💡 Optional: Use generic return type
- private async ghFetch(endpoint: string, options: RequestInit = {}): Promise<any> {
+ private async ghFetch<T>(endpoint: string, options: RequestInit = {}): Promise<T> {
const url = endpoint.startsWith('http') ? endpoint : `https://api.github.com${endpoint}`;
// ...
- return response.json();
+ return response.json() as Promise<T>;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/adapters/github.ts` around lines 190 - 209, Change ghFetch's loose
Promise<any> return to a typed result: update the ghFetch method signature to
use a generic like ghFetch<T = unknown>(...) : Promise<T> (or at minimum
Promise<unknown>) and cast the parsed JSON to T when returning (e.g., return
response.json() as Promise<T>); update callers to supply/assert the expected T
where needed. This preserves runtime behavior while restoring type safety in the
adapter and prevents propagating any throughout the codebase; refer to the
ghFetch method in this file when making the change.
Comment on lines
+158
to
+168
| if (!repoFull || !repoFull.includes('/')) { | ||
| throw new Error('GitHub repo invalid; must be owner/repo in .method.json or environment.'); | ||
| } | ||
|
|
||
| const [owner, repo] = repoFull.split('/'); | ||
| const adapter = new GitHubAdapter({ | ||
| workspace, | ||
| token, | ||
| owner: owner!, | ||
| repo: repo!, | ||
| }); |
There was a problem hiding this comment.
Edge case: github_repo = "/" passes validation but yields empty owner/repo.
'/' .includes('/') is true, so the check passes. '/'.split('/') produces ['', '']. The non-null assertions owner! and repo! resolve to empty strings, which will produce malformed GitHub API URLs like https://api.github.com/repos///issues.
🛡️ Proposed fix: validate both parts are non-empty
- if (!repoFull || !repoFull.includes('/')) {
+ const parts = repoFull?.split('/');
+ if (!parts || parts.length !== 2 || !parts[0] || !parts[1]) {
throw new Error('GitHub repo invalid; must be owner/repo in .method.json or environment.');
}
- const [owner, repo] = repoFull.split('/');
+ const [owner, repo] = parts;
const adapter = new GitHubAdapter({
workspace,
token,
- owner: owner!,
- repo: repo!,
+ owner,
+ repo,
});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/mcp.ts` around lines 158 - 168, The repo validation incorrectly allows
"/" because it only checks includes('/'); update the validation around repoFull
in mcp.ts so that after splitting repoFull (e.g., const [owner, repo] =
repoFull.split('/')), you verify both owner and repo are non-empty (and
optionally trimmed) and throw the existing Error if either is falsy; then pass
owner and repo (without non-null assertions) into the GitHubAdapter constructor
to avoid creating malformed API URLs.
…g spaces - Add missing YAML frontmatter to three ASAP backlog items (branch-naming-consistency, red-phase-playback-coverage, repo-lane-conformance) - Sanitize personal absolute paths (/Users/james/...) from all verification witnesses across cycles 0001-0020 - Remove trailing spaces from 0021 design doc - Expand path-sanitization test to cover all witness files dynamically - Add github_labels to allowed frontmatter keys in docs.test.ts
Owner
Author
PR Feedback Resolution SummaryAll 28 review comments have been addressed. Here is the disposition:
Additional fixes in
|
Owner
Author
|
@coderabbitai review please 🥕 |
Rate Limit Exceeded
|
3 tasks done
3 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR delivers Cycle 0021: Two-way GitHub Sync.
Changes
GitHubAdapterwithpushBacklog()andpullBacklog()for bidirectional sync.--pushand--pullflags tomethod sync github.method_sync_githubtool via the MCP server.Workspace.updateBody()andWorkspace.moveBacklogItem()to handle local updates and lane movements.CHANGELOG,BEARING,VISION) to reflect 21 closed cycles.Verification
tests/github-adapter.test.ts.