Skip to content

Chore: remove the accidentally committed output.txt from the repository root - #429

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

Chore: remove the accidentally committed output.txt from the repository root#429
AmaadMartin wants to merge 1 commit into
mainfrom
fix/remove-stray-root-output-txt

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 1, 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):
    Related: Test: drop the npm install from the AgentLoader discovery integration fixture #276 (the change that introduced the artifact; there is no issue backing this cleanup)
  2. Or, if no issue exists, describe the change:

Problem: A 5-byte file named output.txt, containing the string hello with no trailing newline, is tracked at the repository root. It is the only tracked root entry that is not a config, a licence, a manifest, or a doc — it is not source, not a fixture, not documentation, and not a build input.

$ od -c output.txt
0000000   h   e   l   l   o
0000005
$ git ls-files --error-unmatch output.txt
output.txt

git log --oneline -- output.txt returns exactly one commit, 8d5cc0ac ("feat: skills: support script execution", #276) — the change that introduced materializeFiles (core/src/utils/file_utils.ts) and the two skill script tools that call it. The file's name and bytes match the {name: 'output.txt', content: 'hello'} fixture used in that PR's unit tests. Provenance claim, stated precisely: as merged, those unit tests vi.mock materializeFiles, so running them today writes nothing to disk. The defensible statement is that the artifact was produced by an in-progress local run while #276 was being developed and then swept in by a blanket git add — not that the merged unit suite recreates it.

There is no behavioural bug; the file is inert. The defect is repository hygiene.

Solution: delete the file. One file, one deletion, nothing else:

$ git diff main --stat
 output.txt | 1 -
 1 file changed, 1 deletion(-)

Evidence the file is orphaned. git grep -n "output\.txt" -- . ':!node_modules' on the pre-change tree returns exactly four hits, none of which refer to this file:

Hit Why it is unrelated
core/test/code_executors/unsafe_local_code_executor_test.ts:301 The string new_output.txt inside a JS snippet the executor runs; different filename, written inside the executor's own scratch directory.
core/test/code_executors/unsafe_local_code_executor_test.ts:318 Assertion on that same sandboxed new_output.txt.
core/test/tools/skills/run_skill_inline_script_tool_test.ts:226 In-memory File fixture name; materializeFiles is mocked in that file, so nothing touches the disk.
core/test/tools/skills/run_skill_script_tool_test.ts:210 Same pattern, same mock.

It is also invisible to every tool in the repo: absent from .gitignore and .prettierignore (grep -n output .gitignore .prettierignore → no match), not produced/consumed/cleaned by any root package.json script, not matched by npm run lint or npm run format:check (both glob **/*.ts), not matched by scripts/check_license.sh (*.js/*.ts only), and not matched by any include in vitest.config.ts or tsconfig.json. The root workspace package is private and declares no files, so it was never published either way. Nothing is being rewritten: the blob remains reachable in history (git show 8d5cc0ac:output.txt still prints hello).

No source, test, or config file is touched. In particular this PR does not change the process.cwd() default in materializeFiles, does not touch the two skill script tools, and does not touch the tests that merely happen to contain the string output.txt.

Scope change since the previous revision of this PR (disclosed, not silent). An earlier revision of this branch also appended a root-anchored .gitignore block (/output.txt, /output_from_script*.txt, …). That has been dropped and the branch rebased onto current main, because an ignore rule is an explicit non-goal of the approved cleanup: output.txt is a generic filename, and constraining the producer belongs to the change that fixes the cwd default, not to a hygiene deletion. The underlying observation stands and is recorded here as follow-up rather than acted on: four integration cases in tests/integration/tools/run_skill_script_tool_test.ts (lines 306, 326, 346) and tests/integration/tools/run_skill_inline_script_tool_test.ts (lines 165, 224, 245) really do materialize into process.cwd(), and their cleanup is a bare fs.unlink at the end of the it() body with no afterEach/afterAll/try-finally, so an assertion failing before the unlink leaves the artifact in the working tree. Open PRs #298, #305, #353, #410, #437, #556 and #564 already target that root cause.

Collision check (required, recorded here). Scanned all 531 open PRs on the fork (gh pr list --state open --limit 1000), then gh pr diff --name-only on every plausibly adjacent one. Overlaps found:

PR Files Relationship
#271 (opened 2026-07-29) output.txt Byte-identical deletion. Oldest of the set.
#473 (opened 2026-08-01) output.txt Byte-identical deletion.
#539 (opened 2026-08-02) output.txt Byte-identical deletion.
#438 output.txt + both cwd-writing integration tests Deletes the file and rewrites the two integration tests (out of scope here).
#298 / #305 / #353 / #410 / #437 / #556 skill-script sources and tests Fix the cwd root cause; #353, #410 and #556 also carry the deletion as a side effect.
#564 skill-script sources and tests Persists script outputs as artifacts; adjacent to the root cause, does not touch the file.

With the .gitignore hunk removed this PR is now equivalent to #271, #473 and #539. Only one of these should merge#271 is the oldest and is a fine choice; this one is kept open only because it is the branch this task tracks. Nothing new was built to compete with them: the diff was reduced, not written.

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:

  • I have added or updated unit tests for my change. — No test is added, deliberately. The change contributes zero lines of executable code, so the new-code coverage requirement is satisfied vacuously (0/0). A test asserting fs.existsSync('output.txt') === false would pin a property of the process working directory rather than of the library, and is trivially defeated by process.cwd() differing under vitest. The regression proof is the git state itself: git ls-files --error-unmatch output.txt must exit non-zero.
  • All unit tests pass locally. — Whole unit:core project, results below.

Proof the check can fail (A/B, in place of a mutation test). With the deletion reverted (git checkout HEAD~1 -- output.txt), git ls-files --error-unmatch output.txt prints output.txt and exits 0 and test ! -e output.txt exits 1; with the deletion applied git ls-files --error-unmatch output.txt prints error: pathspec 'output.txt' did not match any file(s) known to git and exits 1 while test ! -e output.txt exits 0. Both directions were re-run on the pushed commit.

Verification run on the pushed commit (cf2a216a4), from the repository root:

npm install                        # exit 0; git status clean, no lockfile churn
npm run build                      # exit 0
npx vitest run --project unit:core # exit 0 — Test Files 168 passed (168), Tests 2351 passed (2351)
git status --porcelain             # empty; no artifact regenerated
git diff main --stat               # output.txt | 1 -
npm run lint                       # exit 0 (eslint "**/*.ts")
npm run format:check               # exit 0 — "All matched files use Prettier code style!"

The whole unit:core project is green and the working tree is untouched afterwards, which is the postcondition this change is judged on. Test-file count is unchanged from the pre-change tree (168); the branch differs from main by one deleted non-code file, so no test can be added or removed by it.

As an optional non-regression spot check the plan also allows npx vitest run --project integration tests/integration/skills, which exercises the real materializeFiles path with the repository root as cwd. {inline,loader,script_sh}/agent_test.ts pass and leave the tree clean; script_js/agent_test.ts fails in this sandbox and fails identically on pristine main — its beforeAll runs npm install inside tests/integration/skills/script_js against a proxied registry, measured at 1m04s against a 60s hook timeout, and the body then asserts on live model output. It is a pre-existing environment failure, not a regression; no unrelated test was "fixed", skipped, or weakened here.

git status --porcelain was empty after every run — no output.txt was regenerated by any suite.

Manual End-to-End (E2E) Tests:

  1. Check out this branch and confirm the file is gone from both the working tree and the index: ls output.txt → "No such file or directory"; git ls-files output.txt → empty output.
  2. Confirm the artifact is still reachable in history (nothing is rewritten, only removed going forward): git show 8d5cc0ac:output.txt → prints hello.
  3. Confirm no legitimate file was caught: git status --porcelain is empty and git diff main --stat lists exactly output.txt | 1 -.

Checklist

  • I have read the CONTRIBUTING.md document.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas. — N/A; the diff contains no code.
  • I have added tests that prove my fix is effective or that my feature works. — See the A/B git-state proof above; a unit test is intentionally not added.
  • New and existing unit tests pass locally with my changes. — unit:core 168/168 files, 2351/2351 tests; the only failure anywhere is the pre-existing, baselined script_js integration suite noted above.

…ry root

output.txt (contents: 'hello', 5 bytes, no trailing newline) was committed by
accident in google#276, the change that introduced materializeFiles and
the skill script tools. It is not source, a fixture, documentation, or a build
input, and nothing in the repository references it: the only four occurrences
of the string "output.txt" in the tree are an in-sandbox filename in the
unsafe local code executor test and in-memory fixture names for a mocked
materializeFiles.

Deletion only. No ignore rule is added: the skill script integration tests do
materialize files into process.cwd(), but constraining that producer belongs to
the change that fixes the cwd default, not to this cleanup.
@AmaadMartin
AmaadMartin force-pushed the fix/remove-stray-root-output-txt branch from 5e1585b to cf2a216 Compare August 3, 2026 00:53
@AmaadMartin AmaadMartin changed the title Chore: remove the stray root output.txt and ignore root-level skill script-test artifacts Chore: remove the accidentally committed output.txt from the repository root Aug 3, 2026
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