Skip to content

Bound plugin git output retention and reject partial refs - #2550

Merged
HypForge merged 2 commits into
masterfrom
integration/plugin-git-output-bound
Oct 8, 2026
Merged

HypForge merged 2 commits into
masterfrom
integration/plugin-git-output-bound

Conversation

@HypForge

@HypForge HypForge commented Oct 8, 2026

Copy link
Copy Markdown
Contributor

Git-hosted plugin failures retain all subprocess output: an 8 MiB stderr stream becomes an 8 MiB error message. This bounds raw capture to 64 KiB per retained pipe while continuing to drain output. It discards unused progress stdout and refuses oversized ref output so an install or update cannot accept a partial commit ref. Truncated diagnostics drop the incomplete final line before credential redaction.

Ordinary diagnostics, ref selection, update timeouts and staging cleanup are preserved. No dependency, configuration or schema changes.

Independent Astra round 1 accepted exact head 95b1de3336d6b7c14c12a209710f4d757b6075c9 against target 55950413e578313d2a370d72a1eeabe6ed294475, with no findings and low shipping risk. Validation: 45 focused tests, full suite (7,837 passed, 4 skipped), typecheck and the real local-git install smoke. The author also passed the package/build-types gate. The same synthetic 8 MiB probe now returns a 71-byte failure message and removes staging. CPU and memory review passed, including copied-buffer ownership and work after saturation.

Raw capture is a bound on retained subprocess output, not total process memory or encoded-message size. A huge diagnostic line can leave only a truncation notice. POSIX executable fixtures skip Windows; the local-git smoke does not prove a production remote install. Full independent evidence follows in a PR comment. Fresh CI and the required merge queue remain delivery gates.

Native-Parent: nn-maint-plugin-git-output-20261007

@HypForge

HypForge commented Oct 8, 2026

Copy link
Copy Markdown
Contributor Author

Plugin git output: independent review, round 1

Accepted at 95b1de3336d6b7c14c12a209710f4d757b6075c9. Findings: none. Independent shipping risk: low. Local acceptance does not authorize publication or landing and does not establish GitHub CI success.

Reviewer team-sink-instance-reviewer@hypforge, native seat 01M4CB7HW4B4PNAMKNQQ4471HF; child plugin-git-output-review-r1-20261008; existing parent nn-maint-plugin-git-output-20261007, original repair owner team-steward@hypforge. Target is 55950413e578313d2a370d72a1eeabe6ed294475. Round 1 of the default 2 for this distinct deliverable. Started 2026-10-08T00:47:46.379Z, checkpoint 2026-10-08T01:47:46.379Z; completed before checkpoint.

Candidate and inspected scope

Used clean detached git-output-review-r1 beneath the owned managed reviewer worktree, plus separate clean git-output-review-base at the exact target for reproduction. Both use the existing compatible ignored dependency link after package-manifest equality verification; this repository has no tracked package-lock.json. Native address, actual cwd, Node v24.2.0, clean candidate, target/author ancestry and diff check are recorded in review-provenance.json. Previous sink worktrees and reports remain untouched.

Whole committed diff: four files, 224 additions/22 deletions. Inspected git_fetch.js, new git_output.js, update_check.js and the added tests, plus install/fetch/CLI update callers, default-ref fallback, redaction, staged artifact validation/copy/cleanup and the existing smoke. The integrated candidate differs from author f0640bc only by the already landed context-graph project.js target change. All four author file hashes, twelve author artifact hashes and owner integration log hashes verified. Ran the full suite on the integrated head to cover that changed target context.

Read applicable LLP 0000/0002, 0007 plugin installation/locking and 0008 prebuilt runtime dependencies. The repair preserves prebuilt git artifact installation, locking, best-effort update checks and no install-time npm execution. No accepted design, dependency, configuration or persisted schema changes. The small shared helper is used by both affected subprocess wrappers; the existing self-update 64 KiB diagnostic convention supplies its scale, while functional stdout needs explicit overflow refusal rather than the updater's tail semantics.

Behavioral review and reproduced evidence

The helper copies at most 65,536 raw bytes into its own buffer for each retained pipe, drains later chunks, and decodes only the captured prefix after subprocess settlement. Unused clone/checkout/version stdout is resumed without capture. Successful overflowing functional stdout changes the subprocess result to failure and returns no partial stdout. Thus rev-parse cannot install a truncated value and ls-remote cannot select an apparently valid prefix while omitting later refs. Default-branch lookup preserves its existing alternate symbolic-ref fallback.

Overflowing diagnostic stderr retains complete prefix lines plus a truncation marker before existing credential redaction. Dropping an incomplete final line prevents truncation from removing the closing delimiter needed to redact URL userinfo. Small diagnostics preserve their text; raw-byte collection preserves UTF-8 split across input chunks. A single oversized line can intentionally leave only the marker, and later diagnostic details can be omitted. This loss of diagnostic detail is the stated bound, not a successful git outcome.

Spawn, nonzero exit and timed-out update probes retain their failure classifications. Data handlers continue draining both pipes; resolution still follows close for normal child execution. Existing update timeout/kill handling, 24-hour policy and sequential CLI probes remain unchanged. Fetch failures return before artifact copy/lock publication, with staging removal in finally. No retry history or process-wide output collection was added. The unchanged clone path still has no new timeout or artifact-size limit; this task bounds retained subprocess output, not the entire installation workload.

My exact-target large-output reproduction returned git_clone_failed with 8,388,642 message bytes after 8 MiB of stderr. The same real fake-git subprocess against the candidate returned 71 message bytes, remained a failure and cleaned temporary staging. The script itself exits 0 to report the reproduced behavior; the old large message is the defect evidence, not a passing resource contract. See review-before.json and review-after.json. Original author baseline tests (42 pass/3 intended failures) and all earlier reproduction artifacts remain preserved, not overwritten by green results.

Independently executed checks

All final-head commands below used the clean assigned candidate; <evidence> means this report directory.

Command Result Evidence
node <evidence>/reproduce.mjs . at target55950413 Exit 0, defect reproduced: 8,388,642-byte failure message, staging removed review-before.json
Same command at candidate95b1de33 Exit 0, bounded 71-byte failure message, staging removed review-after.json
node --test test/core/plugin-install-git-source.test.js Exit 0, 45 pass review-focused.log
npm test Exit 0, 7,841 total / 7,837 pass / 4 skip / 0 fail, 65.5 s review-full.log
npm run typecheck Exit 0 review-typecheck.log
npm run smoke -- plugin_install_git_url Exit 0 review-smoke.log
node --expose-gc --max-old-space-size=32 <evidence>/review-capture-probe.mjs Exit 0, boundary/ownership/saturation assertions pass review-capture-probe.json and .err
git diff 55950413 95b1de33 --check Exit 0 review-provenance.json

The 45 focused checks include real subprocess 8 MiB stdout/stderr drain on failure and success, functional stdout overflow, ordinary/peeled ref selection, split credentials, a credential cut at the retention boundary, split UTF-8, missing git and update timeout. Outer fixture deadlines turn pipe deadlocks into failures. The full suite includes applicable LLP reference and tracked-file hygiene checks; no separate redundant gate rerun was needed.

The existing smoke builds and clones a real local bare git repository, invokes the actual CLI dispatcher install/list path in disposable state, checks the installed artifact hash, manifest hash, pinned SHA, lock/source entries and user-facing result, and asserts install/git/artifact spans plus metric and credential-safe logs. DEV_RUN_ID: smoke-plugin_install_git_url-2026-10-08T00-48-13-018Z-66429. This proves normal local-git install wiring and telemetry, not a production remote or installed-daemon journey. Author package/build:types proof was inspected through its hashed checkpoint, not relabeled as my execution.

Explicit CPU and memory assessment

No CPU or memory concern requiring repair found in the changed or affected paths. Capture has constant per-chunk bookkeeping and copies only bytes remaining within the cap. It adds no whole-output concatenation or growing chunk inventory. Draining remains O(total emitted bytes); decoding, newline search, existing redaction and parsing operate on a bounded prefix. My independent saturation probe emitted 100,000 further chunks after the cap and observed zero further copies, using about 4.6 ms CPU locally. This is a synthetic local measurement, not a universal performance promise.

One empty pipe allocates no capture buffer; one nonempty small pipe allocates 64 KiB. This bounded allocation tradeoff is acceptable for infrequent install/update subprocesses. At most two retained buffers (128 KiB) belong to one command, and discarded stdout owns none. That is the raw capture bound, not total process memory: stream buffers, bounded decoded/redacted strings, UTF-8 replacement expansion and constant markers are additional. No claim limits the final encoded diagnostic to exactly 64 KiB.

The independent ownership probe mutated each original 8 MiB input after capture and verified the stored prefix stayed unchanged. While keeping twelve capture objects alive, forced GC found zero original input buffers retained; approximately 797 KB of ArrayBuffers remained, consistent with twelve bounded captures plus small fixtures. The probe also verified empty and one-byte allocation, exact 64 KiB success, 64 KiB-plus-one functional refusal, one-byte UTF-8 chunking and complete-line diagnostic truncation. It uses direct event emission to isolate the collector; actual process pipe behavior is covered separately above. Its roughly 155 MB RSS reflects deliberate large allocations and allocator retention, not a claimed low-RSS bound; the V8 heap cap is not a process-memory cap.

Collectors are scoped to each child invocation; callers retain bounded strings or scalar update state. Install sequencing and update-rate limiting are unchanged. No state grows with repeated attempts or uptime in this repair. Existing clone duration, artifact traversal/hash allocation and unrelated subprocess helpers are outside this output-retention fix.

Independent shipping assessment and limits

Low risk at the exact candidate and target. Affected users install git-hosted plugins or refresh their update state. The intended difference is bounded diagnostics; unusually wide functional ref output now fails visibly through existing error state rather than accepting a partial ref. Potential unintended impacts are pipe deadlock, loss of small diagnostics, exposed truncated credentials or incorrect installed/update refs. Focused actual-subprocess tests and my capture probe directly exercise those risks, while the real local-git smoke verifies the ordinary path and its pinned artifact/lock state. This assessment rests on those specific proofs, not merely the green full suite.

The change is small and reversible by code rollback, with no data/config migration or new dependency. Failed overlarge ref output does not reach artifact replacement, and the existing cleanup path is exercised. Deliberately omitted diagnostic suffixes cannot be reconstructed after the command ends; this is an accepted ephemeral diagnostic limit, not deletion of installed plugins or user recordings.

POSIX executable fixtures ran on macOS/Node24 and skip Windows. No claim is made about Windows execution, a real network receiver, remote service authentication, production incident frequency or production memory. No owner configuration, services or credentials were changed. No GitHub current-head CI, branch-policy/queue eligibility or landing was observed in this review. The owner retains those delivery gates and all publication authority under GITHUB.md.

No findings require correction. Return this exact-head acceptance and low-risk evidence to the live existing steward-owned parent, retaining both review checkouts until owner release. This completes round 1 of 2 for plugin git output; unrelated sink review history is separate.

@HypForge
HypForge marked this pull request as ready for review October 8, 2026 00:58
@HypForge HypForge added the neutral:approved neutral reviewed this and holds it for a maintainer merge (own or adopted PR; LLP 0025/0030) label Oct 8, 2026
@HypForge
HypForge added this pull request to the merge queue Oct 8, 2026
Merged via the queue into master with commit 16e8791 Oct 8, 2026
8 checks passed
@HypForge
HypForge deleted the integration/plugin-git-output-bound branch October 8, 2026 01:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

neutral:approved neutral reviewed this and holds it for a maintainer merge (own or adopted PR; LLP 0025/0030)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant