Skip to content

Chore: remove stray output.txt from the repo root - #271

Open
AmaadMartin wants to merge 1 commit into
mainfrom
fix/remove-stray-output-txt
Open

Chore: remove stray output.txt from the repo root#271
AmaadMartin wants to merge 1 commit into
mainfrom
fix/remove-stray-output-txt

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Jul 29, 2026

Copy link
Copy Markdown
Owner

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

  1. Link to an existing issue (if applicable):
    No existing issue.
  2. Or, if no issue exists, describe the change:
    Problem: A 5-byte file named output.txt (contents: the literal text hello, no trailing newline) is tracked at the repository root. It is scratch output that was committed by accident in 8d5cc0ac ("feat: skills: support script execution", feat: skills: support script execution google/adk-js#276) and has never been touched since — git log -- output.txt shows exactly one commit. It plays no role in the build, the test suite, packaging, documentation, or CI, yet every fresh clone of the SDK ships it, and anyone exercising the skill script-execution tools from the repo root gets confusing git status noise against it.

Background on how it got committed (context only — no behavioural fix is in scope): materializeFiles in core/src/utils/file_utils.ts writes agent-produced output files into process.cwd() by default. Manually running the new script-execution tools from the repo root during development of google#276 materialized the executor's sample output file into the working tree, where it was swept into the commit. That is the documented, intended behaviour of materializeFiles — it is how a skill script's output reaches the user's working directory — so it is deliberately not changed here.

Solution: git rm output.txt. That is the entire diff: 1 file changed, 1 deletion(-). Deletion alone is the correct and complete fix — deliberately not done here:

  • No .gitignore entry. output.txt is a generic name that a legitimate, deliberately materialized skill-script output could use; blanket-ignoring it at the repo root would silently hide such a file from a future contributor. The safety audit below shows nothing writes it, so there is nothing to ignore.
  • No change to materializeFiles or its dir = process.cwd() default — that would be a behavioural change to the skills feature, with its own compatibility analysis.
  • No git history rewrite. The blob stays in history at 8d5cc0ac; a normal forward deletion is what is wanted.
  • No test asserting the file's absence. Such a test would be filesystem- and cwd-dependent, is not a behavioural assertion about the SDK, and would break the one-file diff.

Safety audit — does anything depend on the path? grep -rn "output\.txt" . --exclude-dir=.git returns exactly four matches in three files, all verified to be unrelated to the tracked repo-root file, and all left untouched by this PR:

  1. core/test/tools/skills/run_skill_script_tool_test.ts:210 — an in-memory File object literal used as a fake executor result; ../../../src/utils/file_utils.js is vi.mocked at the top of the file, so the real writer never runs and nothing hits disk.
  2. core/test/tools/skills/run_skill_inline_script_tool_test.ts:226 — same pattern, same module-level vi.mock.
  3. core/test/code_executors/unsafe_local_code_executor_test.ts:269,286 — the executed snippet writes new_output.txt (a different filename), and UnsafeLocalCodeExecutor materializes into a mkdtemp temp directory, never the repo root.
  4. core/test/utils/file_utils_test.ts — the only caller of the real materializeFiles; every call passes an explicit mkdtemp directory that is removed in afterEach, and it never uses the name output.txt.

No source file, script, config, workflow, packaging manifest, or doc reads or writes the repo-root output.txt. scripts/check_license.sh only scans *.js/*.ts; vitest.config.ts coverage include is limited to */src/**/*.ts, so the absolute coverage thresholds are mathematically unaffected; .prettierignore and the lint-staged globs do not match .txt; npx secretlint "**/*" simply has one fewer file to scan and .secretlintrc.json needs no change.

Collision check: all 534 open PRs on the staging fork were re-listed (gh pr list --limit 1000) and every plausibly adjacent PR's file list inspected (gh pr diff <n> --name-only).

Seven PRs now also delete output.txt. All seven were opened after this one (#271, 2026-07-29T21:12:36Z), which is the earliest and the only one whose diff is nothing but the deletion:

PR Opened Diff Relationship
#429 fix/remove-stray-root-output-txt 2026-08-01 1 file, +0/−1 Byte-identical to this PR (verified via gh pr diff) — a pure duplicate. It previously also added a .gitignore block; that has since been dropped.
#473 fix/delete-root-output-txt-artifact 2026-08-01 1 file, +0/−1 Byte-identical to this PR — a pure duplicate.
#539 fix/drop-stray-root-output-txt-file 2026-08-02 1 file, +0/−1 Byte-identical to this PR — a pure duplicate.
#438 fix/remove-stray-root-output-txt-artifact 2026-08-01 3 files, +41/−23 This deletion plus rework of the two tests/integration/tools/ skill-script suites.
#353 fix/skill-script-explicit-output-dir 2026-07-31 14 files, +402/−197 Requires an explicit destination directory for skill script output, and bundles this deletion into that refactor.
#410 fix/skill-script-outputs-to-artifact-service 2026-07-31 15 files, +913/−214 Routes skill script output to the artifact service instead of the process cwd, and bundles this deletion into that refactor.
#556 fix/skill-script-output-dir-containment 2026-08-03 16 files, +637/−143 Contains skill script output in a declared output directory instead of the process cwd, and bundles this deletion into that refactor.

Resolution — deliberately not stacked, and not withdrawn. Stacking a one-line hygiene deletion onto a behavioural refactor (#353, #410, #438, #556) would make a trivially reviewable change wait on a hard one and would invert the dependency: the deletion does not need any of them. Nor is there anything to stack onto for the three exact duplicates. These all reconcile automatically in any merge order — whichever lands first, the others drop the hunk on their next rebase and this PR becomes an empty diff that should simply be closed. Reviewers wanting the bundled route can close this one at no cost; reviewers wanting the minimal, isolated change should prefer this one and close the rest.

On the .gitignore question: the entry is deliberately omitted here. output.txt is a generic name a legitimate, deliberately materialized skill-script output could use, so ignoring it at the repo root would silently hide such a file from a future contributor — and ignoring the artifact is a band-aid on one filename, whereas the durable fix is to stop materializeFiles defaulting to process.cwd() (the subject of the separate PRs below).

The remaining adjacent PRs all address that cause rather than this already-committed artifact, and none of them touches output.txt, so they are file-disjoint from this diff:

Those PRs stop new artifacts from appearing; this PR removes the one already committed.

Not a breaking change. output.txt is not part of any package's public API, not referenced by any files/exports entry, not imported by any module, not loaded by any fixture, and not consumed by any CI step. No consumer of @google/adk can observe its removal. The commit is chore: so release-please produces no release note, and no CHANGELOG.md is hand-edited.

Testing Plan

Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.
Unit Tests:
[x] I have added or updated unit tests for my change. — N/A, intentionally. This change adds zero lines of code, so new-line coverage is vacuously satisfied (0/0), and there is no behaviour to pin. The "prove the test can fail" requirement likewise has no subject: a mutation test requires a test, and the only test one could write here (!fs.existsSync('output.txt')) is a cwd-dependent assertion about the checkout rather than about the SDK, and would violate the one-file diff invariant. Instead, the existing tests that so much as mention the string were run before and after the deletion and produced byte-identical results (below).
[x] All unit tests pass locally.

Targeted run of every test file that mentions output.txt (only targeted tests were run, per the repo guidance):

npx vitest run --project unit:core \
  core/test/tools/skills/run_skill_script_tool_test.ts \
  core/test/tools/skills/run_skill_inline_script_tool_test.ts \
  core/test/code_executors/unsafe_local_code_executor_test.ts \
  core/test/utils/file_utils_test.ts

Result after the deletion: Test Files 4 passed (4), Tests 39 passed (39).
Result before the deletion (same worktree with output.txt restored, i.e. the base state): Test Files 4 passed (4), Tests 39 passed (39) — identical, confirming the safety audit empirically.

CI-equivalent checks, all on the pushed commit:

Command Result
npm run build pass
npx vitest run --project unit:core <4 files above> 4 files / 39 tests passed
npm run lint pass (clean)
npm run format:check pass — "All matched files use Prettier code style!"
npm run docs:check pass
npx secretlint "**/*" pass (no findings)
bash scripts/check_license.sh pass — "All files have the correct license header."

npm run ts:check reports 48 pre-existing error locations, all under tests/integration/** and core/test/**, and reports exactly the same 48 with output.txt restored — they are untouched by this PR and out of scope (.github/workflows/validation.yaml does not run ts:check; separate in-flight PRs address it).
Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.

On a clean checkout of this branch:

  1. ls output.txtNo such file or directory.
  2. git ls-files output.txt → prints nothing (untracked).
  3. git show --stat HEAD → exactly output.txt | 1 - / 1 file changed, 1 deletion(-).
  4. git status --porcelain → empty.
  5. Run the four targeted test files above, then git status --porcelain again → still empty, proving the test suite does not regenerate output.txt in the repo root. This was run and confirmed.
  6. grep -rn "output\.txt" . --exclude-dir=.git --exclude-dir=node_modules → only the four in-memory test-fixture matches enumerated above, all unchanged.

Checklist

[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas. — N/A; the change is a file deletion, the rationale is in the commit message and above.
[x] I have added tests that prove my fix is effective or that my feature works. — N/A as explained under Unit Tests; verified instead via the before/after targeted runs and the direct postcondition checks.
[x] New and existing unit tests pass locally with my changes.

The 5-byte file output.txt (contents: "hello") was committed accidentally in
8d5cc0a (google#276) as leftover scratch output from developing the
skill script-execution tools: materializeFiles writes agent-produced output
files into process.cwd() by default, so exercising the tools from the repo
root materialized the sample output into the working tree.

Nothing reads or writes the path. A repo-wide grep finds the string only in
in-memory test fixtures whose file_utils module is mocked
(run_skill_script_tool_test.ts, run_skill_inline_script_tool_test.ts) and in
unsafe_local_code_executor_test.ts, which writes new_output.txt into a mkdtemp
temporary directory. No build, test, lint, docs, packaging, or CI step
references it.
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.

1 participant