Repository navigation
Conversation
…6469) AGENTS.md stated these rules, but no check enforced them, and it claimed a CI check for ticket references that did not exist. - utils/check-copyright.py (in qualityChecks) applies the AGENTS.md copyright rule to the files a branch adds or modifies. The end year is compared with the file's last commit year, so a December change still passes when checked in January. - CLAUDE.md is now committed instead of git-ignored. utils/check-claude-md.py (in qualityChecks) keeps it the canonical redirect stub, which replaces the bootstrap comment in AGENTS.md. - The PR ticket reference workflow fails a PR whose title and body have no PROF-<n> or #<n>. It is a separate workflow so that editing the description re-runs it. - The new ci.yml script-tests job runs utils/tests/*.sh and the ticket-check test with Node.js 22. CI never ran these suites, and two had broken unnoticed: T1g pinned a sigsetjmp call count the tree no longer has, and the Pi adapter cases failed instead of skipping on Node.js older than 22.6. - AGENTS.md: the duplicate CI-script rules are merged into one section, Work Tracking is shortened to the binding rules, and each enforced rule names its check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
utils/tests/test_prepare_patch.sh runs utils/prepare-patch.sh, which checks for jq. ubuntu-22.04 runners ship it today; installing it keeps the job independent of the runner image. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f499864398
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ap = argparse.ArgumentParser(description=__doc__.split("\n\n")[0]) | ||
| ap.add_argument("--root", default=os.path.dirname(os.path.dirname(os.path.abspath(__file__))), | ||
| help="repository root (default: the checkout containing this script)") | ||
| ap.add_argument("--base", default="origin/main", help="base branch (default: origin/main)") |
There was a problem hiding this comment.
When a PR targets one of the repository's release/* branches, as utils/backport-pr.sh and utils/prepare-patch.sh do, qualityChecks invokes this script without --base, so the default compares the branch with origin/main rather than the PR base. The check then examines all changes accumulated on the release branch and can block a backport because of stale headers in unrelated earlier cherry-picks; pass the pull request's github.base_ref (or its corresponding remote ref) to the check.
AGENTS.md reference: AGENTS.md:L403-L410
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 79ff87112: in a pull request the base defaults to origin/$GITHUB_BASE_REF, so a backport into release/* is compared with its own branch. An explicit --base still wins (tested). A direct push outside a PR still compares with main.
| import subprocess | ||
| import sys | ||
|
|
||
| SOURCE = (".c", ".cc", ".cpp", ".h", ".hpp", ".java", ".kt", ".kts", ".py", ".sh") |
There was a problem hiding this comment.
When a PR adds a TypeScript source or a C++ .inc test source—both types already exist under .pi/extensions/ and ddprof-lib/src/test/cpp/—path.endswith(SOURCE) is false, so the checker returns before validating its header. This lets new source files bypass the rule the check is intended to enforce; include all source extensions used by the repository or classify source files by their tracked source directories.
AGENTS.md reference: AGENTS.md:L403-L410
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 79ff87112: .inc, .ts, .js and .groovy are checked now; .inc counts as C++ for the missing-line rule. Tested in T5.
Scan-Build Report
Bug Summary
Reports
|
|||||||||||||||||||||||||||||||||||||||||||||||||
![datadog-datadog-prod-us1-2[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
The copyright gate silently skips non-ASCII filenames under Git’s default quoting, along with TypeScript and extensionless scripts. These gaps allow missing or stale headers through CI.
🤖 Bits Code Review · Commit f499864
| out = git(root, "diff", "--name-status", "-M", "--diff-filter=AMR", "--no-relative", mb) | ||
| for line in out.splitlines(): | ||
| parts = line.split("\t") | ||
| yield parts[0] == "A", parts[-1] |
There was a problem hiding this comment.
With Git’s default core.quotePath setting, a filename such as café.java is returned quoted with escaped bytes. The parser retains that quoting, so path.endswith(SOURCE) rejects the filename and CI silently accepts missing or stale copyright headers. NUL-delimited parsing preserves the actual filenames, including rename destinations.
| out = git(root, "diff", "--name-status", "-M", "--diff-filter=AMR", "--no-relative", mb) | |
| for line in out.splitlines(): | |
| parts = line.split("\t") | |
| yield parts[0] == "A", parts[-1] | |
| out = git(root, "diff", "--name-status", "-z", "-M", "--diff-filter=AMR", "--no-relative", mb) | |
| fields = iter(out.split("\0")[:-1]) | |
| for status in fields: | |
| path = next(fields) | |
| if status.startswith("R"): | |
| path = next(fields) | |
| yield status == "A", path |
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
| if not path.endswith(SOURCE) or path.startswith(EXCLUDED): | ||
| return None | ||
| full = os.path.join(root, path) |
There was a problem hiding this comment.
The extension filter skips source types already present in the repository, including .pi/extensions/require-fresh-review.ts and the extensionless Python .githooks/pre-push. Adding these sources without headers, removing their existing Datadog headers, or leaving stale years passes the gate despite the documented policy. Include TypeScript and recognize extensionless interpreter scripts.
| if not path.endswith(SOURCE) or path.startswith(EXCLUDED): | |
| return None | |
| full = os.path.join(root, path) | |
| if path.startswith(EXCLUDED): | |
| return None | |
| full = os.path.join(root, path) | |
| if not path.endswith(SOURCE + (".ts",)): | |
| if os.path.splitext(path)[1]: | |
| return None | |
| with open(full, "rb") as f: | |
| if f.read(2) != b"#!": | |
| return None |
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
CI Test ResultsRun: #41910612046 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-11-04 21:14:46 UTC |
…6469) - check-copyright.py defaults to origin/$GITHUB_BASE_REF in a pull request, so a backport into release/* is compared with that branch, not with main. - It also checks .inc, .ts, .js and .groovy files and extensionless scripts that start with #! (git hooks, .claude/commands); .inc counts as C++ for the missing-line rule. - It parses `git diff -z`, so file names with non-ASCII bytes are no longer quoted, which made them skip the extension test. - check-claude-md.py ignores the block that the cairn plugin's SessionStart hook injects into CLAUDE.md; without that, every session with cairn left a CLAUDE.md that failed the check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- check-claude-md.py ignored any text between cairn markers, so a commit could put arbitrary content in CLAUDE.md and pass CI. It now ignores one cairn block only while it is uncommitted and HEAD's CLAUDE.md is canonical; a clean CI checkout always compares the exact file. - check-copyright.py reads only regular files with a code extension or no extension: no symlinks, directories or submodule entries, and no binary reads before the extension filter. File names that are not valid UTF-8 no longer abort it. - Tests pin --base over GITHUB_BASE_REF, `#` versus `#!`, two cairn blocks and a committed block. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…OF-16469) - check-claude-md.py reads HEAD:./CLAUDE.md, so a --root below the repository top compares against its own committed file, and a missing git binary or non-UTF-8 output no longer aborts the check. - README lists the new skips (symlinks, directories, submodule entries). - Tests: the symlink fixture points inside the fixture repo, the non-UTF-8 case names its file, a cairn block plus other drift fails, and --root below the top reads the right committed file. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- T6j: without git on PATH, an uncommitted cairn block is not ignored. - T6k: a committed non-UTF-8 CLAUDE.md gives a clean mismatch, not a decode traceback. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
What does this PR do?:
Puts a CI check behind three rules that
AGENTS.mdstates but nothing enforced, and runs theutils/test suites in CI.utils/check-copyright.py, part ofqualityChecks, applies theAGENTS.mdcopyright rule to the files a branch adds, modifies or renames. In a PR it compares withorigin/$GITHUB_BASE_REF, elsewhere withorigin/main. It reads regular files with a code extension (C/C++ incl..inc, Java, Kotlin, Groovy, Python, shell, TS/JS) and extensionless#!scripts. It skips symlinks, directories and submodule entries.utils/check-claude-md.py, also inqualityChecks, keeps it the canonical redirect stub, which replaces the bootstrap comment inAGENTS.md. The cairn plugin's SessionStart hook injects one managed block into the working copy. The check ignores that block only while it is uncommitted andHEAD:CLAUDE.mdis canonical. A clean CI checkout therefore always compares the exact file, so text cannot be smuggled in inside cairn markers.PR ticket referenceworkflow fails a PR whose title and body contain neitherPROF-<n>nor#<n>. Bot accounts are exempt.script-testsjob inci.ymlrunsutils/tests/*.shand the ticket-check test, with Node.js 22 for the Pi adapter cases.AGENTS.md:Motivation:
Step 1 of PROF-16469: advisory text cannot gate, so rules that a machine can check get a check.
AGENTS.mdalso claimed that CI validated ticket references, but no workflow, script or ruleset did.Additional Notes:
build-logicKotlin files, tests), the rest are Datadog end years that were not bumped. The check follows theAGENTS.mdrule as written. If that is too strict, relax the rule ("add a line to files that have none") rather than the check.[bot]login suffix, not by title.ci.ymldoes not run onedited, so fixing the description would not clear the failure.utils/tests/suites never ran in CI, and two had broken unnoticed. Both are fixed here.test_quality_check_scripts.shT1g pinned asigsetjmpcall count that the tree no longer has. It now pins only the exception count.test_review_marker.shfailed, instead of skipping, its Pi cases on Node.js older than 22.6.CLAUDE.mdmust delete it before pulling. Otherwise git refuses: "untracked working tree file would be overwritten".AGENTS.md: there is no link to the internal SDLC playbook, because the repository is public.check-copyright.pyneedsrefs/remotes/origin/main, or a--base. A localqualityChecksrun in a clone with a differently named remote fails with exit 2. CI fetches full history and is not affected.GITHUB_BASE_REF) compares withmain, also forrelease/*branches.#<n>right after a word character. They are left as is.How to test the change?:
bash utils/tests/test_quality_check_scripts.sh, now 84 groups. New:check-copyright.pyover a fixture branch with new, stale, ranged, renamed, deleted, excluded, upstream-only,Present, December-committed and uncommitted files, plus.ts/.inc, shebang scripts, non-ASCII and non-UTF-8 names, symlinks, a binary, a submodule entry, andGITHUB_BASE_REFversus--base;check-claude-md.py, including cairn blocks (uncommitted, two blocks, committed, plus drift),--rootbelow the repository top, nogit, and a non-UTF-8 committed file;.github/scripts/tests/test_check_pr_ticket.sh: 14 cases, including the template'sJIRA-XXXXplaceholder, HTML entities, anchors and bot authors.check-copyright.pymakes the suite fail../gradlew :ddprof-lib:qualityChecks(run throughbuild-and-summarize) passes and runs both new checks.node:22-bookworm, with the job's steps, passes all 8 suites.For Datadog employees:
credentials of any kind, I've requested a security review (run the
dd:platform-security-reviewskill, or file a request via the PSEC review form).
bewairealso runs automatically on every PR.🤖 Generated with Claude Code