Skip to content

[3266 by Claude Opus 5.5] Add bounded phase diagnostics to protected signed RPM acceptance failures - #3267

Merged
integry merged 1 commit into
mainfrom
3266/claude-opus-5-5-add-bounded-phase-diagnos-20261019-1857-9wq
Oct 19, 2026
Merged

integry merged 1 commit into
mainfrom
3266/claude-opus-5-5-add-bounded-phase-diagnos-20261019-1857-9wq

Conversation

@propr-dev

@propr-dev propr-dev Bot commented Oct 19, 2026

Copy link
Copy Markdown
Contributor

AI Implementation Summary

Closes #3266

Branch: 3266/claude-opus-5-5-add-bounded-phase-diagnos-20261019-1857-9wq
Commits: ✅ Changes committed (7a4eab8)


AI Processing Completed

Execution Details:

Summary:
A failed protected acceptance job will now report which fixed phase failed and how: an exit status, signal, timeout, spawn error code, or interruption. The new failure-injection tests pass. I did not touch the existing run's failure, and nothing here identifies its cause.

What's established about run 39305432099:

  • Identity, helper verification and signing passed, and the job then failed in the signed RPM acceptance step.
  • The old runner discarded child stderr and threw the same message for every non-zero subprocess.
  • So the cause could be a failed image build, a failed isolated network setup, or a failure inside the installed package/browser container. All three are equally consistent with that log.

What remains unknown: which of those phases failed, its exit status, and the root cause. No reproduction evidence has been provided yet, so I made no speculative fix. The next run will name the phase.

What changed

  • New apps/desktop/scripts/signed-rpm-acceptance-diagnostics.mjs:
    • Fixed names only. Every failure is a SignedRpmAcceptanceFailure built only from allowlisted fields: a fixed phase name, an outcome, a numeric exit status (0–255), a Node signal name, an E… spawn code, and a container step from a fixed list. Anything else becomes unknown/error.
    • Nothing from the child. Messages and diagnostics never contain child stdout/stderr, arguments, environment values, paths, key material or a cause.
  • run-signed-installed-rpm-acceptance.mjs:
    • Phases. Each step runs under a fixed phase: docker-info, workspace-setup, context-preparation, image-build, network-create, installed-acceptance, evidence-collection, evidence-export, evidence-write.
    • Cleanup. All owned cleanups (container, network inspect/rm, image, temp workspace) now run even if an earlier one fails. Cleanup failures are listed under cleanup after the original failure; previously the old nested finally could replace the original error.
    • Evidence-collection errors. These are reduced to the phase name only. Node's JSON parse errors can quote file content, and file errors include paths.
    • CLI output. On failure it prints one bounded message plus one JSON line, e.g. {"signedRpmAcceptanceFailure":{"phase":"installed-acceptance","outcome":"exit","exitCode":1,"stage":"package-install"}}, and exits 1.
  • test-signed-installed-rpm.sh: the container script now prints a fixed marker on entering each step (key-trust, signature-verify, package-install, installed-verify, runtime-manifest, browser-acceptance, evidence-export, post-verify, …). The runner keeps only the last complete marker line that matches the allowlist and throws away all other output. This lets the report separate the package install from the browser run.

Behaviour you'd notice:

  • The failure message now reads Signed installed RPM acceptance failed; no success evidence emitted [phase=… outcome=…] instead of the old fixed text.
  • Raw spawn errors, which included the path, and the old "interrupted" message are replaced by the bounded form.
  • A signal now counts as a failure even for commands that were allowed to fail.
  • The function takes optional docker, verify and prepareContext substitutes so the tests can drive it. The CLI always uses the real Docker CLI, real RPM verification and the allowlisted context.

Signature, source and runtime identity checks, the internal network, the no-success-evidence rule, permissions, the workflow and artifact uploads are all unchanged.

Tests (signed-rpm-acceptance-diagnostics.test.mjs, 12 tests, also in the desktop npm test glob):

  • Unit tests: exit, signal, timeout and spawn outcomes, plus marker parsing with split, garbage, spoofed and too-long lines.
  • Full-flow runs: these use a fake Docker CLI that echoes a sentinel secret, its arguments and its environment on every output stream. They cover:
    • each major phase failing, plus a complete passing run.
    • the primary failure surviving three failing cleanups, while a network it doesn't own is not removed.
    • context, evidence and artifact-changed failures.
  • Leak checks: the sentinel and paths are checked never to appear in messages, diagnostics or inspect() output.
  • Mutation checks: dropping the cleanup merge made the tests fail, and so did loosening the stage allowlist.
  • Existing suites: the evidence, workflow, context and RPM signing tests pass (32 pass), but 4 tests were skipped because GnuPG and the RPM tools aren't installed here. So the real-tool signing tests need to run in CI or with operator validation.
  • Lint: eslint passes with zero warnings.
  • Not run: no real Docker, owner secrets or protected dispatch were used.

I also added a section on the diagnostic format to docs/docs/operations/linux-rpm-signing.md. No visual preview applies.

Detailed Logs:

  • Conversation: 82 messages
  • Session: 532066ba-8810-428f-84e7-f2c3756b8cf6

Log files stored at:

  • conversation: /usr/src/app/logs/executions/issue-3266-2026-10-19T19-06-36-273Z-conversation.json
  • output: /usr/src/app/logs/executions/issue-3266-2026-10-19T19-06-36-273Z-output.txt
Latest Conversation Messages
ASSISTANT: [content unavailable]

ASSISTANT: A failed protected acceptance job will now report which fixed phase failed and how: an exit status, signal, timeout, spawn error code, or interruption. The new failure-injection tests pass. I did not ...


This PR was created automatically by ProPR after processing issue #3266.


💡 Need changes?

Comment on this PR to request refinements — the AI agent monitors comments and will update the implementation based on your feedback. Keep iterating until you're satisfied!

…ailed and how: an exit status, signal, timeout, spawn error code, or interruption. The new failure-injection tests pass. I did not touch the existing run's failure, and nothing here identifies its cause.

What's established about run 39305432099:
- Identity, helper verification and signing passed, and the job then failed in the signed RPM acceptance step.
- The old runner discarded child stderr and threw the same message for every non-zero subprocess.
- So the cause could be a failed image build, a failed isolated network setup, or a failure inside the installed package/browser container. All three are equally consistent with that log.

What remains unknown: which of those phases failed, its exit status, and the root cause. No reproduction evidence has been provided yet, so I made no speculative fix. The next run will name the phase.

What changed
- New `apps/desktop/scripts/signed-rpm-acceptance-diagnostics.mjs`:
  - Fixed names only. Every failure is a `SignedRpmAcceptanceFailure` built only from allowlisted fields: a fixed phase name, an outcome, a numeric exit status (0–255), a Node signal name, an `E…` spawn code, and a container step from a fixed list. Anything else becomes `unknown`/`error`.
  - Nothing from the child. Messages and diagnostics never contain child stdout/stderr, arguments, environment values, paths, key material or a `cause`.
- `run-signed-installed-rpm-acceptance.mjs`:
  - Phases. Each step runs under a fixed phase: `docker-info`, `workspace-setup`, `context-preparation`, `image-build`, `network-create`, `installed-acceptance`, `evidence-collection`, `evidence-export`, `evidence-write`.
  - Cleanup. All owned cleanups (container, network inspect/rm, image, temp workspace) now run even if an earlier one fails. Cleanup failures are listed under `cleanup` after the original failure; previously the old nested `finally` could replace the original error.
  - Evidence-collection errors. These are reduced to the phase name only. Node's JSON parse errors can quote file content, and file errors include paths.
  - CLI output. On failure it prints one bounded message plus one JSON line, e.g. `{"signedRpmAcceptanceFailure":{"phase":"installed-acceptance","outcome":"exit","exitCode":1,"stage":"package-install"}}`, and exits 1.
- `test-signed-installed-rpm.sh`: the container script now prints a fixed marker on entering each step (`key-trust`, `signature-verify`, `package-install`, `installed-verify`, `runtime-manifest`, `browser-acceptance`, `evidence-export`, `post-verify`, …). The runner keeps only the last complete marker line that matches the allowlist and throws away all other output. This lets the report separate the package install from the browser run.

Behaviour you'd notice:
- The failure message now reads `Signed installed RPM acceptance failed; no success evidence emitted [phase=… outcome=…]` instead of the old fixed text.
- Raw spawn errors, which included the path, and the old "interrupted" message are replaced by the bounded form.
- A signal now counts as a failure even for commands that were allowed to fail.
- The function takes optional `docker`, `verify` and `prepareContext` substitutes so the tests can drive it. The CLI always uses the real Docker CLI, real RPM verification and the allowlisted context.

Signature, source and runtime identity checks, the internal network, the no-success-evidence rule, permissions, the workflow and artifact uploads are all unchanged.

Tests (`signed-rpm-acceptance-diagnostics.test.mjs`, 12 tests, also in the desktop `npm test` glob):
- Unit tests: exit, signal, timeout and spawn outcomes, plus marker parsing with split, garbage, spoofed and too-long lines.
- Full-flow runs: these use a fake Docker CLI that echoes a sentinel secret, its arguments and its environment on every output stream. They cover:
  - each major phase failing, plus a complete passing run.
  - the primary failure surviving three failing cleanups, while a network it doesn't own is not removed.
  - context, evidence and artifact-changed failures.
- Leak checks: the sentinel and paths are checked never to appear in messages, diagnostics or `inspect()` output.
- Mutation checks: dropping the cleanup merge made the tests fail, and so did loosening the stage allowlist.
- Existing suites: the evidence, workflow, context and RPM signing tests pass (32 pass), but 4 tests were skipped because GnuPG and the RPM tools aren't installed here. So the real-tool signing tests need to run in CI or with operator validation.
- Lint: eslint passes with zero warnings.
- Not run: no real Docker, owner secrets or protected dispatch were used.

I also added a section on the diagnostic format to `docs/docs/operations/linux-rpm-signing.md`. No visual preview applies.
@integry

integry commented Oct 19, 2026

Copy link
Copy Markdown
Owner

/review gpt-6-astra

@propr-dev

propr-dev Bot commented Oct 19, 2026 •

Copy link
Copy Markdown
Contributor Author

✅ AI Code Review Complete requested by @integry

Posted 1 review:

View Task Details

@propr-dev

propr-dev Bot commented Oct 19, 2026

Copy link
Copy Markdown
Contributor Author

🔍 AI Code Review — gpt-6-astra

Overall Evaluation

The PR adds useful, bounded phase diagnostics while preserving the acceptance checks and prioritizing the original failure over cleanup errors. It is merge-ready within the supplied scope, subject to pending checks completing successfully.

✅ Controlled diagnostic output — Failure messages are rebuilt from restricted fields; child stdout, stderr, arguments, and environment values are not included.

✅ Primary failure preservation — Cleanup operations run independently, and their failures are appended without replacing the substantive acceptance failure.

✅ Focused failure injection — Tests cover subprocess failures, stage parsing, evidence rejection, privacy, and continued cleanup.

This was a static review; no commands were run. The supplied issue description is explicitly truncated, so unseen acceptance requirements could not be assessed. The supplied diff was reviewed as complete.

Merge blockers

No merge blockers.

Suggestions

These are optional follow-ups and are not sent to /fix.

S1: 🟢 Test parent interruption explicitly

The new runner classifies parent SIGINT and SIGTERM as interrupted, but the tests exercise child signals rather than parent interruption. An isolated subprocess test could verify interruption classification, subsequent cleanup, and absence of success evidence. This is optional coverage for behavior whose basic interruption mechanism already existed.

S2: 🟢 Clarify tolerated cleanup failures

The runner retains existing allowFailure: true behavior for image removal and resource inspection. Consequently, ordinary nonzero image-removal exits are not included in cleanup diagnostics, and unsuccessful inspection is treated as resource absence. Consider documenting these exceptions to the new statement that cleanup failures are reported, or improving their classification in a follow-up. These tolerances predate this PR and are not introduced regressions.

Score

The implementation addresses the visible objective with focused tests and no verified introduced blocker. Review confidence is limited to static analysis; the authoritative check summary reports no failures and 15 pending checks.

Score: 8/10


🤖 Review Details

  • Model: GPT-6 Astra
  • Time: 26s
  • Tokens: 48,644 (47,961 in / 683 out)
  • Cost: $0.38

View Task


💡 Next step: Comment /fix to address every F# merge blocker, /fix all for every pending blocker and suggestion, or name records explicitly, as in /fix F3 S5.
F# and S# IDs increment across review comments and remain permanent, so selectors such as /fix F3 F5 stay unambiguous across cycles. S# suggestions stay optional: they are implemented only when you name them or request /fix all, and they never relax a merge blocker.

Copy and edit to choose records from this review:

/fix S1 S2

🤖 Review by ProPR

@integry
integry merged commit 9c039ac into main Oct 19, 2026
58 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add bounded phase diagnostics to protected signed RPM acceptance failures

1 participant