Clang GitHub tracker: DB-backed sync, markdown publish, chunked backfill (#136)#137
Merged
snowfox1003 merged 15 commits intocppalliance:developfrom Apr 8, 2026
Merged
Clang GitHub tracker: DB-backed sync, markdown publish, chunked backfill (#136)#137snowfox1003 merged 15 commits intocppalliance:developfrom
snowfox1003 merged 15 commits intocppalliance:developfrom
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a DB-backed Clang GitHub tracker: new Django app, models and migration, services with upsert/watermark APIs, sync and backfill commands, DB-driven preprocessors, a Markdown publisher using persistent local clones, git-op hardening, workspace/settings/env changes, docs, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Cmd as run_clang_github_tracker
participant State as state_manager
participant Services as clang_github_tracker.services
participant DB as Database
participant Sync as sync_clang_github_activity
participant GH as GitHub API
participant Publisher
participant Git as git_ops
User->>Cmd: execute CLI (--since/--until, --skip-*)
Cmd->>State: resolve_start_end_dates(since, until)
State->>Services: get_issue_item_watermark()
Services->>DB: query Max(github_updated_at)
DB-->>Services: watermark
State->>Services: get_commit_watermark()
Services->>DB: query Max(github_committed_at)
DB-->>Services: watermark
State-->>Cmd: (start_commit, start_item, end_date)
Cmd->>Sync: sync_clang_github_activity(start_commit, start_item, end_date)
Sync->>GH: fetch commits/issues/PRs
GH-->>Sync: JSON payloads
Sync->>Services: upsert_commit(...)/upsert_issue_item(...)
Services->>DB: update_or_create / bulk_create+update_conflicts
DB-->>Services: created/updated counts
Sync-->>Cmd: (commits, issue_numbers, pr_numbers)
Cmd->>Publisher: publish_clang_markdown(md_dir, owner, repo, branch, new_files)
Publisher->>Git: clone_repo / pull / reset --hard
Git-->>Publisher: success
Publisher->>Publisher: detect_stale_titled_paths(), copy md tree
Publisher->>Git: git_push(branch, message)
Git-->>Publisher: push result
Publisher-->>Cmd: publish complete
Cmd-->>User: finished
sequenceDiagram
participant User
participant Backfill
participant FS as File system
participant Parser
participant Services
participant DB as Database
User->>Backfill: run backfill command
Backfill->>FS: scan commits/, issues/, prs/ or read CSV
FS-->>Parser: files/rows
Parser->>Parser: validate SHAs, numbers, datetimes
Parser-->>Backfill: collected rows
Backfill->>Services: upsert_issue_items_batch(issue_rows)
Services->>DB: bulk_create/update_conflicts (issue items)
DB-->>Services: (inserted, updated)
Backfill->>Services: upsert_commits_batch(commit_rows)
Services->>DB: bulk_create/update_conflicts (commits)
DB-->>Services: (inserted, updated)
Backfill-->>User: log totals and skipped counts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
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 |
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
github_ops/git_ops.py (2)
172-173:⚠️ Potential issue | 🟠 MajorNormalize scraping token before URL injection
get_github_token(use="scraping")can return whitespace-only values; those currently get injected into the clone URL and can break auth unexpectedly.💡 Suggested fix
if token is None: token = get_github_token(use="scraping") + token = (token or "").strip() + if not token: + token = NoneBased on learnings: In
github_ops/tokens.py,get_github_token(use="scraping")may return empty/whitespace strings without stripping.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@github_ops/git_ops.py` around lines 172 - 173, The token returned by get_github_token(use="scraping") may be whitespace-only; after obtaining the token (the local variable token) strip() it and treat empty results as None before any URL injection. Concretely, after calling get_github_token(use="scraping") ensure you run token = token.strip() and then if token == "": set token = None so downstream code that builds the clone URL does not receive whitespace-only credentials.
207-221:⚠️ Potential issue | 🔴 CriticalRedact token-bearing git output before logging clone failures
The new safe re-raise is good, but Line 207/Line 209 still logs raw stderr/stdout tail, which can include the authenticated clone URL and leak PATs into logs.
🔒 Suggested fix
- err_tail = ((e.stderr or "") + (e.stdout or ""))[-500:] + err_tail = ((e.stderr or "") + (e.stdout or ""))[-500:] + err_tail = re.sub(r"(x-access-token:)[^@\s]+@", r"\1***@", err_tail) logger.warning( "git clone failed (%s -> %s), returncode=%s, stderr/stdout_tail=%r", url_or_slug, dest_dir, e.returncode, err_tail, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@github_ops/git_ops.py` around lines 207 - 221, The code currently builds err_tail from e.stderr/e.stdout and logs it directly (in logger.warning) which can leak PATs; before logging or including in the exception, sanitize err_tail by stripping/masking token-bearing URLs and credentials (e.g., replace patterns like https?://[^/@]+@ with https://<redacted>@ and mask any long alphanumeric token-looking strings), implement this as a small helper (e.g., sanitize_git_output) and use it to produce a safe_err_tail used in the logger.warning call and any further use, while keeping safe_cmd and the re-raised subprocess.CalledProcessError unchanged.clang_github_tracker/sync_raw.py (1)
45-50:⚠️ Potential issue | 🟡 MinorFall back to the committer date when the author date is blank.
The current
author or committerselection only falls back when the wholeauthordict is falsey. Ifcommit.authorexists but lacksdate,_commit_date()returnsNoneeven whencommit.committer.dateis present, which can leave the commit watermark behind and cause unnecessary refetches.Suggested change
commit = commit_data.get("commit") or {} - author = commit.get("author") or commit.get("committer") or {} - date_str = author.get("date") + author = commit.get("author") or {} + committer = commit.get("committer") or {} + date_str = author.get("date") or committer.get("date") if not date_str: return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clang_github_tracker/sync_raw.py` around lines 45 - 50, The _commit_date logic currently uses commit.get("author") or commit.get("committer") which fails when author exists but author["date"] is missing; update _commit_date to explicitly check author_date = commit.get("author", {}).get("date") and if not present fallback to committer_date = commit.get("committer", {}).get("date"), then parse using parse_datetime(date_str) or clang_state.parse_iso(date_str) and return that; reference commit_data, commit, author, committer, date_str, parse_datetime, and clang_state.parse_iso when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clang_github_tracker/management/commands/backfill_clang_github_tracker.py`:
- Around line 92-109: The CSV branch that parses issue/pr rows accepts
non-positive numbers and lets upsert_issue_items_batch drop them silently;
update the parsing logic in backfill_clang_github_tracker.py where num is parsed
(the blocks that append to issue_rows for rt == "issue" and rt == "pr") to
explicitly treat num <= 0 as invalid: increment the existing skipped counter
(the same skip tally used by the raw-json path) and continue/skip adding to
issue_rows instead of appending; keep parse_datetime usage and the eventual call
to upsert_issue_items_batch unchanged so reporting matches the raw-json path.
In `@clang_github_tracker/management/commands/run_clang_github_tracker.py`:
- Around line 245-252: The branch value is being defaulted before trimming, so a
whitespace-only CLANG_GITHUB_CONTEXT_REPO_BRANCH becomes an empty string instead
of falling back; update the logic that sets clang_github_context_repo_branch
(reading settings.CLANG_GITHUB_CONTEXT_REPO_BRANCH and
DEFAULT_CLANG_REPO_BRANCH) to first obtain and .strip() the configured value,
then if the stripped result is empty or falsy assign DEFAULT_CLANG_REPO_BRANCH,
ensuring the final clang_github_context_repo_branch is never an empty-ref.
- Around line 124-132: The code currently raises CommandError on
parse_iso_datetime failures and blindly unpacks resolve_start_end_dates(), which
can be None; instead, change the parse_iso_datetime try/except to catch
ValueError, log or warn and leave since/until as None (do not raise) so default
resolution can run, then call clang_state.resolve_start_end_dates(since, until)
and check its return: if it returns None, return early from the command (exit
success) rather than unpacking into start_commit, start_item, end_date; keep
using the same symbols parse_iso_datetime and resolve_start_end_dates when
locating the change.
- Around line 220-231: The code is appending "-issues"/"-prs" to
settings.CLANG_GITHUB_PINECONE_APP_TYPE before validating it, producing
malformed names like "-issues" when the base is empty; change the logic in
run_clang_github_tracker.py to validate the trimmed base app_type
(settings.CLANG_GITHUB_PINECONE_APP_TYPE) first and only call _run_pinecone_sync
with f"{app_type}-issues" and f"{app_type}-prs" when app_type is non-empty
(otherwise skip or log); specifically, check the trimmed app_type variable right
after it’s assigned and return/continue if falsy before composing the suffixed
names and invoking _run_pinecone_sync.
In `@clang_github_tracker/models.py`:
- Around line 20-25: The batch upsert helpers _flush_commits_chunk and
_flush_issue_items_chunk currently omit the auto_now-managed updated_at from
their update_fields, so bulk_create(..., update_conflicts=True) won't refresh
the timestamp on conflict; edit both functions to include "updated_at" in their
respective update_fields lists (the list passed to bulk_create/update_conflicts
in _flush_commits_chunk and the update_fields list in _flush_issue_items_chunk)
so the timestamp is written during conflict upserts.
In `@clang_github_tracker/preprocessors/issue_preprocessor.py`:
- Around line 45-49: The code uses Django's removed timezone.utc; update the
call site in issue_preprocessor.py to use the stdlib timezone: import datetime
(or from datetime import timezone as dt_timezone) and replace timezone.utc with
datetime.timezone (or dt_timezone) when calling
timezone.make_aware(final_sync_at, ...). Keep the existing checks using
timezone.is_naive(final_sync_at) and the subsequent
ClangGithubIssueItem.objects.filter(is_pull_request=False, updated_at__gt=fs)
logic unchanged.
In `@clang_github_tracker/publisher.py`:
- Around line 111-114: git_user_name and git_user_email are currently stripped
after the `or` fallback which makes whitespace-only settings become empty
strings; instead, read the raw values from getattr(settings, "GIT_AUTHOR_NAME",
None) and getattr(settings, "GIT_AUTHOR_EMAIL", None), call .strip() on those
raw values (guarding for None), and then apply the fallback to "unknown" /
"unknown@noreply.github.com" if the stripped result is falsy; update the
assignments for git_user_name and git_user_email accordingly so git_push()
receives the correct default author metadata.
In `@clang_github_tracker/services.py`:
- Around line 50-52: The current checks only validate sha length; update both
places where sha_clean is computed to enforce hex content by validating against
a 40-hex-character pattern (e.g. use a regex like ^[0-9a-fA-F]{40}$) and raise
ValueError if it fails; specifically replace the length-only guard around
sha_clean with a combined length+hex validation (the block that currently
computes sha_clean and raises for wrong length) and make the same change at the
other occurrence that handles sha_clean to ensure stored SHAs are valid hex.
In `@clang_github_tracker/state_manager.py`:
- Around line 41-44: The function resolve_start_end_dates currently treats an
invalid CLI window (when since and until are both provided but since > until) as
a resume by dropping user bounds and using DB cursors; instead, detect this case
early using the since and until parameters in resolve_start_end_dates and return
the sentinel that signals a graceful no-op (e.g., return (None, None, None)) so
run_clang_github_tracker.py will exit rather than proceed; apply the same check
where the function handles the other branch (the logic around the alternative
handling at lines 74–84) so any invalid manual window consistently becomes a
no-op, not a resume.
In `@clang_github_tracker/tests/test_commands.py`:
- Around line 29-39: The test test_run_clang_github_tracker_dry_run_skip_sync
only exercises --dry-run so it doesn't verify the behavior of
--skip-github-sync; update or add a test that calls call_command(CMD_NAME,
"--skip-github-sync", ...) WITHOUT "--dry-run" and assert that
sync_clang_github_activity (mocked or patched) was not called (or its call count
remains zero) while the expected logging (e.g., "Resolved:") still occurs; use
patch/monkeypatch to replace sync_clang_github_activity and inspect that it was
not invoked to ensure the flag is respected.
In `@clang_github_tracker/tests/test_state_manager.py`:
- Around line 54-57: Prefix intentionally-unused unpacked variables with an
underscore when calling clang_state.resolve_start_end_dates so Ruff no longer
flags them; e.g., where you currently have "sc, si, end =
clang_state.resolve_start_end_dates(...)" change the unused "end" to "_end" (and
in the other occurrence change any unused "sc" to "_sc" while keeping "si" if
it's asserted), and make the analogous rename in the second occurrence around
lines 103-105 so only actually-used variables remain without the underscore
prefix.
In `@operations/md_ops/github_export.py`:
- Around line 190-204: The current deletion logic only matches the "#n - "
prefix and can remove non-markdown siblings (e.g., images or txt); update the
predicate inside the loop that builds delete_paths to ensure listed_filename is
a markdown file before treating it as an old title. Concretely, in the loop that
iterates over files_by_dir.get(new_dir, []), add a check using the file
extension (e.g., os.path.splitext(listed_filename)[1].lower() in {'.md',
'.markdown'}) in addition to the existing listed_filename.startswith(prefix) and
listed_filename != new_filename checks so only .md/.markdown files are
considered for deletion.
---
Outside diff comments:
In `@clang_github_tracker/sync_raw.py`:
- Around line 45-50: The _commit_date logic currently uses commit.get("author")
or commit.get("committer") which fails when author exists but author["date"] is
missing; update _commit_date to explicitly check author_date =
commit.get("author", {}).get("date") and if not present fallback to
committer_date = commit.get("committer", {}).get("date"), then parse using
parse_datetime(date_str) or clang_state.parse_iso(date_str) and return that;
reference commit_data, commit, author, committer, date_str, parse_datetime, and
clang_state.parse_iso when making the change.
In `@github_ops/git_ops.py`:
- Around line 172-173: The token returned by get_github_token(use="scraping")
may be whitespace-only; after obtaining the token (the local variable token)
strip() it and treat empty results as None before any URL injection. Concretely,
after calling get_github_token(use="scraping") ensure you run token =
token.strip() and then if token == "": set token = None so downstream code that
builds the clone URL does not receive whitespace-only credentials.
- Around line 207-221: The code currently builds err_tail from e.stderr/e.stdout
and logs it directly (in logger.warning) which can leak PATs; before logging or
including in the exception, sanitize err_tail by stripping/masking token-bearing
URLs and credentials (e.g., replace patterns like https?://[^/@]+@ with
https://<redacted>@ and mask any long alphanumeric token-looking strings),
implement this as a small helper (e.g., sanitize_git_output) and use it to
produce a safe_err_tail used in the logger.warning call and any further use,
while keeping safe_cmd and the re-raised subprocess.CalledProcessError
unchanged.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4af11391-bcc1-4e9d-8021-49c06b9dbc46
📒 Files selected for processing (30)
.env.exampleclang_github_tracker/apps.pyclang_github_tracker/management/commands/backfill_clang_github_tracker.pyclang_github_tracker/management/commands/run_clang_github_tracker.pyclang_github_tracker/migrations/0001_initial.pyclang_github_tracker/migrations/__init__.pyclang_github_tracker/models.pyclang_github_tracker/preprocessors/issue_preprocessor.pyclang_github_tracker/preprocessors/pr_preprocessor.pyclang_github_tracker/publisher.pyclang_github_tracker/services.pyclang_github_tracker/state_manager.pyclang_github_tracker/sync_raw.pyclang_github_tracker/tests/test_backfill.pyclang_github_tracker/tests/test_commands.pyclang_github_tracker/tests/test_preprocessors.pyclang_github_tracker/tests/test_publisher.pyclang_github_tracker/tests/test_services.pyclang_github_tracker/tests/test_state_manager.pyclang_github_tracker/workspace.pyconfig/settings.pyconfig/test_settings.pydocs/Pinecone_preprocess_guideline.mddocs/Schema.mddocs/Workspace.mddocs/service_api/README.mddocs/service_api/clang_github_tracker.mdgithub_ops/git_ops.pyoperations/md_ops/github_export.pyoperations/tests/test_github_export.py
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (4)
github_ops/git_ops.py (1)
237-243: Security improvement looks good; consider documenting thefrom Nonerationale.Using
from Noneintentionally suppresses the original exception chain to prevent the token-bearing command from appearing in tracebacks. This is the right choice for security, though it removes some debugging context. The originalstdout/stderrare preserved in the re-raised exception.A brief inline comment explaining the security rationale would help future maintainers understand this is intentional.
💡 Optional: Add clarifying comment
# Never re-raise with the real cmd: it embeds the token in the clone URL. + # Use `from None` to suppress the chained exception (which also contains the token). safe_cmd: list[str] = ["git", "clone", url_or_slug, str(dest_dir)]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@github_ops/git_ops.py` around lines 237 - 243, Add a short inline comment above the raise in the git clone error handling explaining that suppressing the exception chain with "from None" is intentional to prevent the original exception (which may contain the token-bearing command) from appearing in tracebacks; reference the local variables safe_cmd, e, and the subprocess.CalledProcessError being raised so future maintainers understand this security rationale while stdout/stderr are still preserved on the new exception.clang_github_tracker/tests/test_backfill.py (1)
12-94: Add one boundary test for the periodic batch flush.These cases only exercise the final flush on tiny inputs. The new 10k-record upsert branch is the risky part of this feature, so a focused test that forces that threshold would catch off-by-one or dropped-record bugs before a large backfill does.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clang_github_tracker/tests/test_backfill.py` around lines 12 - 94, Add a boundary test that forces the periodic batch flush to hit the 10k upsert branch by creating an input (CSV or raw files) with exactly the batch threshold (e.g., 10000 records) plus one to exercise the final flush; implement it as a new pytest function (e.g., test_backfill_triggers_10000_batch_flush) that writes a CSV or raw records under tmp_path, calls call_command("backfill_clang_github_tracker", "--from-csv=...") (or "--from-raw") and then asserts the expected number of ClangGithubIssueItem/ClangGithubCommit rows exist, so the code paths in the backfill command that handle the 10000-record upsert are exercised and no records are dropped.clang_github_tracker/tests/test_publisher.py (1)
113-150: Add a regression test for clone-error redaction.This file covers
git_pushfailures and the missing-.gitclone path, but not the security-sensitive case called out in the PR: a clone failure must not leak a PAT in the raisedCommandError. A targetedclone_reposide-effect test would keep that guarantee from regressing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clang_github_tracker/tests/test_publisher.py` around lines 113 - 150, Add a regression test that ensures a failing clone does not leak the PAT by simulating clone_repo raising an exception containing the token and asserting the raised CommandError message is redacted; specifically, in the test function test_publish_clang_markdown_clones_when_no_git_dir (or a new sibling test) patch clone_repo to raise an error whose text includes "tok", call publish_clang_markdown(acme, priv, ...) and assert the raised exception is a CommandError (or the module's wrapper) whose message does NOT contain the raw token but contains a redacted placeholder; reference clone_repo, publish_clang_markdown, and CommandError to locate the code path that formats and raises the error so the test will fail if token redaction regresses.clang_github_tracker/management/commands/backfill_clang_github_tracker.py (1)
201-248: Consider logging skipped count in the final summary.The issues loop tracks
i_skipbut the intermediate progress logs (lines 236-242) don't include skipped count. The final log at line 248 includes it, which is good. The PRs loop follows the same pattern.Minor consistency nit: the commits intermediate log includes
skipped(line 188), but issues/PRs intermediate logs don't. Consider adding for consistency, though this is optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clang_github_tracker/management/commands/backfill_clang_github_tracker.py` around lines 201 - 248, The intermediate progress log inside the issues processing loop is missing the skipped counter; update the logger.info call (the one executed when i_read_n % _RAW_CHUNK_EVERY == 0) to include i_skip (and optionally i_ok) so it reports parsed_ok/skipped along with i_ins_total and i_upd_total; locate the block that builds issue_rows and calls clang_services.upsert_issue_items_batch and augment that logger.info to include i_skip for consistency with the commits log and the final logger.info that already shows skipped.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clang_github_tracker/preprocessors/issue_preprocessor.py`:
- Line 22: The import for get_raw_source_issue_path is coming from the wrong
module and will raise ImportError; update the import used by
preprocess_for_pinecone() to point at the actual provider of that helper (either
import get_raw_source_issue_path from clang_github_tracker.workspace if the
clang tracker owns it, or import the shared helper from the module that actually
exposes it — e.g. the shared issue-path helper used elsewhere in the repo) and
update any call sites in preprocess_for_pinecone() to use that correctly
imported function name.
In `@clang_github_tracker/preprocessors/pr_preprocessor.py`:
- Line 22: The import at the top of
clang_github_tracker/preprocessors/pr_preprocessor.py incorrectly references
get_raw_source_pr_path from github_activity_tracker.workspace (which doesn't
export it), causing ImportError; update the import to the correct provider of
get_raw_source_pr_path used by preprocess_for_pinecone(): either import it from
the clang tracker workspace module (e.g. from clang_github_tracker.workspace
import get_raw_source_pr_path) if this repo owns the helper, or change the call
site in preprocess_for_pinecone() to use the existing shared PR-path helper that
is actually exported (replace the import with the correct module name that
exposes get_raw_source_pr_path).
In `@clang_github_tracker/services.py`:
- Around line 93-115: The function upsert_commits_batch accepts batch_size but
doesn't validate it, causing range(...) to raise for 0 or silently skip for
negatives; add an upfront guard in upsert_commits_batch to reject non-positive
batch_size (e.g., if batch_size <= 0: raise ValueError(...)) so callers get a
clear error; apply the same validation pattern to the other batch-oriented
helper that also passes batch_size into range (referenced by the comment range
usage and _flush_commits_chunk) to ensure consistent behavior.
- Around line 103-108: The merge loop that builds merged: dict[str, datetime |
None] currently overwrites duplicates with the last seen dt, which can regress
timestamps; change the logic in the block that iterates rows (the loop using
variables sha, dt and the merged dict) so that when inserting or updating
merged[s] you keep the most recent timestamp (max) rather than the last one —
compare existing = merged.get(s) and set merged[s] = the later of existing and
dt, handling None by treating None as smaller than any datetime (i.e., if
existing is None use dt, if dt is None keep existing), and apply the same fix to
the analogous block that computes github_committed_at/github_updated_at so
neither value can be moved backward by later duplicates.
In `@docs/service_api/clang_github_tracker.md`:
- Around line 23-25: The docs incorrectly state that start_after_watermark adds
one second; update the documentation to match the implementation and tests by
changing the description for start_after_watermark to use a 1 millisecond offset
(i.e., max_dt + timedelta(milliseconds=1)) and ensure any related text for
get_issue_item_watermark/get_commit_watermark remains consistent with that
behavior; reference the function name start_after_watermark in the docs and
replace "seconds=1" with "milliseconds=1" (or wording "1 ms") so the text aligns
with clang_github_tracker.services.start_after_watermark().
In `@operations/md_ops/github_export.py`:
- Around line 175-206: The helper _stale_titled_paths_vs_listing can mark
non-markdown remote files (e.g. "#5 - diagram.png") as stale; update the helper
to only consider .md files by validating extensions: skip any
new_repo_rel/new_filename that doesn't end with ".md" and skip any
listed_filename that doesn't end with ".md" before matching the "#n - " prefix;
update the loop in _stale_titled_paths_vs_listing (and keep existing logic for m
= _NUMBER_PREFIX.match) so only .md ↔ .md comparisons are made (this avoids
touching detect_renames/detect_renames_from_dirs).
---
Nitpick comments:
In `@clang_github_tracker/management/commands/backfill_clang_github_tracker.py`:
- Around line 201-248: The intermediate progress log inside the issues
processing loop is missing the skipped counter; update the logger.info call (the
one executed when i_read_n % _RAW_CHUNK_EVERY == 0) to include i_skip (and
optionally i_ok) so it reports parsed_ok/skipped along with i_ins_total and
i_upd_total; locate the block that builds issue_rows and calls
clang_services.upsert_issue_items_batch and augment that logger.info to include
i_skip for consistency with the commits log and the final logger.info that
already shows skipped.
In `@clang_github_tracker/tests/test_backfill.py`:
- Around line 12-94: Add a boundary test that forces the periodic batch flush to
hit the 10k upsert branch by creating an input (CSV or raw files) with exactly
the batch threshold (e.g., 10000 records) plus one to exercise the final flush;
implement it as a new pytest function (e.g.,
test_backfill_triggers_10000_batch_flush) that writes a CSV or raw records under
tmp_path, calls call_command("backfill_clang_github_tracker", "--from-csv=...")
(or "--from-raw") and then asserts the expected number of
ClangGithubIssueItem/ClangGithubCommit rows exist, so the code paths in the
backfill command that handle the 10000-record upsert are exercised and no
records are dropped.
In `@clang_github_tracker/tests/test_publisher.py`:
- Around line 113-150: Add a regression test that ensures a failing clone does
not leak the PAT by simulating clone_repo raising an exception containing the
token and asserting the raised CommandError message is redacted; specifically,
in the test function test_publish_clang_markdown_clones_when_no_git_dir (or a
new sibling test) patch clone_repo to raise an error whose text includes "tok",
call publish_clang_markdown(acme, priv, ...) and assert the raised exception is
a CommandError (or the module's wrapper) whose message does NOT contain the raw
token but contains a redacted placeholder; reference clone_repo,
publish_clang_markdown, and CommandError to locate the code path that formats
and raises the error so the test will fail if token redaction regresses.
In `@github_ops/git_ops.py`:
- Around line 237-243: Add a short inline comment above the raise in the git
clone error handling explaining that suppressing the exception chain with "from
None" is intentional to prevent the original exception (which may contain the
token-bearing command) from appearing in tracebacks; reference the local
variables safe_cmd, e, and the subprocess.CalledProcessError being raised so
future maintainers understand this security rationale while stdout/stderr are
still preserved on the new exception.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9ad9de49-4521-4083-81ed-0bd2d6e9f3ca
📒 Files selected for processing (34)
.env.example.gitignoreclang_github_tracker/__init__.pyclang_github_tracker/apps.pyclang_github_tracker/management/commands/backfill_clang_github_tracker.pyclang_github_tracker/management/commands/run_clang_github_tracker.pyclang_github_tracker/migrations/0001_initial.pyclang_github_tracker/migrations/__init__.pyclang_github_tracker/models.pyclang_github_tracker/preprocessors/__init__.pyclang_github_tracker/preprocessors/issue_preprocessor.pyclang_github_tracker/preprocessors/pr_preprocessor.pyclang_github_tracker/publisher.pyclang_github_tracker/services.pyclang_github_tracker/state_manager.pyclang_github_tracker/sync_raw.pyclang_github_tracker/tests/test_backfill.pyclang_github_tracker/tests/test_commands.pyclang_github_tracker/tests/test_preprocessors.pyclang_github_tracker/tests/test_publisher.pyclang_github_tracker/tests/test_services.pyclang_github_tracker/tests/test_state_manager.pyclang_github_tracker/workspace.pyconfig/settings.pyconfig/test_settings.pydocs/Pinecone_preprocess_guideline.mddocs/Schema.mddocs/Workspace.mddocs/service_api/README.mddocs/service_api/clang_github_tracker.mdgithub_ops/git_ops.pygithub_ops/tests/test_git_ops.pyoperations/md_ops/github_export.pyoperations/tests/test_github_export.py
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
clang_github_tracker/tests/test_publisher.py (1)
137-144: Consider moving theshutilimport to the top of the file.The inline
import shutilinside_clone_side_effectworks but is unconventional. Moving it to the module-level imports would be cleaner.Suggested change
import subprocess from pathlib import Path +import shutil from unittest.mock import patchThen remove line 140:
def _clone_side_effect(_slug, dest, **_kw): dest = Path(dest) if dest.exists(): - import shutil - shutil.rmtree(dest)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clang_github_tracker/tests/test_publisher.py` around lines 137 - 144, The inline import of shutil inside the helper function _clone_side_effect should be moved to module-level imports; add "import shutil" to the top-of-file imports and remove the "import shutil" statement from inside _clone_side_effect so the function simply uses shutil.rmtree(dest) without importing locally.clang_github_tracker/preprocessors/pr_preprocessor.py (1)
32-74: Consider extracting shared preprocessing logic.The
issue_preprocessor.pyandpr_preprocessor.pyimplementations are nearly identical, differing only in:
is_pull_requestfilter value- ID suffix regex pattern
build_*_documentandget_raw_source_*_pathfunctionsA shared helper could reduce duplication, though the current approach is clear and maintainable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clang_github_tracker/preprocessors/pr_preprocessor.py` around lines 32 - 74, Both preprocessors duplicate the same logic; extract a shared helper (e.g., preprocess_for_pinecone_generic) that takes parameters for the differing bits: the model filter flag (is_pull_request), the ID-suffix regex (e.g., _PR_ID_SUFFIX vs issue regex), the path getter (get_raw_source_pr_path / get_raw_source_issue_path) and the document builder (build_pr_document / build_issue_document). Move the common sequence that builds numbers from the queryset and failed_ids, checks path.is_file(), reads JSON and appends documents (currently implemented inside preprocess_for_pinecone and the issue counterpart using ClangGithubIssueItem.objects.filter, failed_ids loop, json.loads, etc.) into that helper, and have preprocess_for_pinecone simply call it with owner/repo, final_sync_at and the PR-specific symbols.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clang_github_tracker/publisher.py`:
- Around line 151-158: prepare_repo_for_pull and pull are called without
handling subprocess.CalledProcessError so failures will propagate as raw
exceptions; wrap each call (prepare_repo_for_pull and pull in this block) in
try/except catching subprocess.CalledProcessError, log an error via logger.error
with the clone_dir, branch and the caught exception, and re-raise a
user-friendly CommandError (same pattern used by clone_repo and git_push) so
callers receive consistent, descriptive errors; leave _reset_hard_to_upstream
as-is but ensure imports include subprocess and CommandError if not already
present.
---
Nitpick comments:
In `@clang_github_tracker/preprocessors/pr_preprocessor.py`:
- Around line 32-74: Both preprocessors duplicate the same logic; extract a
shared helper (e.g., preprocess_for_pinecone_generic) that takes parameters for
the differing bits: the model filter flag (is_pull_request), the ID-suffix regex
(e.g., _PR_ID_SUFFIX vs issue regex), the path getter (get_raw_source_pr_path /
get_raw_source_issue_path) and the document builder (build_pr_document /
build_issue_document). Move the common sequence that builds numbers from the
queryset and failed_ids, checks path.is_file(), reads JSON and appends documents
(currently implemented inside preprocess_for_pinecone and the issue counterpart
using ClangGithubIssueItem.objects.filter, failed_ids loop, json.loads, etc.)
into that helper, and have preprocess_for_pinecone simply call it with
owner/repo, final_sync_at and the PR-specific symbols.
In `@clang_github_tracker/tests/test_publisher.py`:
- Around line 137-144: The inline import of shutil inside the helper function
_clone_side_effect should be moved to module-level imports; add "import shutil"
to the top-of-file imports and remove the "import shutil" statement from inside
_clone_side_effect so the function simply uses shutil.rmtree(dest) without
importing locally.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a3f0e2d8-0f9b-46c5-ae36-ffb8ccf224d6
📒 Files selected for processing (34)
.env.example.gitignoreclang_github_tracker/__init__.pyclang_github_tracker/apps.pyclang_github_tracker/management/commands/backfill_clang_github_tracker.pyclang_github_tracker/management/commands/run_clang_github_tracker.pyclang_github_tracker/migrations/0001_initial.pyclang_github_tracker/migrations/__init__.pyclang_github_tracker/models.pyclang_github_tracker/preprocessors/__init__.pyclang_github_tracker/preprocessors/issue_preprocessor.pyclang_github_tracker/preprocessors/pr_preprocessor.pyclang_github_tracker/publisher.pyclang_github_tracker/services.pyclang_github_tracker/state_manager.pyclang_github_tracker/sync_raw.pyclang_github_tracker/tests/test_backfill.pyclang_github_tracker/tests/test_commands.pyclang_github_tracker/tests/test_preprocessors.pyclang_github_tracker/tests/test_publisher.pyclang_github_tracker/tests/test_services.pyclang_github_tracker/tests/test_state_manager.pyclang_github_tracker/workspace.pyconfig/settings.pyconfig/test_settings.pydocs/Pinecone_preprocess_guideline.mddocs/Schema.mddocs/Workspace.mddocs/service_api/README.mddocs/service_api/clang_github_tracker.mdgithub_ops/git_ops.pygithub_ops/tests/test_git_ops.pyoperations/md_ops/github_export.pyoperations/tests/test_github_export.py
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
github_ops/tests/test_git_ops.py (1)
49-68: Nice start—add oneclone_repo()failure-path assertion.These helper cases validate the sanitizer itself, but they do not prove the changed
clone_repo()exception path actually redacts subprocess output. One forcedCalledProcessErrortest here would guard the real secret-leak boundary.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@github_ops/tests/test_git_ops.py` around lines 49 - 68, Add a test that forces clone_repo to hit the subprocess failure path by mocking subprocess.run (or the internal runner used by clone_repo) to raise subprocess.CalledProcessError with output containing a secret token, then call clone_repo and assert the raised exception message (or logged/returned error) does not include the raw secret but does include the sanitized form from sanitize_git_output; reference clone_repo, subprocess.CalledProcessError, and sanitize_git_output so the test locates and validates the real redaction boundary.clang_github_tracker/tests/test_backfill.py (1)
12-94: Add one multi-batch case for the new chunked backfill path.These tests only exercise the final-drain path. The PR’s main behavior change is periodic batch upserts every 10k files, so an off-by-one in the boundary flush or final remainder handling would still pass here. Please add one case that lowers the batch size in-test and verifies rows from both an intermediate flush and the trailing remainder land in the DB.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clang_github_tracker/tests/test_backfill.py` around lines 12 - 94, Add a new test (e.g., test_backfill_multi_batch) that exercises the chunked backfill path by creating more raw files than the command's batch threshold, monkeypatching the batch-size constant inside the management command module (management.commands.backfill_clang_github_tracker — look for a symbol like BATCH_SIZE/CHUNK_SIZE/FILES_PER_BATCH) to a small value (e.g., 10), monkeypatch get_raw_repo_dir to point to your tmp_path root (reuse the pattern from test_backfill_from_raw), write >batch files (issues/prs/commits) so at least one intermediate flush and a trailing remainder occur, run call_command("backfill_clang_github_tracker", "--from-raw") and assert that items from an intermediate flush and from the final remainder (using ClangGithubIssueItem and ClangGithubCommit filters) exist in the DB.clang_github_tracker/management/commands/backfill_clang_github_tracker.py (1)
88-90: Stream the CSV import instead of buffering the whole file.
commit_rowsandissue_rowsgrow until EOF, so the first DB write happens only after the entire CSV is parsed. On the large backfills this PR is targeting, that defeats the chunked-backfill goal and needlessly spikes memory. Flush every N rows here as well, then clear the buffers after each batch.Also applies to: 141-142
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clang_github_tracker/management/commands/backfill_clang_github_tracker.py` around lines 88 - 90, commit_rows and issue_rows buffer the entire CSV before the first DB write, causing high memory usage; modify the loop that populates commit_rows and issue_rows (and tracks skipped) to flush every N rows: when len(commit_rows) + len(issue_rows) >= BATCH_SIZE (choose a constant like BATCH_SIZE), call the existing batch write logic used elsewhere to persist those rows to the DB, then clear() both commit_rows and issue_rows and continue parsing; apply the same streaming/flush pattern to the other occurrence noted around lines 141-142 so neither buffer grows to EOF and skipped is handled incrementally during each batch flush.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clang_github_tracker/management/commands/run_clang_github_tracker.py`:
- Around line 214-216: Guard remote publish so we don't push stale files when no
Markdown was generated: change the branch that currently calls
self._push_markdown(md_output_dir, new_files) to only invoke it when new_files
is non-empty (e.g., if new_files: self._push_markdown(...)), and otherwise skip
publishing (or log and regenerate/persist the rename map before retrying); this
touches the call site that passes new_files and the
publish_clang_markdown/_push_markdown flow so the publish code never runs with
an empty new_files dict.
In `@clang_github_tracker/preprocessors/issue_preprocessor.py`:
- Around line 61-69: The loop currently skips issues when the raw file is
missing or JSON parsing fails (the path check and the json.loads() except block
around get_raw_source_issue_path and path.read_text), which silently drops them;
instead of using continue, record/surface the failure for the caller by adding
the issue id (number) to the retry/failure collection (e.g. failed_ids or a
failed_issue_numbers list) or by raising a specific exception that the caller
can handle; also ensure the logger.warning includes the exception details from
the json.loads() except and the missing-path branch logs an error-level message
so the failure is visible.
In `@clang_github_tracker/services.py`:
- Around line 37-44: The upsert currently overwrites persisted fields with
incoming values verbatim; change the update_or_create defaults handling for
ClangGithubIssueItem so you merge existing row values with incoming ones before
writing: fetch the existing instance when present (or use the returned obj from
update_or_create), and set defaults such that is_pull_request =
existing.is_pull_request or incoming_is_pull_request, github_created_at =
existing.github_created_at if incoming_github_created_at is None else
incoming_github_created_at, and github_updated_at/github_committed_at =
max(existing_val, incoming_val) (treat None as minimum) before saving; apply the
same merge logic to the other update_or_create sites noted (the blocks around
lines referenced: the other uses at 63-66, 90-96, 150-160) so null/older
incoming values don’t erase stronger persisted state.
In `@clang_github_tracker/tests/test_preprocessors.py`:
- Around line 15-46: The test test_issue_preprocessor_db_and_failed_ids doesn't
exercise the retry path because only the DB-selected issue (number 10) has a
readable raw file; update the test so the failed-ID path is observable by either
(a) making the DB-selected set empty (remove or avoid creating
ClangGithubIssueItem for number 10) so the code must process the provided input
"llvm-project:issue:99", or (b) add a raw file for issue 99 in the tmp_path and
have the patched get_raw_source_issue_path return that file for n==99, then
assert that preprocess_for_pinecone(["llvm-project:issue:99"], final) triggers
the build for 99 (check mock_build.call_count and that docs include the parsed
99 result); modify the test accordingly around the get_raw_source_issue_path
patch and the assertions so the failed-ID retry path
(issue_preprocessor.preprocess_for_pinecone and get_raw_source_issue_path) is
actually validated.
In `@clang_github_tracker/tests/test_state_manager.py`:
- Around line 94-105: The test test_resolve_since_floor_without_until should
assert exact equality to lock the since-only behavior: after calling
clang_state.resolve_start_end_dates(since, None) assert that both start values
derived for streams equal the provided since (replace the loose assert si >=
since with assert si == since and also assert _sc == since where _sc is the
other stream start), and update the docstring to reflect that the explicit since
overrides DB watermarks rather than taking a max. Ensure you reference the
resolve_start_end_dates call and the ClangGithubIssueItem setup so the test
still seeds the DB entry.
In `@config/settings.py`:
- Around line 247-249: CLANG_GITHUB_CONTEXT_REPO_BRANCH is set to an empty
string which then flows into publish_clang_markdown(), causing invalid git ref
errors; update the call path so publish_clang_markdown() is only invoked when
all three GitHub context pieces are non-empty (owner, repo, and
CLANG_GITHUB_CONTEXT_REPO_BRANCH) or alternatively change
CLANG_GITHUB_CONTEXT_REPO_BRANCH to a safe non-empty default; locate references
to CLANG_GITHUB_CONTEXT_REPO_BRANCH and the function publish_clang_markdown()
and add a guard that checks owner, repo, and branch are truthy before attempting
to construct refs or call publish_clang_markdown().
In `@github_ops/git_ops.py`:
- Around line 227-243: When re-raising the subprocess.CalledProcessError in the
clone failure handler, sanitize the captured e.stdout and e.stderr with
sanitize_git_output and pass those sanitized values into the new
CalledProcessError instead of the raw e.stdout/e.stderr so secrets (PATs) can’t
leak; locate the block that builds safe_cmd and change the raise to use
safe_stdout = sanitize_git_output(e.stdout or "") and safe_stderr =
sanitize_git_output(e.stderr or "") (or equivalent) and pass
safe_stdout/safe_stderr into subprocess.CalledProcessError(e.returncode,
safe_cmd, safe_stdout, safe_stderr).
---
Nitpick comments:
In `@clang_github_tracker/management/commands/backfill_clang_github_tracker.py`:
- Around line 88-90: commit_rows and issue_rows buffer the entire CSV before the
first DB write, causing high memory usage; modify the loop that populates
commit_rows and issue_rows (and tracks skipped) to flush every N rows: when
len(commit_rows) + len(issue_rows) >= BATCH_SIZE (choose a constant like
BATCH_SIZE), call the existing batch write logic used elsewhere to persist those
rows to the DB, then clear() both commit_rows and issue_rows and continue
parsing; apply the same streaming/flush pattern to the other occurrence noted
around lines 141-142 so neither buffer grows to EOF and skipped is handled
incrementally during each batch flush.
In `@clang_github_tracker/tests/test_backfill.py`:
- Around line 12-94: Add a new test (e.g., test_backfill_multi_batch) that
exercises the chunked backfill path by creating more raw files than the
command's batch threshold, monkeypatching the batch-size constant inside the
management command module (management.commands.backfill_clang_github_tracker —
look for a symbol like BATCH_SIZE/CHUNK_SIZE/FILES_PER_BATCH) to a small value
(e.g., 10), monkeypatch get_raw_repo_dir to point to your tmp_path root (reuse
the pattern from test_backfill_from_raw), write >batch files
(issues/prs/commits) so at least one intermediate flush and a trailing remainder
occur, run call_command("backfill_clang_github_tracker", "--from-raw") and
assert that items from an intermediate flush and from the final remainder (using
ClangGithubIssueItem and ClangGithubCommit filters) exist in the DB.
In `@github_ops/tests/test_git_ops.py`:
- Around line 49-68: Add a test that forces clone_repo to hit the subprocess
failure path by mocking subprocess.run (or the internal runner used by
clone_repo) to raise subprocess.CalledProcessError with output containing a
secret token, then call clone_repo and assert the raised exception message (or
logged/returned error) does not include the raw secret but does include the
sanitized form from sanitize_git_output; reference clone_repo,
subprocess.CalledProcessError, and sanitize_git_output so the test locates and
validates the real redaction boundary.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4869c74a-9e19-4978-9d3d-b302c78842b1
📒 Files selected for processing (34)
.env.example.gitignoreclang_github_tracker/__init__.pyclang_github_tracker/apps.pyclang_github_tracker/management/commands/backfill_clang_github_tracker.pyclang_github_tracker/management/commands/run_clang_github_tracker.pyclang_github_tracker/migrations/0001_initial.pyclang_github_tracker/migrations/__init__.pyclang_github_tracker/models.pyclang_github_tracker/preprocessors/__init__.pyclang_github_tracker/preprocessors/issue_preprocessor.pyclang_github_tracker/preprocessors/pr_preprocessor.pyclang_github_tracker/publisher.pyclang_github_tracker/services.pyclang_github_tracker/state_manager.pyclang_github_tracker/sync_raw.pyclang_github_tracker/tests/test_backfill.pyclang_github_tracker/tests/test_commands.pyclang_github_tracker/tests/test_preprocessors.pyclang_github_tracker/tests/test_publisher.pyclang_github_tracker/tests/test_services.pyclang_github_tracker/tests/test_state_manager.pyclang_github_tracker/workspace.pyconfig/settings.pyconfig/test_settings.pydocs/Pinecone_preprocess_guideline.mddocs/Schema.mddocs/Workspace.mddocs/service_api/README.mddocs/service_api/clang_github_tracker.mdgithub_ops/git_ops.pygithub_ops/tests/test_git_ops.pyoperations/md_ops/github_export.pyoperations/tests/test_github_export.py
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
docs/Schema.md (1)
357-357:⚠️ Potential issue | 🟡 MinorFix watermark offset unit in the schema text.
Line 357 says
Max + 1s, but the current behavior is+1ms. This should be aligned to avoid documenting a wider resume gap than implemented.Doc patch
-| **ClangGithubIssueItem** | One row per issue or PR **number** (`unique`). `is_pull_request` distinguishes types. `github_created_at` / `github_updated_at` mirror GitHub API times; **`github_updated_at`** (with `Max` + 1s) drives **API fetch** resume. Django **`updated_at`** (`auto_now`) bumps on every upsert and drives **Pinecone** incrementality vs `PineconeSyncStatus.final_sync_at`. | +| **ClangGithubIssueItem** | One row per issue or PR **number** (`unique`). `is_pull_request` distinguishes types. `github_created_at` / `github_updated_at` mirror GitHub API times; **`github_updated_at`** (with `Max` + 1ms) drives **API fetch** resume. Django **`updated_at`** (`auto_now`) bumps on every upsert and drives **Pinecone** incrementality vs `PineconeSyncStatus.final_sync_at`. |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/Schema.md` at line 357, The schema text for ClangGithubIssueItem incorrectly states the watermark offset as "Max + 1s"; update the documentation string that references github_updated_at to say "Max + 1ms" (or "+1ms") so it matches the actual implemented behavior and avoids overstating the resume gap; ensure the phrase near "github_updated_at (with Max + 1s) drives API fetch resume" is changed to "github_updated_at (with Max + 1ms) drives API fetch resume".
🧹 Nitpick comments (2)
.gitignore (1)
55-55: Scope this ignore rule to repo root to avoid overmatching.
nulcurrently ignores any path segment namednulanywhere. If this is meant to ignore only the top-level artifact, prefer/nul.Suggested diff
-nul +/nul🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore at line 55, Update the ignore rule that currently contains the pattern "nul" so it only matches the repository root by replacing the bare "nul" entry with "/nul"; search for occurrences of the "nul" pattern in the .gitignore and change them to the anchored "/nul" form to avoid matching any path segment named "nul" deeper in the tree.github_ops/tests/test_git_ops.py (1)
49-68: Cover the re-raised exception objects too.These tests prove the regex helper works, but the leak prevention lives in the
clone_repo()/push()exception objects. Please add a regression test that assertscmd,stdout, andstderrare redacted after a simulated git failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@github_ops/tests/test_git_ops.py` around lines 49 - 68, Add a regression test that simulates a git failure from clone_repo() or push() and verifies the exception object's cmd, stdout, and stderr are sanitized by sanitize_git_output; specifically, call clone_repo() (or push()) with inputs that produce a fake/monkeypatched git failure (raise the same exception type your code uses), catch the exception, and assert that exception.cmd, exception.stdout, and exception.stderr do not contain secrets like "ghp_SUPER_SECRET" or "github_pat_XXXX" and that they include the redacted placeholder (e.g., "https://<redacted>@github.com"); use the same helper sanitize_git_output in your assertions and target the exception attributes named cmd, stdout, and stderr so the test covers the re-raised exception objects as well as the helper function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clang_github_tracker/management/commands/run_clang_github_tracker.py`:
- Around line 184-216: The md_export directory and push logic (md_output_dir =
get_workspace_root() / "md_export", md_output_dir.mkdir..., write_md_files(...),
and self._push_markdown(...)) must be wrapped in a per-target inter-process lock
to prevent concurrent runs from clobbering the shared clone; implement a lock
file keyed by owner/repo (e.g., lock_path = get_workspace_root() /
f"md_export.{OWNER}.{REPO}.lock") and acquire an exclusive blocking lock (with a
sensible timeout) before creating/writing/exporting and before calling
_push_markdown, releasing the lock in a finally block; ensure the locking
surrounds both the markdown export branch (when not skip_markdown_export) and
the remote push branch (when not skip_remote_push) so the sequence from
write_md_files to _push_markdown is atomic across processes.
In `@clang_github_tracker/publisher.py`:
- Around line 188-204: When retrying a publish where new_files == {},
detect_stale_titled_paths(md_output_dir, new_files) returns empty and leaves old
titled siblings in clone_dir; persist the repo-relative rename manifest during
the export step and reload it here before computing stale paths so stale
detection uses the saved map instead of the empty new_files. Modify the export
code to write the rename/paths manifest and update the publish block (around
detect_stale_titled_paths, all_stale, and before calling _copy_md_tree) to load
that manifest when new_files is empty, then pass the restored map into
detect_stale_titled_paths so stale deletes occur correctly while still allowing
_push_markdown(md_output_dir, new_files) to run for retries.
In `@clang_github_tracker/services.py`:
- Around line 57-71: The upsert logic in upsert_issue_item, upsert_commit and
their bulk counterparts (upsert_issue_items_batch, upsert_commits_batch) reads
rows into Python, merges fields via _merge_issue_item_fields, then writes back,
which allows stale-read races that can regress
github_updated_at/github_committed_at and is_pull_request; fix by moving the
merge into the DB atomically: replace the read-then-update pattern with an
INSERT ... ON CONFLICT (number or sha) DO UPDATE that computes merged values
server-side (e.g., use GREATEST for timestamps and CASE/COALESCE for
is_pull_request) against existing table columns in the UPDATE clause for
ClangGithubIssueItem/commit table, or alternatively enforce single-writer
serialization so only one process writes these tables at a time.
In `@clang_github_tracker/state_manager.py`:
- Around line 66-70: Update the docstring paragraph describing watermark resume
behavior to match the implementation in start_after_watermark(): state cursor
resumes at Max(...) + 1 millisecond (not +1 second). Specifically, change the
text that currently reads "Max(github_* timestamp) + 1 second" to indicate "+ 1
millisecond" and ensure the mention of watermarks using Max(github_committed_at)
and Max(github_updated_at) on ClangGithubCommit / ClangGithubIssueItem remains
accurate.
In `@Dockerfile`:
- Line 39: The Dockerfile currently sets Git global safe.directory to '*' (RUN
git config --system --add safe.directory '*'), which is too permissive; change
this to only trust the container workspace path used by the app (e.g., '/app' or
'/app/workspace') by replacing the wildcard with that specific path so Git
ownership safety checks remain scoped to the application workspace.
In `@github_ops/git_ops.py`:
- Around line 229-245: clone_repo() already redacts tokens before re-raising
CalledProcessError, but push(), pull(), and prepare_repo_for_pull() still
re-raise or propagate exceptions that include token-embedded URLs; update those
functions (push, pull, prepare_repo_for_pull) to mirror clone_repo(): construct
a safe_cmd that replaces the token-embedded URL with a redacted form, call
sanitize_git_output on e.stdout/e.stderr (or use empty string), log using the
sanitized values, and re-raise subprocess.CalledProcessError using e.returncode,
safe_cmd, safe_stdout, safe_stderr (from sanitize_git_output) instead of
propagating the raw exception so the PAT is never present in the exception or
logs.
---
Duplicate comments:
In `@docs/Schema.md`:
- Line 357: The schema text for ClangGithubIssueItem incorrectly states the
watermark offset as "Max + 1s"; update the documentation string that references
github_updated_at to say "Max + 1ms" (or "+1ms") so it matches the actual
implemented behavior and avoids overstating the resume gap; ensure the phrase
near "github_updated_at (with Max + 1s) drives API fetch resume" is changed to
"github_updated_at (with Max + 1ms) drives API fetch resume".
---
Nitpick comments:
In @.gitignore:
- Line 55: Update the ignore rule that currently contains the pattern "nul" so
it only matches the repository root by replacing the bare "nul" entry with
"/nul"; search for occurrences of the "nul" pattern in the .gitignore and change
them to the anchored "/nul" form to avoid matching any path segment named "nul"
deeper in the tree.
In `@github_ops/tests/test_git_ops.py`:
- Around line 49-68: Add a regression test that simulates a git failure from
clone_repo() or push() and verifies the exception object's cmd, stdout, and
stderr are sanitized by sanitize_git_output; specifically, call clone_repo() (or
push()) with inputs that produce a fake/monkeypatched git failure (raise the
same exception type your code uses), catch the exception, and assert that
exception.cmd, exception.stdout, and exception.stderr do not contain secrets
like "ghp_SUPER_SECRET" or "github_pat_XXXX" and that they include the redacted
placeholder (e.g., "https://<redacted>@github.com"); use the same helper
sanitize_git_output in your assertions and target the exception attributes named
cmd, stdout, and stderr so the test covers the re-raised exception objects as
well as the helper function.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 142da9f7-be84-4b4a-ae2a-eaac744e2610
📒 Files selected for processing (35)
.env.example.gitignoreDockerfileclang_github_tracker/__init__.pyclang_github_tracker/apps.pyclang_github_tracker/management/commands/backfill_clang_github_tracker.pyclang_github_tracker/management/commands/run_clang_github_tracker.pyclang_github_tracker/migrations/0001_initial.pyclang_github_tracker/migrations/__init__.pyclang_github_tracker/models.pyclang_github_tracker/preprocessors/__init__.pyclang_github_tracker/preprocessors/issue_preprocessor.pyclang_github_tracker/preprocessors/pr_preprocessor.pyclang_github_tracker/publisher.pyclang_github_tracker/services.pyclang_github_tracker/state_manager.pyclang_github_tracker/sync_raw.pyclang_github_tracker/tests/test_backfill.pyclang_github_tracker/tests/test_commands.pyclang_github_tracker/tests/test_preprocessors.pyclang_github_tracker/tests/test_publisher.pyclang_github_tracker/tests/test_services.pyclang_github_tracker/tests/test_state_manager.pyclang_github_tracker/workspace.pyconfig/settings.pyconfig/test_settings.pydocs/Pinecone_preprocess_guideline.mddocs/Schema.mddocs/Workspace.mddocs/service_api/README.mddocs/service_api/clang_github_tracker.mdgithub_ops/git_ops.pygithub_ops/tests/test_git_ops.pyoperations/md_ops/github_export.pyoperations/tests/test_github_export.py
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
.gitignore (1)
55-55: Scopenulignore rule to repo root.
nulcurrently matches at any directory depth. If this is meant to ignore the accidental top-level temp artifact, prefer/nulto avoid masking legitimate nested paths.Proposed tweak
-nul +/nul🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.gitignore at line 55, The current gitignore entry "nul" is too broad and will match files at any depth; change the pattern to scope it to the repo root by replacing the bare "nul" entry with "/nul" (or otherwise anchor the pattern) so only the top-level temporary artifact is ignored and nested paths are not masked.clang_github_tracker/preprocessors/issue_preprocessor.py (1)
65-69: Consider narrowing the exception types (optional).The broad
except Exceptionfollows the project's graceful-degradation pattern and is acceptable. However, narrowing to specific exceptions could improve clarity without changing behavior.♻️ Optional: narrow exception types
try: data = json.loads(path.read_text(encoding="utf-8")) - except Exception as e: # pylint: disable=broad-exception-caught + except (OSError, json.JSONDecodeError) as e: logger.warning("preprocess issue #%s: read failed %s", number, e) continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clang_github_tracker/preprocessors/issue_preprocessor.py` around lines 65 - 69, The current broad except in the try block around json.loads(path.read_text(...)) should be narrowed to specific exceptions: catch json.JSONDecodeError for invalid JSON, UnicodeDecodeError (or OSError) for read/encoding issues from path.read_text, and optionally FileNotFoundError; call logger.warning("preprocess issue #%s: read failed %s", number, e) in each handler (or a shared handler) so behavior stays the same but exception types are explicit; update the try/except around the json.loads and path.read_text calls in issue_preprocessor.py accordingly and keep the continue on failure.clang_github_tracker/services.py (1)
72-77: Log the merged PR state, not the raw input flag.If an existing PR row is replayed with
is_pull_request=False,_merge_issue_item_fields()still persistsTrue, but this debug line emitsFalse. That makes the log disagree with the stored row on the exact merge case this helper handles.♻️ Minimal fix
logger.debug( "clang issue item #%s %s (pr=%s)", number, "created" if created else "updated", - is_pull_request, + is_pr, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clang_github_tracker/services.py` around lines 72 - 77, The debug log is using the raw input is_pull_request flag which can differ from the persisted merged state; after calling _merge_issue_item_fields() (or after the merge logic), replace the raw is_pull_request in the logger.debug with the actual merged/updated value returned or stored (e.g., the variable returned from _merge_issue_item_fields or issue_item["is_pull_request"] / merged) so the log reflects the true persisted PR/merged state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clang_github_tracker/services.py`:
- Around line 223-225: In the docstring that starts with "Duplicate ``number``
values merge: ``github_updated_at`` uses the latest timestamp;
``github_created_at`` uses a later row’s value when non-None, otherwise keeps
the prior value; ``is_pull_request`` is True if any row", replace the smart
quote in "row’s" with a standard ASCII apostrophe ("row's") so the docstring
reads "row's" to satisfy RUF002 and silence the lint error; update the docstring
text in the same location in services.py accordingly.
- Around line 260-270: The current watermark logic in
get_commit_watermark()/start_after_watermark uses Max(github_committed_at) which
drops late-arriving/backdated commits; change to a sync-time based checkpoint or
add a sliding overlap plus SHA deduplication: persist and read a last_sync_at
timestamp (e.g., a ClangGithubSyncCheckpoint record) and use that as the lower
bound in queries instead of Max(github_committed_at), or keep using the max
committer date but subtract a small overlap window (e.g., 5–10 minutes) in
start_after_watermark and ensure downstream processing deduplicates by commit
SHA (github_sha on ClangGithubCommit) so backdated commits reintroduced in the
overlap are ignored if already seen. Ensure code changes touch
get_commit_watermark, start_after_watermark and the query path that consumes the
watermark and add/verify SHA-based uniqueness enforcement.
In `@docs/Schema.md`:
- Line 357: The Schema.md line incorrectly states the API fetch resume watermark
adds 1 second; update the ClangGithubIssueItem description to say the
`github_updated_at` resume watermark uses Max + 1 millisecond. Mention
`github_updated_at (with Max + 1ms)` instead of seconds to match the
implementation in `clang_github_tracker.services.start_after_watermark()` and
the test `test_resolve_db_watermark_plus_one_millisecond()`.
---
Nitpick comments:
In @.gitignore:
- Line 55: The current gitignore entry "nul" is too broad and will match files
at any depth; change the pattern to scope it to the repo root by replacing the
bare "nul" entry with "/nul" (or otherwise anchor the pattern) so only the
top-level temporary artifact is ignored and nested paths are not masked.
In `@clang_github_tracker/preprocessors/issue_preprocessor.py`:
- Around line 65-69: The current broad except in the try block around
json.loads(path.read_text(...)) should be narrowed to specific exceptions: catch
json.JSONDecodeError for invalid JSON, UnicodeDecodeError (or OSError) for
read/encoding issues from path.read_text, and optionally FileNotFoundError; call
logger.warning("preprocess issue #%s: read failed %s", number, e) in each
handler (or a shared handler) so behavior stays the same but exception types are
explicit; update the try/except around the json.loads and path.read_text calls
in issue_preprocessor.py accordingly and keep the continue on failure.
In `@clang_github_tracker/services.py`:
- Around line 72-77: The debug log is using the raw input is_pull_request flag
which can differ from the persisted merged state; after calling
_merge_issue_item_fields() (or after the merge logic), replace the raw
is_pull_request in the logger.debug with the actual merged/updated value
returned or stored (e.g., the variable returned from _merge_issue_item_fields or
issue_item["is_pull_request"] / merged) so the log reflects the true persisted
PR/merged state.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2a32f239-2205-4cee-ac5f-75a3fe55d676
📒 Files selected for processing (35)
.env.example.gitignoreDockerfileclang_github_tracker/__init__.pyclang_github_tracker/apps.pyclang_github_tracker/management/commands/backfill_clang_github_tracker.pyclang_github_tracker/management/commands/run_clang_github_tracker.pyclang_github_tracker/migrations/0001_initial.pyclang_github_tracker/migrations/__init__.pyclang_github_tracker/models.pyclang_github_tracker/preprocessors/__init__.pyclang_github_tracker/preprocessors/issue_preprocessor.pyclang_github_tracker/preprocessors/pr_preprocessor.pyclang_github_tracker/publisher.pyclang_github_tracker/services.pyclang_github_tracker/state_manager.pyclang_github_tracker/sync_raw.pyclang_github_tracker/tests/test_backfill.pyclang_github_tracker/tests/test_commands.pyclang_github_tracker/tests/test_preprocessors.pyclang_github_tracker/tests/test_publisher.pyclang_github_tracker/tests/test_services.pyclang_github_tracker/tests/test_state_manager.pyclang_github_tracker/workspace.pyconfig/settings.pyconfig/test_settings.pydocs/Pinecone_preprocess_guideline.mddocs/Schema.mddocs/Workspace.mddocs/service_api/README.mddocs/service_api/clang_github_tracker.mdgithub_ops/git_ops.pygithub_ops/tests/test_git_ops.pyoperations/md_ops/github_export.pyoperations/tests/test_github_export.py
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
github_ops/git_ops.py (1)
219-245:⚠️ Potential issue | 🔴 CriticalSanitize the clone timeout path too.
except subprocess.TimeoutExpired: raisestill propagates the originalcmd, which containsclone_urlwith the PAT. A hung clone will therefore leak the token even though the non-zero-exit path is now redacted.🐛 Minimal fix
- except subprocess.TimeoutExpired: + except subprocess.TimeoutExpired as e: + safe_url_or_slug = sanitize_git_output(url_or_slug) + safe_cmd = ["git", "clone", safe_url_or_slug, str(dest_dir)] + if depth is not None: + safe_cmd.extend(["--depth", str(depth)]) logger.warning( "git clone timed out after %ss (%s -> %s)", GIT_CMD_TIMEOUT_SECONDS, - url_or_slug, + safe_url_or_slug, dest_dir, ) - raise + raise subprocess.TimeoutExpired( + safe_cmd, + e.timeout, + output=None if e.output is None else sanitize_git_output(e.output), + stderr=None if e.stderr is None else sanitize_git_output(e.stderr), + ) from None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@github_ops/git_ops.py` around lines 219 - 245, The TimeoutExpired handler currently re-raises the original exception (which contains the sensitive clone cmd), so modify the except subprocess.TimeoutExpired block to build a sanitized safe_cmd (like in the CalledProcessError path: ["git","clone", url_or_slug, str(dest_dir)] plus depth if set), sanitize any output using sanitize_git_output, and re-raise a new subprocess.TimeoutExpired(safe_cmd, GIT_CMD_TIMEOUT_SECONDS, output=safe_stdout, stderr=safe_stderr) from None so the original cmd/PAT is not leaked; keep the existing logger.warning call but ensure you do not re-raise the original exception.
♻️ Duplicate comments (2)
github_ops/git_ops.py (1)
326-334:⚠️ Potential issue | 🔴 Critical
remote_urlstill carries credentials into both the real and “safe” commands.If the origin was cloned from a credential-bearing HTTPS URL,
git remote get-urlcan hand that same URL back here. Then the actualpush/pull/fetchkeeps using the embedded PAT instead of the currenttoken, and the re-raisedsafe_*_cmdstill exposes it. Normalizeremote_urlto a credential-free form first, then build both the authenticated command and the safe command from that.When a repository is cloned from an HTTPS URL containing credentials, does `git remote get-url origin` return that credential-bearing URL unchanged?Also applies to: 400-408, 455-476
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@github_ops/git_ops.py` around lines 326 - 334, remote_url may contain embedded credentials from `git remote get-url`, so both `push_url` and `safe_push_cmd` are currently built from the same credential-bearing value; first strip credentials from `remote_url` to produce a credential-free base (use a normalization helper or parse and remove userinfo), assign that cleaned value to `remote_url_clean`, then call `_url_with_token(remote_url_clean, token)` to create `push_url` and build `safe_push_cmd` from `remote_url_clean` (not the original `remote_url`); update occurrences in `git_ops.py` (e.g. where `remote_url`, `push_url`, and `safe_push_cmd` are used) and reuse the same normalization for the other affected blocks (lines around 400-408 and 455-476).clang_github_tracker/state_manager.py (1)
66-70:⚠️ Potential issue | 🟡 MinorDocstring says "+1 second" but implementation uses "+1 millisecond".
The docstring states
Max(github_* timestamp) + 1 second, butstart_after_watermark()inservices.pyaddstimedelta(milliseconds=1). Update the docstring to match the implementation.✏️ Suggested fix
- - **Starts:** If ``since`` is set (without a valid closed window): ``start_commit`` - and ``start_item`` are both ``since``. If ``since`` is not set: both are - ``Max(github_* timestamp) + 1 second`` from the DB when a watermark exists, else + - **Starts:** If ``since`` is set (without a valid closed window): ``start_commit`` + and ``start_item`` are both ``since``. If ``since`` is not set: both are + ``Max(github_* timestamp) + 1 millisecond`` from the DB when a watermark exists, else🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clang_github_tracker/state_manager.py` around lines 66 - 70, Update the docstring in state_manager.py so its description of the watermark offset matches the implementation: change the phrase indicating "+1 second" to "+1 millisecond" (to reflect the timedelta(milliseconds=1) used in start_after_watermark() in services.py), ensuring any mention of start_commit/start_item offset or "Max(...)+1" uses "1 millisecond" consistently.
🧹 Nitpick comments (2)
clang_github_tracker/preprocessors/pr_preprocessor.py (1)
45-50: Considerdatetime.timezone.utcfor Django 5+ forward compatibility.
django.utils.timezone.utcwas deprecated in Django 4.1 and removed in Django 5.0. While the project currently pinsDjango>=4.2,<5, using the stdlib equivalent now would ease future upgrades.Optional forward-compatibility fix
-from datetime import datetime +from datetime import datetime, timezone as dt_timezone ... - fs = timezone.make_aware(fs, timezone.utc) + fs = timezone.make_aware(fs, dt_timezone.utc)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clang_github_tracker/preprocessors/pr_preprocessor.py` around lines 45 - 50, The code uses django.utils.timezone.utc (via timezone.make_aware) which is deprecated; update the implementation to use the stdlib timezone by importing datetime and passing datetime.timezone.utc to timezone.make_aware when handling final_sync_at (the block that checks timezone.is_naive(fs) and calls timezone.make_aware(fs, timezone.utc)); ensure you add the appropriate import for datetime (or from datetime import timezone as dt_timezone) and replace timezone.utc with datetime.timezone.utc so ClangGithubIssueItem query logic remains unchanged.github_ops/tests/test_git_ops.py (1)
339-366: Fragile index-based assertion at line 365.The assertion
assert remote in excinfo.value.cmd[4]assumes a specific command structure. If the git command format changes (e.g., additional flags), this test will break even though the redaction logic is correct.Consider using a membership check instead:
Suggested fix
- assert "PAT" not in " ".join(excinfo.value.cmd) - assert remote in excinfo.value.cmd[4] + cmd_str = " ".join(excinfo.value.cmd) + assert "PAT" not in cmd_str + assert remote in cmd_str🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@github_ops/tests/test_git_ops.py` around lines 339 - 366, The test test_prepare_repo_fetch_failure_redacts_token_from_reraised_exception_cmd currently checks excinfo.value.cmd[4] which is brittle; change the assertion to verify the remote appears in any element of excinfo.value.cmd (for example using any(remote in part for part in excinfo.value.cmd) or by checking remote in " ".join(excinfo.value.cmd)) instead of indexing into [4], keeping the existing redaction assertion that "PAT" is not present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clang_github_tracker/services.py`:
- Around line 49-57: upsert_issue_item accepts booleans because bool subclasses
int; add an upfront guard in upsert_issue_item to require a positive integer and
explicitly reject bools (e.g. if not isinstance(number, int) or
isinstance(number, bool) or number <= 0: raise ValueError("number must be a
positive int, not bool")). Apply the same validation to the bulk/batch code path
that filters/iterates incoming numbers (the place that builds the list/filter
for batch upserts) so it excludes booleans and non‑positive integers before
querying/creating ClangGithubIssueItem.
- Around line 87-89: The code currently only strips whitespace for the SHA
(variable sha_clean) causing case-sensitive duplicates; canonicalize the SHA by
applying a consistent case (e.g., sha_clean = (sha or "").strip().lower())
before the length check and also before the dedupe/upsert path referenced later
(the second occurrence around line 154) so all comparisons and DB upserts use
the same canonical lowercase SHA.
In `@clang_github_tracker/tests/test_services.py`:
- Around line 23-35: The test's assertion row.updated_at > first_updated is
flaky due to clock-resolution; change the test to make the second save
deterministic by freezing/mocking time around the second upsert call or by
asserting non-strict equality and verifying the actual change via a field that
we control (e.g. assert row.github_updated_at == t1 and assert row.updated_at >=
first_updated). Specifically, wrap the second call to
clang_services.upsert_issue_item (the second invocation with
github_updated_at=t1) in a time freeze/mock (or use a deterministic timestamp
injection if upsert_issue_item/ClangGithubIssueItem supports it) or replace the
strict > check on ClangGithubIssueItem.updated_at with a >= plus the explicit
check that ClangGithubIssueItem.github_updated_at == t1 to guarantee
deterministic behaviour.
In `@github_ops/git_ops.py`:
- Around line 409-417: The git pull subprocess call in pull() can block
indefinitely; add the same timeout guard used by push() and
prepare_repo_for_pull() to the subprocess.run invocation (i.e., pass the same
timeout parameter/value those functions use) so network fetch/merge operations
will raise on timeout instead of blocking the scheduled publish job. Ensure the
timeout value matches the constant or variable used by push() /
prepare_repo_for_pull() to keep behavior consistent.
---
Outside diff comments:
In `@github_ops/git_ops.py`:
- Around line 219-245: The TimeoutExpired handler currently re-raises the
original exception (which contains the sensitive clone cmd), so modify the
except subprocess.TimeoutExpired block to build a sanitized safe_cmd (like in
the CalledProcessError path: ["git","clone", url_or_slug, str(dest_dir)] plus
depth if set), sanitize any output using sanitize_git_output, and re-raise a new
subprocess.TimeoutExpired(safe_cmd, GIT_CMD_TIMEOUT_SECONDS, output=safe_stdout,
stderr=safe_stderr) from None so the original cmd/PAT is not leaked; keep the
existing logger.warning call but ensure you do not re-raise the original
exception.
---
Duplicate comments:
In `@clang_github_tracker/state_manager.py`:
- Around line 66-70: Update the docstring in state_manager.py so its description
of the watermark offset matches the implementation: change the phrase indicating
"+1 second" to "+1 millisecond" (to reflect the timedelta(milliseconds=1) used
in start_after_watermark() in services.py), ensuring any mention of
start_commit/start_item offset or "Max(...)+1" uses "1 millisecond"
consistently.
In `@github_ops/git_ops.py`:
- Around line 326-334: remote_url may contain embedded credentials from `git
remote get-url`, so both `push_url` and `safe_push_cmd` are currently built from
the same credential-bearing value; first strip credentials from `remote_url` to
produce a credential-free base (use a normalization helper or parse and remove
userinfo), assign that cleaned value to `remote_url_clean`, then call
`_url_with_token(remote_url_clean, token)` to create `push_url` and build
`safe_push_cmd` from `remote_url_clean` (not the original `remote_url`); update
occurrences in `git_ops.py` (e.g. where `remote_url`, `push_url`, and
`safe_push_cmd` are used) and reuse the same normalization for the other
affected blocks (lines around 400-408 and 455-476).
---
Nitpick comments:
In `@clang_github_tracker/preprocessors/pr_preprocessor.py`:
- Around line 45-50: The code uses django.utils.timezone.utc (via
timezone.make_aware) which is deprecated; update the implementation to use the
stdlib timezone by importing datetime and passing datetime.timezone.utc to
timezone.make_aware when handling final_sync_at (the block that checks
timezone.is_naive(fs) and calls timezone.make_aware(fs, timezone.utc)); ensure
you add the appropriate import for datetime (or from datetime import timezone as
dt_timezone) and replace timezone.utc with datetime.timezone.utc so
ClangGithubIssueItem query logic remains unchanged.
In `@github_ops/tests/test_git_ops.py`:
- Around line 339-366: The test
test_prepare_repo_fetch_failure_redacts_token_from_reraised_exception_cmd
currently checks excinfo.value.cmd[4] which is brittle; change the assertion to
verify the remote appears in any element of excinfo.value.cmd (for example using
any(remote in part for part in excinfo.value.cmd) or by checking remote in "
".join(excinfo.value.cmd)) instead of indexing into [4], keeping the existing
redaction assertion that "PAT" is not present.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c6d197e9-60cd-4d21-bad1-5f796b7b2d71
📒 Files selected for processing (35)
.env.example.gitignoreDockerfileclang_github_tracker/__init__.pyclang_github_tracker/apps.pyclang_github_tracker/management/commands/backfill_clang_github_tracker.pyclang_github_tracker/management/commands/run_clang_github_tracker.pyclang_github_tracker/migrations/0001_initial.pyclang_github_tracker/migrations/__init__.pyclang_github_tracker/models.pyclang_github_tracker/preprocessors/__init__.pyclang_github_tracker/preprocessors/issue_preprocessor.pyclang_github_tracker/preprocessors/pr_preprocessor.pyclang_github_tracker/publisher.pyclang_github_tracker/services.pyclang_github_tracker/state_manager.pyclang_github_tracker/sync_raw.pyclang_github_tracker/tests/test_backfill.pyclang_github_tracker/tests/test_commands.pyclang_github_tracker/tests/test_preprocessors.pyclang_github_tracker/tests/test_publisher.pyclang_github_tracker/tests/test_services.pyclang_github_tracker/tests/test_state_manager.pyclang_github_tracker/workspace.pyconfig/settings.pyconfig/test_settings.pydocs/Pinecone_preprocess_guideline.mddocs/Schema.mddocs/Workspace.mddocs/service_api/README.mddocs/service_api/clang_github_tracker.mdgithub_ops/git_ops.pygithub_ops/tests/test_git_ops.pyoperations/md_ops/github_export.pyoperations/tests/test_github_export.py
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
clang_github_tracker/tests/test_commands.py (1)
51-66:⚠️ Potential issue | 🟡 MinorThese date-window tests still don't prove the resolved values are right.
The alias case only checks that
--dry-runlogsResolved:, and the sync case only checks that the kwargs exist. Both still pass if--from-date/--to-dateor--since/--untilget wired to the wrong datetimes. Assert the exact values passed intosync_clang_github_activity.Minimal tightening
sync_mock.assert_called_once() call_kw = sync_mock.call_args[1] - assert "start_commit" in call_kw - assert "start_item" in call_kw - assert "end_date" in call_kw + assert call_kw["start_commit"].date().isoformat() == "2024-01-01" + assert call_kw["start_item"].date().isoformat() == "2024-01-01" + assert call_kw["end_date"].date().isoformat() == "2024-01-02"Apply the same exact-value check to the
--from-date/--to-datealias test instead of treating it as dry-run smoke coverage.Also applies to: 87-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clang_github_tracker/tests/test_commands.py` around lines 51 - 66, Replace the loose dry-run logging check in test_run_clang_github_tracker_since_until_aliases with a precise assertion on the mocked sync_clang_github_activity call: call the command with "--from-date=2024-01-01" and "--to-date=2024-06-30" as you already do, then assert sync_mock was called exactly once and that its kwargs include since and until equal to the expected datetime/date values produced by the parser (i.e., the exact parsed values for 2024-01-01 and 2024-06-30); do the same tightening for the other test that uses --since/--until (the one around lines 87-90), checking sync_clang_github_activity’s called kwargs for exact equality rather than only checking presence or log output.clang_github_tracker/sync_raw.py (1)
103-116:⚠️ Potential issue | 🟠 MajorFix numeric validation: use type check instead of isinstance to reject bools, and append only validated numbers.
The guard
isinstance(num, int) and num > 0acceptsTrue/False(sinceboolsubclassesintin Python), butupsert_issue_item()explicitly rejects bools via_invalid_issue_number()and raisesValueError. A malformed payload with a boolnumberwould abort the entire sync after raw JSON is already written. Additionally,pr_numberandissue_numberare appended to the return lists beforenumis validated, so invalid data leaks into those lists.Replace the isinstance check with
type(num) is intorisinstance(num, int) and not isinstance(num, bool), and move the append to occur only after the guard passes (i.e., inside theifblock after upsert succeeds).Also applies to: 124-132
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clang_github_tracker/sync_raw.py` around lines 103 - 116, The numeric validation currently uses isinstance(num, int) which accepts bools and also appends pr_number/issue_number before validation; update the logic around normalize_pr_json/num and clang_services.upsert_issue_item so you explicitly reject bools (e.g., use type(num) is int or isinstance(num, int) and not isinstance(num, bool)) and only append pr_numbers/issue_numbers after the number passes that check and the upsert_issue_item call succeeds; ensure the append happens inside the validated if block (and apply the same change to the symmetric issue-handling block that uses the same pattern).
♻️ Duplicate comments (2)
github_ops/git_ops.py (1)
341-343:⚠️ Potential issue | 🔴 CriticalThese “safe” git commands can still carry the PAT.
remote_urlcomes from git config, not from your sanitizer. If the clone's origin is already credentialed, reusingremote_urlverbatim insafe_push_cmd,safe_pull_cmd, andsafe_fetch_cmdmakesstr(e)unsafe again inclang_github_tracker/publisher.pywhenever stderr/stdout are empty.Pattern to apply in all three blocks
remote_url = result.stdout.strip() + safe_remote_url = sanitize_git_output(remote_url) @@ - safe_push_cmd = ["git", "-C", str(repo_dir), "push", remote_url] + safe_push_cmd = ["git", "-C", str(repo_dir), "push", safe_remote_url]Apply the same
safe_remote_urlsubstitution to thepull()andprepare_repo_for_pull()command builders.#!/bin/bash set -euo pipefail tmp="$(mktemp -d)" trap 'rm -rf "$tmp"' EXIT git -C "$tmp" init demo >/dev/null git -C "$tmp/demo" remote add origin 'https://x-access-token:SECRET@github.com/acme/private.git' printf 'Configured remote URL:\n' git -C "$tmp/demo" remote get-url originIf the output still contains
SECRET,safe_*_cmdneeds to usesanitize_git_output(remote_url)before re-raising.Also applies to: 415-417, 490-498
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@github_ops/git_ops.py` around lines 341 - 343, The "safe" git command builders still embed the raw credentialed remote_url; fix by creating a safe_remote_url = sanitize_git_output(remote_url) and use safe_remote_url instead of remote_url when constructing safe_push_cmd, safe_pull_cmd, and safe_fetch_cmd (and in prepare_repo_for_pull/pull/prepare_repo_for_fetch command builders), and ensure any re-raise/log that includes the command or remote uses safe_remote_url so secrets are never leaked (refer to safe_push_cmd, safe_pull_cmd, safe_fetch_cmd builders and sanitize_git_output()).clang_github_tracker/publisher.py (1)
208-230:⚠️ Potential issue | 🟠 MajorRetry publishes still need a persisted desired-files manifest.
When
new_files == {}on a retry,stale_mdis always empty, so old titled siblings remain inmd_output_dir.md_repo_rel_mapis then built from that dirty tree, and_copy_md_tree()can copy both the old and new titles back into the clone. That same empty-manifest path also leaves the caller with nothing to clean after a successful retry. Based on learnings, the empty-new_filespublish path is intentional for retry runs.
🧹 Nitpick comments (2)
clang_github_tracker/state_manager.py (1)
62-64: Update this docstring to match the actualNonehandling.
sync_clang_github_activity()does not replaceNonewithtimezone.now(); it forwardsNone, and the fetchers interpret that as an open-ended upper bound. The behavior is fine, but this sentence is stale.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clang_github_tracker/state_manager.py` around lines 62 - 64, Update the docstring that describes ``end_date`` handling to reflect actual behavior: instead of saying ``sync_clang_github_activity`` substitutes ``timezone.now()``, state that ``end_date`` is ``until`` when provided else ``None``, and that ``sync_clang_github_activity`` forwards ``None`` to the fetchers which treat ``None`` as an open-ended/“through now” upper bound; update the sentence in clang_github_tracker.state_manager (referencing sync_clang_github_activity and the ``end_date`` variable) accordingly.clang_github_tracker/services.py (1)
79-83: Log the merged PR flag.
is_pris the value that actually gets persisted. Using the rawis_pull_requestinput can logpr=Falseeven when the row stays marked as a PR.♻️ Minimal tweak
logger.debug( "clang issue item #%s %s (pr=%s)", number, "created" if created else "updated", - is_pull_request, + is_pr, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@clang_github_tracker/services.py` around lines 79 - 83, The debug log is using the raw input is_pull_request instead of the persisted value is_pr; update the logger.debug call in services.py (the block that logs "clang issue item #%s %s (pr=%s)") to pass is_pr instead of is_pull_request so the message reflects the actual stored PR flag for the record.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clang_github_tracker/publisher.py`:
- Around line 133-141: The call to get_github_token(use="write") can raise
ValueError and should be wrapped and re-raised as CommandError to match existing
error handling; update the token assignment block (the get_github_token call
that sets token) to catch ValueError (and optionally other token-related
exceptions), and raise CommandError with a clear message including the original
exception details so token lookup failures are handled the same way as
clone/pull/push errors elsewhere in publisher.py.
In `@clang_github_tracker/tests/test_state_manager.py`:
- Around line 72-90: The test test_resolve_invalid_range_clears_bounds should
assert the specific DB-watermark fallback values instead of merely non-None:
after seeding ClangGithubIssueItem and ClangGithubCommit with wm and calling
clang_state.resolve_start_end_dates(since, until) you should assert that sc
equals wm + 1 millisecond and si equals wm (and end is still None), ensuring the
function falls back to the intended DB watermark values rather than any
arbitrary datetimes.
In `@github_ops/git_ops.py`:
- Around line 219-253: The git clone error path builds and logs commands using
the raw url_or_slug which may contain credentials; instead reuse the sanitized
value safe_url_or_slug (produced via sanitize_git_output) whenever constructing
the safe_cmd and when passing data to logger.warning or raising
subprocess.CalledProcessError: update the logger.warning call to use
safe_url_or_slug and change both places that build safe_cmd to insert
safe_url_or_slug (not url_or_slug), and ensure the CalledProcessError is raised
with sanitized stdout/stderr (sanitize_git_output already used) and the redacted
command list.
---
Outside diff comments:
In `@clang_github_tracker/sync_raw.py`:
- Around line 103-116: The numeric validation currently uses isinstance(num,
int) which accepts bools and also appends pr_number/issue_number before
validation; update the logic around normalize_pr_json/num and
clang_services.upsert_issue_item so you explicitly reject bools (e.g., use
type(num) is int or isinstance(num, int) and not isinstance(num, bool)) and only
append pr_numbers/issue_numbers after the number passes that check and the
upsert_issue_item call succeeds; ensure the append happens inside the validated
if block (and apply the same change to the symmetric issue-handling block that
uses the same pattern).
In `@clang_github_tracker/tests/test_commands.py`:
- Around line 51-66: Replace the loose dry-run logging check in
test_run_clang_github_tracker_since_until_aliases with a precise assertion on
the mocked sync_clang_github_activity call: call the command with
"--from-date=2024-01-01" and "--to-date=2024-06-30" as you already do, then
assert sync_mock was called exactly once and that its kwargs include since and
until equal to the expected datetime/date values produced by the parser (i.e.,
the exact parsed values for 2024-01-01 and 2024-06-30); do the same tightening
for the other test that uses --since/--until (the one around lines 87-90),
checking sync_clang_github_activity’s called kwargs for exact equality rather
than only checking presence or log output.
---
Duplicate comments:
In `@github_ops/git_ops.py`:
- Around line 341-343: The "safe" git command builders still embed the raw
credentialed remote_url; fix by creating a safe_remote_url =
sanitize_git_output(remote_url) and use safe_remote_url instead of remote_url
when constructing safe_push_cmd, safe_pull_cmd, and safe_fetch_cmd (and in
prepare_repo_for_pull/pull/prepare_repo_for_fetch command builders), and ensure
any re-raise/log that includes the command or remote uses safe_remote_url so
secrets are never leaked (refer to safe_push_cmd, safe_pull_cmd, safe_fetch_cmd
builders and sanitize_git_output()).
---
Nitpick comments:
In `@clang_github_tracker/services.py`:
- Around line 79-83: The debug log is using the raw input is_pull_request
instead of the persisted value is_pr; update the logger.debug call in
services.py (the block that logs "clang issue item #%s %s (pr=%s)") to pass
is_pr instead of is_pull_request so the message reflects the actual stored PR
flag for the record.
In `@clang_github_tracker/state_manager.py`:
- Around line 62-64: Update the docstring that describes ``end_date`` handling
to reflect actual behavior: instead of saying ``sync_clang_github_activity``
substitutes ``timezone.now()``, state that ``end_date`` is ``until`` when
provided else ``None``, and that ``sync_clang_github_activity`` forwards
``None`` to the fetchers which treat ``None`` as an open-ended/“through now”
upper bound; update the sentence in clang_github_tracker.state_manager
(referencing sync_clang_github_activity and the ``end_date`` variable)
accordingly.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a735fe39-cdae-4634-bd7f-791f89072c91
📒 Files selected for processing (35)
.env.example.gitignoreDockerfileclang_github_tracker/__init__.pyclang_github_tracker/apps.pyclang_github_tracker/management/commands/backfill_clang_github_tracker.pyclang_github_tracker/management/commands/run_clang_github_tracker.pyclang_github_tracker/migrations/0001_initial.pyclang_github_tracker/migrations/__init__.pyclang_github_tracker/models.pyclang_github_tracker/preprocessors/__init__.pyclang_github_tracker/preprocessors/issue_preprocessor.pyclang_github_tracker/preprocessors/pr_preprocessor.pyclang_github_tracker/publisher.pyclang_github_tracker/services.pyclang_github_tracker/state_manager.pyclang_github_tracker/sync_raw.pyclang_github_tracker/tests/test_backfill.pyclang_github_tracker/tests/test_commands.pyclang_github_tracker/tests/test_preprocessors.pyclang_github_tracker/tests/test_publisher.pyclang_github_tracker/tests/test_services.pyclang_github_tracker/tests/test_state_manager.pyclang_github_tracker/workspace.pyconfig/settings.pyconfig/test_settings.pydocs/Pinecone_preprocess_guideline.mddocs/Schema.mddocs/Workspace.mddocs/service_api/README.mddocs/service_api/clang_github_tracker.mdgithub_ops/git_ops.pygithub_ops/tests/test_git_ops.pyoperations/md_ops/github_export.pyoperations/tests/test_github_export.py
Collaborator
Author
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
github_ops/tests/test_git_ops.py (1)
305-414: Consider extracting a shared assertion helper for redacted subprocess errors.The repeated
cmdredaction checks are very similar; a small helper would reduce duplication and future test drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@github_ops/tests/test_git_ops.py` around lines 305 - 414, The tests test_push_failure_redacts_token_from_reraised_exception_cmd, test_pull_failure_redacts_token_from_reraised_exception_cmd, test_pull_timeout_redacts_token_from_reraised_exception_cmd, and test_prepare_repo_fetch_failure_redacts_token_from_reraised_exception_cmd duplicate identical assertions that the token is redacted and the remote URL is present; extract a small helper (e.g., assert_cmd_redacted(exc, token, remote) or assert_redacted_cmd(exception, secret, remote_url)) and replace the repeated assert lines in each test with a call to that helper; ensure the helper accepts either a CalledProcessError or TimeoutExpired and checks token not in " ".join(cmd) (or exc.cmd) and remote in the joined cmd (and also verifies timeout for TimeoutExpired when used).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Dockerfile`:
- Around line 36-39: The safe.directory pattern currently uses
'/app/workspace/*' which can fail on older Git versions; update the Dockerfile's
RUN git config --system --add safe.directory invocation to register
'/app/workspace' (no trailing /*) so the system-level safe.directory covers the
workspace and all nested repo paths (replace the existing RUN git config
--system --add safe.directory '/app/workspace/*' line with one that uses
'/app/workspace').
---
Nitpick comments:
In `@github_ops/tests/test_git_ops.py`:
- Around line 305-414: The tests
test_push_failure_redacts_token_from_reraised_exception_cmd,
test_pull_failure_redacts_token_from_reraised_exception_cmd,
test_pull_timeout_redacts_token_from_reraised_exception_cmd, and
test_prepare_repo_fetch_failure_redacts_token_from_reraised_exception_cmd
duplicate identical assertions that the token is redacted and the remote URL is
present; extract a small helper (e.g., assert_cmd_redacted(exc, token, remote)
or assert_redacted_cmd(exception, secret, remote_url)) and replace the repeated
assert lines in each test with a call to that helper; ensure the helper accepts
either a CalledProcessError or TimeoutExpired and checks token not in "
".join(cmd) (or exc.cmd) and remote in the joined cmd (and also verifies timeout
for TimeoutExpired when used).
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a6daa677-ae85-4286-8fe3-f00817314dc5
📒 Files selected for processing (35)
.env.example.gitignoreDockerfileclang_github_tracker/__init__.pyclang_github_tracker/apps.pyclang_github_tracker/management/commands/backfill_clang_github_tracker.pyclang_github_tracker/management/commands/run_clang_github_tracker.pyclang_github_tracker/migrations/0001_initial.pyclang_github_tracker/migrations/__init__.pyclang_github_tracker/models.pyclang_github_tracker/preprocessors/__init__.pyclang_github_tracker/preprocessors/issue_preprocessor.pyclang_github_tracker/preprocessors/pr_preprocessor.pyclang_github_tracker/publisher.pyclang_github_tracker/services.pyclang_github_tracker/state_manager.pyclang_github_tracker/sync_raw.pyclang_github_tracker/tests/test_backfill.pyclang_github_tracker/tests/test_commands.pyclang_github_tracker/tests/test_preprocessors.pyclang_github_tracker/tests/test_publisher.pyclang_github_tracker/tests/test_services.pyclang_github_tracker/tests/test_state_manager.pyclang_github_tracker/workspace.pyconfig/settings.pyconfig/test_settings.pydocs/Pinecone_preprocess_guideline.mddocs/Schema.mddocs/Workspace.mddocs/service_api/README.mddocs/service_api/clang_github_tracker.mdgithub_ops/git_ops.pygithub_ops/tests/test_git_ops.pyoperations/md_ops/github_export.pyoperations/tests/test_github_export.py
… backfill upserts, batch DB helpers, stale-export detection in github_export, safer git clone errors without leaking tokens, and aligned tests including isolated test_settings for publish - cppalliance#136
…act git clone logs; ignore /nul - cppalliance#136
…r_watermark as +1 ms - cppalliance#136
…ted git clone errors - cppalliance#136
…nup uses export tree - cppalliance#136
… redaction for clone/pull - cppalliance#136
…_raw issue-number guards - cppalliance#136
jonathanMLDev
approved these changes
Apr 8, 2026
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
2 participants
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.
Introduces a dedicated clang_github_tracker app (models + initial migration), switches sync/resume to DB watermarks instead of state.json, adds raw/CSV backfill with periodic batch upserts (10k-file chunks), Markdown export + Git publish to a repo configured via CLANG_GITHUB_CONTEXT_REPO_OWNER / _NAME / _BRANCH, batch upsert helpers in services, detect_stale_titled_paths in github_export (with tests), and safer git clone failures that do not embed PATs in raised exceptions. Updates settings, .env.example, workspace paths, preprocessor/docs touch-ups, and test coverage (including clearing context-repo env in test_settings so CI/local tests do not hit real publish).
Summary by CodeRabbit
New Features
Improvements
Configuration
Documentation
Tests
Chores