Skip to content

Galileo to splunk regex migration tool - #230

Open
ridhima-splunk wants to merge 2 commits into
mainfrom
regex-splunk-migration-tool
Open

Galileo to splunk regex migration tool#230
ridhima-splunk wants to merge 2 commits into
mainfrom
regex-splunk-migration-tool

Conversation

@ridhima-splunk

@ridhima-splunk ridhima-splunk commented Aug 21, 2026

Copy link
Copy Markdown

@github-actions

Copy link
Copy Markdown
Contributor


Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.


I have read the CLA Document and I hereby sign the CLA


You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot.

@fercor-cisco

Copy link
Copy Markdown
Collaborator

Generated by Claude Code

Findings were verified by cross-checking ~40 rename targets against the actual SDK, cross-checking every rule against the spec in splunk-ao-migration-tool/README.md, and empirically running the tool against adversarial fixtures in a scratch directory. Everything marked "Reproduced" below was observed, not inferred.

Issues Found

  • [Critical]: Running the tool on a single file renames unrelated files across the whole
    parent directory. collect_path_renames takes the raw CLI paths and, for a file argument,
    sets base = p.parent and walks it recursively. Worse, for a nonexistent path
    (p.is_file() and p.is_dir() both false) base is still p.parent, so a typo'd argument
    walks and renames from the cwd — collect_paths warns and skips the bad path, but
    collect_path_renames does not.

    • File: splunk-ao-migration-tool/splunk_ao_migrate/migrate.py (lines 213-250, invoked at 340)
    • Reproduced: splunk-ao-migrate main.py renamed unrelated/galileo_helper.py
      splunk_ao_helper.py and unrelated/keep-galileo-notes.mdkeep-splunk-ao-notes.md,
      while the report said Files scanned: 1. The renamed files' contents were never
      migrated
      , so renames and content rewrites are silently inconsistent.
    • Suggestion: restrict the rename scan to the same path set that collect_paths returns
      (plus explicitly-passed directories), i.e. only ever rename entries the tool actually
      migrated. Never derive a walk root from a file's parent, and skip nonexistent paths.
  • [Critical]: Protect users have their required galileo dependency deleted and their
    Protect imports broken — the exact opposite of what the spec mandates. Spec §1.4 and §5.1
    say Protect users must keep galileo installed and must keep importing
    invoke_protect from galileo.

    • File: splunk-ao-migration-tool/splunk_ao_migrate/rules.py (line 463 DEP_RULES bare-galileo
      rule; line 59 IMPORT_RULES generic rule), migrate.py (lines 120-124)
    • Reproduced: galileo>=2.3.0splunk-ao>=2.3.0 (also producing a duplicate
      splunk-ao requirement when splunk-ao was already listed), and
      from galileo import invoke_protectfrom splunk_ao import invoke_protect, a guaranteed
      ImportError. The Protect warning fires for the .py file but not for
      requirements.txtmigrate_file passes no warning_rules on the dep/toml branch.
    • Suggestion: (a) add a Protect guard to IMPORT_RULES — do not rewrite from galileo import
      lines whose imported names are Protect symbols; (b) pass WARNING_RULES on the dep/toml
      branch so the dependency file is flagged too; (c) at minimum, if any scanned file matches
      the Protect pattern, refuse to rewrite the bare galileo dependency and print a loud
      warning. A cross-file pre-scan for Protect usage before touching dep files is the clean fix.
  • [Major]: Any X-Galileo-* header outside the 3-entry HEADER_RULES allowlist gets a
    space injected, silently producing an invalid header name.

    • File: splunk-ao-migration-tool/splunk_ao_migrate/rules.py (lines 221-227, 327)
    • Reproduced: {"X-Galileo-SDK": v, "X-Galileo-Trace-ID": t, "X-Galileo-Custom": c}
      {"X-Splunk AO-SDK": v, "Splunk-AO-Trace-ID": t, "X-Splunk AO-Custom": c}. The enumerated
      header is correct; the others are corrupted.
    • X-Galileo-SDK specifically matters: the SDK really does use Splunk-AO-SDK
      (src/splunk_ao/experiments.py:684), so a needed rule is missing and its absence
      actively corrupts.
    • Suggestion: add X-Galileo-SDK → Splunk-AO-SDK, and make the brand rule refuse to fire
      when Galileo sits between hyphens ((?<!-)\bGalileo\b(?!-)) so unknown hyphenated
      header/identifier forms are left intact for manual review rather than silently broken.
  • [Major]: The tool renames galileo_core types that its own warning text says must not be
    renamed, producing a guaranteed ImportError.

    • File: splunk-ao-migration-tool/splunk_ao_migrate/rules.py (lines 113-114 vs 347-355)
    • WARNING_RULES says: "Review any galileo_core types (e.g. Metrics) used in your code; they
      are internal types and should not be renamed"
      — while SYMBOL_RULES unconditionally applies
      \bMetrics\b → Evaluators and \bMetric\b → Evaluator.
    • Reproduced: from galileo_core.schemas.metrics import Metricsimport Evaluators;
      one: Metricone: Evaluator. (The module path itself is correctly preserved.)
    • \bMetric\b/\bMetrics\b are also broad enough to rename any unrelated user or
      third-party Metric/Metrics symbol.
    • Suggestion: suppress the bare Metric/Metrics rules on any line containing
      galileo_core, or drop them from the auto-applied set and demote them to warnings. The
      package README (lines 179-184) discloses this as a limitation but claims "typical user
      application code is not affected" — I'd push back: Metrics from galileo_core is a
      documented type, and spec §8 explicitly promises galileo_core imports need no migration.
  • [Major]: Bare Galileo / GALILEO in Python code becomes a two-word string, producing a
    SyntaxError.

    • File: splunk-ao-migration-tool/splunk_ao_migrate/rules.py (lines 320-331, applied to .py
      via PYTHON_RULES at 382-389)
    • Reproduced: GALILEO = 1SPLUNK AO = 1; class Galileo:class Splunk AO:;
      python3 -m py_compileSyntaxError: invalid syntax.
    • Suggestion: brand rules insert whitespace and therefore are only ever safe in prose. Apply
      them to comment/docstring text and doc files only, or gate them on the match not being in a
      code position. A cheap 90% improvement: after rewriting a .py file, compile() the result
      and refuse the write (with a warning) if it no longer parses. That single guard would have
      caught this and several other findings.
  • [Major]: Every rewritten file loses its permissions and its line endings.

    • File: splunk-ao-migration-tool/splunk_ao_migrate/migrate.py (lines 137-150)
    • Reproduced: -rwxr-xr-x (755) → -rw------- (600) — mkstemp creates 0600 and
      os.replace keeps it, so the executable bit and all group/other access are dropped from
      every file the tool touches. Separately, CRLF input became LF output on macOS
      (read_text translates on read, os.fdopen(..., "w") with newline=None translates on
      write); on Windows the inverse rewrites every line of an LF file. Root CI spans Windows.
    • Suggestion: shutil.copystat(path, tmp) (or os.chmod from a prior os.stat) before
      os.replace; read with newline="" and write with newline="" to round-trip line
      endings byte-for-byte.
  • [Major]: The tool is not idempotent — re-running it on already-migrated docs corrupts
    them, and install instructions are mangled on the first pass.

    • File: splunk-ao-migration-tool/splunk_ao_migrate/rules.py (lines 289-311
      DOC_PLACEHOLDER_RULES)
    • Reproduced on a correct, already-migrated .md: `pip install splunk_ao`
      `pip install Splunk AO`; logging.getLogger("splunk_ao")getLogger("Splunk AO").
      The backtick lookbehind only guards a backtick immediately before the token, so it does
      not protect tokens inside a longer backtick span.
    • Reproduced on a first pass: pip install galileopip install Splunk AO;
      pip install galileo[langchain]pip install Splunk AO[langchain]; uv add galileo
      uv add Splunk AO. Correct output is splunk-ao.
    • Suggestion: make the doc pipeline code-fence aware — skip fenced blocks and inline-code
      spans in the prose/placeholder passes entirely. Additionally special-case
      pip install/uv add/poetry add operands to splunk-ao. Idempotency deserves an
      explicit test: migrate twice, assert the second run is a no-op.
  • [Major]: ~/.galileo/ is rewritten to ~/.splunk_ao/, and the package README documents a
    ~/.splunk/ directory that does not exist.

    • File: splunk-ao-migration-tool/splunk_ao_migrate/rules.py (line 59);
      splunk_ao_migrate/README.md (lines 168-170)
    • Spec §5.3 states ~/.galileo/ is inherited from galileo-core and unchanged. Confirmed
      in the SDK: SplunkAOConfig(GalileoConfig) overrides only
      config_filename = "splunk-ao-config.json" (src/splunk_ao/config.py:70-74); the directory
      comes from galileo_core.
    • Reproduced: Path.home() / ".galileo"Path.home() / ".splunk_ao";
      "~/.galileo/splunk-ao-config.json""~/.splunk_ao/splunk-ao-config.json".
    • The README's "the local config directory has moved from ~/.galileo/ to ~/.splunk/" is
      fabricated and instructs users to move files somewhere the SDK never reads.
    • Suggestion: exclude .galileo from the generic rule (\bgalileo\b(?<!\.galileo) or an
      explicit skip for .galileo/), and delete/correct the README's "Manual steps" section.
  • [Major]: The GALILEO_OBSERVE_KEY guidance is factually wrong, and a required rule is
    missing — causing a silent A2A interop break.

    • File: splunk-ao-migration-tool/splunk_ao_migrate/rules.py (lines 141-145, 356-362);
      splunk_ao_migrate/README.md (lines 90, 158-161)
    • The code comment, the warning text, and the README all assert the wire value
      "galileo_observe" "must stay unchanged for wire compatibility". It was not kept:
      splunk-ao-a2a/src/splunk_ao_a2a/_constants.py:10 is
      SPLUNK_AO_OBSERVE_KEY = "splunk_ao_observe", used as the A2A request-metadata key on both
      send (_client_patches.py:110) and receive (_context.py:56).
    • Consequence: a migrated A2A app keeps "galileo_observe" in its metadata while the new SDK
      reads "splunk_ao_observe". Context propagation silently stops working — no error.
    • Suggestion: add a rule rewriting the string value "galileo_observe""splunk_ao_observe",
      delete the contradictory warning (the constant rename is already correct and unconditional,
      so warning "do not rename" about a token the tool always renames is pure confusion), and fix
      both README passages. This gap should also be added to the spec README §6/§8.
  • [Major]: Zero tests, zero CI coverage, and the package cannot be built or installed as
    documented.

    • Files: whole PR; splunk-ao-migration-tool/splunk_ao_migrate/pyproject.toml (lines 21-23)
    • pyproject.toml sits inside the package directory, so its project root is
      splunk_ao_migrate/ — but packages = ["splunk_ao_migrate"] then points at
      splunk_ao_migrate/splunk_ao_migrate/, which does not exist. The documented
      pip install ./splunk_ao_migrate (README line 23) cannot find the package. The python -m
      and direct-script invocations do work.
    • No test file is added for ~130 ordered regex rules whose correctness is entirely
      order-dependent. AGENTS.md requires adding the closest focused regression test first.
      Every Critical/Major finding above is a case a table-driven test would have caught.
    • No workflow references the tool, so nothing runs in CI. pre-commit would have flagged
      at least the unused import below (ruff runs repo-wide; only CHANGELOG.md and
      src/splunk_ao/resources/ are excluded), which suggests hooks were not run.
    • Suggestion: move pyproject.toml up to splunk-ao-migration-tool/ with
      packages = ["splunk_ao_migrate"], or keep it in place and drop the packages key. Add a
      table-driven tests/ (input → expected output per rule group, plus an idempotency test and
      a "migrated Python still compiles" test) and a CI job. Given the blast radius of an
      in-place, file-renaming codemod, I don't think this should merge without tests.
  • [Minor]: AGENTS.md and ARCHITECTURE.md still say the migration tool is not a buildable
    package. AGENTS.md:98 ("Migration documentation and examples, not a buildable package"),
    AGENTS.md:101 ("The three buildable packages…") and ARCHITECTURE.md:17 all become stale
    the moment this pyproject.toml with a console script lands. AGENTS.md "Change Workflow"
    item 4 requires updating both in the same change.

    • Suggestion: update both files, or (better) resolve the packaging question first — if the
      tool is meant to be python -m-only, drop the pyproject.toml and the docs stay true.
  • [Minor]: Four dead rules and several unused module-level constants. --dry-run output is
    also misleading.

    • rules.py:139-140 — both config-file rules are unreachable: the generic import rule
      (line 59) fires first, turning galileo-python-config.json into
      splunk_ao-python-config.json, so the documented target splunk-ao-config.json is never
      produced
      (confirmed empirically). This is a real output bug, not just dead code.
    • rules.py:172-176\blog_stream\s*= can never match; SYMBOL_RULES line 128 already
      rewrote log_streamagent_stream. (Output is still correct. log_stream_name= genuinely
      does need its own rule.)
    • rules.py:417 DOC_RULES is never imported. rules.py:25 Rule.is_warning is never read.
    • migrate.py:52-55 _DEP_NAMES, _DEP_GLOB, _ENV_PREFIXES are all unused.
    • rules.py:17 imports field, which is unused (ruff F401, and F is selected).
    • _print_diff prints one line per rule firing, so a single source line appears several
      times with contradictory intermediate content (from galileo import galileo_context
      from splunk_ao import galileo_context… splunk_ao_context), and Substitutions: N
      counts rule firings rather than changed lines. For doc files the count is inflated further
      because three passes' matches are concatenated.
    • Suggestion: fix the config-file rules by anchoring them before the generic rule (move into
      IMPORT_RULES ahead of line 59) or by matching the post-rewrite form; delete the dead
      rules/constants/import; and aggregate matches per line so the diff shows one
      before/after pair.
  • [Minor]: Env-var rules include three mappings with no target in the SDK, and one that is
    likely harmful. SPLUNK_AO_HOME_DIR and SPLUNK_AO_INGEST_BETA_DISABLED appear nowhere in
    src/; SPLUNK_AO_API_ENDPOINT exists only in examples/. GALILEO_HOME_DIR in particular
    is a galileo-core setting and is absent from the spec §4 table, so renaming it silently
    drops the user's setting (nothing reads the new name).

    • File: splunk-ao-migration-tool/splunk_ao_migrate/rules.py (lines 197, 207, 215)
    • Suggestion: drop GALILEO_HOME_DIR (or demote to a warning), and reconcile the rule list
      against the spec §4 table exactly — every row present, no extra rows.
  • [Minor]: Python-floor bump is half-applied and TOML output style is inconsistent.

    • File: splunk-ao-migration-tool/splunk_ao_migrate/rules.py (lines 445-473)
    • Reproduced: requires-python = ">=3.10"">=3.11" ✓, but Poetry's
      python = "^3.10" is left untouched, so a Poetry project is not actually moved to the 3.11
      floor the spec requires. Output is also inconsistent: splunk-ao = "^2.3.0" (unquoted) vs
      "splunk-ao" = { git = … } (quoted).
    • The comment at lines 446-448 ("hyphen makes splunk-ao an invalid bare TOML key, so it must
      be quoted"
      ) is factually wrong — TOML bare keys permit dashes, and both forms parse under
      tomllib. So the quoting is unnecessary and the two code paths disagree for no reason.
    • Suggestion: add a rule for Poetry's python = "^3.x" / >=3.x floor; drop the added quotes
      and the incorrect comment.
    • Also: requires-python = ">=3.10.1" becomes ">=3.11.1", since the pattern replaces the
      matched prefix and leaves the trailing .1. Worth anchoring the patch component.
  • [Minor]: _classify returns skip for .yaml/.yml/.json/.ipynb/Dockerfile/shell
    scripts, but spec checklist line 457 requires updating "CI/CD secrets and deployment configs".
    Skips are at least reported, so this is visible rather than silent.

    • File: splunk-ao-migration-tool/splunk_ao_migrate/migrate.py (lines 59-72)
    • Suggestion: either extend coverage (env-var rules alone would handle most YAML/JSON safely)
      or state the gap explicitly in the README's Limitations section and in the final report's
      "Next steps".
  • [Minor]: A venv directory named env/ is walked and rewritten. _SKIP_DIRS covers
    .venv/venv but not env, and site-packages is not excluded by name.

    • File: splunk-ao-migration-tool/splunk_ao_migrate/migrate.py (line 157)
    • Suggestion: add env, .env.d, site-packages, .mypy_cache, .pytest_cache, .ruff_cache,
      .eggs, *.egg-info. Better still, respect .gitignore or refuse to run outside a git
      working tree without --force.
  • [Minor]: A file is rewritten whenever any rule fires, not when content actually changes,
    and warnings for doc files are computed against post-URL-pass content so reported line numbers
    can drift from the original file.

    • File: splunk-ao-migration-tool/splunk_ao_migrate/migrate.py (lines 107, 131)
    • Suggestion: gate the write on tr_content != content; run the warning pass against the
      original content for docs, as the Python branch effectively does.
  • [Nit]: Nine lines in rules.py exceed the 120-char limit in AGENTS.md (lines 72, 78,
    105, 133, 197-201). ruff format via pre-commit would fix these; E501 is not in the selected
    lint set, so CI would not fail on them today.

  • [Nit]: main() always returns 0, even when warnings requiring manual review were emitted.
    A --check-style non-zero exit would make the tool usable in CI.

    • File: splunk-ao-migration-tool/splunk_ao_migrate/migrate.py (line 346)
  • [Nit]: Broken doc references. migrate.py:18 points at splunk-ao-migration-tool/PROPOSAL.md,
    which does not exist and is printed in --help. reporter.py:75,82 sends users to
    splunk-ao-migration-tool/splunk_ao_migrate/README.md (the tool's own README) instead of the
    actual migration guide at splunk-ao-migration-tool/README.md. The PR body links
    MIGRATION_TOOLS_GUIDE.md, which is not on this branch.

  • [Nit]: reporter.py:50 pads column alignment with
    {' ' * (4 if dry_run else 9)}. An f-string width spec (f"{label:<18}{len(changed)}") is
    clearer and won't drift if the labels change.

Questions

  1. The spec README contradicts the tool in two places, and the tool is right. Should this PR
    also fix the README, since the PR description says the rules are based on it?
    • §2.2 says from galileo.metric import …from splunk_ao.metric import …. There is no
      src/splunk_ao/metric.py; the module is evaluator.py. The tool's
      galileo.metric → splunk_ao.evaluator is correct.
    • §3.3 and checklist line 452 say GalileoMetricSplunkAOMetric. SplunkAOMetric does
      not exist anywhere in the SDK.
      The tool's GalileoMetric → SplunkAOEvaluator is correct,
      and its extra SplunkAOMetric → SplunkAOEvaluator rule usefully repairs code written
      against the wrong doc. Leaving §3.3 as-is will actively mislead customers.
  2. Is in-place rewriting plus on-disk renaming the right default? Renaming directories is
    the highest-blast-radius thing here and the least reversible. Would --rename-paths as an
    opt-in flag (default off) be safer, given finding 1?
  3. Should the tool refuse to run on a dirty git tree (or outside git) without --force?
    The report tells users to git diff afterwards, which only works if they had a clean tree.
  4. What is the intended distribution model? If it is python -m only, dropping
    pyproject.toml avoids the broken-build finding and keeps AGENTS.md/ARCHITECTURE.md
    accurate. If it is meant to be pip install-able, it needs a working layout, a CI job, and a
    release story like the other three packages.
  5. splunk-ao-migration-tool/examples/after_splunk_ao.py appears stale relative to the tool
    (it still says project/log_stream are constructor args where the tool emits agent_stream).
    Not part of this diff — worth refreshing, and ideally asserting in a test that
    before_galileo.py migrates to exactly after_splunk_ao.py.

@fercor-cisco fercor-cisco left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ridhima-splunk I didn't have time for a full review but I ran it through Claude Code and I posted its findings. Can you please triage the comments to see which ones are relevant?

Some comments like "the tool is not idempotent" may be just limitations that we need to document.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants