bug-fix

name

bug-fix

description

Locate the root cause in php-src C code and implement the fix so the failing .phpt now passes. Use when the user says "fix the bug", "implement the fix for GH-12345", "/bug-fix", "now write the patch", "find and fix the root cause", or otherwise wants to advance a bug-fix SPEC.md past Reproduction into Analysis & Plan and Implementation. Refuses if Reproduction phase is not done. Commits test + fix as a single commit on the current branch. Never pushes.

bug-fix

Third step of the bug-fix flow. Owns two phases: Analysis & Plan and Implementation.

Inputs

1. SPEC.md in the task directory resolved per _shared/task-dir.md (env var → .current → cwd), type: bug, Reproduction phase done.
2. $PHP_SRC_DIR with built sapi/cli/php.
3. The failing .phpt recorded under the Reproduction phase steps.

What it produces

• C-code changes under $PHP_SRC_DIR/{Zend,main,ext,sapi,...} as required.
• Updated SPEC.md: Analysis & Plan and Implementation phases populated and done.
• A rebuilt sapi/cli/php and a passing new .phpt.

Phases owned

Analysis & Plan

Steps:

• root-cause — pin the offending function / file / line, write a paragraph explaining the mechanism. Cite specific paths.
• plan-1, plan-2, … — one step per intended file-level change. Each names a concrete file + the change in 1–2 sentences.

Post-guardrail: every plan step references a concrete file path.

Implementation

Steps:

• impl-1, impl-2, … — one per plan-N. Records what was actually changed (path:line, brief diff summary).

Post-guardrails:

• make -j$(nproc) exits 0
• sapi/cli/php run-tests.php <new-test> PASSES

Workflow

1. Pre-guardrails

• SPEC.md type: bug, Reproduction phase done, new .phpt recorded.
• Refuse if any of the above is missing; point the user at /bug-reproduce-test.
• Research phase soft-check: if Research phase exists with status todo, print one warning line and proceed:

  ⚠ Research phase still todo. Run /research first for prior-art (or mark it `skipped` in SPEC.md).
  

  done, skipped, or blocked → no warning.

2. Locate the root cause

Consult the Research phase first if it is done. Often it has already pinned the regression commit (git-log step) or surfaced a maintainers' discussion that names the offending function. Cite the relevant Research finding in the root-cause step body — Per Research's git-log step, commit a1b2c3d is the regression source: it added the BOTH branch but missed the USE_KEY branch.

Otherwise (or in addition), use Grep across Zend/, main/, ext/, sapi/, TSRM/ keyed off:

• function names mentioned in repro.php
• error messages observed in the .diff
• subsystem hint from Context
• file paths surfaced by Research (commits, prior PRs)

Read the candidate functions in full. Trace the data flow that the reproduction exercises. When you've narrowed it to a single function and a single mechanism, write the root-cause step:

- [x] **root-cause** — `ext/standard/array.c:6210` `php_array_filter()` zval-copies the value before passing to the callback in the USE_KEY branch; the BOTH branch at line 6242 uses `Z_TRY_ADDREF_P` which preserves references. The reported reference loss happens because USE_KEY misses the ADDREF.

If you cannot locate the root cause to a single function: write what you found (candidate set), mark root-cause as blocked with the gap, raise an Open Question, and stop. Don't push into Implementation without a confirmed root cause.

2b. Scope-rescope check

Compare the file paths involved in root-cause against the keywords sent to /research (the Research phase's keywords step). If the root cause sits in files outside the original research scope — different subsystem, different function family, different test directory — Research was scoped wrong.

Append this step to the Research phase BEFORE planning:

- [ ] **research-rescope** — re-run /research with keywords <new-keywords-derived-from-root-cause>; pending user invocation.

Then stop with:

Research scope was narrower than the actual fix. Run

  /research <new-keywords>

before continuing. The bug or fix may have been discussed under different terms than the original Context suggested.

Do not plan past this. The user re-runs /research, then re-invokes /bug-fix which resumes at step 3.

3. Plan the change

For each file you intend to touch, add a plan-N step:

- [ ] **plan-1** — `ext/standard/array.c` USE_KEY branch (line 6210): replace `ZVAL_COPY_VALUE` with `Z_TRY_ADDREF_P` to mirror the BOTH branch behaviour.
- [ ] **plan-2** — `ext/standard/tests/array/array_filter_use_key_ref.phpt` (already exists) — no edit; verifying it passes after change.

Scope discipline: keep the diff inside the declared root cause. Don't refactor unrelated code. If you spot a sibling bug nearby that the user didn't ask about, write it as a Suggested follow-up under Open Questions, don't expand the plan.

3b. Test-modification check (mandatory if plan touches any existing .phpt)

If any plan-N step writes to a .phpt other than the brand-new test added in /bug-reproduce-test for this SPEC, stop planning and run the provenance lookup defined in _shared/test-modification-policy.md.

For each affected test, add a step:

- [ ] **test-mod-<basename>** — provenance: authoring commit <SHA> via PR #<N>; intent <summary>; reason for change <reason>; BC <impact>.

Add the corresponding ## Test-modification justification: <path> section to SPEC.md per the policy doc. The section requires all five bullets:

• Authoring commit (SHA + author + date)
• Linked PR / issue (with URL)
• Original intent (one paragraph)
• Reason this fix changes it (one paragraph, with maintainer or internals@ citation if applicable)
• BC impact

If any bullet cannot be filled with concrete, citable evidence — a SHA, a PR URL, a maintainer review link, or a quoted internals@ thread — mark the step blocked and stop. Do not proceed by guessing intent from filenames or commit dates.

Forbidden framings (per _shared/test-modification-policy.md):

• Calling the test's expected output accidental, stale, leftover, vestigial, weird legacy, indefensible, etc. without the justification block completed.
• Speculating about maintainer preferences ("Bob Weinand would prefer …", "internals reviewers typically …") without a quoted prior review.
• Presenting "two valid paths" when path B requires modifying this test and the justification block is not yet filled. Path B does not exist until provenance is documented.

Mark Analysis & Plan phase done when every plan-N step is concrete AND every existing-.phpt modification has a completed test-mod-* step with a populated justification section.

4. Implement

pre-guardrail: every existing-.phpt modification listed in any plan-N step has a matching test-mod-* step with status done AND a populated ## Test-modification justification: <path> section in SPEC.md. Otherwise refuse with:

Provenance lookup required for <path>.
See _shared/test-modification-policy.md.
The five-bullet justification block is missing or incomplete.

For each plan-N step:

1. Read the target file in full (not just the section).
2. Apply the change with Edit.
3. Match the surrounding code style — K&R, tabs, /* */ comments, emalloc/efree for engine memory, PHP_*/ZEND_* macros per layer. See _shared/php-coding-standards.md.
4. Don't add comments explaining the bug fix — git history and the issue link cover that.
5. Mark the corresponding impl-N step done with the actual path and a one-line summary of the change.

5. Build

cd "$PHP_SRC_DIR"
make -j$(nproc)

If it fails: read the error, fix it, rebuild. Max 3 attempts. If still failing after 3, mark the active impl-N step blocked with the build log, stop.

6. Run the new test — expect PASS

sapi/cli/php run-tests.php -v <test-path>

• PASS → both Implementation post-guardrails are green. Mark Implementation phase done.
• FAIL → analyse the .diff. Most likely the change is incomplete or wrong-spot. Iterate: revisit the root cause, refine plan-N/impl-N steps, retry. Max 2 iterations of refinement. If still failing after 2 iterations, mark the last impl-N blocked with the analysis and stop.

7. Commit

Stage the new .phpt (from /bug-reproduce-test, already git add'd but uncommitted) and every C file touched in Implementation:

git -C "$PHP_SRC_DIR" add <path-to-new.phpt> <each-impl-path>
git -C "$PHP_SRC_DIR" status --short

Compose the commit message: subject ≤70 chars, body wrapped at 79, trailers Fixed GH-NNNNN + Closes GH-NNNNN. Use git commit with a HEREDOC:

git -C "$PHP_SRC_DIR" commit -m "$(cat <<'EOF'
Fix GH-NNNNN: <one-line subject from issue title>

<one-paragraph body explaining why — drawn from Root Cause step,
wrapped at 79 chars>

Fixed GH-NNNNN
Closes GH-NNNNN
EOF
)"

Capture the new commit SHA:

git -C "$PHP_SRC_DIR" rev-parse HEAD

Record the SHA in the Progress Log entry. The user owns the commit — if they git reset HEAD~ they're back to the staged state and can re-run / amend.

Never git push, git remote *, git pull, git request-pull. This skill commits locally and stops.

8. Update SPEC.md and report

Append the Progress Log entry citing the commit SHA. Report the result:

bug-fix complete.
Files changed:
  ext/standard/array.c           (+1 -1)
  ext/standard/tests/array/gh12345.phpt   (new)

Commit: a1b2c3d  Fix GH-NNNNN: <subject>

Next: run /bug-verify

Boundaries

• Stay inside the diff the plan declares. If you discover a peer bug, surface it under Open Questions; don't silently widen the fix.
• No remote git mutations. No git push, no git remote add/remove/set-url/rename/prune/update, no git pull, no git request-pull. Local commit + add are allowed (see Step 7). No branch creation — commits land on whatever branch is currently checked out.
• No silent modification of existing .phpt files. Any change to a pre-existing test goes through _shared/test-modification-policy.md provenance lookup and requires a ## Test-modification justification: <path> section in SPEC.md. Without that block, refuse to modify.
• No "two valid paths" framing if path B modifies an existing test that has not been provenance-checked. Path B does not exist until provenance is documented.
• No new tests. Test was written in /bug-reproduce-test. If the test needs adjustment because expected output was wrong, mark the existing phpt-failing step blocked instead of editing the test silently — the user must confirm an expectation change.
• No --enable-* flag changes to ./configure unless the bug is gated by one. Touching the build flags requires Open Question + user confirmation.

Edge cases

Situation	Handling
Root cause spans multiple functions	Add a plan-N per touched function; keep them in one phase.
The fix changes a public API signature (PHPAPI, header file)	Treat as Major scope; raise Open Question for the user to confirm BC implications before implementing.
Fix is one line, no second plan-N makes sense	One plan-1 + one impl-1 is fine. Don't fabricate steps.
make fails with a missing-symbol error after changing a .c file	Likely a header is also needed; add a plan-N for the header and retry.
Test still fails after the fix appears correct	Inspect .diff carefully; verify --EXPECT-- matches actual post-fix output. If --EXPECT-- was wrong, that's a Reproduction-phase regression — block and ask.
Fix requires a configure.ac change	Out of scope for this skill; raise Open Question.
Bug exists on multiple branches	This skill operates on the currently checked-out branch, which must match SPEC.md's target_branch (the lowest maintained branch the bug affects, resolved by /bug-context per _shared/branch-detection.md). To propagate the fix to higher maintained branches, run /upmerge after /bug-verify.

Quality checklist

• root-cause step names a specific file:line and a mechanism (not "somewhere in array.c")
• Each plan-N step names a concrete file
• Each impl-N step records the actual path:line touched and a one-line summary
• Coding standards followed (K&R tabs, emalloc/efree, /* */ comments, naming conventions)
• make -j$(nproc) exits 0
• New .phpt passes via run-tests.php
• No drive-by refactors of unrelated code
• Commit landed on the current branch with subject Fix GH-NNNNN: <subject> and trailers Fixed GH-NNNNN + Closes GH-NNNNN; commit SHA recorded in Progress Log
• No git push, no git remote * mutation, no git pull
• Every existing-.phpt modification has a ## Test-modification justification: <path> section in SPEC.md with all five bullets filled and citable
• If the root cause sits outside the original Research keywords, a research-rescope step was added and the user re-ran /research before planning continued
• No forbidden framings used (no "accidental"/"stale"/"indefensible" labels on existing tests without justification; no reviewer-preference speculation without citation; no "two valid paths" without provenance on both paths)
• Both phases marked done; Progress Log appended

Out of scope

• Locating the bug at user level — Reproduction owns that.
• Regression sweep across related tests — /bug-verify.
• Posting to GitHub — never auto.