fix(claude-ops): keep native drift report-only without a store and validate its inputs

[kyle-sexton]

No related issue: follow-up to late review findings on the merged #5465 (Codex P1 and P2) and #5467 (Claude correctness).

Summary

• P1 (feat(claude-ops): file native-surface drift from changelog apply #5465). /claude-ops:changelog apply Phase 7 treated every overlap.py self-check exit 3 as passing. With no overlap store, self-check exits 3 and declares the run report-only, but the phase still filed work items and promoted its summary to the baseline. That could happen in a foreign repository, or after the store went missing.
• P2 (feat(claude-ops): file native-surface drift from changelog apply #5465). native_drift.py accepted any syntactically valid JSON. A wrong-shaped input, such as a [] summary, crashed with a traceback and exit 1 instead of the documented exit-2 input error.
• feat(claude-ops): inventory built-in agents and tools as native-surface lanes #5467. parse_tools_table did not bound its scan. If the docs page reformatted the tools table, the parser absorbed unrelated backticked rows from later sections. It then reported every real tool as undocumented instead of reporting itself broken.

Fix

• Report-only mode: native_drift.py diff sets "report_only": true when --store is omitted or names no file, the same test overlap.py self-check uses. Items move to unfiled, nothing is filed, and the baseline is kept. context/native-drift.md branches on that field, not on the exit code.
• Input shape: load() checks each input kind's top-level and nested shape (summary, detect, store, inventory) and exits 2 with malformed <kind> input <path>: ….
• Tools table: parse_tools_table stops at the first heading after the table header, whether or not any row matched.
• claude-ops 0.70.1.

Verification

• native_drift.test.sh: 33 tests OK. New tests cover report-only filing nothing, a present store filing normally, and wrong-shaped inputs for every flag; they fail on the old code.
• test_inventory.py: 133 tests OK. The reviewer's reproduction now yields {} and a broken block.
• The live tools page still gives 45 documented, 35 undocumented and 1 docs_only, the same as before.
• changelog-status.test.sh: 70/70.
• Pinned ruff: clean. check-changed-skills.sh origin/main: 0 failed.
• check-spoke-plugin-root.sh, validate-plugin-contracts.mjs (0 warnings) and generate-catalog.mjs --check: all pass.
• check-changelog-parity.sh: all four modes pass. typos: clean.

Related

• feat(claude-ops): file native-surface drift from changelog apply #5465 and feat(claude-ops): inventory built-in agents and tools as native-surface lanes #5467: the Codex and Claude threads there cite this PR.

🤖 Generated with Claude Code

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-30T03:05:24.556583Z	3eeea28	Manual request

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

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]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: eed6e03450

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Reviewing PR #5525

• Gather context (PR diff, changed files, CLAUDE.md/AGENTS.md conventions)
• Understand request (invoke /review:code-review lane)
• Review native_drift.py changes (report-only gating, input validation)
• Review docs_crosscheck.py (parse_tools_table bound fix)
• Review test coverage changes
• Review context/native-drift.md and SKILL.md/CHANGELOG updates
• Post findings

View job run

[claude]

Claude finished @kyle-sexton's task in 2m 9s —— View job

---

Security review — no issues found

• Gather context (diff, changed files, custom instructions)
• Run /review:security-review skill
• Post findings

Reviewed the full diff (origin/main...HEAD, 8 files) against the CI security-review criteria: injection surfaces, trust-boundary handling, authorization gaps, and instruction-surface deletions. No security issues found.

What this PR touches, security-relevant angle by angle:

• Markdown/mention injection into filed work-item bodies (native_drift.py): clip() now collapses all whitespace (so an embedded newline can't forge a new line) and strips backticks; quote() wraps each fact in its own > `fact` line so it can't escape a code span, forge a Drift key: <key> marker, open a fence, or land a live @mention/#issue reference. This is exercised directly by an adversarial test, test_a_crafted_fact_cannot_forge_a_key_line_or_leave_its_span (test_native_drift.py:434), which round-trips a fact containing Drift key:, a closing fence, an @mention, and an issue reference and asserts none survive. This closes the gap the P2 finding on feat(claude-ops): file native-surface drift from changelog apply #5465 described.
• Fail-open defaults removed: diff()'s report_only parameter is now keyword-only with no default (native_drift.py:575-582), and a caller must derive it from store is None. A test (test_report_only_has_no_fail_open_default) asserts the old positional call now raises TypeError instead of silently defaulting to "file it." This is the right fix for the P1 finding: it makes the report-only decision a required, explicit fact rather than an implicit default a future caller could omit.
• Input hardening (load()/shape_error(), same file): malformed shape, deeply nested JSON (RecursionError from parsing), and oversized integer literals now exit 2 with a message instead of an uncaught traceback (exit 1) that could leak a Python stack trace to CI output. A (TypeError, AttributeError) backstop in main() catches any shape shape_error() doesn't check, so no code path can produce an unhandled traceback on attacker-shaped-but-foreign-repo input.
• docs_crosscheck.py's parse_tools_table now stops at the next heading even without a row match, so a reformatted (untrusted, fetched-over-HTTPS) docs page can't have its later, unrelated backticked rows absorbed into the tools table and reported as false "documented" entries. The existing _ROW_MAX length cap and no-backtracking row regexes (pre-existing, unchanged here) still bound the untrusted-fetch parsing.
• No instruction-surface deletions: the diff to SKILL.md and context/native-drift.md only adds/strengthens rules (explicit report-only branching, "never put a fact in a body except through quote"); nothing is removed or softened.
• No new network calls, shell/subprocess use, secrets handling, or authorization-relevant code in this diff.

No CRITICAL, IMPORTANT, or SUGGESTION findings to report.

[kyle-sexton]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3eeea28940

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Claude finished @kyle-sexton's task in 2m 39s —— View job

---

Claude finished the security review

• Gather context (PR diff, review comments, custom instructions)
• Run /review:security-review for PR fix(claude-ops): keep native drift report-only without a store and validate its inputs #5525 per repo custom instructions
• Post findings

Security review

Reviewed the diff (gh pr diff 5525) against origin/main, scoped to the 8 changed files. This
PR's own subject is hardening native_drift.py's input validation and the anti-injection
quoting for facts that flow into filed work-item bodies, so I gave that path close attention.

No security findings. Specifically:

• Fact-quoting (the core hardening in this PR). clip() (native_drift.py:354-359) collapses
  whitespace (no embedded newlines) and replaces every backtick, and quote()
  (native_drift.py:362-365) wraps each fact in its own backtick code span on a blockquote line.
  Together these prevent upstream-sourced "facts" (docs text, detect evidence, store rows) from
  forging a Drift key: <key> line, breaking out of the code span, opening a fence, or
  triggering a @mention/#issue link when the body is filed via /work-items:track. The new
  test test_a_crafted_fact_cannot_forge_a_key_line_or_leave_its_span exercises exactly this
  exploit path and passes.
  native_drift.py#L354-L365
• Input-shape validation. shape_error() (native_drift.py:98-181) is checked against every
  field the rest of the module reads without a type guard (diff_surfaces, evaluate_row,
  candidate_key, etc.), and a (TypeError, AttributeError) backstop
  (native_drift.py:789-792) converts anything the shape check misses into the documented exit 2
  instead of a traceback. I traced the fields read downstream (surface aliases/markers/
  description, detect candidates/native/component/evidence, store rows[].native) against
  what shape_error requires and didn't find a gap that would reach an unguarded .get()/in/
  string op on attacker-influenced data.
• The two Codex P2 comments on this PR are already resolved in the current head (commit
  4079303, "harden native_drift inputs and filed-body facts"), not outstanding:
  • "Reject detect reports with missing required fields": discovery and integrity are now
    required dicts for kind == "detect", not optional
    (native_drift.py#L148-L163).
  • "Validate nested summary records before diffing": _record_ok() now checks each surface
    record's aliases/markers/description shape before diff_surfaces does old in new_rec.get("aliases", [])
    (native_drift.py#L88-L95).
• Malformed-JSON hardening. load() (native_drift.py:184-200) catches ValueError and
  RecursionError in addition to OSError/JSONDecodeError, so a deeply nested [ payload or an
  oversized integer literal exits 2 rather than crashing the interpreter; covered by
  test_unparsable_json_is_a_usage_error.
• docs_crosscheck.py table-scan fix (plugins/claude-ops/skills/inventory/scripts/docs_crosscheck.py:128-130)
  is a correctness/availability fix (a reformatted docs page no longer causes unrelated later rows
  to be absorbed into the tools table), not a new trust-boundary change — fetch_text(url) and the
  row-length cap it feeds into were unchanged by this diff.

No command injection, path traversal, SSRF, or credential-handling issues in the changed files;
nothing here calls a shell or a network API with attacker-influenced input in a new way. GitHub
Actions workflow files are untouched by this PR.
· branch fix/native-drift-report-only

[claude]

Claude finished @kyle-sexton's task in 4m 1s —— View job

---

Reviewing PR #5525 (/review:code-review)

• Gather context (diff, changed files, CLAUDE.md/AGENTS.md conventions)
• Review native_drift.py (report-only gating, shape_error input validation)
• Review docs_crosscheck.py (parse_tools_table bound fix)
• Review test coverage (test_native_drift.py, test_inventory.py)
• Review docs/changelog updates (SKILL.md, context/native-drift.md, CHANGELOG.md)
• Post findings

No blocking findings

Reviewed the full diff (8 files) for correctness and CLAUDE.md/AGENTS.md alignment, scoped to code-review criteria (security is covered by the separate security lane, already posted above).

Both open Codex P2 threads on this PR are resolved in the current head (daeb326).

• Codex: "Reject detect reports with missing required fields" — confirmed fixed. shape_error's detect branch now requires schema == 1 and treats discovery/integrity/candidates as required (not optional via _opt), so {} exits 2 (malformed detect input) instead of silently becoming a detect block with an empty candidate list. test_wrong_shaped_inputs_are_usage_errors (test_native_drift.py:528-534) exercises this by dropping each required key from a valid detect payload and asserting exit 2.
• Codex: "Validate nested summary records before diffing" — confirmed fixed. The new _record_ok helper (native_drift.py:151-158) validates each surface record's aliases/markers as list[str] and description as str before diff_surfaces reads them, closing the old in new_rec.get("aliases") TypeError path the finding described. test_wrong_shaped_inputs_are_usage_errors's surface(aliases=5) / surface(description=7) / surface(markers="hidden") cases (test_native_drift.py:505-522) cover exactly this.

I traced every field diff(), diff_surfaces(), evaluate_row(), summarize(), and component_id() read from loaded JSON back through shape_error()/_record_ok()/_opt()/_strs() to confirm no remaining unguarded read; the (TypeError, AttributeError) backstop in main() (native_drift.py:789-792) covers what's left. The report_only keyword-only-with-no-default change correctly removes the fail-open path from #5465's P1, and parse_tools_table's new if line.startswith("#"): break (docs_crosscheck.py:131-132) closes the unbounded-scan gap from #5467, both backed by targeted regression tests that fail on the pre-fix code per the PR's verification section.

Nothing else in this diff (plugin.json version bump, CHANGELOG, SKILL.md/context doc prose) is inconsistent with the code changes.

Note: I could not execute the test suite or ruff in this sandbox (git/network and arbitrary shell commands required approval that wasn't available), so the verification above is static-analysis-based; the PR's own test-run claims are author-reported and unverified by me.

[claude]

Claude finished @kyle-sexton's task in 1m 55s —— View job

---

Security review — no issues found

• Gather context (diff, changed files, custom instructions)
• Run /review:security-review skill
• Post findings

Note on scope: the dispatched HEAD SHA (daeb326c) predates the branch's actual current tip. The checkout here is at 86156a3 (merge of origin/main into this branch, including 8329bac "require candidate identity leaves in a detect report"), which is what the PR now shows, so I reviewed the diff up through that commit and link to it below.

Claude's prior pass on this PR (commit eed6e034) found no issues. Since then, two more commits landed on this branch (4079303 / 8329bac) that implement exactly the fixes Codex's two P2 findings asked for. I re-reviewed native_drift.py end to end against both the original criteria and those two specific gaps.

Codex P2 — "Reject detect reports with missing required fields": closed. shape_error's detect branch now requires schema == 1 and that every candidate's native.name, component.plugin, and component.skill are present strings (not merely optional). A {} detect report, or one missing candidates[].native.name, now fails shape validation and exits 2 via InputError instead of silently becoming a detect block a later summarize could promote to baseline with None:None:None-keyed candidates.

Codex P2 — "Validate nested summary records before diffing": closed. _record_ok now requires each surface record's aliases/markers to be lists of strings and description to be a string before diff_surfaces runs. That's precisely what the reported exploit path (aliases: 7 → old in new_rec.get("aliases", []) → TypeError: argument of type 'int' is not iterable) needed: aliases can no longer reach that line as anything but a list. As a backstop, main() now also catches (TypeError, AttributeError) around the loaded-input processing and exits 2, so any shape shape_error doesn't explicitly enumerate still can't produce an uncaught traceback (exit 1) on foreign/malformed input.

Re-checked the rest of the security surface for regressions from these two commits:

• load()'s exception set widened to (OSError, ValueError, RecursionError) (native_drift.py:198) — ValueError is a superset covering JSONDecodeError/UnicodeDecodeError, and RecursionError now catches a deeply-nested-JSON DoS-shaped input at exit 2 instead of a crash. No new exception is silently swallowed elsewhere.
• The new registrations(e) call inside shape_error's inventory branch is safe against arbitrary data shapes: registrations wraps non-list input in a list and filters to isinstance(_, dict), so it can't raise regardless of what an attacker puts under builtin_commands/bundled_skills/bundled_workflows.
• quote()/clip() (unchanged since the prior review, still exercised by test_a_crafted_fact_cannot_forge_a_key_line_or_leave_its_span) continue to neutralize the markdown/mention-injection path into filed work-item bodies; nothing in this increment touches that code path.
• context/native-drift.md and SKILL.md changes in this range only add instructions (naming self-check exit 1 as another report-only trigger, spelling out that a fact may only enter a body through quote) — no instruction-surface deletion or softening.

No CRITICAL, IMPORTANT, or SUGGESTION findings to report.

[claude]

Claude finished @kyle-sexton's task in 2m 14s —— View job

---

Claude finished the security review

• Gather context (PR diff, review comments, custom instructions)
• Run /review:security-review for PR fix(claude-ops): keep native drift report-only without a store and validate its inputs #5525 per repo custom instructions
• Post findings

Security review

Scope: gh pr diff 5525 against the 8 changed files. This lane already reviewed this branch at
commit 3eeea28 (clean) and daeb326 (code-review, clean). The only delta since then is commit
8329bac plus a docs-only trailing-space fix, so I focused on that delta and re-checked the rest for anything the earlier passes might have missed given the fuller context now available.

No security findings.

• The Codex P2 this commit addresses is fixed correctly. Before 8329bac, shape_error's
  detect branch accepted {"native": {}, "component": {}} because _opt treats a missing
  object as fine; candidate_key then built the dedupe key as native-drift:candidate:None:None:None.
  If that summary became the baseline, a real candidate's key would never match it and could be
  filed as spuriously "new." The fix at
  native_drift.py#L159-L167
  now requires isinstance(c["native"].get("name"), str), isinstance(c["component"].get("plugin"), str),
  and isinstance(c["component"].get("skill"), str); each access is guarded by a preceding
  isinstance(..., dict) in the same and chain, so there's no KeyError/TypeError path for a
  candidate missing native or component entirely — _objects(candidates) already required each
  candidate to be a dict, and the short-circuit means c["native"] is only read once
  c.get("native") is confirmed to be a dict. A real overlap.py detect report still validates.
  New tests (test_native_drift.py) cover an empty-identity candidate and one missing
  component.skill.
• Nothing else changed in the reviewable surface since the last pass. git diff 3eeea28..HEAD -- native_drift.py docs_crosscheck.py shows only the hunk above; daeb326 (already code-reviewed) is a doc trailing-space fix only.
• Re-confirmed from the earlier passes, still holding at HEAD: the fact-quoting in clip()/quote() (native_drift.py#L359-L370) still prevents an upstream-sourced fact from forging a Drift key: line, escaping its code span, opening a fence, or triggering a mention/issue link in a filed work-item body; the shape_error/_record_ok validation plus the (TypeError, AttributeError) backstop in main() (native_drift.py#L794-L797) still closes every unguarded-read path I traced (diff_surfaces, evaluate_row, candidate_key, component_id); and docs_crosscheck.py's parse_tools_table bound fix (stopping at the next heading, docs_crosscheck.py#L131-L132) closes the unbounded-scan gap without introducing a new trust boundary — fetch_text's URLs (COMMANDS_URL, TOOLS_URL, CHANGELOG_URL) remain hardcoded constants, not attacker-influenced.
• Instruction-surface check: context/native-drift.md and SKILL.md only add rules (explicit report-only branching on "report_only" rather than exit code, the existing "never put a fact in a body except through quote" rule carried forward unchanged); nothing is deleted, narrowed, or softened, so the instruction-exception-register lens finds nothing to flag.
• No command injection, path traversal, SSRF, or credential handling in the changed files; no GitHub Actions workflow files are touched by this PR.
  · branch fix/native-drift-report-only

[claude]

Claude finished @kyle-sexton's task in 4m 18s —— View job

---

Reviewing PR #5525 (/review:code-review)

• Gather context (PR diff, changed files, CLAUDE.md/AGENTS.md conventions)
• Invoke /review:code-review skill and follow it exactly
• Post findings

No blocking findings

Reviewed the full diff (gh pr diff 5525, 8 files, current head 86156a3) for correctness and CLAUDE.md/AGENTS.md alignment, per the code-review lane's criteria (security is out of scope for this lane; it's already covered by the separate security-review lane above).

All three Codex P2 threads on this PR are resolved in the current head. I independently traced every field diff(), summarize(), diff_surfaces(), evaluate_row(), inventory_verdict(), and candidate_key()/component_id() read from loaded JSON back through shape_error() / _record_ok() / _opt() / _strs() / _objects() in native_drift.py, to confirm no unguarded read remains:

• Detect candidate identity (native_drift.py#L156-L168): each detect candidate must now carry native.name, component.plugin, and component.skill as strings, so {"native": {}, "component": {}} exits 2 instead of keying the baseline native-drift:candidate:None:None:None. This was the latest Codex finding (commit 8329bac); summarize()'s candidate_key(c) call and diff()'s candidate-fileability loop both consume this same validated detect input, so the fix covers both call sites.
• Inventory aliases type guard (native_drift.py#L115-L122): registrations(e) results are checked for _strs(r.get("aliases")) before surface_record()'s aliases = sorted({a for r in regs for a in (r.get("aliases") or []) ...}) would otherwise raise TypeError: 'int' is not iterable on a truthy non-list value.
• Summary surface records (_record_ok, native_drift.py#L88-L95): aliases/markers must be list[str], description must be str, closing the old in new_rec.get("aliases") TypeError path diff_surfaces would otherwise hit.
• The (TypeError, AttributeError) backstop in main() (native_drift.py#L794-L797) exits 2 for anything the shape check still misses, so no path produces an unhandled traceback.

Report-only / fail-open fix: diff()'s report_only parameter is keyword-only with no default (native_drift.py#L576-L585), derived in main() from store is None (L781-L790); a missing-store run now routes items to unfiled instead of items (L692-L696). test_report_only_has_no_fail_open_default correctly asserts the old positional-call shape now raises TypeError.

docs_crosscheck.py's parse_tools_table (docs_crosscheck.py#L131-L132): the new if line.startswith("#"): break correctly stops the scan at the next heading even when zero rows matched yet — I traced the old control flow and confirmed it would otherwise continue past a heading like ## Configure tools (since rows was still empty) and later match an unrelated backticked row in a following section. The new test_unmatched_table_rows_never_reach_a_later_section reproduces exactly this and fails without the fix.

main()'s reordered write() call (native_drift.py#L780-L799): write(payload, args.out) now runs after the try/except block instead of inside it. This is correct — it stops write()'s own exceptions from being mis-swallowed as an "input" error — and doesn't change behavior for has-key, which returns early inside the try.

Nothing else in this diff (plugin.json version bump, CHANGELOG.md, SKILL.md/context/native-drift.md prose) is inconsistent with the code changes; the docs accurately describe report_only, unfiled, and quote as implemented.

Verification note: this sandbox requires approval for python3 -m unittest / gh network calls that wasn't available to me, so I could not independently execute native_drift.test.sh or test_inventory.py. The review above is static/data-flow analysis tracing every read against the new validation, not a test run. The PR's own verification section (33/133/70 tests OK) is author-reported and unverified by me.
· branch fix/native-drift-report-only